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::<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.
This commit is contained in:
Patrick Walton 2025-04-14 14:17:48 -07:00 committed by François Mockers
parent dc48fd41e8
commit fc7705c0a6
4 changed files with 46 additions and 37 deletions

View File

@ -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,
(

View File

@ -316,13 +316,10 @@ where
.add_systems(
ExtractSchedule,
(
(
extract_mesh_materials::<M>,
early_sweep_material_instances::<M>,
)
.chain()
.before(late_sweep_material_instances)
.before(ExtractMeshesSet),
extract_mesh_materials::<M>.in_set(ExtractMaterialsSet),
early_sweep_material_instances::<M>
.after(ExtractMaterialsSet)
.before(late_sweep_material_instances),
extract_entities_needs_specialization::<M>.after(extract_cameras),
),
)
@ -415,7 +412,8 @@ where
///
/// See the comments in [`RenderMaterialInstances::mesh_material`] for more
/// information.
static DUMMY_MESH_MATERIAL: AssetId<StandardMaterial> = AssetId::<StandardMaterial>::invalid();
pub(crate) static DUMMY_MESH_MATERIAL: AssetId<StandardMaterial> =
AssetId::<StandardMaterial>::invalid();
/// A key uniquely identifying a specialized [`MaterialPipeline`].
pub struct MaterialPipelineKey<M: Material> {
@ -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<M>(
/// 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<RenderMaterialInstances>,
mut removed_visibilities_query: Extract<RemovedComponents<ViewVisibility>>,
) {

View File

@ -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,8 +113,7 @@ impl InstanceManager {
};
let mesh_material = mesh_material_ids.mesh_material(instance);
let mesh_material_binding_id =
if mesh_material != AssetId::<StandardMaterial>::invalid().untyped() {
let mesh_material_binding_id = if mesh_material != DUMMY_MESH_MATERIAL.untyped() {
render_material_bindings
.get(&mesh_material)
.cloned()

View File

@ -196,7 +196,9 @@ impl Plugin for MeshRenderPlugin {
.init_resource::<RenderMaterialInstances>()
.configure_sets(
ExtractSchedule,
ExtractMeshesSet.after(view::extract_visibility_ranges),
ExtractMeshesSet
.after(view::extract_visibility_ranges)
.after(late_sweep_material_instances),
)
.add_systems(
ExtractSchedule,
@ -1131,8 +1133,7 @@ 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::<StandardMaterial>::invalid().untyped() {
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 => {