-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Yet another Migrate bevy_sprite to required components #15562
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
Yet another Migrate bevy_sprite to required components #15562
Conversation
@@ -111,7 +111,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) { | |||
compressed: asset_server.load("data/compressed_image.png.gz"), |
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.
Not sure how to handle this generically now.
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 added Handle<A>: Into<C>
trait bounds. That means C needs to implement From<Handle<A>
, which is true for Sprite.
May not be the best solution.
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 have a mild preference for this design. It's more flexible and the migration is easier. Either one is fine by me though.
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.
Thanks for working on this.
I very much prefer this implementation to the alternative PR.
I know that there's no promise of stability but at least reducing churn for users when upgrading is an important factor to consider in this case, in my opinion.
I particularly like that it keeps Sprite
and TextureAtlas
separate, like it was discussed at length, approved and merged with #5103 , whereas the blessed PR kinda reverts that change.
I'll try to argue my case on Discord, I guess.
Edit: Discussion on Discord
Responded with thoughts here: #15489 (comment) |
Merging #15489 instead, per the discussion linked just above :) |
Objective
Migrate bevy_sprite Sprite to required components.
Alternative to #15489
It does not use any of the proposals from here so feel free to disregard this PR.
Solution
Previously:
Now:
Maybe SpriteProperties should just be part of Sprite, not sure. I am open to changing that. Having a dedicated wrapper component for an asset is nice though, e.g. for change detection.
I did not touch TextureAtlas, but I can see the motivation for that in the context of editor usability/suggestion - I just do not think required components are the right tool for that.
Testing
I ran all tests that I modified and everything seemed to work, but I did not compare to main.
bevy_sprite tests were also run and passed.
Migration Guide
Asset handles for sprite images must now be wrapped in the Sprite component. Raw handles as components no longer render sprites. The component previously known as Sprite is now called SpriteProperties.
As a result SpriteBundle has been deprecated. Instead, use the Sprite components directly.
Now:
If you have any queries for sprites using Handle
make sure to change them to Sprite.