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
feat(spine-ts): add support for pixi.js #2299
Conversation
thanks to warmanw#5604 on discord for the file
This is great! The polypack atlas support seems to have an issue though. This is what I get on latest Chrome on macOS, but I guess it's an issue related to generating regions out of the polypack atlas. Should look like this, and is what you get when using the Spine atlas instead. This is 100% in line with what I planned myself, so that's great! I'll do a formatting pass and code review and will add docs and examples as appropriate. Thanks a ton! Before I can merge this, I'll need a CLA, so we can ship it under the Spine Runtimes license. See Contributing at the bottom of the README.md. Sorry for the red tape :( |
Since I'm not a Pixi export, I'm leaving a few questions/comments on the files in this PR. Hope you don't mind. |
Opened a TODO for me here: #2305 |
SlotMesh.auxColor[2] = finalVertices[4]; | ||
SlotMesh.auxColor[3] = finalVertices[5]; | ||
|
||
this.tint = SlotMesh.auxColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the mesh tint isn't part of the vertex data, I assume it's passed as a uniform? If so, how can Pixi batch meshes with different tints? I couldn't find anything on that on the web.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is part of the vertex data but it's not during the "mesh" part of the lifecycle.
During rendering, pixi checks all objects that can be rendered together and interleaves the tint at that point.
Currently there is no way to inject an already interleaved mesh data into the data that pixi is interleaving itself.
Un-interleaving and letting pixi interleave again during the render step allows for batching between different skeleton and other pixi objects such as sprites and texts since pixi's batch renderer uploads up to 32 textures per batch
|
||
public dispose(): void { | ||
// I am not entirely sure about this... | ||
this.texture.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Textures are cached in the static textureMap
field. I suppose on dispose()
the texture should get removed from the cache? I'm not familiar with the Pixi resource life cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are 2 textures in pixi
BaseTexture
holds the actual Image, HTMLCanvas, Compressed Texture or whatever is the actual data of your image.
Texture
is a "view" on that BaseTexture. Mostly used to extract a rectangle or to set a pivot on a texture.
Destroying a Texture only affects that texture, but destroying a BaseTexture breaks all Textures that are a "view" of that.
Since Pixi doesn't support polygon packing and the spine runtime already handles the region extracting, I connected the Spine Texture directly to a Pixi Texture that its a view to the entire BaseTexture and let the runtime do the regioning during the Attachment processing.
Destroying this.texture
is totally fine here. Destroying the underlaying BaseTexture (actually releasing the bytes that make up the image from memory) would mean that any other SpineTexture that depends on those same bytes will be broken too.
TLDR;
Should "dispose" release completely the image data from memory? (if we ever need that image again we need to load the png again)
this.texture.baseTexture.scaleMode = SpineTexture.toPixiTextureFilter(minFilter); | ||
this.texture.baseTexture.mipmap = SpineTexture.toPixiMipMap(minFilter); | ||
|
||
// pixi only has one filter for both min and mag, too bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch. Using the min filter is the sensible choice then. Should be documented in the docs on the website later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will ping pixi internally to see why this is the case.
import { Container } from "@pixi/display"; | ||
|
||
export interface ISpineOptions { | ||
removeUnusedSlots?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would this be useful? A skeleton generally has a fixed number of slots. The maximum number of SlotMesh
instances would thus be the number of slots in the skeleton. Does recycling have any benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens with slots that are inside skins? 🤔
In any case, that was an idea that lead nowhere, the implementation now is not good since it removes the children one at a time. Since children is an array removing one at a time could lead to many splicings and that not the nicest way to empty an array...
I think we should remove that 😅
public static readonly skeletonCache: Record<string, SkeletonData> = Object.create(null); | ||
|
||
public static from(skeletonAssetName: string, atlasAssetName: string, options?: ISpineOptions & { scale?: number }): Spine { | ||
const cacheKey = `${skeletonAssetName}-${atlasAssetName}-${options?.scale ?? 1}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can cached items be evicted? Should we add a Spine.clearSkeletonDataCache()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skeletonCache
is public so you could call delete on each key or set them to null if you really wanted to, but we could also make a function to clear the cache 🤔
@@ -0,0 +1,117 @@ | |||
spineboy-polypack.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I generate a polypack file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// .from(...) is a shortcut + cache for creating the skeleton data at a certain scale | ||
// Here would be the "long way" of doing it (without cache): | ||
|
||
// const skeletonAsset = Assets.get(skeletonAssetName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's awesome that there's a high-level and low-level way of loading things.
/** | ||
* This is locked behind https://github.com/pixijs/pixijs/issues/8957 | ||
* I don't want to make a custom event emitter and do `this.spineEvents.on` because that's just as "far" as `this.state.addListener` | ||
* So, until pixi fixes the custom event system, I'll stick to spine native events. - @miltoncandelero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine.
type SkeletonBinaryAsset = Uint8Array; | ||
|
||
function isJson(resource: any): resource is SkeletonJsonAsset { | ||
return resource.hasOwnProperty("bones"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably check for more fields definitely only found in a Spine skeleton .json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could try finding some metadata and maybe spine version? 🤔
Alright, first code review pass is complete. It's great! Just a few questions from my end in the review. Once we have the CLA, I'll merge and polish it up. Since the public API surface won't change, my largely cosmetic changes should not affect your development. |
Ok, I finished the first pass of the reviews. Since the CLA is a txt how do you want it? PDF and digisign or printed, signed and scanned? 🤔 on the visual bug I didn't export the polygon packed spine myself, I asked somebody from discord to do it and I only took the texture and atlas file. Maybe the skeleton was not exactly the same as the one I had? 😅 |
Just a PDF with a signature is enough. Sorry, I hate this too.
…On Wed, Jun 7, 2023, 19:23 Milton Candelero ***@***.***> wrote:
Ok, I finished the first pass of the reviews. Since the CLA is a txt how
do you want it? PDF and digisign or printed, signed and scanned? 🤔
—
Reply to this email directly, view it on GitHub
<#2299 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD5QBAAWCWQ4PHKX4Y2AWDXKC2K3ANCNFSM6AAAAAAYZMP4XQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Email sent and confirmation received 👍 |
Super cool, thanks! I'll get to this next week, before my holiday. |
Uff, sorry, I didn't see your update on the visual bug. I see if I can figure it out. I'm currently merging this in an am updating the parts as discussed in the review. |
@miltoncandelero the visual bug with polygonally packed atlases appears to be a bug in the texture packer. For Spineboy, the gun (and lower back leg part) are region attachments, meaning they will map to a rectangular area in the texture atlas page. However, due to polygonal packing, those rectangular areas in the texture atlas have pixels from other images. These show up during rendering. We'll fix that in the editor. |
OK, after more investigations, it seems the latest 4.1 editor release doesn't create broken atlases. @miltoncandelero could you tell me what editor version you used? |
I'm sorry but I don't know 😅 See, I don't have a spine licence so I asked somebody from the discord server to export it for me and I didn't ask what version they were using. Sorry 😅 |
Hi guys @badlogic, @miltoncandelero. In addition want to ask about purpose of this project. Probably it is more to @miltoncandelero. The |
@f0nar I haven't had any issues with dynamic masks nor mesh deforms 🤔 Do you have a file/minimal repro? As for the second part, By letting the official runtime do it's job, pixi code is just rendering. That is what "to be as out of the way as possible" means, this library won't try to outsmart it's parent lib ( |
@miltoncandelero got it. Sounds good. Then my migration plan is great idea) Otherwise, here is a small repo with example https://github.com/f0nar/pixi-spine-testing of not working features. You can compare result to the one in |
spine-pixi is a work in progress which I haven't had time to finish yet
(examples, docs, testing).
Could you please open a separate issue for the problems you ran into?
…On Tue, Oct 17, 2023, 13:27 Vladyslav Pohorielov ***@***.***> wrote:
@miltoncandelero <https://github.com/miltoncandelero> got it. Sound good.
Then my migration plan is great idea)
Otherwise, here is a small repo with example
https://github.com/f0nar/pixi-spine-testing of not working features. You
can compare result to the one in public/preview.gif or you can run it
with pixi-spine to see that it works there (just comment and uncomment
imports in index.ts). Take a look to mesh point deformation and animated
clipping vertices.
—
Reply to this email directly, view it on GitHub
<#2299 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD5QBBIPVF4TF32SAB3UPTX7ZTQ5AVCNFSM6AAAAAAYZMP4XSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWGIYTSMJUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hello!
This pull request adds support for Pixi.js, to the spine-ts runtimes.
The main idea behind this is to be feature complete and as out-of-the-way as possible. This pr does not aim to re-implement anything already existing in either spine-ts or in pixi.js
To achieve this, the renderer un-interleaves the geometry that spine-ts generates so that pixi renderer can interleave it again.
Bone transformation is calculated by spine-ts, skeleton position on screen and posterior transformations are handled by pixi.
This code is currently being battle tested at my daytime job at Killabunnies to develop 3 games (I can't give more details about those games yet, sorry 😅 )
Key features:
Rendering
Assets
Asset
class and you must load both the skeleton and the atlas file. We will not try to guess the atlas corresponding to a skeleton. This allows for greater flexibility when using a single skeleton with different textures.Ergonomics
Spine.from(...)
that creates the skeleton data and tries to cache it, similar to what the Phaser runtime does. TheSpine
constructor works similar to the constructors of other spine-ts implementation, requiring anSkeletonData
Debug
Events:
DisplayObject.on(...)
typing not allow to dispatch custom event pixijs/pixijs#8957 so the events are not forwarded from spine's original events. For now it is advised to connect directly to the events in thestate
variable.Things missing