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

Add support for Spine 3.8 #3

Merged
merged 8 commits into from
Sep 7, 2024
Merged

Add support for Spine 3.8 #3

merged 8 commits into from
Sep 7, 2024

Conversation

pocams
Copy link
Contributor

@pocams pocams commented Oct 15, 2022

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:

  • Are you interested in Spine 3.8 support?
  • I added a spine38 feature and used #[cfg] for the few API changes - does this seem reasonable, or would you rather have a completely separate 3.8 branch the way the Spine runtimes do?
  • If multiple versions, what's the best way to handle multiple spine_c.rs files? This PR has the 3.8 version in it so it's not suitable for merging to main as is.

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

@jabuwu
Copy link
Owner

jabuwu commented Oct 15, 2022

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 pocams marked this pull request as ready for review October 16, 2022 18:11
@pocams
Copy link
Contributor Author

pocams commented Oct 16, 2022

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

@pocams pocams marked this pull request as draft October 16, 2022 20:37
@pocams pocams marked this pull request as ready for review October 17, 2022 04:24
@jabuwu
Copy link
Owner

jabuwu commented Oct 17, 2022

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
Copy link
Contributor Author

pocams commented Oct 17, 2022

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
Copy link
Owner

jabuwu commented Oct 17, 2022

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
Copy link

GCPanic commented Aug 10, 2023

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

@jabuwu
Copy link
Owner

jabuwu commented Aug 10, 2023

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

What's the rationale for using Spine 3.8?

@GCPanic
Copy link

GCPanic commented Aug 11, 2023

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
Copy link
Owner

jabuwu commented Aug 11, 2023

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
Copy link
Contributor Author

pocams commented Aug 12, 2023

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
Copy link

@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
Copy link

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
Copy link

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
Copy link
Owner

jabuwu commented Aug 22, 2024

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: 1380b1a 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 jabuwu changed the base branch from main to spine38 September 7, 2024 14:04
@jabuwu
Copy link
Owner

jabuwu commented Sep 7, 2024

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

@jabuwu jabuwu merged commit 8c66261 into jabuwu:spine38 Sep 7, 2024
@jabuwu
Copy link
Owner

jabuwu commented Sep 7, 2024

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

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.

5 participants