melonjs / melonJS

a fresh, modern & lightweight HTML5 game engine
https://melonjs.org
MIT License
5.92k stars 643 forks source link

Resizing a NineSliceSprite after creation does not work properly when using Animated sprite #1115

Open NemoStein opened 2 years ago

NemoStein commented 2 years ago

OS platform / Browser

Windows 10, Chrome 105

melonJS version

v13.0.0

Bug description

The NineSliceSprite internal properties nss_width and nss_height never gets updated, making it impossible to change the NineSliceSprite size.

Setting the (undocumented) properties nss_width/nss_height instead of width/height seems to work, even if the sprite is animated.

I tried to patch the NineSliceSprite class with something like this:

get width() { return super.width; }
set width(value) {
    super.width = value
    this.nss_width = value;
}

But every frame calls the Sprite.setRegion method, which resets the width/height of the sprite

I think the easiest (but far from ideal) approach is a new method NineSliceSprite.changeSize(w, h) (or something like this) NineSliceSprite.resize can be used, but resize() is already taken by Rect.
Furthermore, it would create even more confusion when trying to set the width/height properties directly (why resize() works but width/height doesn't?)

The ideal solution is to properly fix the get/set width/height behaviour.

Steps to reproduce the bug

// Create NineSliceSprite with size 32x32
const sprite = new NineSliceSprite(0, 0, {
  image: 'nineslice',
  width: 32,
  height: 32
})

// Change width/height
sprite.width = 128

// NineSliceSprite don't change size
game.world.addChild(sprite)
obiot commented 2 years ago

oh boy, I could swear this one was working ! I'll fix it anyway and add a corresponding test unit to prevent breaking it again if this is/was a regression. Thanks for reporting it 👍

obiot commented 2 years ago

actually just setting the private nss_width and nss_height to the proper value works, you can do that for now, and I'll see how to properly fix this, but I agree, most certainly through a setter/getter.

obiot commented 2 years ago

@NemoStein

adding the following getter/setter to the NineSliceSprite class seems to do the trick on my side :

    /**
     * width of the NineSliceSprite
     * @public
     * @type {number}
     * @name width
     */
    get width() {
        return super.width;
    }
    set width(value) {
        super.width = this.nss_width = value;
    }

    /**
     * height of the NineSliceSprite
     * @public
     * @type {number}
     * @name height
     */
    get height() {
        return super.height;
    }
    set height(value) {
        super.height = this.nss_height = value;
    }

however I do not have any animated example for dialog box, so if you could also kindly confirm it's working on your side it would be great ! if it works I'll push those changes to the master branch, if not can you maybe share a minimal example here I can reuse for debugging?

obiot commented 2 years ago

I committed the changes, maybe easier to pull the latest changes on your side : https://github.com/melonjs/melonJS/commit/b0ee2b274e1e87fc10fad3772e4494383e4ecd10

let me know, once you confirm you are good with it I'll publish the 13.1 version.

thanks again!

NemoStein commented 2 years ago

Sadly the fix wasn't enough. Everytime the Sprite.setRegion() is called the sprite width/height is reset to the sprite original frame width/height https://github.com/melonjs/melonJS/blob/709d217e35e3bb6cce0d1debbac14a4b4ff793b4/src/renderable/sprite.js#L460-L461

Here is a sample that reproduces the issue: https://github.com/NemoStein/melonjs-9slice-sample

obiot commented 2 years ago

oh I see...... honestly I don't know how to fix this one quickly... the thing is that the sprite resizing/scaling feature rely on Matrix transformation to display the final sprite "size" and if you look at the Sprite Class code we actually never change the size of the sprite. So I really need to think about this one...

a quick and dirty fix is certainly to override the update method, call the

update(dt) {
    // .... do your stuff
   var dirty = super.update(dt);
   if (dirty) {
        // most likely the sprite frame was changed
        // Change width/height
       sprite.width = sprite.nss_width = 128;
   }
   return dirty;
}

I'm afraid I have nothing better to offer for now, as I know you are on a tight schedule with the game jam !

NemoStein commented 2 years ago

Don't worry, I'll keep using the nss_* workaround for now...

It's just a progress loading bar, after all! ;D

obiot commented 2 years ago

ok, I'll keep it like this for now and go on with the 13.1 release. I will see later how we can properly implement this.

I must admit though I never thought about using a 9-slice sprite for a loading bar :)

obiot commented 2 years ago

13.1.0 was just released ! Followed by a quick 13.1.1 fix but just for date and change tracking :)