Move sprite batches to resource #17636
Conversation
Currently, `prepare_sprite_image_bind_group` spawns sprite batches onto an individual representative entity of the batch. This poses significant problems for multi-camera setups, since an entity may appear in multiple phase instances. Instead, move batches into a resource that is keyed off the view and the representative entity. Long term we should switch to mesh2d and use the existing BinnedRenderPhase functionality rather than naively queueing into transparent and doing our own ad-hoc batching logic. Fixes bevyengine#16867, bevyengine#17351
tychedelia
added
C-Bug
|
I guess this is technically a breaking change but can probably be safely picked into |
|
Ran codes provided by each of the linked issues and they seem to work as intended now |
Yes, the |
crates/bevy_sprite/src/render/mod.rs
Outdated
| @@ -650,7 +653,8 @@ pub fn prepare_sprite_image_bind_groups( | |||
|
|
|||
| let image_bind_groups = &mut *image_bind_groups; | |||
|
|
|||
| for transparent_phase in phases.values_mut() { | |||
| for (retained_view, transparent_phase) in phases.iter_mut() { | |||
| let mut batch_key = None; | |||
There was a problem hiding this comment.
I dislike how the way the flow of the method requires that this is initialized with None, i would personally collect all distinct phase items into something like a HashMap<AssetId<Image>, Vec<Entity>>
There was a problem hiding this comment.
Without having reminded myself of how this code works, not having thought enough about what you mean, using HashMap to hold collections of entities with not-highly-optimised keys seems likely to introduce significant performance regressions.
|
Do you have any Tracy traces to show the performance of bevymark using the sprite mode with say 100-150k sprites? |
| @@ -483,7 +484,10 @@ pub struct SpriteViewBindGroup { | |||
| pub value: BindGroup, | |||
| } | |||
|
|
|||
| #[derive(Component, PartialEq, Eq, Clone)] | |||
| #[derive(Resource, Deref, DerefMut, Default)] | |||
| pub struct SpriteBatches(HashMap<(RetainedViewEntity, MainEntity), SpriteBatch>); | |||
There was a problem hiding this comment.
Do we have an optimised hash of a pair of entities yet? If not then perhaps we need one? In the degenerate case with one batch per sprite I suppose this would hurt. But, sprites should use sprite sheets so that should not happen for large numbers of sprites in practice.
There was a problem hiding this comment.
I think using EntityHash should work no matter how many entities you use in the key. I'll include that instead of the default hasher and confirm.
There was a problem hiding this comment.
This didn't seem to matter in my testing.
|
This appears to be solidly faster, which is somewhat surprising to me but maybe the cost of batch spawning is a lot greater than even all the hashing?
cargo run --example bevymark --release --features bevy/trace_tracy -- --waves 100 --per-wave 1000 |
| @@ -690,8 +694,7 @@ pub fn prepare_sprite_image_bind_groups( | |||
| }); | |||
|
|
|||
| batch_item_index = item_index; | |||
| batches.push(( | |||
| item.entity(), | |||
| current_batch = Some(batches.entry((*retained_view, item.main_entity())).insert( | |||
There was a problem hiding this comment.
Do we need to use the whole RetainedViewEntity in the key, it seems like just using retained_view.main_entity would be enough to avoid collisions?
There was a problem hiding this comment.
I think it probably would be fine, especially because this is not yet retained and we are rebuilding this every frame, but we're moving towards using RetainedViewEntity everywhere for safety reasons. In my brief benching, using just the MainEntity didn't seem to have a notable performance impact.
alice-i-cecile
added
S-Ready-For-Final-Review
Objective
Currently,
prepare_sprite_image_bind_groupspawns sprite batches onto an individual representative entity of the batch. This poses significant problems for multi-camera setups, since an entity may appear in multiple phase instances.Solution
Instead, move batches into a resource that is keyed off the view and the representative entity. Long term we should switch to mesh2d and use the existing BinnedRenderPhase functionality rather than naively queueing into transparent and doing our own ad-hoc batching logic.
Fixes #16867, #17351
Testing
Tested repros in above issues.