Fix: Provide CPU mesh processing with MaterialBindingId #19083
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
dsgallups
changed the title
alice-i-cecile
added
C-Bug
alice-i-cecile
added
S-Needs-Review
crates/bevy_pbr/src/render/mesh.rs
Outdated
| .unwrap_or_default() | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let shared = RenderMeshInstanceShared::from_components( |
There was a problem hiding this comment.
Instead of supercharging the from_components method with the material_bindings_index, it may be clearer to introduce a new method (for_cpu_building or from_components_and_material_binding_index or with_material_binding_index, ...) that takes the material_bindings_index as an additional parameter, and use this new method here.
The previous from_components would just internally call this new method and give it default() for the material_bindings_index. This version of from_components could be renamed to for_gpu_building or without_material_binding_index.
This way the comment about overriding the binding index would have its own place in the without_material_binding_index method specifically.
There was a problem hiding this comment.
Ok, I've followed these comments. Can you verify this is what you described? Thanks!
Co-authored-by: Gilles Henaux <[email protected]>
Using @Henauxg's request to create a new constructor for `RenderMeshInstanceShared`. The `RenderMeshInstanceShare::from_components` now calls this method with default index id.
Added comments that appear correct on the surface. I'm *pretty sure* this is accurate.
| /// Unlike the gpu builder, the cpu builder does not have an equivalent | ||
| /// [`RenderMeshGpuBuilder::update`]. Therefore, we need to have | ||
| /// the [`MaterialBindingId`] ahead of time. | ||
| /// | ||
| /// Otherwise, a mesh with a handle to a base color texture will all appear identical | ||
| /// based on the most recently inserted mesh. |
There was a problem hiding this comment.
Need a verification that this comment is true. That there really isn't an equivalent...and if there isn't, well...I need to find out
There was a problem hiding this comment.
I don't really think we need to document what will happen if this code doesn't work. In general, I think these comments might be saying a bit too much. It's important to document on the gpu side that this will be updated later but its's clear from the usage at the call site that we retrieve the binding id ahead of time.
| let material_bindings_index = (mesh_material != DUMMY_MESH_MATERIAL.untyped()) | ||
| .then(|| { | ||
| render_material_bindings | ||
| .get(&mesh_material) | ||
| .copied() | ||
| .unwrap_or_default() | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
should we be unwrap_or_defaulting here?
There was a problem hiding this comment.
I'm not really sure what the check for DUMMY_MESH_MATERIAL is doing here, is it's presumably already effectively covered by the unwrap_or_default when you call get on render_material_bindings -- if a dummy asset is being used we'll default there, which is the right call.
| /// during `RenderMeshGpuBuilder::update`. | ||
| /// | ||
| /// Therefore, this does not need to be queried. | ||
| #[inline] |
There was a problem hiding this comment.
Unnecessary inline, see #2 https://matklad.github.io/2021/07/09/inline-in-rust.html#Inlining-in-Practice
| /// Unlike the gpu builder, the cpu builder does not have an equivalent | ||
| /// [`RenderMeshGpuBuilder::update`]. Therefore, we need to have | ||
| /// the [`MaterialBindingId`] ahead of time. | ||
| /// | ||
| /// Otherwise, a mesh with a handle to a base color texture will all appear identical | ||
| /// based on the most recently inserted mesh. |
There was a problem hiding this comment.
I don't really think we need to document what will happen if this code doesn't work. In general, I think these comments might be saying a bit too much. It's important to document on the gpu side that this will be updated later but its's clear from the usage at the call site that we retrieve the binding id ahead of time.
| let material_bindings_index = (mesh_material != DUMMY_MESH_MATERIAL.untyped()) | ||
| .then(|| { | ||
| render_material_bindings | ||
| .get(&mesh_material) | ||
| .copied() | ||
| .unwrap_or_default() | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
I'm not really sure what the check for DUMMY_MESH_MATERIAL is doing here, is it's presumably already effectively covered by the unwrap_or_default when you call get on render_material_bindings -- if a dummy asset is being used we'll default there, which is the right call.
| @@ -837,12 +837,43 @@ pub struct RenderMeshInstanceGpuQueues(Parallel<RenderMeshInstanceGpuQueue>); | |||
| pub struct MeshesToReextractNextFrame(MainEntityHashSet); | |||
|
|
|||
| impl RenderMeshInstanceShared { | |||
| fn from_components( | |||
| /// A gpu builder will provide the mesh instance id | |||
| /// during `RenderMeshGpuBuilder::update`. | |||
There was a problem hiding this comment.
| /// during `RenderMeshGpuBuilder::update`. | |
| /// during `RenderMeshInstanceGpuBuilder::update`. |
| @@ -1305,6 +1335,8 @@ pub struct ExtractMeshesSet; | |||
| /// [`MeshUniform`] building. | |||
| pub fn extract_meshes_for_cpu_building( | |||
| mut render_mesh_instances: ResMut<RenderMeshInstances>, | |||
| mesh_material_ids: Res<RenderMaterialInstances>, | |||
There was a problem hiding this comment.
Note: these appear to be correctly ordered via the constraints on late_sweep_material_instances.
Objective
Fixes #19027
Solution
Query for the material binding id if using fallback CPU processing
Testing
I've honestly no clue how to test for this, and I imagine that this isn't entirely failsafe :( but would highly appreciate a suggestion!
To verify this works, please run the the texture.rs example using WebGL 2.
Additionally, I'm extremely naive about the nuances of pbr. This PR is essentially to kinda get the ball rolling of sorts. Thanks :)