ESM Scripts #5764
ESM Scripts #5764
Conversation
|
Just personally reviewing this. I'm actually leaning more towards instance based prop attributes, because that's how it currently works, and it just makes so much more sense to think of this.value. Thoughts @willeastcott export const attributes = {
aNumber: { name: 'String', default: 10, min: 0, max: 199 }
}
class MyModule {
aNumber = 10
constructor(entity, app){}
update(dt){}
destroy(){}
} |
|
This is vastly different functionality from the current ScriptType. Few things:
Also, class redefenition - can be a blocker, in case of multi-application context, swap, and lazy-loading, using a class - can lead to blockers. If there is a way to provide class alternative for a ScriptType, it would be great to not reduce the functionality, but actually improve it as well. Justifying only by syntaxis (class vs prototype) IMHO would be not a reasonable feature. |
|
Thanks @Maksims. Great to get your input on this. There's still ongoing discussion around this, so we'll take this all into account as we go.
Not sure I follow on this point, could you elaborate? |
|
@marklundin If you don't declare the instance property, IntelliSense is gonna spit out a bunch of errors because Would it be possible to decorate the actual property declaration in the class and not have the |
|
I am curious if it is a replacement to ScriptType, or a feature next to it. Some notes:
I wish this new script type, would be a sister to ScriptType. The way I see it is that if some or all of the ScriptType features are needed, one would simply create a ScriptType script, or somehow inherit all its features in a new type. If one doesn't need ScriptType features, he could create a new type, possibly inheriting EventHandler. It would be great to have paths supported, so a same module name could live in a different folder asset and we could do imports using paths. |
|
Well, no one really stopping developers of doing their own classes for script systems already, the only thing you need a way to attach them from the Editor, which you probably can do by small classic ScriptType with json+array attributes. If there is a change to the way ScripyTypes would be created, it should be a significantly better than a current system. Lets first specify what ScriptType is not satisfying, and what are shortcomings are based on developer needs. |
|
Thinking about this more, we probably should focus on the primary task here, which is to add support for ES Modules, anything outside this is secondary, and it doesn't actually require any significant change to the API. Any issues/pain points the existing ScriptType API we can tackle as a separate issue and shouldn't really block the main objective. I'll spend some time thinking about this more |
Well, if this will be replacement to a ScriptType, then it is a relevant conversation. |
|
Thanks for all your input. We've outlined some more detailed design thoughts in this RFC #5770 |
class Test {
static attributes = {
layer: {
type: 'object',
default: 0,
title: 'Layer',
description: 'Which Layer to print the debug lines on'
}
};
}
const entity = new pc.Entity("test");
entity.addComponent('esmscript');
entity.esmscript.add(Test);In the old system you could just set
These error messages should also contain the given "wrong type value" for reference, could just add Another potential issue: Before we would do something like Last thing I just want to point out: Since the esmscript classes are classes without inheriting from e.g. |
|
A couple of questions:
|
|
(2) Just tested, yes, it is set then: class Test {
static attributes = {
layer: { type: 'number', default: 123}
};
initialize() {
console.log("TEST", this.layer);
}
}
const entity = new pc.Entity("test");
demo.app.root.addChild(entity);
entity.addComponent('esmscript');
entity.esmscript.add(Test);Outputs: TEST 123 I wonder about possible refactoring of something like (as in // Handle any and all attribute changes
this.on('attr', function (name, value, prev) {
if (value !== prev) {
if (name === 'font') {
if (this.font) {
this.fontData = opentype.parse(this.font.resource);
} else {
this.fontData = null;
this.destroyCharacters();
}
}
if (this.fontData) {
this.createText();
}
}
});Sorry about (5), I realized it was my own mistake - I took @LeXXik's AMMO Debug Drawer some years ago as basis and tweaked it a lot for making it "standalone" and for making a repo to show how it works and just some testing between the two module types (legacy vs ScriptType): I never realized before that I used it wrong, since there was no error and everything just worked. So testing this PR actually found a mistake in my own script 👍 (6) I also don't know how to enable/disable a script besides reimplementing pretty much pc.ScriptType (8) Just tested, it depends on the last argument of class Test {
initialize() {
console.log("Test#initialize called");
}
}
const entity = new pc.Entity("test");
demo.app.root.addChild(entity);
//entity.enabled = false;
entity.addComponent('esmscript');
entity.esmscript.add(Test, null, true); // last arg called "enabled" is "true" = enabled -> call's inittialize |
|
Regarding the whole of this PR, why not use classes notation with existing system and extend it, instead of creating a new one? I just did this: class TestClass extends pc.ScriptType {
initialize() {
console.log('initialize');
}
update(dt) {
this.entity.rotate(dt * 90, 0, 0);
}
}And then: entity.script.create(TestClass);And it worked perfectly. With only a few adjustments to the existing pc.ScriptType, we can ensure that attributes are parsed from static property, and we do not lose any functionality, it works already. It will be easier to add Editor support, and you can do imports as you want. No two script systems. Same execution order. Easy to migrate. No need to deprecate anything. |
|
I’ve opted for a separate system for a few reasons. We’re planning to allow a global script priority, so users can specify an execution order to types of scripts. This is outlined in a separate issue but existing scripts specify a local script execution order, and so the semantics are different. Also, I believe it’s important a script shouldn’t require a base class in order to work. It shouldn’t break, or worse partially work with wrong results if it doesn’t inherit from the right class. It need only implement the methods it uses and expose the attributes it requires. Removing the dep on a super class gives much more flexibility for users, keeps the dependancy between a users scripts and the engine as loose as possible and we also don’t have to worry about maintaining a base class that is difficult to upgrade else it breaks users projects. A base class imported by the user can be versioned independently from the engine. Also attribute events etc are useful, but not everyone uses them, users should be able to use what they need and omit what they don’t. Also, there’s a fair bit of conditional logic to handle legacy scripts, and I’m reluctant to add another layer for ESM Scripts which is likely needed as they’d need separate Handlers to load them and maybe in other places too. Taken together, keeping the ESM Scripts as a logically separate component/system makes sense. It keeps things isolated, allows us to provided new functionality without breaking existing code, and is done in a way inline with current practice of upgrades (Model vs Render component). |
|
@kungfooman I'm AFK at the moment, but thanks for reviewing this all. Will get back to you both when I can. Appreciate all the work 🙏 |
Such system can be implemented to work with an existing scripting system.
On what basis this is a requirement? Is it a problem with an existing system? Is it a blocker for some commercial projects?
Not having a base class will have "loose rules" over of how the class should be defined/used. Underlying system will have to forever support such rules. Having a base class will actually provide a stable versioning and migrations approach. That allows users to consciously migrate to another base class if required. If some features are not desirable, e.g. attribute events, then a feature for the pc.createScript in form of options, or static property on a class - is a good way to control such features.
Yep, there are two systems already, Legacy Scripts, and Scripts 2.0. So adding a third system will not make it better here.
Every move of PlayCanvas to deprecate some component, e.g.: Model > Render, Audio > Sound, Legacy Scripts > Scripts 2.0, Animation > Anim. Introducing a full on new script component has a major impact:
I still have not seen a real problem with a current ScriptType that justified a full switch to a new component system. Please provide actual reasons backed by problems from commercial projects. |
|
One of the lacking features for us with ScriptType is inability to add modules from NPM ecosystem. Given how modules are widespread today, some libraries don't even bother generating IIFE or ES5 variants, so it adds to our burden to bundle them in a way that would work in PlayCanvas. Importing modules should be as natural. It is a hassle at the moment. I personally don't use TypeScript, but I'm aware of projects that wish they could choose if they want to use TypeScript or Javascript, similar to Babylon. I believe the ESM modules system would allow to make it happen. |
We can add ESM modules with existing ScriptType. ESM modules mostly require small improvements to ScriptType, and integration to Editor, it does not require whole new component system at all. |
|
To be honest, this has caused a few headaches in the past... const c = Color()
script.color = c
c.r = 1
(script.color.r === 1) // nope |
This is consistent with the rest of the engine: do not attach short-lived/reusable objects, but copy their values to avoid unintended edits later, e.g.: rigidbody.angularFactor
rigidbody.angularVelocity
rigidbody.linearFactor
rigidbody.linearVelocity
element.alignment
element.color
element.outlineColor
// dozens more ...Pretty much every property on components within PlayCanvas engine follow that practice, ScriptComponent attributes are not an exception. |
|
Fully disagree. Invisibly changing the behaviour of a public member when it's explicitly declared is super weird. class MyClass {
// This attribute is changed to a copy???
colorAttr = new Color()
} |
|
If you do this in your code, this is not a copy, that creates a new member and assigns your vector: but the example Max added: I would expect the setter of the color would copy the color to its private property, to make sure the user cannot change its content without it being pushed through the setter, where this might need to be handled. We had issues with this in many cases, where the user would initialize multiple bounding boxes with the same vectors, and when one changes, the other would change too. Also, they'd use Vec3.ZERO to assign it to a property, and later internally this would modify the ZERO static member, causing hard to find issues. |
|
Also it is common to do this: element.color = pc.Color.RED; |
|
I'm very happy to see discussion around ownership details! Right now values like vectors aren't even cloned yet properly: function cloneEntityAndFitStuff(entity) {
const clone = entity.clone();
const cloneScript = clone.esmscript.get("Test");
cloneScript.a.scale(0.000001);
cloneScript.b.scale(0.000001);
cloneScript.c.scale(0.000001);
return clone;
}
class Test {
static attributes = {
a: { type: 'vec3', default: [ 1, 2, 3]},
b: { type: 'vec3', default: [ 11, 22, 33]},
c: { type: 'vec3', default: [111,222,333]},
};
initialize() {
console.log("TEST", this.a, this.b, this.c);
}
}
const entity = new pc.Entity('test');
demo.app.root.addChild(entity);
entity.addComponent('esmscript');
entity.esmscript.add(Test);
const clone = cloneEntityAndFitStuff(entity);
const {a, b, c} = entity.esmscript.get("Test");
console.log("Initial/uncloned entity values are now", JSON.stringify({a, b, c}, null, 2));
console.log("Test *should* be false", entity.esmscript.get("Test").a === clone.esmscript.get("Test").a);Outputs:
Basically because this is entity.esmscript.get("Test").a === clone.esmscript.get("Test").aNow lets convert the same test script to make it use function cloneEntityAndFitStuff(entity) {
const clone = entity.clone();
const cloneScript = clone.script.Test;
cloneScript.a.scale(0.000001);
cloneScript.b.scale(0.000001);
cloneScript.c.scale(0.000001);
return clone;
}
class Test extends pc.ScriptType {
static _ = (
this.attributes.add('a', {type: 'vec3', default: [ 1, 2, 3]}),
this.attributes.add('b', {type: 'vec3', default: [ 11, 22, 33]}),
this.attributes.add('c', {type: 'vec3', default: [111,222,333]})
);
initialize() {
console.log("TEST", this.a, this.b, this.c);
}
}
pc.registerScript(Test, 'Test');
const entity = new pc.Entity('test');
demo.app.root.addChild(entity);
entity.addComponent('script');
entity.script.create("Test");
const clone = cloneEntityAndFitStuff(entity);
const {a, b, c} = entity.script.Test;
console.log("Initial/uncloned entity values are now", JSON.stringify({a, b, c}, null, 2));
console.log("Test *should* be false", entity.script.Test.a === clone.script.Test.a);Outputs:
Using
I'm also happy that we don't inject a reference from somewhere else into the script. Whereas this PR script system is replacing "internals":
We could kinda forget about the difference if we would write code more like this: entity.script.Test.a.copy(d);This also relates to this PR: #4925 One problem of flipping internals via loose ownership: if you assign a frozen object, the type is suddenly "read-only", but no one defined that type being read-only anywhere. So we ended up with more code testing that in different places (where possible): Debug.assert(!Object.isFrozen(center), `The constructor of 'BoundingBox' does not accept a constant (frozen) object as a 'center' parameter`);
Debug.assert(!Object.isFrozen(halfExtents), `The constructor of 'BoundingBox' does not accept a constant (frozen) object as a 'halfExtents' parameter`);To sum up what I think:
|
|
Thanks @kungfooman for pointing out the bug. Cloned entities shouldn't share attributes - I'll address this and add tests. @Maksims @mvaligursky, my main issue isn't with the copying itself, but with how current scripts invisibly override property accessors and change their semantics. This is especially problematic in class syntax, as user-defined accessors get removed and overridden, like so: class Script {
set colorAttr(v) {
// Overridden
}
get colorAttr() {
// Also overridden
}
}This creates confusion and inconsistency for users. It's hard to predict when and why accessors are overridden, plus the behaviour varies by type (vectors are copied, curves cloned). Assigning attribute via the editor/launcher should be efficient, but can be handled outside the class and shouldn't override user code or alter accessor semantics. If the engine needs to monitor attribute changes, we should consider using proxies. Users should explicitly handle attribute changes across scripts, even if it's more verbose but clearer. |
I think it should a balance, of convenience and stability. Copying has no runtime memory impact, and a small compute impact, while provides safety. While reference usage gives better init memory, faster execution, but can suffer from safety. Common devs without knowing the implications will end up in problematic situations more often, than they require very precise performance and efficiency in such non-impactful area. If a power user requires such micro-optimisation they are not limited to implement own accessors. Don't forget, we are power users, while majority don't know insides of the engine, and safety for them is more important. |
|
This has now been updated based on feedback
The scripting system should now be resistant to mangling, by requiring an explicit class identifier. I've also added tests to cover this. Also it seems that
Agree with this @kungfooman , although I don't like the idea of overriding a class's props. I'm less concerned with the issue of changing a types shape and more that if a user adds a |
|
I still haven't seen a strong reason for another component system that does exactly the same. Instead of improving ScriptType and extending it. Basically allowing users to use same components as they had, and even attributes in Editor won't need to be changed and hierarchy. No need to parse new extra stuff, no need to support two separate systems! |
|
Hey @Maksims, so these are friction points in the current ScriptType. Overriding of class propsThis becomes more confusing in class based syntax, but it's also inconsistent applied across types class MyClass extends ScriptType {
vec = new Vec3()
curve = new Curve()
}
vec = script.MyClass.vec
vec.x = 10 // this works
script.MyClass.vec = myOtherVec
myOtherVec.x = 10 // does not work
vec.x = 10 // this works
curve = script.MyClass.curve
curve.x = 10 // this works
script.MyClass.curve = myOtherCurve
myOtherCurve.x = 10 // does not update class
curve.x = 10 // this does not workThis is not an argument against copying per-se, just that it should be clear and transparent to the user what is happening, and something they can opt in and out of. Performing invisible copies is hard to debug especially when they're inconsistent. In the engine code itself, this is fine. Inflexible inheritanceAnecdotal feedback shows users prefer to choose their own base class. This provides more flexibility to the user. Conformance to an Attributes can't be inheritedYou can't extend a Script as it's attributes will not work. Attributes are tied to the file they're declared in. Class name is susceptible to manglingThe Scripts override component props
ImportingCurrent scripts are loaded using a script tag which doesn't work in Node, Deno etc. Using ESM imports is now a standardised way of importing code across environments. Concatenation + Hot reloadingNot a huge one but existing scripts are compiled using simple string concatenation. Modules would need to be properly bundled in order to retain correct module scope and provide better more modern bundling fearures. Additionally, arguably HMR code should be removed from production code. OverallEven though these could be individually solved. It would be non-trivial to iteratively update the current scripts without breaking existing projects. Using a separate components system gives more flexibility to move forward in a safe way. Moreso, it allows us to logically isolate newer language features from older ones. If a user elects to use ESM modules, we can make assumptions and use newer language features. Similarly, if they want to use older scripts, they can do so without having to load newer features. It also provides a clean an straightforward way to migrate users. |
I'm sorry, but your code is incorrect. These assignments do work. Current behavior is this: class TestClass extends pc.ScriptType {
static name = 'test';
vec = new pc.Vec3();
curve = new pc.Curve();
}
// ...
const script = entity.script.test;
const otherVec = new pc.Vec3();
script.vec = otherVec;
otherVec.x = 10;
script.vec.x === 10; // true
In JavaScript there is no class NetworkScript extends pc.ScriptType {
// implements networking
}
class Barrel extends NetworkScript {
static name = 'barrel';
}So you can use inheritance the way you want. Also, this is common practice in the industry and across pretty much all engines - to inherit the base class. Can you please provide a poll or some source of PlayCanvas user preferences on this topic? We can tackle that issue for existing
This is not an engine issue, but the way the Editor loads and parses scripts. So it has no context of other scripts. Nothing to do with the engine. This will be same issue for your ESM Scripts implementation. Please either upvote the relevant ticket one Editor's repo or create a new Issue there.
The same issue for the ESM Script implementation.
You can load scripts in node.js: you can import and you can provide exports. We use PlayCanvas in node.js and have no problems with it.
Do you have a good stats on this? Haven't had a single issue or heard of one regarding concatenation process. It does what it supposed to: concatenates scripts, minifies them and ensures they work exactly same way as non-concatenated or minifed. As for concatenation or minifacation for engine-only users. They can already use whatever systems they want, and minify the way they want. There are no blockers. Hot-reloading, is extremely slim part of the codebase, and people use it. We use it even on node.js back-end production with multiplayer projects, where we can update gameplay not only during development, but also hot-patch stuff in production. And it is extremely powerful. Please provide a strong argument and production cases where it caused any issue that it requires a removal.
I strongly disagree. The users and the engine have their ways and have been working pretty well. It can be improved - and progressive improvements are the way to go. Unless there is a major roadblock, which then requires an alternative (e.g. Audio > Sound component, or Model > Render). This has to be done and can be done like everything before, updating the engine without breaking stuff. This is the way the engine has been developed since the beginning - it always works, and doesn't break older stuff. It is important for developers that trust the engine.
I'm sorry, but I've provided a major list of issues with a new system, and it does not solve all the list you've provided regarding the current system. So the new system will suffer as much from the same issues. As they need to be tackled separately.
There is no need to migrate. That's the point. I might point out, that I've referring to the majority of users, which are Editor users. Engine-only users are important too, but the solution has to serve both environments. Implementing a whole new scripting component system, that is almost identical to ScriptType, and it does not still solve the majority of issues you have listed here because they come from Editor and other places. I'm sorry, but I don't believe this should be part of the Engine. |
|
We've opted to use the existing |
ES Module Component System
Moving forward we intend to add first class support for ES Modules a Scripting interface for PlayCanvas. This PR add's native engine support for ESM Scripts as a separate component system.
ESM Script Structure
ESM Scripts need only to implement the required properties and methods it needs.
New API
To register a Script cawith the engine you use
entity.esmscript.add(Module, attributes). However for many users, this will be handled automatically in the editor.This provides:
pcWhy not use the old Script system
This interface aims to replicate the interface to the old scripting system as closely as possible, improve upon it, whilst also mitigating some of the issues it faces in a new, more modern ESM ecosystem. Some of these can be found below
See #4767
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.