-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
SpriteMesh + SpriteMaterial #22484
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
SpriteMesh + SpriteMaterial #22484
Conversation
made a SpriteMesh component that acts as a Sprite but uses the mesh backend. Only has minimal functionality.
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
Can you do some benchmarking to see how performance compares before and after this change? We should have suitable stress tests; a before/after frame time histogram using tracy would be ideal. |
| /// How the sprite's image will be scaled. | ||
| pub image_mode: SpriteImageMode, | ||
| /// The sprite's alpha mode, defaulting to `Mask(0.5)`. | ||
| /// If you wish to render a sprite with transparent pixels, |
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 believe this should say translucent pixels? Transparent would mean a pixel with an alpha of 0 and in those case alpha masking should be used.
| for entity in sprites { | ||
| commands | ||
| .entity(entity) | ||
| .insert(Mesh2d(quad.clone().unwrap())); |
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 know this should realistically never fail, but I'd prefer if you used an if let and either log an error or do nothing.
Co-authored-by: IceSentry <[email protected]>
IceSentry
left a comment
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.
LGTM This is a very good starting point towards moving Sprites to Mesh2d. Thank you again for working on this!
To give more numbers, on my machine when running bevymark with 120k entities I get 61fps with Sprite and 97fps with SpriteMesh (using AlphaMask)
The previous PR has been closed in favor of bevyengine/bevy#22484 That one was opened just a week ago and seems more up to date with current bevy. As far as I understand, it also does everything I want from this, which is being able to easily implement shaders for my sprites.
tychedelia
left a comment
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.
This looks good as a start. I'm a little apprehensive about merging this prior to our 2d->3d rework being complete but also think it could be helpful as a migration target for me once that work gets started in earnest. Since this is relatively easy to back out and it's early enough in the cycle, I'm in favor of merging this now in its temporary state.
Objective
The objective is reworking
Spritesto use theMeshbackend instead of the current, severely outdated, one.The
2D Ecosystemwould grately benefit from sprites having access to all the mesh utilities, such asExtensionMaterials, without having to be updated and maintained separately.We could allow things such as attaching a shader to a Sprite to give it an outline, which is a very accessible feature in other engines that Bevy currently lacks any streamlined support for (#16170).
This PR notably contributes to #13265, while also completely fixing #15021 and possibly other sprite-related issues.
Solution
I've introduced a new
SpriteMeshcomponent. It is an exact replica ofSprite, having the same API, except for an extraalpha_modefield.The purpose of this is to eventually remove
Spriteand all its backend, and renameSpriteMeshtoSprite, which should hopefully be a seamless transition for our end-users.Internally,
SpriteMeshis just an abstraction over adding aMesh2dand the newSpriteMaterialto an entity, and a user should be able to just do the latter if they prefer, essentially merging the ease-of-use ofSpritesand flexibility ofMeshes.In its current state,
SpriteMeshis completely usable and produces the same behavior asSprite. There is however a notable exception:max_corner_scaleparameter when slicing now produces different results. Before, in thesprite_sliceexample, it was set to0.2, causing the corners to be half of their scale. I am fairly confident this was a bug since I haven't found any correlation between0.2and what was being displayed. Instead, theSpriteMeshnow requires0.5to produce the same result,0.2resulting in a much smaller corner (a fifth of its size).max_corner_scale=0.2max_corner_scale=0.2max_corner_scale=0.5Otherwise, replacing a
Spritewith aSpriteMeshshould have no visual difference.There were also a few bugs that I encountered with the
Sprites, related to scaling and flipping them a certain way producingweird behavior such as the sprite completely disappearing. These are not present in the new
SpriteMesh.While the API and visual behavior remain the same, the underlying logic has been completely rewritten:
No custom
RenderApplogic is used anymore,SpriteMesh/SpriteMaterialexclusively uses the API provided byMaterial2d.SlicingandTilingare now done purely inside the shader through UV mapping and manipulation (all done in constant time). Before, they generated multipleSpriteInstancesfor each tile / slice, being extremely unmaintainable and adding significant overhead. This fixes Move sprite 9 patches to a shader to avoid thin lines #15021.Testing
I mostly ran tests by overlapping
SpriteswithSpriteMesheshaving the exact same configuration and making sure there are no differences in behavior. However, I did essentially have to rework the wholeSpriteAPI to work withMeshesand I can't guarantee that I haven't missed anything.I tested the new
SpriteMeshesinbevymark. I'd say there's an ~2-2.5x increase in performance overSpriteswhen alpha masking them, and a ~0.5x decrease when alpha blending. However, alpha masking is definitely the more common choice for Sprites, and the alpha blending performance should be improved significantly in the near future as part of Mesh2d improvements tracking issues #13265.Problems
These are a few notable problems that should ideally be resolved before fully replacing
Spritewith the newSpriteMesh:The
TextureAtlasLayoutis an asset that theSpriteMaterialdepends on to generate itsBindGroup. It is currently just read when theSpriteMeshis added (or changed) and baked into theSpriteMaterial, which means there's no hot reloading for it (changing aTextureAtlasLayoutwon't update the material). I believe this is best fixed by reading theTextureAtlasLayoutasset insideas_bind_group_shader_type(), same as theSprite'sImage.Creating a new
SpriteMaterialfor each newSpriteMeshand its changes is really bad for performance, which is why I've added a rudimentary material caching solution through aHashMap<SpriteMesh, Handle<SpriteMaterial>. However, I do believe there's significant potential for further optimizations in this area.Currently, a significant chunk of
SpriteMaterialUniform(about40 bytes) is taken by data used for slicing. This data is useless forSpriteswhich don't use slicing, and so it adds overhead. Ideally, this data would be placed in a separate binding that is only written and read on the GPU if a certainShaderDefis set.