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
cart
left a comment
There was a problem hiding this comment.
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.
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.
Cool, I'll move this to a new example.
| pub enum VertexLayoutKey { | ||
| Mesh(u64), | ||
| Sprite(VertexLayoutSpriteKey), | ||
| Ui, |
There was a problem hiding this comment.
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.
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.
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.
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 <[email protected]>
@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 <[email protected]>
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]>
Discussion: https://discord.com/channels/691052431525675048/743663924229963868/908484759833960489
Objective
Vertex buffer attributes for the
Meshshader are particularly problematic because the buffer is created by iterating aBTreeMapwithout 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.Meshwith the order provided by the vertex buffer layout on theMeshitself.This will allow us to move forward with implementing skeletal animations (#95).
Solution
VertexBufferLayoutinSpecializedPipelineimplementations with layouts that can be configured at runtime onMesh.Meshfor the PBR renderer, and are cached by theRenderAssetimplementation forMesh. TheSpritePipelinecaches its own vertex buffer layouts for the 2D renderer.Meshas 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 publicMeshmethods.SpecializedPipelinehas a new cache reference accepted by thespecializemethod. This was the easiest way I found to gain access to the vertex buffer layout cache in the specialization compiler.TODO