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
| 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.
There was a problem hiding this comment.
Sorry, to be more clear, is there a reason this can't just be simplified to:
render_material_bindings
.get(&mesh_material)
.copied()
.unwrap_or_default()Maybe I'm missing something.
There was a problem hiding this comment.
Now that you've mentioned it...I've taken this from
I feel like the changes made in this PR wouldn't resolve the case where the mesh is the dummy material...there is no mesh to re-extract next frame for cpus I believe...so this fix might be concealing a bug
crates/bevy_pbr/src/render/mesh.rs
Outdated
| 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.
| @@ -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.
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.
- Pared down the comments of internal functions - removed inline macro Co-authored-by: charlotte <[email protected]>
dsgallups
force-pushed
the
dsgallups/i-like-jam
branch
from
1327635 to
40d0b86
Compare
crates/bevy_pbr/src/render/mesh.rs
Outdated
| 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.
Sorry, to be more clear, is there a reason this can't just be simplified to:
render_material_bindings
.get(&mesh_material)
.copied()
.unwrap_or_default()Maybe I'm missing something.
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 :)