Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Spine configurable with custom Sprite and Mesh constructors #490

Conversation

bchevalier
Copy link

@bchevalier bchevalier commented Mar 7, 2023

This change can allow the user to make their Spine Sprite and Mesh anything they want.
It was the simplest way I could think of to add illumination to Spine characters.

Here is how the spine animation instantiation changes:

const illuminatedAnimation = new Spine(
        spineSkeletonData,
        SpineIlluminatedSprite,
        SpineIlluminatedMesh,
  );

Where SpineIlluminatedSprite is a custom class that satisfy the SpineSprite property requirements:

class SpineIlluminatedSprite extends IlluminatedSprite {
  region?: TextureRegion = null;
  attachment?: IAttachment = null;
  spineAnimation?: Spine = null;

  constructor(texture: Texture) {
    // some initialization stuff here
  }
}

Problems

  1. This requirement is not enforced by the typescript syntax, I wonder what is the best way to achieve this. The main issue is that my custom spine component has to be an extension of both a custom sprite and a spine sprite to work!
    The best solution, in my opinion, would be change the SpineSprite to use composition and include a sprite property instead of inheriting from the Sprite class. This would also reduce the amount of de-opts in the pixi rendering pipeline.
  2. This change is made on v3 and not v4 because our project can only use v3 at the moment! What is the right branch on which to submit this change?

@bchevalier bchevalier changed the base branch from master to spine-v3 March 7, 2023 01:55
@bchevalier bchevalier changed the base branch from spine-v3 to master March 7, 2023 01:55
@bchevalier
Copy link
Author

bchevalier commented Mar 8, 2023

@ivanpopelyshev I made the change you requested.
About the version, is there a way that this change can make its way to spine v3 with a new release?
I cannot identify a branch on which to submit the PR.

Note that I branched off of this: https://github.com/pixijs/spine/releases/tag/pixi-spine_v3.1.2

@ivanpopelyshev
Copy link
Collaborator

Branch is here: https://github.com/pixijs/spine/tree/pixi6 . It can be a pain (we used rush multimodules) but i think i can release it

@bchevalier bchevalier changed the base branch from master to pixi6 March 8, 2023 06:35
@bchevalier
Copy link
Author

@ivanpopelyshev thanks, I just changed the base. So the PR is now ready on my side.

@miltoncandelero
Copy link
Contributor

Wait, @bchevalier and @ivanpopelyshev why passing a different constructor instead of overloading the functions that are there for that very purpose?

https://github.com/pixijs/spine/blob/master/packages/base/src/SpineBase.ts#L749

why not make something like

export class IlluminatedSpine extends Spine {
    override newContainer() {
        return new IlluminatedContainer();
    }

    override newSprite(tex: Texture) {
        return new IlluminatedSprite(tex);
    }

    override newGraphics() {
        return new IlluminatedGraphics();
    }

    override newMesh(texture: Texture, vertices?: Float32Array, uvs?: Float32Array, indices?: Uint16Array, drawMode?: number) {
        return new IlluminatedMesh(texture, vertices, uvs, indices, drawMode);
    }
}

@bchevalier
Copy link
Author

suggested change works, closing the PR

@bchevalier bchevalier closed this Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants