Skip to content
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

[Merged by Bors] - Use a sorted Map for vertex buffer attributes #1796

Closed
wants to merge 2 commits into from

Conversation

mtsr
Copy link
Contributor

@mtsr mtsr commented Apr 1, 2021

The VertexBufferLayout returned by crates\bevy_render\src\mesh\mesh.rs:308 was unstable, because HashMap.iter() has a random order. This caused the pipeline_compiler to wrongly consider a specialization to be different (crates\bevy_render\src\pipeline\pipeline_compiler.rs:123), causing each mesh changed event to potentially result in a different PipelineSpecialization. This in turn caused Draw to emit a set_pipeline much more often than needed.

This fix shaves off a BindPipeline and two BindDescriptorSets (for the Camera and for global renderresources) for every mesh after the first that can now use the same specialization, where it didn't before (which was random).

StableHashMap was not a good replacement, because it isn't Clone, so instead I replaced it with a BTreeMap which is OK in this instance, because there shouldn't be many insertions on Mesh.attributes after the mesh is created.

@mtsr mtsr changed the title Use a sorted Map vertex buffer attributes Use a sorted Map for vertex buffer attributes Apr 1, 2021
@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times A-Rendering Drawing game state to the screen labels Apr 1, 2021
@@ -210,7 +210,7 @@ pub struct Mesh {
primitive_topology: PrimitiveTopology,
/// `bevy_utils::HashMap` with all defined vertex attributes (Positions, Normals, ...) for this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better?

@cart
Copy link
Member

cart commented Apr 13, 2021

Very nice catch! I think this is a reasonable change and I doubt the BTreeMap will register on benchmarks. In fact, hashing the full string might end up being more expensive than BTree insertions when the tree size is small.

@cart
Copy link
Member

cart commented Apr 13, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 13, 2021
The `VertexBufferLayout` returned by `crates\bevy_render\src\mesh\mesh.rs:308` was unstable, because `HashMap.iter()` has a random order. This caused the pipeline_compiler to wrongly consider a specialization to be different (`crates\bevy_render\src\pipeline\pipeline_compiler.rs:123`), causing each mesh changed event to potentially result in a different `PipelineSpecialization`. This in turn caused `Draw` to emit a `set_pipeline` much more often than needed.

This fix shaves off a `BindPipeline` and two `BindDescriptorSets` (for the Camera and for global renderresources) for every mesh after the first that can now use the same specialization, where it didn't before (which was random).

`StableHashMap` was not a good replacement, because it isn't `Clone`, so instead I replaced it with a `BTreeMap` which is OK in this instance, because there shouldn't be many insertions on `Mesh.attributes` after the mesh is created.
@bors bors bot changed the title Use a sorted Map for vertex buffer attributes [Merged by Bors] - Use a sorted Map for vertex buffer attributes Apr 13, 2021
@bors bors bot closed this Apr 13, 2021
jihiggins pushed a commit to jihiggins/bevy that referenced this pull request Apr 18, 2021
The `VertexBufferLayout` returned by `crates\bevy_render\src\mesh\mesh.rs:308` was unstable, because `HashMap.iter()` has a random order. This caused the pipeline_compiler to wrongly consider a specialization to be different (`crates\bevy_render\src\pipeline\pipeline_compiler.rs:123`), causing each mesh changed event to potentially result in a different `PipelineSpecialization`. This in turn caused `Draw` to emit a `set_pipeline` much more often than needed.

This fix shaves off a `BindPipeline` and two `BindDescriptorSets` (for the Camera and for global renderresources) for every mesh after the first that can now use the same specialization, where it didn't before (which was random).

`StableHashMap` was not a good replacement, because it isn't `Clone`, so instead I replaced it with a `BTreeMap` which is OK in this instance, because there shouldn't be many insertions on `Mesh.attributes` after the mesh is created.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
The `VertexBufferLayout` returned by `crates\bevy_render\src\mesh\mesh.rs:308` was unstable, because `HashMap.iter()` has a random order. This caused the pipeline_compiler to wrongly consider a specialization to be different (`crates\bevy_render\src\pipeline\pipeline_compiler.rs:123`), causing each mesh changed event to potentially result in a different `PipelineSpecialization`. This in turn caused `Draw` to emit a `set_pipeline` much more often than needed.

This fix shaves off a `BindPipeline` and two `BindDescriptorSets` (for the Camera and for global renderresources) for every mesh after the first that can now use the same specialization, where it didn't before (which was random).

`StableHashMap` was not a good replacement, because it isn't `Clone`, so instead I replaced it with a `BTreeMap` which is OK in this instance, because there shouldn't be many insertions on `Mesh.attributes` after the mesh is created.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants