Skip to content
This repository has been archived by the owner on Nov 20, 2024. It is now read-only.

fix: textures change was not take into account #31

Closed
wants to merge 2 commits into from

Conversation

davidetan
Copy link
Contributor

In Spine, during an animation, it can happen that a texture is changed for the same attachment (this happens for sequences).

A texture change should lead to an addRenderable call. However, the Spine validateAttachments doesn't take into consideration the texture change and does not set the spineAttachmentsDirty to true in that case.

This PR allowsto take this into account and provide a way to set spineAttachmentsDirty to true.
However, it does not do that in the validateAttachments.

The only way to do that would be:

  • change the order of validateAttachments and transformAttachments
  • store somewhere that a texture changed

I'll keep this as a draft since I didn't think yet how to move the "texture validation" within the validateAttachments function.

This PR fixes this: #26

@davidetan
Copy link
Contributor Author

davidetan commented Aug 1, 2024

I've just notice the existence of checkAndUpdateTexture.
I guess that one should be used in SpinePipe as in MeshPine and the others.


The alternative solution I found considering the usage of checkAndUpdateTexture is the adding the logic on validateRenderable like this:

validateRenderable(spine: Spine): boolean
{
    spine._applyState();
    // loop through and see if the mesh lengths have changed..

    const gpuSpine = this.gpuSpineData[spine.uid];
    const drawOrder = spine.skeleton.drawOrder;

    for (let i = 0, n = drawOrder.length; i < n; i++)
    {
        const slot = drawOrder[i];
        const attachment = slot.getAttachment();

        if (attachment instanceof RegionAttachment || attachment instanceof MeshAttachment)
        {
            const cacheData = spine._getCachedData(slot, attachment);
            const batchableSpineSlot = gpuSpine.slotBatches[cacheData.id] ||= new BatchableSpineSlot();

            const texture = (attachment.region?.texture.texture) || Texture.EMPTY;

            if (batchableSpineSlot.texture._source !== texture._source)
            {
                return !batchableSpineSlot.batcher.checkAndUpdateTexture(batchableSpineSlot, texture);
            }
        }
    }

    return spine.spineAttachmentsDirty;
}

I don't know if that's more correct that the previous.

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great! Thanks @davidetan

src/Spine.ts Outdated
@@ -419,6 +420,8 @@ export class Spine extends Container implements View
);
}

if (attachment.region.texture.texture !== previousAttachment) this.spineAttachmentsDirty = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this need a ? too? attachment.region?.texture.texture

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I forgot to add it. But is it correct doing just this or is it better to use the batcher checkAndUpdateTexture in the validateRenderable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oooh you are right! il modify this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just notive that in the piece of code in the message above, I return in the for loop at the first texture change. That's wrong, I should have loop over all the attachments.
But you probably know better than me that :p

@GoodBoyDigital
Copy link
Member

heya @davidetan , do you have a spine animation i can verify our fix with pls? Thanks!

@davidetan
Copy link
Contributor Author

heya @davidetan , do you have a spine animation i can verify our fix with pls? Thanks!

Yes! You can get the dragon example from here: https://github.com/EsotericSoftware/spine-runtimes/tree/4.2/examples/dragon/export

You need:

  • dragon-ess.skel
  • dragon.atlas
  • dragon.png, dragon_2.png, dragon_3.png, dragon_4.png, dragon_5.png

Put them in the examples/assets folder'

Then you can just change the following lines in the basic example:

PIXI.Assets.add({alias: "spineboyData", src: "./assets/dragon-ess.skel"});
PIXI.Assets.add({alias: "spineboyAtlas", src: "./assets/dragon-pma.atlas"});
...
spineboy.state.setAnimation(0, "flying", true);

The wings of the dargon goes crazy without the fix.

@apelev
Copy link

apelev commented Aug 2, 2024

Contributor Author

Hello. Here is the animation I posted in the issue > https://we.tl/t-BtUbZWvh6F

@GoodBoyDigital
Copy link
Member

heya @davidetan , do you have a spine animation i can verify our fix with pls? Thanks!

Yes! You can get the dragon example from here: https://github.com/EsotericSoftware/spine-runtimes/tree/4.2/examples/dragon/export

You need:

  • dragon-ess.skel
  • dragon.atlas
  • dragon.png, dragon_2.png, dragon_3.png, dragon_4.png, dragon_5.png

Put them in the examples/assets folder'

Then you can just change the following lines in the basic example:

PIXI.Assets.add({alias: "spineboyData", src: "./assets/dragon-ess.skel"});
PIXI.Assets.add({alias: "spineboyAtlas", src: "./assets/dragon-pma.atlas"});
...
spineboy.state.setAnimation(0, "flying", true);

The wings of the dargon goes crazy without the fix.

thanks :)

@davidetan
Copy link
Contributor Author

Closing because of this: #46

@davidetan davidetan closed this Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants