PhaserEditor2D / PhaserEditor2D-v3

A web-based IDE for HTML5 game development. Powered by Phaser.
https://phasereditor2d.com
MIT License
432 stars 58 forks source link

Prefab properties as setters and getters #132

Closed jrabek closed 3 years ago

jrabek commented 3 years ago

Currently in order to initialize a prefab property at init it is necessary to create an event handler per https://help.phasereditor2d.com/v3/scene-editor/prefab-user-properties.html#initializing-other-properties.

The downside to this approach is that it both requires an additional event handler to be setup, and if the property is changed later in code then the change won't be reflected in the UI without the variable being checked periodically (e.g. in update).

To simplify the code and improve the usability, a getter and a setter should be created instead so the implementer can decide if a new variable is needed or if just a property on a Phaser object inside the prefab needs to be updated (e.g. the text on a button when updating the text property of the prefab).

So instead of

     this.scene.events.once(Phaser.Scenes.Events.UPDATE, this.start, this);

      // button (prefab fields)
      this.text = "Let's Go!";

     start() {
        this.textObject && (this.textObject.text = this.text)
    }

It can just be

    public set text(value: string) {
        /* START-USER-CODE */ 
        this.textObject && (this.textObject.text = value)
        /* END-USER-CODE */
    }
jrabek commented 3 years ago

Note that a getter should also be created.

PhaserEditor2D commented 3 years ago

Good, I agree.

But I'm not convinced that generating a getter/setter is a good idea. It will generate something like this:

    public set text(value: string) {
        /* START-USER-CODE-text */ 
        this.textObject && (this.textObject.text = value)
        /* END-USER-CODE-text */
    }

Note the comment says START-USER-CODE-text, because text is the name of the property. The compiler needs to know what's the user code of the property, so when it overrides the file it keeps that code.

The problem is if the user changes the name of the property, then the code of the getter/setter is overridden and is lost.

Maybe, it would be better to delegate to the user the writing of the getter/setter. It also allows the user to create different patterns, like extending a common class that contains those properties. So, when you create a property, you can change it to be Virtual, and the compiler will not generate the code of the property. Only the "setting" code will be generated, where the object is created in the scene. What do you think?

PhaserEditor2D commented 3 years ago

By the way, what's your OS?

jrabek commented 3 years ago

By the way, what's your OS?

I'm on a Mac (Big Sur).

jrabek commented 3 years ago

Regarding the codegen. I haven't through through all the issues.

It seems the editor would know when the property name changes and what the value was before so it could update START-USER-CODE-text to START-USER-CODE-newText when the property name changes along with the setter signature.

That said, I think your idea is a good compromise and less error prone for codegen. The editor can generate something like the following for a prefab called "Button"

interface ButtonProperties {
  text: string
}

And then in Button.ts:

class Button extends ButtonProperties {
...
    public set text(value: string) {        
        this.textObject && (this.textObject.text = value)
    }
...
}
PhaserEditor2D commented 3 years ago

Hi!

Just did it. You can try checking the Custom Definition field. When that field is enabled, the compiler doesn't generate the definition of the field. However, it generates the initialization of the field in the constructor:

image


// You can write more code here

/* START OF COMPILED CODE */

class DinoPrefab extends Phaser.GameObjects.Image {

    constructor(scene: Phaser.Scene, x: number, y: number, texture?: string, frame?: number | string) {
        super(scene, x, y, texture || "dino", frame);

        this.rotated = false;

        /* START-USER-CTR-CODE */
        // Write your code here.
        /* END-USER-CTR-CODE */
    }

    /* START-USER-CODE */

    set rotated(value: boolean) {
        this.angle = value? 45 : 0;
    }

    /* END-USER-CODE */
}

/* END OF COMPILED CODE */

// You can write more code here

You can try it with this build:

PhaserEditor2D-3.14.0-next-macos.zip (updated, the same as below)

By the way, the idea of generating an interface is interesting. It forces the user to write the getters/setters. However, if the field is initialized in the constructor, it also forces the user to define a setter. A getter is optional. What do you think?

PhaserEditor2D commented 3 years ago

New version here: it sets Custom Definition false by default: PhaserEditor2D-3.14.0-next-macos.zip

jrabek commented 3 years ago

Thanks! I will download the updated version and try it out.

jrabek commented 3 years ago

Regarding your question of using an interface: The current approach of setting the variable but not defining it does force at least the setter to be created. It would be a similar result to an interface in terms of convenience. Either solution would work for typescript.

It seems like the use cases for a prefab property are:

1) Mapping a prefab property to the property of one of the objects inside the prefab. Example: A button with an image and a text object where the text property of the prefab maps to the text property of the text object.

2) A behavior variable that is evaluated asynchronously (i.e. not when it is set) during the init or update phase. Example: A canFireLaser on an enemy prefab that is evaluated during the AI logic.

While it does seem setter generation would cover both cases, I do agree that clearing the "custom definition" flag accidentally should not delete any code. So the current solution you did is probably good enough for now.

PhaserEditor2D commented 3 years ago

I agree. Let's keep it that way for the moment. If we find something better, I implement it.

jrabek commented 3 years ago

By the way: I did download the change you made and it looks good! Nice to be able to handle the two cases above without any hacks/extra code.

PhaserEditor2D commented 3 years ago

Good. Yet I have to implement the prefab's awake event.

jrabek commented 3 years ago

@PhaserEditor2D I noticed a bug in this build. If you set the visibility of the created property to "class" instead of "method" the class property does not get | undefined added so typescript complains that the variable is not initialized since the text object is created in create and not the constructor.

Seems the alternative would be to add the objects in the constructor which phaser editor also seems to do(?).

PhaserEditor2D commented 3 years ago

Thanks @jrabek !

I will take a look.

PhaserEditor2D commented 3 years ago

The solution is to declare the property as type|undefined. It is a regression.

jrabek commented 3 years ago

Thanks! It is unfortunate that GameObjects (e.g. Phaser.GameObjects.Container) allow for objects to be added to the scene in the constructor but for Scenes you have to do it in create.

This means that all class level properties always have to be checked for undefined since they aren't defined in the constructor leading to a lot of extra, unneeded code to be written.

For example I have a splash page scene with a button and the button logically should always be defined but since it isn't defined in the constructor I have to do something like this every time I assign a property since typescript doesn't allow for optional property on the left hand of an assignment (i.e. this.button?.onClicked =)

    create() {
        this.editorCreate();
        if (this.button) {
            this.button.onClicked = () => this.letsGo;
        }
    }
PhaserEditor2D commented 3 years ago

Yes, Phaser uses the preload, create, and update methods. You can create objects in the preload method, but most of the time you should do it in the create method because first all assets should be loaded, and the scene should be set up. There is another method, init, that is called before starting a scene. Probably, creating the objects in the scene's constructor is not a good idea, because of the way the Phaser lifecycle works. A custom container is a different case, the children are created in the container constructor, but the container itself is created in the create method.

Maybe in Phaser 4 things are different. I think creating a game object don't require the scene.

Regardging typescript, I guess you can disable the strictNullCheck flag in the tsconfig.json file of your project or a sub-folder of your project.

PhaserEditor2D commented 3 years ago

Fixed the issue with the undefined declaration. This build also includes the prefab-awake event:

PhaserEditor2D-3.14.0-next-macos.zip

(You should disable the Networks cache the first time you open the editor)

jrabek commented 3 years ago

Awesome! I'll download it and try it out.

jrabek commented 3 years ago

So the undefine works well!

However I did notice a codegen code ordering issue within the prefab constructor: To illustrate:

1) Select any GameObject contained within a prefab and set its scope level to "class" so that a this.someImage = someImage within the constructor.

2) Create a property on the prefab like imageTintColor so that this.imageTintColor = #000000 is also created in the constructor.

3) The code in the // custom definition props has the properties set first then the gameObject assignments to this (class level visibility)

The problem now is that the imageTintColor won't be applied to the image since it isn't set yet.

It seems the order would always be setting the GameObjects in the prefab and then assigning properties since some of the properties most likely apply to the GameObjects in the prefab.

This avoids the extra step of checking in an update or start function and makes sure the GameObjects have more deterministic initial values since GameObjects can start paused (I believe?) in which case update won't be called.

PhaserEditor2D commented 3 years ago

Hi @jrabek

I agree. I changed the order: https://github.com/PhaserEditor2D/PhaserEditor2D-v3/commit/9ad60b78d008df0c972c0c08296e1e6410cb333c

Let me upload a new release. (I think I'm needing to setup a build server)

PhaserEditor2D commented 3 years ago

Uploaded! PhaserEditor2D-3.14.0-next-macos.zip

jrabek commented 3 years ago

Closing since this seems to be working as expected now. Will file a new issue if something comes up.

PhaserEditor2D commented 3 years ago

Thanks.