PhaserEditor2D / PhaserEditor2D-v3

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

Tint values are set when prefab is used in scene even though they aren't overridden #153

Closed jrabek closed 3 years ago

jrabek commented 3 years ago

Description

I have a button prefab that extends a text box prefab and overrides the tint values. If I use this button prefab in a scene, the generated code in the scene is also setting the tint values even though they are not overridden.

Note that the margins and textures are also overridden but those values aren't repeated. Just seems to be the tint?

Button.ts

export default class Button extends ScaledTextBox {

    constructor(scene: Phaser.Scene, x?: number, y?: number) {
        super(scene, x, x);

        this.ninepatch.setTexture("button");
        this.ninepatch.tintTopLeft = 16742575;
        this.ninepatch.tintTopRight = 16742575;
        this.ninepatch.tintBottomLeft = 16742575;
        this.ninepatch.tintBottomRight = 16742575;
        this.ninepatch.ninePatchWidth = 472;
        this.ninepatch.ninePatchHeight = 241;
        this.ninepatch.marginLeft = 32;
        this.ninepatch.marginTop = 36;
        this.ninepatch.marginRight = 34;
        this.ninepatch.marginBottom = 36;
        this.defaultText.setPosition(29, 68);
        this.defaultText.setStyle({"fontSize":"96px"});

Splash.ts

        // letGoButton
        const letGoButton = new Button(this, 530, 573);
        this.add.existing(letGoButton);
        letGoButton.defaultText.tintTopLeft = 16742575;
        letGoButton.defaultText.tintTopRight = 16742575;
        letGoButton.defaultText.tintBottomLeft = 16742575;
        letGoButton.defaultText.tintBottomRight = 16742575;
        letGoButton.defaultText.setPosition(61, 57);
        letGoButton.defaultText.text = "Let's Go!";
        letGoButton.defaultText.setStyle({"fontSize":"120px"});
jrabek commented 3 years ago

And here is the corresponding section of the Splash.scene file.

 {
            "prefabId": "d88a6292-163e-4e2e-a5ee-7bb095332c65",
            "id": "8dcbc133-b4bf-4b92-bb72-da03bd844f36",
            "unlock": [
                "x",
                "y"
            ],
            "label": "letGoButton",
            "scope": "CLASS",
            "components": [],
            "x": 530,
            "y": 573,
            "nestedPrefabs": [
                {
                    "prefabId": "688f022d-618a-46b6-aaf7-28ac66c8cf6f",
                    "id": "99ccce0b-2b69-47fc-b016-daa96c282a07",
                    "unlock": [
                        "fontSize",
                        "text"
                    ],
                    "label": "defaultText",
                    "components": []
                },
                {
                    "prefabId": "3956c367-67e5-48c9-8f81-5266f5b0b202",
                    "id": "99ccce0b-2b69-47fc-b016-daa96c282a07",
                    "unlock": [
                        "fontSize",
                        "x",
                        "y",
                        "text"
                    ],
                    "label": "defaultText",
                    "components": [],
                    "x": 61,
                    "y": 57,
                    "text": "Let's Go!",
                    "fontSize": "120px"
                }
            ]
        }
PhaserEditor2D commented 3 years ago

I think this is solved. If you experience the issue again please re-open this.

jrabek commented 3 years ago

I am still seeing this happen after pulling the latest develop and using the latest nine patch plugin.

Base Class:

/* START OF COMPILED CODE */

export default class Button extends ScaledTextBox {

    constructor(scene: Phaser.Scene, x?: number, y?: number) {
        super(scene, x ?? 0, y ?? 0);

        this.ninepatch.setTexture("button");
        this.ninepatch.setOrigin(0.5, 0.5);
        this.ninepatch.tintTopLeft = 16742575;
        this.ninepatch.tintTopRight = 16742575;
        this.ninepatch.tintBottomLeft = 16742575;
        this.ninepatch.tintBottomRight = 16742575;

Editor created instance of the button prefab:

        const letsGoButton = new Button(this, 770, 702);
        this.add.existing(letsGoButton);
        letsGoButton.ninepatch.tintTopLeft = 16742575;
        letsGoButton.ninepatch.tintTopRight = 16742575;
        letsGoButton.ninepatch.tintBottomLeft = 16742575;
        letsGoButton.ninepatch.tintBottomRight = 16742575;

I recompiled the entire project to be sure.

jrabek commented 3 years ago

@PhaserEditor2D I can't seem to reopen the issue?

jrabek commented 3 years ago

Note the tint is not unlocked in the scene file:

{
            "prefabId": "d88a6292-163e-4e2e-a5ee-7bb095332c65",
            "id": "46d9c4eb-4b08-4ccf-9e5f-f81f3f24e4fe",
            "unlock": [
                "x",
                "y"
            ],
            "label": "letsGoButton",
            "scope": "CLASS",
            "components": [],
            "x": 770,
            "y": 702,
            "nestedPrefabs": [
                {
                    "prefabId": "74c11041-88c3-45a2-9280-c9a2ea37ea79",
                    "id": "adb44fad-34d5-48d3-8536-23fcfdd2ef24",
                    "unlock": [
                        "width",
                        "height"
                    ],
                    "label": "ninepatch",
                    "components": [],
                    "width": 385,
                    "height": 248
                },
                {
                    "prefabId": "8589b4a0-281f-4aa8-8920-46c7e05ac47c",
                    "id": "565b6662-7629-4c04-8913-5043e6828125",
                    "unlock": [
                        "originX",
                        "originY",
                        "text",
                        "fontSize"
                    ],
                    "label": "staticTextObject",
                    "components": [],
                    "text": "Let's Go!",
                    "fontSize": "96px"
                }
            ]
        }
jrabek commented 3 years ago

I also noticed this is happening for the texture property of the ninepatch and the setting of the origin for the text.

In this case I have a derived class, SpeechLowerRightScaledTextBox, that specifies the texture and margins for this particular type of ScaledTextBox.

export default class SpeechLowerRightScaledTextBox extends ScaledTextBox {

    constructor(scene: Phaser.Scene, x?: number, y?: number) {
        super(scene, x ?? 0, y ?? 0);

        this.ninepatch.setTexture("speech_bubble0001");
        this.ninepatch.marginLeft = 33;
        this.ninepatch.marginTop = 31;
        this.ninepatch.marginRight = 70;
        this.ninepatch.marginBottom = 54;

I then have a SpeechButton which extends that:

export default class SpeechButton extends SpeechLowerRightScaledTextBox {

    constructor(scene: Phaser.Scene, x?: number, y?: number) {
        super(scene, x, y);

        this.ninepatch.setOrigin(0.5, 0.5);
        this.ninepatch.setSize(400, 252);
        this.ninepatch.updateDisplayOrigin();
        this.staticTextObject.setOrigin(0.5, 0.5);

But when I use instances of the SpeechButton some subset of the properties get set even though they aren't unlocked or overridden. In this case it is the texture and the origin.

        const answerButton2 = new SpeechButton(this, 300, 562);
        this.add.existing(answerButton2);
        answerButton2.ninepatch.setTexture("speech_bubble0001");
        answerButton2.staticTextObject.setOrigin(0.5, 0.5);
jrabek commented 3 years ago

It doesn't cause visual issues in the final game but when reading the code it makes it really unclear where properties should be set and are getting set.

jrabek commented 3 years ago

Ah, so this may be a hint: I noticed in the scene file that there are properties that are unlocked but then values aren't specified for them.

            "nestedPrefabs": [
                {
                    "prefabId": "ffc0cdec-6455-4f2d-9f74-00e841297a68",
                    "id": "899748e4-e978-4c7e-83c0-4906bf4ea51c",
                    "unlock": [
                        "width",
                        "height",
                        "originX",
                        "originY"
                    ],
                    "label": "ninepatch",
                    "components": [],
                    "width": 400,
                    "height": 252
                },
                {
                    "prefabId": "9cf03430-78c3-4d9e-b551-cc3eba84347d",
                    "id": "3ea974e8-6461-4944-9c77-e33e2be54281",
                    "unlock": [
                        "originX",
                        "originY"
                    ],
                    "label": "staticTextObject",
                    "components": []
                }
            ]

It may be something weird where ScaledTextBox has the origin as 0,0 which I don't think is the Phaser default. So in the ScaledTextBox scene file I see that originX and originY get set even though they aren't unlocked.

In ScaledTextBox scene file:

                {
                    "type": "NinePatch",
                    "id": "2879eac0-1f0e-49de-8932-c126e520dcb6",
                    "label": "ninepatch",
                    "scope": "NESTED_PREFAB",
                    "components": [],
                    "texture": {},
                    "originX": 0,
                    "originY": 0,
                    "width": 313,
                    "height": 203
                },

Then in the leaf child class SpeechButton, its scene file has the originX and originY as unlocked without specifying a value which may fall back to some internal default to match the default behavior of Phaser?

jrabek commented 3 years ago

So there actually is a bug that would result in the incorrect display: If I unlock the staticTextObject's origin and set it to 0, 0 then the code to set the unlocked origin is not emitted.

Basically unlocking and setting to 0,0 causes this not to appear:

answerButton1.staticTextObject.setOrigin(0.5, 0.5);

The problem is that the origin in SpeechButton is actually 0.5, 0.5 and so the override won't be applied correctly.

/* START OF COMPILED CODE */

export default class SpeechButton extends SpeechLowerRightScaledTextBox {

    constructor(scene: Phaser.Scene, x?: number, y?: number) {
        super(scene, x, y);

        this.ninepatch.setOrigin(0.5, 0.5);
PhaserEditor2D commented 3 years ago

Hi!

Thanks for the detailed description. I will take a look.

PhaserEditor2D commented 3 years ago

I reproduced the bug with the tint values, but I cannot reproduce the bugs of the origin and the textures.

The problem with the tint properties is that for the editor UI they have a string color value, but for the compiler, they have a number value. So at certain points of the code, there were comparisons with strings and numbers. I fixed it.

The idea behind serialization and code generation is: properties are included in code and .scene files when the value is different from the default value. In a common object, the default value is the one defined in Phaser. In a prefab instance, the default value is the one defined in the prefab.

If a property of a prefab instance is unlocked, but its value is not different from the default value, then it is not included in the generated code and the .scene file. Unlocking a property doesn't mean "forcing it to write it down", it means, it is mutable.

Please, if you can give me a very small project just to reproduce the issue with the texture and the origin it would be great.

Regarding the origin. There are objects in Phaser with default origin in 0,0 (like the Text object) and objects with default origin in 0.5, 0.5. However, for the editor, all objects have origin in 0.5, 0.5.

When a text object is serialized, if the origin is different from 0.5, then it will be included. However, when the text object is generated to code, the origin component performs a tweak and checks if the object is a text or a bitmaptext, and uses 0 as the default origin. This is something I would like to change because is hardcoded and not friendly with extensions, but it requires a huge refactoring on how default values are computed.

https://github.com/PhaserEditor2D/PhaserEditor2D-v3/blob/d8bb9530469472c41ca1cd454e6284e8fb494401/source/editor/plugins/phasereditor2d.scene/src/ui/sceneobjects/object/OriginComponent.ts#L48

jrabek commented 3 years ago

Please, if you can give me a very small project just to reproduce the issue with the texture and the origin it would be great.

I'll send the project to you on Discord so you can check out what I am seeing above.

When a text object is serialized, if the origin is different from 0.5, then it will be included. However, when the text object is generated to code, the origin component performs a tweak and checks if the object is a text or a bitmaptext, and uses 0 as the default origin.

So is the class / prefab hierarchy considered when this check is made? It seems like for the code for an instance of a prefab with nested prefabs to be emitted property, the properties of each prefab (unlocked and overridden, or just the default values) in the hierarchy would need to be applied in order starting from the base/original prefab.

If just the properties of the instance of the prefab is considered (and not the hierarchy) then you can end up in a situation like I mentioned in https://github.com/PhaserEditor2D/PhaserEditor2D-v3/issues/153#issuecomment-952104400 where the instance of the prefab will have the default values of the property (origin being 0,0) but the parent prefab is actually overriding them (origin being 0.5, 0.5) so when running the game the origin becomes (0.5, 0.5) instead of being (0, 0) as expected.

PhaserEditor2D commented 3 years ago

Thanks for sending the project.

Yes, the hierarchy is considered in applying values.