-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add vertex buffer layout to Mesh
#3120
Conversation
82c6e58
to
8e15297
Compare
I'm not entirely happy with this, but it's working. Any suggestions will be much appreciated! |
The latest commit shows how to use the new resource for adding a custom vertex attribute. It reuses the For funsies, try adding a SPOILERIt will require the following patch to 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,
+ },
],
)
}; |
1911ed7
to
acfc2d2
Compare
Mesh
acfc2d2
to
31550e6
Compare
pipelined/bevy_render2/src/render_resource/pipeline_specializer.rs
Outdated
Show resolved
Hide resolved
430b56d
to
7cd4923
Compare
7cd4923
to
d58274d
Compare
@cart This is now rebased on the This supersedes my comment from last night where the caches had been on |
I think there is still an issue with adding arbitrary vertex attributes, because the |
It could be a good idea to also extend the shader preprocessor to provide functionality to automatically insert the mesh attributes in 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: |
Would this fix #3030 ? |
d58274d
to
ca4a167
Compare
I just fixed the issue with
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 |
ca4a167
to
b510eff
Compare
Rebased with support for |
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.
Is it ready to merge? |
b510eff
to
f6ab7c2
Compare
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 think this is on the right track. Just needs a few tweaks here and there and merge conflict resolutions.
examples/shader/shader_material.rs
Outdated
transform.rotate(Quat::from_rotation_y(angle * time.delta_seconds())); | ||
} | ||
|
||
fn cube_vertex_colors() -> Vec<[f32; 4]> { |
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'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.
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.
Cool, I'll move this to a new example.
pub enum VertexLayoutKey { | ||
Mesh(u64), | ||
Sprite(VertexLayoutSpriteKey), | ||
Ui, |
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 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.
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.
Do we want to store the attribute layouts on each individual sprite? On the sprite-batch? Somewhere else? Similar questions for UI.
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 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); |
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 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.
- 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
f6ab7c2
to
a159f2d
Compare
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:
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. |
Yes, |
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>
@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 |
Superseded by #3959. |
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>
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>
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 aBTreeMap
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:
Mesh
.Mesh
with the order provided by the vertex buffer layout on theMesh
itself.This will allow us to move forward with implementing skeletal animations (#95).
Solution
VertexBufferLayout
inSpecializedPipeline
implementations with layouts that can be configured at runtime onMesh
.Mesh
for the PBR renderer, and are cached by theRenderAsset
implementation forMesh
. TheSpritePipeline
caches its own vertex buffer layouts for the 2D renderer.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 publicMesh
methods.SpecializedPipeline
has a new cache reference accepted by thespecialize
method. This was the easiest way I found to gain access to the vertex buffer layout cache in the specialization compiler.TODO