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

Wrong arguments passed for blackbox prefab based on NinePatch #148

Closed jrabek closed 3 years ago

jrabek commented 3 years ago

Description

When creating a black box prefab based on a NinePatch object in order to add custom fields to the prefab, it seems the arguments passed to the prefab constructor are incorrect.

Here is what the init looks like for a black box prefab that has a type in the prefab scene file of NinePatch.

        // scaledTextBox
        const scaledTextBox = new ScaledTextBox(this, 123, 124, 123, 124, undefined, undefined, "button0004");
        this.add.existing(scaledTextBox);

Here is a NinePatch created as just a NinePatch (no prefab involved).

        // ninepatch
        const ninepatch = this.add.ninePatch(674, 478, 373, 216, "button0004");
        ninepatch.setOrigin(0, 0);

In the case of the prefab the x,y coordinates are repeated twice and there are two unknown undefined parameters and then the texture key. The width and height are not passed in.

jrabek commented 3 years ago

@PhaserEditor2D I made a small change (which I am not sure is the completely correct way to do it) but seems to resolve the issue:

                buildCreatePrefabInstanceCodeDOM(args) {
                    const obj = args.obj;
                    const support = obj.getEditorSupport();
                    const call = args.methodCallDOM;
                    call.arg(args.sceneExpr);
                    call.argFloat(obj.x);
                    call.argFloat(obj.y);
                    call.argFloat(obj.width);
                    call.argFloat(obj.height);
                    // this.buildCreatePrefabInstanceCodeDOM_XY_Arguments(args);
                    // this.buildCreatePrefabInstanceCodeDOM_Size_Arguments(args);
                    if (support.isUnlockedProperty(sceneobjects.TextureComponent.texture)) {
                        this.addTextureFrameArgsToObjectFactoryMethodCallDOM(args.methodCallDOM, obj);
                    }
                }

Original Typescript: https://github.com/PhaserEditor2D/PhaserEditor2D-v3-extras/blob/main/source/plugins/phasereditor2d.extras.ninepatch/src/NinePatchCodeDOMBuilder.ts#L22

PhaserEditor2D commented 3 years ago

Hi @jrabek

The workaround should be:

buildCreatePrefabInstanceCodeDOM(args) {
                    const obj = args.obj;
                    const support = obj.getEditorSupport();
                    const call = args.methodCallDOM;
                    call.arg(args.sceneExpr);
                    //call.argFloat(obj.x);
                    //call.argFloat(obj.y);
                    //call.argFloat(obj.width);
                    //call.argFloat(obj.height);
                    this.buildCreatePrefabInstanceCodeDOM_XY_Arguments(args);
                    this.buildCreatePrefabInstanceCodeDOM_Size_Arguments(args);
                    if (support.isUnlockedProperty(sceneobjects.TextureComponent.texture)) {
                        this.addTextureFrameArgsToObjectFactoryMethodCallDOM(args.methodCallDOM, obj);
                    }
                }

But even the call to this.buildCreatePrefabInstanceCodeDOM_Size_Arguments(args); is also a bug, because the ninepatch object uses the ninePatchWidth andninePatchHeight properties instead of the width and height.

However, I'm not going to fix these bugs right now. First, I want to remove the ninePatchWidth/Height properties and use only the width/height.

I used the custom ninePatchWidth/Height properties for redrawing the object each time the size is changed. But it is too expensive, it redraws for the width, and later redraw for the height. So, I'm not going to redraw the ninepatch each time a property changes else the user has to call the redraw at the moment he thinks all changes are set. By default, it will redraw the object always at the first scene update.

I will make it right now so I hope to have these changes ready soon.

jrabek commented 3 years ago

@PhaserEditor2D Ah perfect. Yeah I have only recently been digging into the Phaser Editor code more deeply so I wasn't completely sure!

And your fix makes total sense. I actually was curious why a new width and height property was needed but what you are saying makes sense.

When building the auto scaling text box since scaling the text is expensive, I use the dirty flag approach and only do any sort of update on the next update call all at once to avoid expensive recalculations / redraws.

Thanks for the quick turn!

PhaserEditor2D commented 3 years ago

Good. Yes, I should do something similar to the dirty flag in the editor implementation of the ninepatch object. In the ninepatch class provided to the user, maybe it is a strategy the user can implement himself. I think the code is pretty straight forward and you can change the internals. Only the public interface should remain the same.

PhaserEditor2D commented 3 years ago

Done.

Commits:

The commit includes other changes:

You should import again the NinePatch user files into the project. These changes also require to work with the latest version of the editor in the develop branch.

jrabek commented 3 years ago

Perfect! Will check it out as soon as I am done with my current set of changes.

jrabek commented 3 years ago

Oh, one thing I noticed in the NinePatch code: Should the public methods for the NinePatch declare return types? I had seen you had added that in other places.

PhaserEditor2D commented 3 years ago

Oh, one thing I noticed in the NinePatch code: Should the public methods for the NinePatch declare return types? I had seen you had added that in other places.

Do you mean in the phaser editor code? I think mostly I don't use it, unless I'm writing an interface or abstract method. But that's possible I have different coding styles. Practically phaser editor was the first thing I coded in typescript, so it is a continuous learning :)

jrabek commented 3 years ago

Ah I meant in the NinePatch.ts file that has to be in the project for the NinePatch to work at run time.

For example get marginBottom() would be get marginBottom(): number.

jrabek commented 3 years ago

It honestly depends on the linting rules that are setup. I had just noticed you had added void recently to editorCreate(): void so I was assuming you were adding return types to all generated code.

PhaserEditor2D commented 3 years ago

Yes, I added return type to the generated code because someone asked about it. Probably you were who asked about it :)

So I think it is better if I add it to the ninepatch code so it keeps consistent. Can you open an issue about it, please?

jrabek commented 3 years ago

haha entirely possible!

I opened it here https://github.com/PhaserEditor2D/PhaserEditor2D-v3-extras/issues/1 since it is in the other repo.