From fc7705c0a63a29f445404411c1fd37c6ecd86799 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Mon, 14 Apr 2025 14:17:48 -0700 Subject: [PATCH] Fix the ordering of the systems introduced in #18734. (#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 #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::::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. --- crates/bevy_pbr/src/lib.rs | 9 ++++++- crates/bevy_pbr/src/material.rs | 20 +++++++------- .../bevy_pbr/src/meshlet/instance_manager.rs | 27 +++++++++---------- crates/bevy_pbr/src/render/mesh.rs | 27 ++++++++++--------- 4 files changed, 46 insertions(+), 37 deletions(-) diff --git a/crates/bevy_pbr/src/lib.rs b/crates/bevy_pbr/src/lib.rs index fdfcdc7b48..1810bc67eb 100644 --- a/crates/bevy_pbr/src/lib.rs +++ b/crates/bevy_pbr/src/lib.rs @@ -466,7 +466,14 @@ impl Plugin for PbrPlugin { // Extract the required data from the main world render_app - .add_systems(ExtractSchedule, (extract_clusters, extract_lights)) + .add_systems( + ExtractSchedule, + ( + extract_clusters, + extract_lights, + late_sweep_material_instances, + ), + ) .add_systems( Render, ( diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index a611fb4ff9..b00bb955a1 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -316,13 +316,10 @@ where .add_systems( ExtractSchedule, ( - ( - extract_mesh_materials::, - early_sweep_material_instances::, - ) - .chain() - .before(late_sweep_material_instances) - .before(ExtractMeshesSet), + extract_mesh_materials::.in_set(ExtractMaterialsSet), + early_sweep_material_instances:: + .after(ExtractMaterialsSet) + .before(late_sweep_material_instances), extract_entities_needs_specialization::.after(extract_cameras), ), ) @@ -415,7 +412,8 @@ where /// /// See the comments in [`RenderMaterialInstances::mesh_material`] for more /// information. -static DUMMY_MESH_MATERIAL: AssetId = AssetId::::invalid(); +pub(crate) static DUMMY_MESH_MATERIAL: AssetId = + AssetId::::invalid(); /// A key uniquely identifying a specialized [`MaterialPipeline`]. pub struct MaterialPipelineKey { @@ -637,6 +635,10 @@ pub struct RenderMaterialInstance { last_change_tick: Tick, } +/// A [`SystemSet`] that contains all `extract_mesh_materials` systems. +#[derive(SystemSet, Clone, PartialEq, Eq, Debug, Hash)] +pub struct ExtractMaterialsSet; + pub const fn alpha_mode_pipeline_key(alpha_mode: AlphaMode, msaa: &Msaa) -> MeshPipelineKey { match alpha_mode { // Premultiplied and Add share the same pipeline key @@ -782,7 +784,7 @@ fn early_sweep_material_instances( /// This runs after all invocations of [`early_sweep_material_instances`] and is /// responsible for bumping [`RenderMaterialInstances::current_change_tick`] in /// preparation for a new frame. -fn late_sweep_material_instances( +pub(crate) fn late_sweep_material_instances( mut material_instances: ResMut, mut removed_visibilities_query: Extract>, ) { diff --git a/crates/bevy_pbr/src/meshlet/instance_manager.rs b/crates/bevy_pbr/src/meshlet/instance_manager.rs index eba34f7cb7..661d4791ae 100644 --- a/crates/bevy_pbr/src/meshlet/instance_manager.rs +++ b/crates/bevy_pbr/src/meshlet/instance_manager.rs @@ -1,10 +1,10 @@ use super::{meshlet_mesh_manager::MeshletMeshManager, MeshletMesh, MeshletMesh3d}; use crate::{ - Material, MaterialBindingId, MeshFlags, MeshTransforms, MeshUniform, NotShadowCaster, - NotShadowReceiver, PreviousGlobalTransform, RenderMaterialBindings, RenderMaterialInstances, - StandardMaterial, + material::DUMMY_MESH_MATERIAL, Material, MaterialBindingId, MeshFlags, MeshTransforms, + MeshUniform, NotShadowCaster, NotShadowReceiver, PreviousGlobalTransform, + RenderMaterialBindings, RenderMaterialInstances, }; -use bevy_asset::{AssetEvent, AssetId, AssetServer, Assets, UntypedAssetId}; +use bevy_asset::{AssetEvent, AssetServer, Assets, UntypedAssetId}; use bevy_ecs::{ entity::{Entities, Entity, EntityHashMap}, event::EventReader, @@ -113,16 +113,15 @@ impl InstanceManager { }; let mesh_material = mesh_material_ids.mesh_material(instance); - let mesh_material_binding_id = - if mesh_material != AssetId::::invalid().untyped() { - render_material_bindings - .get(&mesh_material) - .cloned() - .unwrap_or_default() - } else { - // Use a dummy binding ID if the mesh has no material - MaterialBindingId::default() - }; + let mesh_material_binding_id = if mesh_material != DUMMY_MESH_MATERIAL.untyped() { + render_material_bindings + .get(&mesh_material) + .cloned() + .unwrap_or_default() + } else { + // Use a dummy binding ID if the mesh has no material + MaterialBindingId::default() + }; let mesh_uniform = MeshUniform::new( &transforms, diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs index e65e9ec40c..4bae79b807 100644 --- a/crates/bevy_pbr/src/render/mesh.rs +++ b/crates/bevy_pbr/src/render/mesh.rs @@ -196,7 +196,9 @@ impl Plugin for MeshRenderPlugin { .init_resource::() .configure_sets( ExtractSchedule, - ExtractMeshesSet.after(view::extract_visibility_ranges), + ExtractMeshesSet + .after(view::extract_visibility_ranges) + .after(late_sweep_material_instances), ) .add_systems( ExtractSchedule, @@ -1131,19 +1133,18 @@ impl RenderMeshInstanceGpuBuilder { // yet loaded. In that case, add the mesh to // `meshes_to_reextract_next_frame` and bail. let mesh_material = mesh_material_ids.mesh_material(entity); - let mesh_material_binding_id = - if mesh_material != AssetId::::invalid().untyped() { - match render_material_bindings.get(&mesh_material) { - Some(binding_id) => *binding_id, - None => { - meshes_to_reextract_next_frame.insert(entity); - return None; - } + let mesh_material_binding_id = if mesh_material != DUMMY_MESH_MATERIAL.untyped() { + match render_material_bindings.get(&mesh_material) { + Some(binding_id) => *binding_id, + None => { + meshes_to_reextract_next_frame.insert(entity); + return None; } - } else { - // Use a dummy material binding ID. - MaterialBindingId::default() - }; + } + } else { + // Use a dummy material binding ID. + MaterialBindingId::default() + }; self.shared.material_bindings_index = mesh_material_binding_id; let lightmap_slot = match render_lightmaps.render_lightmaps.get(&entity) {