Scripts 2.1 (Class support)

[Maksims]

Improves ScriptType to be extendible by class, while preserving current scripting features (attributes, events, components).
This allows you to define scripts as classes.

This does not change any current behavior only adds support for class.

Scripts-related data in the hierarchy and templates will work as before, even if the old script is replaced with a new definition.

Example:

class Rotator extends pc.ScriptType { // should extend ScriptType
    static name = 'rotator'; // if defined, then this name will be used instead of class name

    speed = 42;

    update(dt) {
        this.entity.rotate(dt * this.speed, 0, 0);
    }
}

// add script to the script registry
app.scripts.add(Rotator);

// attach a script to an entity
entity.script.create(Rotator);

// can be attached by a script name too (allows lazy-loading for scripts)
entity.script.create('rotator');

Attributes work as before:

class Rotator extends pc.ScriptType {
    static name = 'rotator';

    update(dt) {
        this.entity.rotate(dt * this.speed, 0, 0);
    }
}

// attributes can be defined as before
Rotator.attributes.add('speed', { type: 'number', default: 42 });

You can use this notation in Editor already, by registering the script (before attributes):

class Rotator extends pc.ScriptType {
    static name = 'rotator';

    update(dt) {
        this.entity.rotate(dt * this.speed, 0, 0);
    }
}

pc.registerScript(Rotator);

Rotator.attributes.add('speed', { type: 'number', default: 42 });

You can define multiple scripts per file and import them as needed:

export class Rotator extends pc.ScriptType {
    // ...
}
export class Translator extends pc.ScriptType {
    // ...
}

Currently pc.createScript automatically adds it to the script registry.
So module scripts when loaded, should be added to the script registry:

import { Rotator } from './scripts/rotator.mjs';

// ...
pc.registerScript(Rotator);

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[kungfooman]

Improves ScriptType to be extendible by class, while preserving current scripting features (attributes, events, components).
This allows you to write scripts as module javascript files.

Maybe I understand wrong, what is new about that? @aidinabedi implemented that and showcased it as example in his 2020 PR: #1908

I also use bundler-free ESM in combination with pc.ScriptType without any problems: https://github.com/kungfooman/playcanvas-test-es/blob/master/es6/rotate.js

Anyway, what I like is established and introducing the static name for registering script's - allowing bundlers/minifiers to change all class names, but sticking to this custom name 👍

[Maksims]

Maybe I understand wrong, what is new about that? @aidinabedi implemented that and showcased it as example in his 2020 PR: #1908

Unfortunately that PR has not been merged.
This PR will implement mjs workflow, which is not part of other PR. Classes actually are not strictly necessary for mjs, but it is most non-modern part that also have some specifics in the way it should work with mjs. Especially for Editor. Its more complex than it looks.

[Maksims]

Updated slightly description.
I've done more testing, and it is usable in Editor already:

class Rotator extends pc.ScriptType {
    static name = 'rotator';

    update(dt) {
        this.entity.rotate(dt * this.speed, 0, 0);
    }
}

pc.registerScript(Rotator);

Rotator.attributes.add('speed', { type: 'number', default: 42 });

Attributes are parsing as expected as well.

[mvaligursky]

This looks nice, and seems like a worthwhile improvement to the existing script system. Any thoughts @willeastcott @slimbuck @marklundin

It does not go as far as #5764 is going with the new scripting system, but it's a nice improvement for existing system, and I think both should be considered for the inclusion in the engine.

The ESM Script PR is aiming at engine being fully imported by the module scripts (for example like https://github.com/playcanvas/model-viewer), allowing unused parts of the engine to be three-shaken out during a bundling step (follow up PRs)

[willeastcott]

@Maksims Presumably you didn't mean to include the update to package-lock.json. Can you back that out, please?

[willeastcott]

Why use ?. here? Isn't the script component guaranteed to be present?

[marklundin]

One thing; the Class.name can be mangled in bundling and shouldn't be relied upon to be stable.

Mozilla: Minifiers and bundling name prop

Ideally we don't rely on name. Can __name be a required member instead of relying on the inferred name? Does this have any implication for existing scripts?

[Maksims]

Minifier will be impactful when static name is not set on a class.
Basically there are multiple ways to specify a name for a script:

Current:

let Test = pc.createScript('test');

Using a static name property:

class Test extends pc.ScriptType {
    static name = 'test';
}

Using pc.registerScript:

class Test extends pc.ScriptType {
    
}
pc.registerScript(Test, 'test');

And this also works even right now, but not pretty:

class Test extends pc.ScriptType {
    static __name = 'test';
}

We could define a different name for the static property to avoid collision with built-in property name on the prototype. E.g. scriptName.

[marklundin]

I think you can check if the name property was explicitly defined as a static member rather than inferred using getOwnPropertyDescriptor, which could be useful, but would need to verify. I wonder if for ES Modules, which are more likely to be minified/mangled, that we require/enforce that a name or similar is specifically defined 🤔

[marklundin]

This PR seems to break the swap functionality somehow. In the following the swap method doesn't get called.

class Test extends pc.ScriptType {
    swap(old){
        console.log('does not call')
    }
}
pc.registerScript(Test, 'test')

[marklundin]

This may be related to the above, but I tried a swap in the current build, and it throws Uncaught SyntaxError: Identifier 'Test' has already been declared, I assume because the class has already been declared and behaves like a const or let. I could make it work using a var, but syntactically it's a bit strange

var Test = class extends pc.ScriptType {
    initialize(){}
    update(){}
}

Is there maybe a better way of doing this?

[Maksims]

This may be related to the above, but I tried a swap in the current build, and it throws Uncaught SyntaxError: Identifier 'Test' has already been declared, I assume because the class has already been declared and behaves like a const or let. I could make it work using a var, but syntactically it's a bit strange

var Test = class extends pc.ScriptType {
    initialize(){}
    update(){}
}

Is there maybe a better way of doing this?

Yes, they are related. Unfortunately as scripts are executed on load in global context, const collisions (class collisions are same) are happening. var Test = class extends pc.ScriptType { - does works around it, but you are right, it does not look slick.

With ESM modules, this won't be a problem, as when you import script it wont collide.

As swap is a power-user feature, it can be well documented on how to write scripts for hot-swapping. I'm afraid there might be no pretty easy workaround here.

[marklundin]

One thing that feels a little strange, when you define class attribute properties it's not immediately clear what the default would be:

class MyClass { speed = 2 }
MyClass.attributes.add('speed', { default: 1 })

Ideally, we ignore the default in the the attributes and just depend upon the class prop.

[mvaligursky]

I'd vote for the attribute (which is processed later) to override the variable value on the class.

[marklundin]

It would be good if default was optional

[Maksims]

I've looked into issue of conflicting attributes with class properties. Because attributes are declared as a prototype accessors after class definition, but properties are not defined as accessors (get/set), there is no way in javascript to detect if a property exists on a class definition.
The only thing we can detect is the accessors (get/set methods) from a class definition:

class Test {
    foo = 42;

    set bar(value) {
        this.foo = value;
    }
    get buz() {
        return this.foo;
    }
}

Test.prototype.hasOwnProperty('foo') === false;
Test.prototype.hasOwnProperty('bar') === true;
Test.prototype.hasOwnProperty('buz') === true;

So there are a few things we can do:

1. Introduce new property for the attributes, like "predefined: true", so it will not override the accessor, and use its value as a default one.
2. In Editor, during parsing, we can instantiate a class instance, and check its properties, so we can mark attributes as "predefined" automatically.
3. Predefined attributes - means users won't have events when attribute is set and can handle their values set/get and copying if needed.

I'm afraid there is no easy solution to this here.

There is also a problem for users, that we handle all parsing of raw data, in async way for attributes. If we encourage them to use their own accessors, they won't have automatic data handling. And will require their own parsers.

[marklundin]

Hey @Maksims, sorry for taking so long on this. Just reviewing now, is this PR still required? it seems like the name property already works if it's explicitly defined or inferred via the constructor/function name. I may be missing something though.

[Maksims]

Hey @Maksims, sorry for taking so long on this. Just reviewing now, is this PR still required? it seems like the name property already works if it's explicitly defined or inferred via the constructor/function name. I may be missing something though.

If current implementation already works well with the name, and even if name is not provided (defaults to name of a class), then yep, its fine to close it.