Unify `RenderMaterialInstances` and `RenderMeshMaterialIds`, and fix an associated race condition. #18734

pcwalton · 2025-04-06T03:13:41Z

Currently, RenderMaterialInstances and RenderMeshMaterialIds are very similar render-world resources: the former maps main world meshes to typed material asset IDs, and the latter maps main world meshes to untyped material asset IDs. This is needlessly-complex and wasteful, so this patch unifies the two in favor of a single untyped RenderMaterialInstances resource.

This patch also fixes a subtle issue that could cause mesh materials to be incorrect if a MeshMaterial3d<A> was removed and replaced with a MeshMaterial3d material in the same frame. The problematic pattern looks like:

extract_mesh_materials runs and, seeing the Changed<MeshMaterial3d> condition, adds an entry mapping the mesh to the new material to the untyped RenderMeshMaterialIds.
extract_mesh_materials<A> runs and, seeing that the entity is present in RemovedComponents<MeshMaterial3d<A>>, removes the entry from RenderMeshMaterialIds.
The material slot is now empty, and the mesh will show up as whatever material happens to be in slot 0 in the material data slab.

This commit fixes the issue by splitting out extract_mesh_materials into three phases: extraction, early sweeping, and late sweeping, which run in that order:

The extraction system, which runs for each material, updates RenderMaterialInstances records whenever MeshMaterial3d components change, and updates a change tick so that the following system will know not to remove it.
The early sweeping system, which runs for each material, processes entities present in RemovedComponents<MeshMaterial3d> and removes each such entity's record from RenderMeshInstances only if the extraction system didn't update it this frame. This system runs after all extraction systems have completed, fixing the race condition.
The late sweeping system, which runs only once regardless of the number of materials in the scene, processes entities present in RemovedComponents<ViewVisibility> and, as in the early sweeping phase, removes each such entity's record from RenderMeshInstances only if the extraction system didn't update it this frame. At the end, the late sweeping system updates the change tick.

Because this pattern happens relatively frequently, I think this PR should land for 0.16.

associated race condition. Currently, `RenderMaterialInstances` and `RenderMeshMaterialIds` are very similar render-world resources: the former maps main world meshes to typed material asset IDs, and the latter maps main world meshes to untyped material asset IDs. This is needlessly-complex and wasteful, so this patch unifies the two in favor of a single untyped `RenderMaterialInstances` resource. This patch also fixes a subtle issue that could cause mesh materials to be incorrect if a `MeshMaterial3d<A>` was removed and replaced with a `MeshMaterial3d` material in the same frame. The problematic pattern looks like: 1. `extract_mesh_materials` runs and, seeing the `Changed<MeshMaterial3d>` condition, adds an entry mapping the mesh to the new material to the untyped `RenderMeshMaterialIds`. 2. `extract_mesh_materials<A>` runs and, seeing that the entity is present in `RemovedComponents<MeshMaterial3d<A>>`, removes the entry from `RenderMeshMaterialIds`. 3. The material slot is now empty, and the mesh will show up as whatever material happens to be in slot 0 in the material data slab. This commit fixes the issue by splitting out `extract_mesh_materials` into *three* phases: *extraction*, *early sweeping*, and *late sweeping*, which run in that order: 1. The *extraction* system, which runs for each material, updates `RenderMaterialInstances` records whenever `MeshMaterial3d` components change, and updates a change tick so that the following system will know not to remove it. 2. The *early sweeping* system, which runs for each material, processes entities present in `RemovedComponents<MeshMaterial3d>` and removes each such entity's record from `RenderMeshInstances` only if the extraction system didn't update it this frame. This system runs after *all* extraction systems have completed, fixing the race condition. 3. The *late sweeping* system, which runs only once regardless of the number of materials in the scene, processes entities present in `RemovedComponents<ViewVisibility>` and, as in the early sweeping phase, removes each such entity's record from `RenderMeshInstances` only if the extraction system didn't update it this frame. At the end, the late sweeping system updates the change tick. Because this pattern happens relatively frequently, I think this PR should land for 0.16.

crates/bevy_pbr/src/material.rs

superdump

Not dropping a final review yet as CI is broken and I need to take another look at a couple of bits when I have more time but what I see all looks fine

tychedelia

Looks good, nasty bug, great step towards #18075.

tychedelia · 2025-04-06T04:17:02Z

crates/bevy_pbr/src/material.rs

            return RenderCommandResult::Skip;
        };
-        let Some(material) = materials.get(*material_asset_id) else {
+        let Ok(material_asset_id) = material_instance.asset_id.try_typed::<M>() else {


Just a note for myself, but this is basically guaranteed to succeed because the phase item was queued in a queue_material_meshes::<M> and so we would never look up a material instance whose asset id's material type doesn't match the draw command's,

Do you want me to add an error! there?

tychedelia · 2025-04-06T17:49:05Z

crates/bevy_pbr/src/material.rs

+    pub(crate) fn mesh_material(&self, entity: MainEntity) -> UntypedAssetId {
+        match self.instances.get(&entity) {
+            Some(render_instance) => render_instance.asset_id,
+            None => AssetId::<StandardMaterial>::invalid().into(),


We should extract this to a constant (in another PR) as we're using this pattern in a few places and the use of StandardMaterial is potentially confusing.

crates/bevy_pbr/src/material.rs

superdump

If we fix checkdocs then this can be merged.

…terial-ids

…an associated race condition. (#18734) Currently, `RenderMaterialInstances` and `RenderMeshMaterialIds` are very similar render-world resources: the former maps main world meshes to typed material asset IDs, and the latter maps main world meshes to untyped material asset IDs. This is needlessly-complex and wasteful, so this patch unifies the two in favor of a single untyped `RenderMaterialInstances` resource. This patch also fixes a subtle issue that could cause mesh materials to be incorrect if a `MeshMaterial3d<A>` was removed and replaced with a `MeshMaterial3d` material in the same frame. The problematic pattern looks like: 1. `extract_mesh_materials` runs and, seeing the `Changed<MeshMaterial3d>` condition, adds an entry mapping the mesh to the new material to the untyped `RenderMeshMaterialIds`. 2. `extract_mesh_materials<A>` runs and, seeing that the entity is present in `RemovedComponents<MeshMaterial3d<A>>`, removes the entry from `RenderMeshMaterialIds`. 3. The material slot is now empty, and the mesh will show up as whatever material happens to be in slot 0 in the material data slab. This commit fixes the issue by splitting out `extract_mesh_materials` into *three* phases: *extraction*, *early sweeping*, and *late sweeping*, which run in that order: 1. The *extraction* system, which runs for each material, updates `RenderMaterialInstances` records whenever `MeshMaterial3d` components change, and updates a change tick so that the following system will know not to remove it. 2. The *early sweeping* system, which runs for each material, processes entities present in `RemovedComponents<MeshMaterial3d>` and removes each such entity's record from `RenderMeshInstances` only if the extraction system didn't update it this frame. This system runs after *all* extraction systems have completed, fixing the race condition. 3. The *late sweeping* system, which runs only once regardless of the number of materials in the scene, processes entities present in `RemovedComponents<ViewVisibility>` and, as in the early sweeping phase, removes each such entity's record from `RenderMeshInstances` only if the extraction system didn't update it this frame. At the end, the late sweeping system updates the change tick. Because this pattern happens relatively frequently, I think this PR should land for 0.16.

There's still a race resulting in blank materials whenever a material of type A is added on the same frame that a material of type B is removed. PR #18734 improved the situation, but ultimately didn't fix the race because of two issues: 1. The `late_sweep_material_instances` system was never scheduled. This PR fixes the problem by scheduling that system. 2. `early_sweep_material_instances` needs to be called after *every* material type has been extracted, not just when the material of *that* type has been extracted. The `chain()` added during the review process in PR #18734 broke this logic. This PR reverts that and fixes the ordering by introducing a new `SystemSet` that contains all material extraction systems. I also took the opportunity to switch a manual reference to `AssetId::<StandardMaterial>::invalid()` to the new `DUMMY_MESH_MATERIAL` constant for clarity. Because this is a bug that can affect any application that switches material types in a single frame, I think this should be uplifted to Bevy 0.16.

…an associated race condition. (bevyengine#18734) Currently, `RenderMaterialInstances` and `RenderMeshMaterialIds` are very similar render-world resources: the former maps main world meshes to typed material asset IDs, and the latter maps main world meshes to untyped material asset IDs. This is needlessly-complex and wasteful, so this patch unifies the two in favor of a single untyped `RenderMaterialInstances` resource. This patch also fixes a subtle issue that could cause mesh materials to be incorrect if a `MeshMaterial3d<A>` was removed and replaced with a `MeshMaterial3d` material in the same frame. The problematic pattern looks like: 1. `extract_mesh_materials` runs and, seeing the `Changed<MeshMaterial3d>` condition, adds an entry mapping the mesh to the new material to the untyped `RenderMeshMaterialIds`. 2. `extract_mesh_materials<A>` runs and, seeing that the entity is present in `RemovedComponents<MeshMaterial3d<A>>`, removes the entry from `RenderMeshMaterialIds`. 3. The material slot is now empty, and the mesh will show up as whatever material happens to be in slot 0 in the material data slab. This commit fixes the issue by splitting out `extract_mesh_materials` into *three* phases: *extraction*, *early sweeping*, and *late sweeping*, which run in that order: 1. The *extraction* system, which runs for each material, updates `RenderMaterialInstances` records whenever `MeshMaterial3d` components change, and updates a change tick so that the following system will know not to remove it. 2. The *early sweeping* system, which runs for each material, processes entities present in `RemovedComponents<MeshMaterial3d>` and removes each such entity's record from `RenderMeshInstances` only if the extraction system didn't update it this frame. This system runs after *all* extraction systems have completed, fixing the race condition. 3. The *late sweeping* system, which runs only once regardless of the number of materials in the scene, processes entities present in `RemovedComponents<ViewVisibility>` and, as in the early sweeping phase, removes each such entity's record from `RenderMeshInstances` only if the extraction system didn't update it this frame. At the end, the late sweeping system updates the change tick. Because this pattern happens relatively frequently, I think this PR should land for 0.16.

…engine#18825) There's still a race resulting in blank materials whenever a material of type A is added on the same frame that a material of type B is removed. PR bevyengine#18734 improved the situation, but ultimately didn't fix the race because of two issues: 1. The `late_sweep_material_instances` system was never scheduled. This PR fixes the problem by scheduling that system. 2. `early_sweep_material_instances` needs to be called after *every* material type has been extracted, not just when the material of *that* type has been extracted. The `chain()` added during the review process in PR bevyengine#18734 broke this logic. This PR reverts that and fixes the ordering by introducing a new `SystemSet` that contains all material extraction systems. I also took the opportunity to switch a manual reference to `AssetId::<StandardMaterial>::invalid()` to the new `DUMMY_MESH_MATERIAL` constant for clarity. Because this is a bug that can affect any application that switches material types in a single frame, I think this should be uplifted to Bevy 0.16.

pcwalton requested review from atlv24, superdump and tychedelia April 6, 2025 03:13

pcwalton added this to the 0.16 milestone Apr 6, 2025

pcwalton added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior labels Apr 6, 2025

github-project-automation bot added this to Rendering Apr 6, 2025

pcwalton added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Apr 6, 2025

Unbreak meshlets

f73eda0

superdump reviewed Apr 6, 2025

View reviewed changes

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved

superdump reviewed Apr 6, 2025

View reviewed changes

tychedelia approved these changes Apr 6, 2025

View reviewed changes

superdump reviewed Apr 6, 2025

View reviewed changes

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved

superdump approved these changes Apr 6, 2025

View reviewed changes

cart approved these changes Apr 8, 2025

View reviewed changes

tychedelia mentioned this pull request Apr 8, 2025

Add mesh tag feature to release notes. bevyengine/bevy-website#2081

Merged

pcwalton added 2 commits April 8, 2025 17:57

Merge remote-tracking branch 'origin/main' into remove-render-mesh-ma…

e4bca83

…terial-ids

Address review comments

0210def

pcwalton added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 9, 2025

superdump added this pull request to the merge queue Apr 9, 2025

Merged via the queue into bevyengine:main with commit 6c61939 Apr 9, 2025
34 checks passed

github-project-automation bot moved this to Done in Rendering Apr 9, 2025

pcwalton mentioned this pull request Apr 12, 2025

Fix the ordering of the systems introduced in #18734. #18825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Unify `RenderMaterialInstances` and `RenderMeshMaterialIds`, and fix an associated race condition. #18734

Unify `RenderMaterialInstances` and `RenderMeshMaterialIds`, and fix an associated race condition. #18734

Uh oh!

pcwalton commented Apr 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

superdump left a comment

Uh oh!

tychedelia left a comment

Uh oh!

tychedelia Apr 6, 2025

Uh oh!

pcwalton Apr 9, 2025

Uh oh!

tychedelia Apr 6, 2025 •

edited

Loading

Uh oh!

pcwalton Apr 9, 2025

Uh oh!

Uh oh!

superdump left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Unify RenderMaterialInstances and RenderMeshMaterialIds, and fix an associated race condition. #18734

Unify RenderMaterialInstances and RenderMeshMaterialIds, and fix an associated race condition. #18734

Uh oh!

Conversation

pcwalton commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

tychedelia left a comment

Choose a reason for hiding this comment

Uh oh!

tychedelia Apr 6, 2025

Choose a reason for hiding this comment

Uh oh!

pcwalton Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

tychedelia Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcwalton Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Unify `RenderMaterialInstances` and `RenderMeshMaterialIds`, and fix an associated race condition. #18734

Unify `RenderMaterialInstances` and `RenderMeshMaterialIds`, and fix an associated race condition. #18734

pcwalton commented Apr 6, 2025 •

edited

Loading

tychedelia Apr 6, 2025 •

edited

Loading