-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Migrate bevy_sprite to required components #15489
Changes from all commits
f4a628a
a52c0e8
2cbeab7
6e799ab
2206617
8657959
da50f7d
fdb3277
cf71380
b4e09ce
61e6e5f
1a35f0c
183687c
eefae1c
409858e
dd14e1a
e139c75
bc2085c
5a74e6f
0dfc16b
98d7111
4c664f1
c35f42d
8961f00
509ef62
c37ac2d
b01fc54
1d9b438
dc366a7
36ad703
01bc756
da69cf3
ff0ca87
80b4d9e
b4e3841
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,22 @@ | ||
use bevy_asset::Handle; | ||
use bevy_color::Color; | ||
use bevy_ecs::{component::Component, reflect::ReflectComponent}; | ||
use bevy_math::{Rect, Vec2}; | ||
use bevy_reflect::{std_traits::ReflectDefault, Reflect}; | ||
use bevy_render::{sync_world::SyncToRenderWorld, texture::Image, view::Visibility}; | ||
use bevy_transform::components::Transform; | ||
|
||
use crate::TextureSlicer; | ||
use crate::{TextureAtlas, TextureSlicer}; | ||
|
||
/// Specifies the rendering properties of a sprite. | ||
/// | ||
/// This is commonly used as a component within [`SpriteBundle`](crate::bundle::SpriteBundle). | ||
/// Describes a sprite to be rendered to a 2D camera | ||
#[derive(Component, Debug, Default, Clone, Reflect)] | ||
#[require(Transform, Visibility, SyncToRenderWorld)] | ||
#[reflect(Component, Default, Debug)] | ||
pub struct Sprite { | ||
/// The image used to render the sprite | ||
pub image: Handle<Image>, | ||
/// The (optional) texture atlas used to render the sprite | ||
ecoskey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pub texture_atlas: Option<TextureAtlas>, | ||
/// The sprite's color tint | ||
pub color: Color, | ||
/// Flip the sprite along the `X` axis | ||
|
@@ -21,9 +27,9 @@ pub struct Sprite { | |
/// of the sprite's image | ||
pub custom_size: Option<Vec2>, | ||
/// An optional rectangle representing the region of the sprite's image to render, instead of rendering | ||
/// the full image. This is an easy one-off alternative to using a [`TextureAtlas`](crate::TextureAtlas). | ||
/// the full image. This is an easy one-off alternative to using a [`TextureAtlas`]. | ||
/// | ||
/// When used with a [`TextureAtlas`](crate::TextureAtlas), the rect | ||
/// When used with a [`TextureAtlas`], the rect | ||
/// is offset by the atlas's minimal (top-left) corner position. | ||
pub rect: Option<Rect>, | ||
/// [`Anchor`] point of the sprite in the world | ||
|
@@ -38,6 +44,38 @@ impl Sprite { | |
..Default::default() | ||
} | ||
} | ||
|
||
/// Create a sprite from an image | ||
pub fn from_image(image: Handle<Image>) -> Self { | ||
Self { | ||
image, | ||
..Default::default() | ||
} | ||
} | ||
|
||
/// Create a sprite from an image, with an associated texture atlas | ||
pub fn from_atlas_image(image: Handle<Image>, atlas: TextureAtlas) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it confusing that it's called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking "atlas" as an adjective (like the image belongs to a texture atlas) but I'm not attached to the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably fine, it just made me pause for a sec |
||
Self { | ||
image, | ||
texture_atlas: Some(atlas), | ||
..Default::default() | ||
} | ||
} | ||
|
||
/// Create a sprite from a solid color | ||
pub fn from_color(color: impl Into<Color>, size: Vec2) -> Self { | ||
Self { | ||
color: color.into(), | ||
custom_size: Some(size), | ||
..Default::default() | ||
} | ||
} | ||
} | ||
|
||
impl From<Handle<Image>> for Sprite { | ||
fn from(image: Handle<Image>) -> Self { | ||
Self::from_image(image) | ||
} | ||
} | ||
|
||
/// Controls how the image is altered when scaled. | ||
|
@@ -58,7 +96,7 @@ pub enum ImageScaleMode { | |
}, | ||
} | ||
|
||
/// How a sprite is positioned relative to its [`Transform`](bevy_transform::components::Transform). | ||
/// How a sprite is positioned relative to its [`Transform`]. | ||
/// It defaults to `Anchor::Center`. | ||
#[derive(Component, Debug, Clone, Copy, PartialEq, Default, Reflect)] | ||
#[reflect(Component, Default, Debug, PartialEq)] | ||
|
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.
(Making this a comment so discussion can happen in a thread)
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>
intoSprite
", makes "movingTextureAtlas
intoSprite
" a pretty small increase in migration overhead.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 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.
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).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.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 aVec3
. 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 removeimpl Component for Handle<T>
. Having a universal "anything that needs exactly oneHandle<Image>
" component context isn't particularly useful and is problematic for a variety of reasons. ImoSpriteTextureArray
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?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 theQuery<(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.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.
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
intoUiImage
—does this then also follow the same "fixed function" reasoning? (I could not see any reference in the hackmd's to show a plan forUiImage
)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 taking the time to address the questions :)
Agreed. People already need to change how the use Sprite, so any further changes should be in the same release.
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.
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.
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 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.