search

jabuwu / rusty_spine

Spine runtime for Rust (and wasm!) transpiled from the official C Runtime.
https://docs.rs/rusty_spine
Other
43 stars 4 forks source link

Add support for Spine 3.8 #3

Closed pocams closed 3 weeks ago

pocams commented 1 year ago

Amazing project - transpiling the C runtimes is genius mad science. I've added support for Spine 3.8 but I have a few open questions:

I'll add docs to the PR as appropriate once it's clear how multiple versions should be handled.

jabuwu commented 1 year ago

Sure, why not. I'm against adding too many versions, at least without the help of some automation. Each new feature introduced is a new feature to be tested which can take a lot of time. I don't want to do branches because that also implies separate crate versions, which I don't want to deal with at the moment.

The spine_c file is a simple solution. Have two separate files and use path to specify location.

#[cfg(not(feature = "spine38"))]
#[path = "spine_c_4.3.rs"]
mod spine_c;

#[cfg(feature = "spine38")]
#[path = "spine_c_3.8.rs"]
mod spine_c;

The git commit needs to be documented somewhere. Currently I do this in src/c/mod.rs, so it's available in the docs. https://docs.rs/rusty_spine/latest/rusty_spine/c/index.html

pocams commented 1 year ago

Thanks for the feedback! I split spine_c.rs as you suggested and added the feature flag and commit hash to the docs.

jabuwu commented 1 year ago

I've taken an initial look over the changes and they look good. One request I have, and I should probably add this to a contributing section of this repo somewhere, but could you run this over these changes:

rustfmt src/lib.rs

It will format all the files to the Rust style guidelines, which is something I've configured in VS Code to happen automatically on save.

Also I think the following should be added to the top of main in each example:

#[cfg(feature = "spine38")]
compile_error!("This example does not work with Spine 3.8");

It seems that Spine 3.8 segfaults if trying to load an incompatible version, so it might not be clear why examples don't work with this feature. I'd love to have some 3.8 inclusive demos but I'm starting to think a larger example suite should be its own repo. I'm not a fan of cluttering this one with lots of Spine files.

Thanks!

pocams commented 1 year ago

Makes sense, I applied the changes. I have to say, though - the more this PR grows, the more hesitant I am about merging it. Support for 3.8 is a very niche use case - it's useful for building tools that work with existing game files but not for real game development. I pictured the changes being minor, but they're starting to touch a lot of places. At this point I'm becoming concerned that it makes the codebase less approachable for contributors while not adding a lot of value.

What do you think about closing this PR and adding a note to the readme suggesting people try my fork if they need 3.8 support? I'm not intending to publish a crate but they can pull it from Github. If anyone besides me ends up using it, we could reconsider merging it in.

jabuwu commented 1 year ago

Yeah that's fine with me. The approachability concern would be partially addressed with proper CI and tests, but it's not something I've looked into very much.

GCPanic commented 1 year ago

Is there's any chance for this to getting new updates?

jabuwu commented 1 year ago

Is there's any chance for this to getting new updates?

What's the rationale for using Spine 3.8?

GCPanic commented 1 year ago

Is there's any chance for this to getting new updates?

What's the rationale for using Spine 3.8?

like he said before, i need to work with some existing game files in spine 3.8

jabuwu commented 1 year ago

Is there any reason that 3.8 files can't be updated to latest Spine?

The reason I'm asking is that I don't have much interest in maintaining older Spine versions, at least without sufficient justification. It might make sense in this case because it's the last major version, but it adds a lot of extra dev work and testing moving forward.

Also, due to versioning issues, 3.8 support might need to be its own crate.

pocams commented 1 year ago

The project I was using this for is effectively complete, so I don’t currently have any plans to update my fork. That said, it does work fine for loading/skinning/rendering 3.8 sprites, so it may be usable for your project as-is if that’s all you need to do. If you’re doing more than that, you may be better off updating the sprites to 4.1 using the Spine GUI tools.

Des-Nerger commented 1 year ago

@pocams, just to clear things up for future readers. Your fork has 2 relevant branches: spine38 and main. For those interested in v3.8, should they prefer the former or the latter?

cherish-ltt commented 1 month ago

I'm come. First of all, thank you to the author. Rusty_spine and bevy_spine are really great. The main reason why I recommend spine3.8 is that it has been used in many works and has been well validated, with fewer bugs, more support, and more searchable solutions. Bevy_Spine is just getting started, and using the latest version of Spine is very reasonable. If a stable version of Bevy_Spine is released someday in the future, I suggest choosing a stable version of Spine, which is great.

cherish-ltt commented 1 month ago

I'm come. First of all, thank you to the author. Rusty_spine and bevy_spine are really great. The main reason why I recommend spine3.8 is that it has been used in many works and has been well validated, with fewer bugs, more support, and more searchable solutions. Bevy_Spine is just getting started, and using the latest version of Spine is very reasonable. If a stable version of Bevy_Spine is released someday in the future, I suggest choosing a stable version of Spine, which is great.

The importance and robustness of Spine 3.8 in Spine is similar to Apple's position in the mobile phone industry (whether it is appropriate or not). I guess the people who need spine3.8 version support now include myself, most of whom want to use it in formal projects, including production level projects and commercial level projects. Thank you again to the author.

jabuwu commented 1 month ago

What's most surprising to me is the overlap of Spine 3.8 and Rust. If you are using Bevy, then it's strange to say that Spine 4.X is the hurdle. Learning Bevy is a much bigger hurdle to both learning and stability. Most Rust crates I come across are not even 1.0 yet.

I do agree that Spine 4.X comes with bugginess, but issues on their GitHub are addressed quickly. I've submitted several bugs myself and they're always addressed within about a week. If I'm made aware of an issue, I trace it down and update this library ASAP.

Ultimately, if people want Spine 3.8 support, then that feature needs a champion. @pocams did the hardest part, which is transpiling 3.8 to Rust, but a lot of maintenance follows. I don't use 3.8, so probably the feature will rot without a serious project using the feature.

If I seem hyperbolic, consider this commit: https://github.com/jabuwu/rusty_spine/commit/1380b1aa956e9a20153b8398c696253b45f0e036 It's hundreds of UB fixes that I had to do by hand, because of new checks in the Rust compiler. I'm happy to do that work because I know people are using 4.2 (including myself). Interest in 3.8 seems to come and go.

jabuwu commented 3 weeks ago

I'm merging this into a branch where we can try to get 3.8 support working, separate from the main codebase.

jabuwu commented 3 weeks ago

For those interested, I have created two new repositories: https://github.com/jabuwu/rusty_spine3.8 https://github.com/jabuwu/bevy_spine3.8