[Merged by Bors] - Mesh vertex buffer layouts #3959

cart · 2022-02-16T06:12:07Z

This PR makes a number of changes to how meshes and vertex attributes are handled, with the goal of enabling easy and flexible custom vertex attributes:

Reworks the Mesh type to use the newly added VertexAttribute internally
- VertexAttribute defines the name, a unique VertexAttributeId, and a VertexFormat
- VertexAttributeId is used to produce consistent sort orders for vertex buffer generation, replacing the more expensive and often surprising "name based sorting"
- Meshes can be used to generate a MeshVertexBufferLayout, which defines the layout of the gpu buffer produced by the mesh. MeshVertexBufferLayouts can then be used to generate actual VertexBufferLayouts according to the requirements of a specific pipeline. This decoupling of "mesh layout" vs "pipeline vertex buffer layout" is what enables custom attributes. We don't need to standardize mesh layouts or contort meshes to meet the needs of a specific pipeline. As long as the mesh has what the pipeline needs, it will work transparently.
Mesh-based pipelines now specialize on &MeshVertexBufferLayout via the new SpecializedMeshPipeline trait (which behaves like SpecializedPipeline, but adds &MeshVertexBufferLayout). The integrity of the pipeline cache is maintained because the MeshVertexBufferLayout is treated as part of the key (which is fully abstracted from implementers of the trait ... no need to add any additional info to the specialization key).
Hashing MeshVertexBufferLayout is too expensive to do for every entity, every frame. To make this scalable, I added a generalized "pre-hashing" solution to bevy_utils: Hashed<T> keys and PreHashMap<K, V> (which uses Hashed<T> internally) . Why didn't I just do the quick and dirty in-place "pre-compute hash and use that u64 as a key in a hashmap" that we've done in the past? Because its wrong! Hashes by themselves aren't enough because two different values can produce the same hash. Re-hashing a hash is even worse! I decided to build a generalized solution because this pattern has come up in the past and we've chosen to do the wrong thing. Now we can do the right thing! This did unfortunately require pulling in hashbrown and using that in bevy_utils, because avoiding re-hashes requires the raw_entry_mut api, which isn't stabilized yet (and may never be ... entry_ref has favor now, but also isn't available yet). If std's HashMap ever provides the tools we need, we can move back to that. Note that adding hashbrown doesn't increase our dependency count because it was already in our tree. I will probably break these changes out into their own PR.
Specializing on MeshVertexBufferLayout has one non-obvious behavior: it can produce identical pipelines for two different MeshVertexBufferLayouts. To optimize the number of active pipelines / reduce re-binds while drawing, I de-duplicate pipelines post-specialization using the final VertexBufferLayout as the key. For example, consider a pipeline that needs the layout (position, normal) and is specialized using two meshes: (position, normal, uv) and (position, normal, other_vec2). If both of these meshes result in (position, normal) specializations, we can use the same pipeline! Now we do. Cool!

To briefly illustrate, this is what the relevant section of MeshPipeline's specialization code looks like now:

impl SpecializedMeshPipeline for MeshPipeline {
    type Key = MeshPipelineKey;

    fn specialize(
        &self,
        key: Self::Key,
        layout: &MeshVertexBufferLayout,
    ) -> RenderPipelineDescriptor {
        let mut vertex_attributes = vec![
            Mesh::ATTRIBUTE_POSITION.at_shader_location(0),
            Mesh::ATTRIBUTE_NORMAL.at_shader_location(1),
            Mesh::ATTRIBUTE_UV_0.at_shader_location(2),
        ];

        let mut shader_defs = Vec::new();
        if layout.contains(Mesh::ATTRIBUTE_TANGENT) {
            shader_defs.push(String::from("VERTEX_TANGENTS"));
            vertex_attributes.push(Mesh::ATTRIBUTE_TANGENT.at_shader_location(3));
        }

        let vertex_buffer_layout = layout
            .get_layout(&vertex_attributes)
            .expect("Mesh is missing a vertex attribute");

Notice that this is much simpler than it was before. And now any mesh with any layout can be used with this pipeline, provided it has vertex postions, normals, and uvs. We even got to remove HAS_TANGENTS from MeshPipelineKey and has_tangents from GpuMesh, because that information is redundant with MeshVertexBufferLayout.

This is still a draft because I still need to:

Add more docs
Experiment with adding error handling to mesh pipeline specialization (which would print errors at runtime when a mesh is missing a vertex attribute required by a pipeline). If it doesn't tank perf, we'll keep it.
Consider breaking out the PreHash / hashbrown changes into a separate PR.
Add an example illustrating this change
Verify that the "mesh-specialized pipeline de-duplication code" works properly

Please dont yell at me for not doing these things yet :) Just trying to get this in peoples' hands asap.

Alternative to #3120
Fixes #3030

cart · 2022-02-16T06:14:38Z

Note that while this is effectively 100% new code and a new approach, I left @parasyte in the commit credits because this started as a transformation of #3120, it builds on some of the ideas in that pr, and I want to honor that.

superdump · 2022-02-16T17:42:35Z

Awes. Ome. Based in the description, I love it. I’ll have a look over the code when I can.

BGR360 · 2022-02-16T19:24:39Z

Woohoo! 🎉

Why didn't I just do the quick and dirty in-place "pre-compute hash and use that u64 as a key in a hashmap" that we've done in the past? Because its wrong! Hashes by themselves aren't enough because two different values can produce the same hash.

It's funny, I had to make the exact same realization recently for a project I'm working on, and I realized it almost too late! The solution I opted for was using indexmap. In that case the "pre-hashed key" that you use after insertion is just a stable index into a vector of (key, value) pairs.

cart · 2022-02-16T23:14:58Z

Another note: I compared many_cubes perf (with 10,000 cubes and 40,000 cubes) on main vs this pr. For whatever reason, this PR is marginally faster. Nothing to write home about, but it was measurable. What matters more is that this added functionality doesnt regress performance, from what I can tell.

superdump

I didn't look at the Hashed stuff here as that is in a separate PR.

I like what you've done. I'd just like the errors to be actionable so maybe instead of an Option, MeshVertexBufferLayout::get_layout could return a Result with some error that contains information about the missing/incorrect attributes to make them easy to log.

crates/bevy_pbr/src/render/light.rs

crates/bevy_pbr/src/render/mesh.rs

crates/bevy_render/src/render_resource/pipeline.rs

crates/bevy_sprite/src/mesh2d/mesh.rs

For some keys, it is too expensive to hash them on every lookup. Historically in Bevy, we have regrettably done the "wrong" thing in these cases (pre-computing hashes, then re-hashing them) because Rust's built in hashed collections don't give us the tools we need to do otherwise. Doing this is "wrong" because two different values can result in the same hash. Hashed collections generally get around this by falling back to equality checks on hash collisions. You can't do that if the key _is_ the hash. Additionally, re-hashing a hash increase the odds of collision! #3959 needs pre-hashing to be viable, so I decided to finally properly solve the problem. The solution involves two different changes: 1. A new generalized "pre-hashing" solution in bevy_utils: `Hashed<T>` types, which store a value alongside a pre-computed hash. And `PreHashMap<K, V>` (which uses `Hashed<T>` internally) . `PreHashMap` is just an alias for a normal HashMap that uses `Hashed<T>` as the key and a new `PassHash` implementation as the Hasher. 2. Replacing the `std::collections` re-exports in `bevy_utils` with equivalent `hashbrown` impls. Avoiding re-hashes requires the `raw_entry_mut` api, which isn't stabilized yet (and may never be ... `entry_ref` has favor now, but also isn't available yet). If std's HashMap ever provides the tools we need, we can move back to that. The latest version of `hashbrown` adds support for the `entity_ref` api, so we can move to that in preparation for an std migration, if thats the direction they seem to be going in. Note that adding hashbrown doesn't increase our dependency count because it was already in our tree. In addition to providing these core tools, I also ported the "table identity hashing" in `bevy_ecs` to `raw_entry_mut`, which was a particularly egregious case. The biggest outstanding case is `AssetPathId`, which stores a pre-hash. We need AssetPathId to be cheaply clone-able (and ideally Copy), but `Hashed<AssetPath>` requires ownership of the AssetPath, which makes cloning ids way more expensive. We could consider doing `Hashed<Arc<AssetPath>>`, but cloning an arc is still a non-trivial expensive that needs to be considered. I would like to handle this in a separate PR. And given that we will be re-evaluating the Bevy Assets implementation in the very near future, I'd prefer to hold off until after that conversation is concluded.

cart · 2022-02-18T05:05:10Z

Rebased on main now that #3963 is merged.

cart · 2022-02-19T00:37:23Z

Added (non-crashy) error reporting for meshes missing attributes required by a pipeline:

crates/bevy_pbr/src/material.rs

examples/shader/custom_vertex_attribute.rs

superdump

Were the vertex tangents flags updated in the shaders? Is difficult for me to check on my phone.

crates/bevy_pbr/src/material.rs

crates/bevy_render/src/mesh/mesh/mod.rs

crates/bevy_sprite/src/mesh2d/material.rs

cart · 2022-02-23T21:31:38Z

@superdump

Were the vertex tangents flags updated in the shaders? Is difficult for me to check on my phone.

No need. We don't write those mesh flags to uniforms or use them in shaders. The shaders just use a "shader def" to check for vertex tangents. I think you're thinking of the MeshFlags struct, which hasn't changed.

cart · 2022-02-23T23:20:55Z

bors r+

This PR makes a number of changes to how meshes and vertex attributes are handled, which the goal of enabling easy and flexible custom vertex attributes: * Reworks the `Mesh` type to use the newly added `VertexAttribute` internally * `VertexAttribute` defines the name, a unique `VertexAttributeId`, and a `VertexFormat` * `VertexAttributeId` is used to produce consistent sort orders for vertex buffer generation, replacing the more expensive and often surprising "name based sorting" * Meshes can be used to generate a `MeshVertexBufferLayout`, which defines the layout of the gpu buffer produced by the mesh. `MeshVertexBufferLayouts` can then be used to generate actual `VertexBufferLayouts` according to the requirements of a specific pipeline. This decoupling of "mesh layout" vs "pipeline vertex buffer layout" is what enables custom attributes. We don't need to standardize _mesh layouts_ or contort meshes to meet the needs of a specific pipeline. As long as the mesh has what the pipeline needs, it will work transparently. * Mesh-based pipelines now specialize on `&MeshVertexBufferLayout` via the new `SpecializedMeshPipeline` trait (which behaves like `SpecializedPipeline`, but adds `&MeshVertexBufferLayout`). The integrity of the pipeline cache is maintained because the `MeshVertexBufferLayout` is treated as part of the key (which is fully abstracted from implementers of the trait ... no need to add any additional info to the specialization key). * Hashing `MeshVertexBufferLayout` is too expensive to do for every entity, every frame. To make this scalable, I added a generalized "pre-hashing" solution to `bevy_utils`: `Hashed<T>` keys and `PreHashMap<K, V>` (which uses `Hashed<T>` internally) . Why didn't I just do the quick and dirty in-place "pre-compute hash and use that u64 as a key in a hashmap" that we've done in the past? Because its wrong! Hashes by themselves aren't enough because two different values can produce the same hash. Re-hashing a hash is even worse! I decided to build a generalized solution because this pattern has come up in the past and we've chosen to do the wrong thing. Now we can do the right thing! This did unfortunately require pulling in `hashbrown` and using that in `bevy_utils`, because avoiding re-hashes requires the `raw_entry_mut` api, which isn't stabilized yet (and may never be ... `entry_ref` has favor now, but also isn't available yet). If std's HashMap ever provides the tools we need, we can move back to that. Note that adding `hashbrown` doesn't increase our dependency count because it was already in our tree. I will probably break these changes out into their own PR. * Specializing on `MeshVertexBufferLayout` has one non-obvious behavior: it can produce identical pipelines for two different MeshVertexBufferLayouts. To optimize the number of active pipelines / reduce re-binds while drawing, I de-duplicate pipelines post-specialization using the final `VertexBufferLayout` as the key. For example, consider a pipeline that needs the layout `(position, normal)` and is specialized using two meshes: `(position, normal, uv)` and `(position, normal, other_vec2)`. If both of these meshes result in `(position, normal)` specializations, we can use the same pipeline! Now we do. Cool! To briefly illustrate, this is what the relevant section of `MeshPipeline`'s specialization code looks like now: ```rust impl SpecializedMeshPipeline for MeshPipeline { type Key = MeshPipelineKey; fn specialize( &self, key: Self::Key, layout: &MeshVertexBufferLayout, ) -> RenderPipelineDescriptor { let mut vertex_attributes = vec![ Mesh::ATTRIBUTE_POSITION.at_shader_location(0), Mesh::ATTRIBUTE_NORMAL.at_shader_location(1), Mesh::ATTRIBUTE_UV_0.at_shader_location(2), ]; let mut shader_defs = Vec::new(); if layout.contains(Mesh::ATTRIBUTE_TANGENT) { shader_defs.push(String::from("VERTEX_TANGENTS")); vertex_attributes.push(Mesh::ATTRIBUTE_TANGENT.at_shader_location(3)); } let vertex_buffer_layout = layout .get_layout(&vertex_attributes) .expect("Mesh is missing a vertex attribute"); ``` Notice that this is _much_ simpler than it was before. And now any mesh with any layout can be used with this pipeline, provided it has vertex postions, normals, and uvs. We even got to remove `HAS_TANGENTS` from MeshPipelineKey and `has_tangents` from `GpuMesh`, because that information is redundant with `MeshVertexBufferLayout`. This is still a draft because I still need to: * Add more docs * Experiment with adding error handling to mesh pipeline specialization (which would print errors at runtime when a mesh is missing a vertex attribute required by a pipeline). If it doesn't tank perf, we'll keep it. * Consider breaking out the PreHash / hashbrown changes into a separate PR. * Add an example illustrating this change * Verify that the "mesh-specialized pipeline de-duplication code" works properly Please dont yell at me for not doing these things yet :) Just trying to get this in peoples' hands asap. Alternative to #3120 Fixes #3030 Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-02-23T23:50:00Z

Pull request successfully merged into main.

Build succeeded:

BGR360 · 2022-02-24T00:44:01Z

👀 👀 👀 🎉 🎉 🎉

This PR makes a number of changes to how meshes and vertex attributes are handled, which the goal of enabling easy and flexible custom vertex attributes: * Reworks the `Mesh` type to use the newly added `VertexAttribute` internally * `VertexAttribute` defines the name, a unique `VertexAttributeId`, and a `VertexFormat` * `VertexAttributeId` is used to produce consistent sort orders for vertex buffer generation, replacing the more expensive and often surprising "name based sorting" * Meshes can be used to generate a `MeshVertexBufferLayout`, which defines the layout of the gpu buffer produced by the mesh. `MeshVertexBufferLayouts` can then be used to generate actual `VertexBufferLayouts` according to the requirements of a specific pipeline. This decoupling of "mesh layout" vs "pipeline vertex buffer layout" is what enables custom attributes. We don't need to standardize _mesh layouts_ or contort meshes to meet the needs of a specific pipeline. As long as the mesh has what the pipeline needs, it will work transparently. * Mesh-based pipelines now specialize on `&MeshVertexBufferLayout` via the new `SpecializedMeshPipeline` trait (which behaves like `SpecializedPipeline`, but adds `&MeshVertexBufferLayout`). The integrity of the pipeline cache is maintained because the `MeshVertexBufferLayout` is treated as part of the key (which is fully abstracted from implementers of the trait ... no need to add any additional info to the specialization key). * Hashing `MeshVertexBufferLayout` is too expensive to do for every entity, every frame. To make this scalable, I added a generalized "pre-hashing" solution to `bevy_utils`: `Hashed<T>` keys and `PreHashMap<K, V>` (which uses `Hashed<T>` internally) . Why didn't I just do the quick and dirty in-place "pre-compute hash and use that u64 as a key in a hashmap" that we've done in the past? Because its wrong! Hashes by themselves aren't enough because two different values can produce the same hash. Re-hashing a hash is even worse! I decided to build a generalized solution because this pattern has come up in the past and we've chosen to do the wrong thing. Now we can do the right thing! This did unfortunately require pulling in `hashbrown` and using that in `bevy_utils`, because avoiding re-hashes requires the `raw_entry_mut` api, which isn't stabilized yet (and may never be ... `entry_ref` has favor now, but also isn't available yet). If std's HashMap ever provides the tools we need, we can move back to that. Note that adding `hashbrown` doesn't increase our dependency count because it was already in our tree. I will probably break these changes out into their own PR. * Specializing on `MeshVertexBufferLayout` has one non-obvious behavior: it can produce identical pipelines for two different MeshVertexBufferLayouts. To optimize the number of active pipelines / reduce re-binds while drawing, I de-duplicate pipelines post-specialization using the final `VertexBufferLayout` as the key. For example, consider a pipeline that needs the layout `(position, normal)` and is specialized using two meshes: `(position, normal, uv)` and `(position, normal, other_vec2)`. If both of these meshes result in `(position, normal)` specializations, we can use the same pipeline! Now we do. Cool! To briefly illustrate, this is what the relevant section of `MeshPipeline`'s specialization code looks like now: ```rust impl SpecializedMeshPipeline for MeshPipeline { type Key = MeshPipelineKey; fn specialize( &self, key: Self::Key, layout: &MeshVertexBufferLayout, ) -> RenderPipelineDescriptor { let mut vertex_attributes = vec![ Mesh::ATTRIBUTE_POSITION.at_shader_location(0), Mesh::ATTRIBUTE_NORMAL.at_shader_location(1), Mesh::ATTRIBUTE_UV_0.at_shader_location(2), ]; let mut shader_defs = Vec::new(); if layout.contains(Mesh::ATTRIBUTE_TANGENT) { shader_defs.push(String::from("VERTEX_TANGENTS")); vertex_attributes.push(Mesh::ATTRIBUTE_TANGENT.at_shader_location(3)); } let vertex_buffer_layout = layout .get_layout(&vertex_attributes) .expect("Mesh is missing a vertex attribute"); ``` Notice that this is _much_ simpler than it was before. And now any mesh with any layout can be used with this pipeline, provided it has vertex postions, normals, and uvs. We even got to remove `HAS_TANGENTS` from MeshPipelineKey and `has_tangents` from `GpuMesh`, because that information is redundant with `MeshVertexBufferLayout`. This is still a draft because I still need to: * Add more docs * Experiment with adding error handling to mesh pipeline specialization (which would print errors at runtime when a mesh is missing a vertex attribute required by a pipeline). If it doesn't tank perf, we'll keep it. * Consider breaking out the PreHash / hashbrown changes into a separate PR. * Add an example illustrating this change * Verify that the "mesh-specialized pipeline de-duplication code" works properly Please dont yell at me for not doing these things yet :) Just trying to get this in peoples' hands asap. Alternative to bevyengine#3120 Fixes bevyengine#3030 Co-authored-by: Carter Anderson <[email protected]>

# Objective - Fixes #8399 The documentation was out of date and misleading since #3959.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 16, 2022

cart marked this pull request as draft February 16, 2022 06:15

cart added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Feb 16, 2022

mockersf mentioned this pull request Feb 16, 2022

UV of Float32x3 causes GPU buffer issue in 0.6.0 (works fine in 0.5.0) #3604

Open

parasyte mentioned this pull request Feb 16, 2022

Add vertex buffer layout to Mesh #3120

Closed

1 task

cart mentioned this pull request Feb 16, 2022

[Merged by Bors] - Proper prehashing #3963

Closed

superdump requested changes Feb 17, 2022

View reviewed changes

Mesh refactor and mesh pipeline specialization

fb43380

cart force-pushed the mesh-vertex-buffer-layouts branch from 4ee408f to fb43380 Compare February 18, 2022 05:04

Add error handling for missing vertex attributes

4b289e6

cart added 5 commits February 18, 2022 16:38

use offset instead of array_stride

1a76752

add doc links

03015aa

fix examples

b7160df

clippy

b566d5e

custom vertex attribute example

82aaea7

cart marked this pull request as ready for review February 22, 2022 22:51

fix set_attribute docs

29afa52

alice-i-cecile reviewed Feb 22, 2022

View reviewed changes

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved

tuple keys -> struct keys

c1ad89c

Toqozz mentioned this pull request Feb 22, 2022

Adding a PointLight crashes the app Toqozz/bevy_debug_lines#16

Closed

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 22, 2022

rparrett reviewed Feb 22, 2022

View reviewed changes

examples/shader/custom_vertex_attribute.rs Outdated Show resolved Hide resolved

superdump reviewed Feb 23, 2022

View reviewed changes

crates/bevy_pbr/src/material.rs Show resolved Hide resolved

crates/bevy_render/src/mesh/mesh/mod.rs Show resolved Hide resolved

crates/bevy_render/src/mesh/mesh/mod.rs Outdated Show resolved Hide resolved

crates/bevy_sprite/src/mesh2d/material.rs Outdated Show resolved Hide resolved

move comment

4493f0f

cart added 2 commits February 23, 2022 14:52

fix "unused but used" variable names

2bae927

Check for invalid pipeline cache entries

46f24a2

bors bot changed the title ~~Mesh vertex buffer layouts~~ [Merged by Bors] - Mesh vertex buffer layouts Feb 23, 2022

bors bot closed this Feb 23, 2022

superdump mentioned this pull request Feb 24, 2022

bevy_pbr2: added vertex color support #3187

Closed

mockersf mentioned this pull request Feb 25, 2022

example mesh2d_manual crash #4036

Closed

This was referenced Apr 17, 2022

Bevy 0.7 Toqozz/bevy_debug_lines#17

Closed

Bevy 0.7 cryscan/bevy-hikari#6

Closed

MaterialPipelineKey should have public fields #4507

Closed

This was referenced Apr 16, 2023

Documentation is out of date on Mesh::get_vertex_buffer_data #8399

Closed

Fix get_vertex_buffer_data doc #8400

Merged

alice-i-cecile pushed a commit that referenced this pull request Apr 17, 2023

Fix get_vertex_buffer_data doc (#8400)

ebc6446

# Objective - Fixes #8399 The documentation was out of date and misleading since #3959.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Mesh vertex buffer layouts #3959

[Merged by Bors] - Mesh vertex buffer layouts #3959

cart commented Feb 16, 2022 •

edited

Loading

cart commented Feb 16, 2022

superdump commented Feb 16, 2022

BGR360 commented Feb 16, 2022

cart commented Feb 16, 2022

superdump left a comment

cart commented Feb 18, 2022

cart commented Feb 19, 2022 •

edited

Loading

superdump left a comment

cart commented Feb 23, 2022

cart commented Feb 23, 2022

bors bot commented Feb 23, 2022

BGR360 commented Feb 24, 2022

[Merged by Bors] - Mesh vertex buffer layouts #3959

[Merged by Bors] - Mesh vertex buffer layouts #3959

Conversation

cart commented Feb 16, 2022 • edited Loading

cart commented Feb 16, 2022

superdump commented Feb 16, 2022

BGR360 commented Feb 16, 2022

cart commented Feb 16, 2022

superdump left a comment

Choose a reason for hiding this comment

cart commented Feb 18, 2022

cart commented Feb 19, 2022 • edited Loading

superdump left a comment

Choose a reason for hiding this comment

cart commented Feb 23, 2022

cart commented Feb 23, 2022

bors bot commented Feb 23, 2022

BGR360 commented Feb 24, 2022

cart commented Feb 16, 2022 •

edited

Loading

cart commented Feb 19, 2022 •

edited

Loading