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

Add vertex buffer layout to Mesh #3120

Closed

Conversation

parasyte
Copy link
Contributor

@parasyte parasyte commented Nov 13, 2021

Discussion: https://discord.com/channels/691052431525675048/743663924229963868/908484759833960489

Objective

Vertex buffer attributes for the Mesh shader are particularly problematic because the buffer is created by iterating a BTreeMap without regard to the vertex attribute locations defined in the shader.

I propose a method of describing vertex buffer layouts that are configurable at runtime. This (or something like it) will be necessary for extending render pipelines with additional vertex attributes.

This PR solves two problems:

  1. Replaces the hardcoded vertex buffer layout with one that can easily be adjusted at runtime on each individual Mesh.
  2. Replaces the original vertex buffer sorting (alphabetically by name) on Mesh with the order provided by the vertex buffer layout on the Mesh itself.

This will allow us to move forward with implementing skeletal animations (#95).

Solution

  • Replace the hardcoded VertexBufferLayout in SpecializedPipeline implementations with layouts that can be configured at runtime on Mesh.
  • Vertex buffer layouts are owned by Mesh for the PBR renderer, and are cached by the RenderAsset implementation for Mesh. The SpritePipeline caches its own vertex buffer layouts for the 2D renderer.
    • The choice for Mesh as the owner was made to accommodate the fact that it is responsible for building its own vertex buffer; it makes sense that it also owns the layout because it already knows which attributes it owns. It is easy to update through public Mesh methods.
  • SpecializedPipeline has a new cache reference accepted by the specialize method. This was the easiest way I found to gain access to the vertex buffer layout cache in the specialization compiler.

TODO

  • Use the pipeline cache for specialization

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Needs-Review C-Usability A simple quality-of-life change that makes Bevy easier to use labels Nov 13, 2021
@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch 2 times, most recently from 82c6e58 to 8e15297 Compare November 13, 2021 07:20
@parasyte
Copy link
Contributor Author

I'm not entirely happy with this, but it's working. Any suggestions will be much appreciated!

@parasyte
Copy link
Contributor Author

parasyte commented Nov 13, 2021

The latest commit shows how to use the new resource for adding a custom vertex attribute. It reuses the Mesh::ATTRIBUTE_COLOR name, but you can give your attributes any name.

For funsies, try adding a color vertex attribute to that example on the pipelined-rendering branch without this PR. :)

SPOILER

It will require the following patch to bevy_render2 (at a minimum! It doesn't handle the shader specialization case). Notice the offsets all have to change.

diff --git a/pipelined/bevy_pbr2/src/render/mod.rs b/pipelined/bevy_pbr2/src/render/mod.rs
index c1d08028..04ec199e 100644
--- a/pipelined/bevy_pbr2/src/render/mod.rs
+++ b/pipelined/bevy_pbr2/src/render/mod.rs
@@ -481,26 +481,32 @@ impl SpecializedPipeline for PbrPipeline {
                 )
             } else {
                 (
-                    32,
+                    48,
                     vec![
                         // Position (GOTCHA! Vertex_Position isn't first in the buffer due to how Mesh sorts attributes (alphabetically))
                         VertexAttribute {
                             format: VertexFormat::Float32x3,
-                            offset: 12,
+                            offset: 28,
                             shader_location: 0,
                         },
                         // Normal
                         VertexAttribute {
                             format: VertexFormat::Float32x3,
-                            offset: 0,
+                            offset: 16,
                             shader_location: 1,
                         },
                         // Uv
                         VertexAttribute {
                             format: VertexFormat::Float32x2,
-                            offset: 24,
+                            offset: 40,
                             shader_location: 2,
                         },
+                        // Color
+                        VertexAttribute {
+                            format: VertexFormat::Float32x4,
+                            offset: 0,
+                            shader_location: 3,
+                        },
                     ],
                 )
             };

@parasyte parasyte marked this pull request as ready for review November 13, 2021 18:18
@parasyte parasyte marked this pull request as draft November 13, 2021 18:33
@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch 5 times, most recently from 1911ed7 to acfc2d2 Compare November 16, 2021 19:23
@parasyte parasyte changed the title Add vertex buffer attribute descriptors to resolve sorting issues Add vertex buffer layout to Mesh Nov 16, 2021
@parasyte parasyte marked this pull request as ready for review November 16, 2021 20:56
@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch from acfc2d2 to 31550e6 Compare November 19, 2021 21:24
@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch 2 times, most recently from 430b56d to 7cd4923 Compare November 25, 2021 01:19
@parasyte parasyte changed the base branch from pipelined-rendering to main November 25, 2021 01:19
@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch from 7cd4923 to d58274d Compare November 25, 2021 01:22
@parasyte
Copy link
Contributor Author

@cart This is now rebased on the main branch. I'm using the RenderPipelineCache now, but it may not be exactly what you were expecting. Converting to/from specialization keys and vertex layout cache keys seemed like a nice way to do the vertex layout lookups. Unfortunately, I did have to change the specialize method signature to gain access to the pipeline cache. I don't know of a better way around that.

This supersedes my comment from last night where the caches had been on MeshPipeline and SpritePipeline respectively.

@parasyte
Copy link
Contributor Author

I think there is still an issue with adding arbitrary vertex attributes, because the VertexLayoutKey does not encode any information it doesn't know about at compile time. But this PR is starting to converge on a reasonable design.

@folke
Copy link
Contributor

folke commented Nov 26, 2021

It could be a good idea to also extend the shader preprocessor to provide functionality to automatically insert the mesh attributes in mesh.wsgl for Vertex, VertexOutput and FragmentInput.

Without that, it's still pretty difficult to add support for conditional attributes. See also what I did to add in ATTRIBUTE_COLOR to the pbr2 pipeline here:
https://github.com/bevyengine/bevy/blob/50fdff677e2c2afa6eadb3eab79aed29468bf099/pipelined/bevy_pbr2/src/render/mesh.wgsl

@mockersf
Copy link
Member

Would this fix #3030 ?

@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch from d58274d to ca4a167 Compare November 26, 2021 12:41
@parasyte
Copy link
Contributor Author

parasyte commented Nov 26, 2021

I just fixed the issue with VertexLayoutKey. Instead of compile-time bitflags, it now uses a u64 from the hasher. It's a bit more work to use with this change (copy the key from the GpuMesh instead of implicit From impls).

Would this fix #3030 ?

I tested the example in that issue against this branch, and it does indeed appear to resolve that bug. Vertex normals do not need to be specified because Mesh knows that they are required by the shader (and it knows how to build the buffer with the correct layout) so the normals are just zeroed.

@parasyte
Copy link
Contributor Author

Rebased with support for UiPipeline and WireframePipeline.

bors bot pushed a commit that referenced this pull request Dec 14, 2021
This makes the [New Bevy Renderer](#2535) the default (and only) renderer. The new renderer isn't _quite_ ready for the final release yet, but I want as many people as possible to start testing it so we can identify bugs and address feedback prior to release.

The examples are all ported over and operational with a few exceptions:

* I removed a good portion of the examples in the `shader` folder. We still have some work to do in order to make these examples possible / ergonomic / worthwhile: #3120 and "high level shader material plugins" are the big ones. This is a temporary measure.
* Temporarily removed the multiple_windows example: doing this properly in the new renderer will require the upcoming "render targets" changes. Same goes for the render_to_texture example.
* Removed z_sort_debug: entity visibility sort info is no longer available in app logic. we could do this on the "render app" side, but i dont consider it a priority.
@greenmughal
Copy link

Is it ready to merge?

@parasyte parasyte force-pushed the features/vertex-buffer-descriptors branch from b510eff to f6ab7c2 Compare December 16, 2021 20:47
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is on the right track. Just needs a few tweaks here and there and merge conflict resolutions.

transform.rotate(Quat::from_rotation_y(angle * time.delta_seconds()));
}

fn cube_vertex_colors() -> Vec<[f32; 4]> {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to not complicate this example with more things on top of what it already does. Custom vertex attributes should be a separate example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll move this to a new example.

pub enum VertexLayoutKey {
Mesh(u64),
Sprite(VertexLayoutSpriteKey),
Ui,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should hard code layouts like this. The UI vertex layout shouldn't be any more "special" than the layout defined in a 3rd party crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to store the attribute layouts on each individual sprite? On the sprite-batch? Somewhere else? Similar questions for UI.

Copy link
Member

Choose a reason for hiding this comment

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

I think UI and sprite layouts should be hard coded for now in the specialize function / not a part of the key:

  • different layouts would need different buffers alongside different batches. this would require more restructuring than im comfortable with at this point in the game.
  • when drawing hundreds of thousands of sprites, per-sprite CPU usage becomes a real problem. adding more work to the key hash would probably register on benchmarks

let vertex_layout = mesh.vertex_layout();

let mut hasher = AHasher::default();
vertex_layout.hash(&mut hasher);
Copy link
Member

Choose a reason for hiding this comment

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

I think the RenderPipelineCache should be responsible for handling vertex layout hashing. A method like insert_vertex_layout should return a VertexLayoutKey when given a vertex layout. This reduces the complexity for callers and ensures consistent, correct hashes. I'd also prefer it if this lived directly on RenderPipelineCache instead of exposing a public vertex_layout_cache field. Seems simpler / more discoverable.

@cart cart added this to the Bevy 0.6 milestone Dec 23, 2021
- Vertex buffer layouts are set on `Mesh` for shader specialization.
  `Mesh` knows how to specialize its own vertex buffer because it owns
  the vertex data.
- The `RenderAsset` implementation for `Mesh` caches the vertex buffer
  layout and adds a cache key to the `GpuMesh` render asset. The
  `MeshPipeline` can lookup the vertex buffer layout by composing
  `VertexLayoutKey` within `MeshPipelineKey`.
- `SpritePipeline` and `UiPipeline` populates the vertex layout cache
  for themselves..
- `SpecializedPipeline::specialize` now takes a reference to the
  `RenderPipelineCache`, which is the best way I could find to get a
  reference to the cache for vertex layout lookups.
- Discussion: https://discord.com/channels/691052431525675048/743663924229963868/908484759833960489
@cart
Copy link
Member

cart commented Jan 8, 2022

Today I spent some time refactoring this to be closer to what my ideal solution looks like (and I addressed some of my own comments). There are a number of changes to make and discuss:

  • "source meshes" and vertex buffer layouts should be decoupled. there is a one-to-many relationship between mesh buffers and vertex buffer layouts (layouts can be arbitrarily defined by shaders). We can store a "standard layout" on the gpu mesh for simplicity / compatibility with built-in pipelines, but direct coupling feels wrong. I've reverted a good portion of the changes to Mesh as a result. I likewise reverted the "name" addition to VertexBufferLayout. But we might want to re-add that, depending on how we decide to handle the one-to-many relationship.
  • I addressed some of my own comments by simplifying the layout key and fully abstracting out the layout cache.
  • For simplicity, I think we might want to remove RenderPipelineCache from the specialize arguments. I'd like to experiment with replacing VertexBufferLayout with VertexBufferLayoutKey in the RenderPipelineDescriptor.
  • We still need to resolve conflicts with main

Given that the 0.6 release is happening tomorrow, I'm removing this from the milestone. I'm not comfortable merging this as is. However I acknowledge that this is an important feature that a number of people are reliant on. I'll prioritize finishing up my proposed changes and merging a solution into main as soon as possible after the 0.6 release. We can also discuss doing a quick followup "point release" if there is interest in that.

@cart cart removed this from the Bevy 0.6 milestone Jan 8, 2022
@parasyte
Copy link
Contributor Author

parasyte commented Jan 8, 2022

  • We still need to resolve conflicts with main

Yes, main moves quickly. I have been trying to keep up, but I'm not willing to rebase daily if this is not going anywhere.

@cart
Copy link
Member

cart commented Jan 8, 2022

Yup you've been doing a great job. This PR dragging on is 100% my fault. Lots of things to balance unfortunately.

Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
@BGR360
Copy link
Contributor

BGR360 commented Feb 8, 2022

Today I spent some time refactoring this to be closer to what my ideal solution looks like

@cart Maybe noob question, but where can I find these changes you've made to the PR? Are they somewhere on your fork of bevy? I'm interested in seeing the way it's structured after your changes

@parasyte
Copy link
Contributor Author

Superseded by #3959.

@parasyte parasyte closed this Feb 16, 2022
@parasyte parasyte deleted the features/vertex-buffer-descriptors branch February 16, 2022 16:54
bors bot pushed a commit that referenced this pull request Feb 23, 2022
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 <mcanders1@gmail.com>
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
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 <mcanders1@gmail.com>
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-Bug An unexpected or incorrect behavior C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants