Migrate bevy_sprite to required components

[ecoskey]

Objective

Continue migration of bevy APIs to required components, following guidance of https://hackmd.io/@bevy/required_components/

Solution

• Make Sprite require Transform and Visibility and SyncToRenderWorld
• move image and texture atlas handles into Sprite
• deprecate SpriteBundle
• remove engine uses of SpriteBundle

Testing

ran cargo tests on bevy_sprite and tested several sprite examples.

---

Migration Guide

Replace all uses of SpriteBundle with Sprite. There are several new convenience constructors: Sprite::from_image, Sprite::from_atlas_image, Sprite::from_color.

WARNING: use of Handle<Image> and TextureAtlas as components on sprite entities will NO LONGER WORK. Use the fields on Sprite instead. I would have removed the Component impls from TextureAtlas and Handle<Image> except it is still used within ui. We should fix this moving forward with the migration.

[KirmesBude]

With my alternative approach I noticed that the asset_decompression example is broken. I assume the same is true here.

[alice-i-cecile]

When reviewing this PR, compare the design to #15562 and see which one you prefer!

[akimakinai]

@ecoskey
Have you emptied the branch by accident?

Files changed 0

[ecoskey]

@ecoskey

Have you emptied the branch by accident?

Files changed 0

Yes. I'll fix it this afternoon, I'm at a river

[KirmesBude]

Looks like there is now easing_function as well, but ci caught it.
Also as per my last comment the asset_decompressing example is still broken.

[kristoff3r]

Mostly LGTM apart from the discussion above which I'd like to have fully closed before merge.

The asset_decompression example is currently broken (as @KirmesBude also mentioned) because it assumes it can insert an image handle as a component that will be used by the sprite. Either we have to add an extra layer of indirection to fix that up, or we can change the decompress function to not be generic.

[kristoff3r]

Is it confusing that it's called from_atlas_image and the argument order is image then atlas?

[ecoskey]

I was thinking "atlas" as an adjective (like the image belongs to a texture atlas) but I'm not attached to the name

[kristoff3r]

It's probably fine, it just made me pause for a sec

[kristoff3r]

Hmm, the animation isn't working in the sprite_animation and sprite_sheet examples. They work on current main.

[ecoskey]

Hmm, the animation isn't working in the sprite_animation and sprite_sheet examples. They work on current main.

should be fixed, as well as asset_decompression

[wilk10]

As a user, I have a preference for the alternative implementation: #15562 .

My reasoning is that:

• the upgrade with 15562 is going to be much smoother. I know there's no promise of stability but reducing churn when upgrading could be a factor to consider in this case.
• this PR sort-of reverses Texture Atlas rework #5103, which back in its time was considered, discussed and merged (moving TextureAtlas to a separate component).
• if there's a good motivation to couple Sprite and TextureAtlas, that change could come later, possibly after 0.15. First upgrade Sprite to required components with the least amount of churn as possible, then couple Sprite and TextureAtlas, with a separate discussion why that is necessary.

I did not follow the initial decision for the blessed solution though, I might miss some context.

(motivation also presented on Discord, not sure where it's better to discuss)

[mgi388]

I’d also be keen to hear the motivation for moving TextureAtlas. I can see it’s blessed but I can’t see why. I’ve been using bevy_spritesheet_animation and it works with bevy’s built in Sprite plus the crate’s own Sprite3d. It uses this system: https://github.com/merwaaan/bevy_spritesheet_animation/blob/4f855d9dbf62e47125076309ea9d5ac7f3aea8bb/src/systems/spritesheet_animation.rs#L21 which has a query using TextureAtlas but not Sprite and so it works for both types of Sprite.

If I’m not mistaken this crate will no longer work for both forms if this PR is merged and I haven’t been able to understand why this PR is more desirable than the current state. I’m happy to be wrong and trust that the blessed approach is blessed for a reason, but I’ve been sitting on this ponder for a while and thought I’d ask.

[cart]

(Making this a comment so discussion can happen in a thread)

Leaving TextureAtlas separate is a smoother upgrade

I do agree here. It would let people largely focus on the required components change as the component boundaries would remain largely untouched. But I do think the "everything is on Sprite now" narrative coupled with "moving Handle<Image> into Sprite", makes "moving TextureAtlas into Sprite " a pretty small increase in migration overhead.

This unification could come later (in the interest of making migrations easier)

Imo not breaking people across multiple releases is as big of a concern (if not bigger) than making a specific upgrade easier or harder. I generally prefer breaking people upfront in a slightly bigger way over breaking people multiple times.

This reverses the decisions made in the Texture Atlas Rework!

This change does revert the "separate-component-ization" of TextureAtlas data, but it doesn't revert the "conceptual unification" of Sprite and TextureAtlasSprite, which imo is the "bigger" motivating change in that PR.

Having a separate Sprite(Handle) allows custom pipelines to also "be" sprites by reusing the other fields, such as TextureAtlas, color, etc.

As others have mentioned, Sprites are intended to be a very specific "fixed function" thing. I think from an ecosystem perspective, trying to build an "implementation and behavior agnostic sprite-building framework" would require more thoughtful design work, would produce dubious gains, and would increase the user-facing complexity of the system. The fact that right now you can replace Handle<Image> with a different component and everything works as expected is an unintentional implementation detail (and I'm honestly not even sure "everything works as expected" is true, especially in the context of the wider ecosystem).

What about custom sprite materials via fully custom render pipelines? What are we going to do when sprite materials are added?

Our answer to "custom sprite materials" is currently Mesh2d. There are no existing "custom material" paths for Sprite. We may ultimately replace Sprite with Mesh2d. I've heard some people express that ambition (provided we can get perf to be roughly the same) and I'd very much like for us to explore that path, as it would allow us to define true shared infrastructure. If we do choose to implement a more constrained sprite materials system (which is definitely also worth considering) that will require a significant rethink of the whole API, and it would still be directly tied to the fixed function sprite system (we would just add extension hooks to the "fixed function" system). Not something we can / should design for here, given how uncertain that area is. We cannot (and should not) try to prepare for this without having a design that we're committed to.

What are we going to do if we add texture arrays? Make a Option<TextureArray> on Sprite?

Certainly an open question. My default answer is "probably". TextureArray support would be a "fixed function sprite behavior". Functionally, everything that is a sprite would need to ask itself "do i have a texture array yes or no" to select how it behaves. Components for opt-in behaviors are definitely also on the table. However TextureArray is a pretty generic concept. A "texture array" is essentially a contextless datatype, similar to something like a Vec3. All components (including floating components) should mean one specific functional thing. A component is data plus the behaviors driven by that data. Behaviors can and should be tied to a specific context. That is one of the primary reasons we want to remove impl Component for Handle<T>. Having a universal "anything that needs exactly one Handle<Image>" component context isn't particularly useful and is problematic for a variety of reasons. Imo SpriteTextureArray would make more sense as a floating component. And that again should be tied to the behaviors of the fixed function sprite system. And at that point, why are we separating it from Sprite?

What are we going to do when we add parallax backgrounds?

These feel like a higher level concept that would orchestrate existing Sprite functionality. Idk if that functionality needs to be baked into sprites themselves. And if we did tie that directly to sprites, it would once again be deeply integrated into the fixed function Sprite system. So why not add it to Sprite?

For pretty much everything (Sprite::color, Sprite::flix_x, Sprite::rect, Sprite::anchor, Sprite::custom_size), these are all tied to very specific implementation details of our Sprite system. We will likely add more of these over time. Custom pipelines can and should define their own fields, as they could easily behave in subtly different ways, or they might have gaps, such as not implementing flip functionality. For this reason I'm against something like SpriteProperties. I also generally object to any X + XProperties design/naming pattern, as that generally implies that those properties should live on the X type. How can something have X's properties without being X?

In terms of extracting things into their own components, I'm most on board for TextureAtlas, as it does feel more like a reasonably standalone set of functionality (I need a texture atlas for some arbitrary context and I want to read this specific index from it). But frankly even that seems like a stretch, as it relies on sprite-specific shader implementation details to read the indices and then sample the image. There are plenty of other valid uses for texture atlases (that have different functional requirements and different usage contexts). If you are defining a custom pipeline, it could easily have different requirements or behave differently.

Next Steps

I'm pretty strongly still on team "put everything in Sprite". A "Sprite" is inherently "the fixed function special cased sprite rendering featureset / pipeline" (a combination of backing ECS systems and render pipelines). This pipeline may eventually be extended to support custom materials, but that is not currently case. If you are defining something that does not use that fixed function pipeline it is not a Sprite as defined by Bevy. It is driven by completely different machinery (and will function differently). Therefore it should define its own component, with its own concept name, and its own fields.

Outside of cases like @merwaaan's bevy_spritesheet_animation example of "cross context custom Sprite3d vs Sprite spritesheet animation" (brought up by @mgi388), which defines a new Sprite3d and shares TextureAtlas between 3d and 2d sprites, I don't see much of a benefit to extracting out TextureAtlas in its current form. The "cross context sprite animation system" is a pretty niche concept, and in the "unified sprite" world, that crate could just split the Query<(Entity, &mut SpritesheetAnimation, &mut TextureAtlas)> into a query for &Sprite and &Sprite3d. It would amount to a few extra lines. I think this split is Good Actually, because Sprite3d is a completely different concept than Sprite backed by completely different infrastructure. The fact that they both have "texture atlas" functionality (and the fact that the behavior works similarly) is incidental. These types of shared use cases are what should motivate the discussion of splitting out components / defining a "shared context and API boundary". But given the use cases that have been enumerated so far, I don't think the few extra lines these authors need to write outweighs the conceptual simplicity and discoverability of the unified Sprite component. I think most of urge to split out TextureAtlas is fundamentally motivated by the desire to allow people to define new "sprite types" that are not Bevy's Fixed Function Sprite System, but still pretend they are / overlap with some of its infrastructure. I don't think this is the right path to encourage generally given how we have currently defined and implemented Sprites. Instead these cases should define their own names and behaviors.

[mgi388]

For my own part, I'm happy with the explanation and justification, thanks. Seeing Sprite as a "fixed function" thing makes sense.

Will there also be a consolidation of TextureAtlas into UiImage—does this then also follow the same "fixed function" reasoning? (I could not see any reference in the hackmd's to show a plan for UiImage)

[KirmesBude]

Thanks for taking the time to address the questions :)

Leaving TextureAtlas separate is a smoother upgrade

I do agree here. It would let people largely focus on the required components change as the component boundaries would remain largely untouched. But I do think the "everything is on Sprite now" narrative coupled with "moving Handle into Sprite", makes "moving TextureAtlas into Sprite " a pretty small increase in migration overhead.

This unification could come later (in the interest of making migrations easier)

Imo not breaking people across multiple releases is as big of a concern (if not bigger) than making a specific upgrade easier or harder. I generally prefer breaking people upfront in a slightly bigger way over breaking people multiple times.

Agreed. People already need to change how the use Sprite, so any further changes should be in the same release.

[...]
For pretty much everything (Sprite::color, Sprite::flix_x, Sprite::rect, Sprite::anchor, Sprite::custom_size), these are all tied to very specific implementation details of our Sprite system. We will likely add more of these over time. Custom pipelines can and should define their own fields, as they could easily behave in subtly different ways, or they might have gaps, such as not implementing flip functionality. For this reason I'm against something like SpriteProperties. I also generally object to any X + XProperties design/naming pattern, as that generally implies that those properties should live on the X type. How can something have X's properties without being X?

I had the same concern in my approach. I thought having these new components just be wrappers around Handles was a nice pattern, that might allow for some nice to have stuff in the future, like From<Handle>. I think I agree though, that for Sprites it should not be done like this though.

In terms of extracting things into their own components, I'm most on board for TextureAtlas, as it does feel more like a reasonably standalone set of functionality (I need a texture atlas for some arbitrary context and I want to read this specific index from it). But frankly even that seems like a stretch, as it relies on sprite-specific shader implementation details to read the indices and then sample the image. There are plenty of other valid uses for texture atlases (that have different functional requirements and different usage contexts). If you are defining a custom pipeline, it could easily have different requirements or behave differently.

I'm pretty strongly still on team "put everything in Sprite". A "Sprite" is inherently "the fixed function special cased sprite rendering featureset / pipeline" (a combination of backing ECS systems and render pipelines). This pipeline may eventually be extended to support custom materials, but that is not currently case. If you are defining something that does not use that fixed function pipeline it is not a Sprite as defined by Bevy. It is driven by completely different machinery (and will function differently). Therefore it should define its own component, with its own concept name, and its own fields.

Outside of cases like @merwaaan's bevy_spritesheet_animation example of "cross context custom Sprite3d vs Sprite spritesheet animation" (brought up by @mgi388), which defines a new Sprite3d and shares TextureAtlas between 3d and 2d sprites, I don't see much of a benefit to extracting out TextureAtlas in its current form. The "cross context sprite animation system" is a pretty niche concept, and in the "unified sprite" world, that crate could just split the Query<(Entity, &mut SpritesheetAnimation, &mut TextureAtlas)> into a query for &Sprite and &Sprite3d. It would amount to a few extra lines. I think this split is Good Actually, because Sprite3d is a completely different concept than Sprite backed by completely different infrastructure. The fact that they both have "texture atlas" functionality (and the fact that the behavior works similarly) is incidental. These types of shared use cases are what should motivate the discussion of splitting out components / defining a "shared context and API boundary". But given the use cases that have been enumerated so far, I don't think the few extra lines these authors need to write outweighs the conceptual simplicity and discoverability of the unified Sprite component. I think most of urge to split out TextureAtlas is fundamentally motivated by the desire to allow people to define new "sprite types" that are not Bevy's Fixed Function Sprite System, but still pretend they are / overlap with some of its infrastructure. I don't think this is the right path to encourage generally given how we have currently defined and implemented Sprites. Instead these cases should define their own names and behaviors.

The reason why I like TextureAtlas remaining a component is that (I think) it is the only/best thing to implement generic spritesheet/index-based animations. Having this be provided by bevy from a central position without any dependency to Sprite is a huge boon the plugin ecosystem in my opinion.
I can provide a plugin for Sprite3d or a plugin Backgrounds that support TextureAtlas and I don't have to think about animations. There can be a separate animation plugin that only interfaces with TextureAtlas. Your example of splitting the query only works if Sprite3d implements its own animation functionality (or the animation crate supports this specific Sprite3d crate).
Maybe that is not the right level of abstraction, but to my knowledge there is no better alternative right now. Moving TextureAtlas to Sprite breaks this usecase right now. I would like to see effort towards finding a solution to abstract over that before we split this up or for the next release if we go forward with this approach.

[wilk10]

Thanks a lot for taking the time to review and weigh in. I disagree with some of the stuff mentioned but I really appreciate the fact that the change has its motivation and it has been communicated well.