From 588deee8933d87d0e90aa04b743283cc8f78e7b5 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Mon, 10 Feb 2025 14:24:44 -0800 Subject: [PATCH 1/2] Sweep bins after queuing so as to only sweep them once. Currently, we *sweep*, or remove entities from bins when those entities became invisible or changed phases, during `queue_material_meshes` and similar phases. This, however, is wrong, because `queue_material_meshes` executes once per material type, not once per phase. This could result in sweeping bins multiple times per phase, which can corrupt the bins. This commit fixes the issue by moving sweeping to a separate system that runs after queuing. This manifested itself as entities appearing and disappearing seemingly at random. Closes #17759. --- crates/bevy_pbr/src/material.rs | 4 ---- crates/bevy_pbr/src/prepass/mod.rs | 14 -------------- crates/bevy_pbr/src/render/light.rs | 2 +- crates/bevy_render/src/lib.rs | 6 +++++- crates/bevy_render/src/render_phase/mod.rs | 21 ++++++++++++++++----- crates/bevy_sprite/src/mesh2d/material.rs | 4 ---- 6 files changed, 22 insertions(+), 29 deletions(-) diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index 32cc445d4268d..ba44d3ecc8744 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -1047,10 +1047,6 @@ pub fn queue_material_meshes( } } } - - // Remove invalid entities from the bins. - opaque_phase.sweep_old_entities(); - alpha_mask_phase.sweep_old_entities(); } } diff --git a/crates/bevy_pbr/src/prepass/mod.rs b/crates/bevy_pbr/src/prepass/mod.rs index d1cb99e502e52..cd8c73ce9ce5b 100644 --- a/crates/bevy_pbr/src/prepass/mod.rs +++ b/crates/bevy_pbr/src/prepass/mod.rs @@ -1229,20 +1229,6 @@ pub fn queue_prepass_material_meshes( _ => {} } } - - // Remove invalid entities from the bins. - if let Some(phase) = opaque_phase { - phase.sweep_old_entities(); - } - if let Some(phase) = alpha_mask_phase { - phase.sweep_old_entities(); - } - if let Some(phase) = opaque_deferred_phase { - phase.sweep_old_entities(); - } - if let Some(phase) = alpha_mask_deferred_phase { - phase.sweep_old_entities(); - } } } diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index f248a1745138e..91c00c00ffa3a 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1955,7 +1955,7 @@ pub fn queue_shadows( } // Remove invalid entities from the bins. - shadow_phase.sweep_old_entities(); + //shadow_phase.sweep_old_entities(); } } } diff --git a/crates/bevy_render/src/lib.rs b/crates/bevy_render/src/lib.rs index 050da078dc540..0b3e57fac1b11 100644 --- a/crates/bevy_render/src/lib.rs +++ b/crates/bevy_render/src/lib.rs @@ -146,6 +146,9 @@ pub enum RenderSet { Queue, /// A sub-set within [`Queue`](RenderSet::Queue) where mesh entity queue systems are executed. Ensures `prepare_assets::` is completed. QueueMeshes, + /// A sub-set within [`Queue`](RenderSet::Queue) where meshes that have + /// become invisible or changed phases are removed from the bins. + QueueSweep, // TODO: This could probably be moved in favor of a system ordering // abstraction in `Render` or `Queue` /// Sort the [`SortedRenderPhase`](render_phase::SortedRenderPhase)s and @@ -201,7 +204,8 @@ impl Render { schedule.configure_sets((ExtractCommands, PrepareAssets, PrepareMeshes, Prepare).chain()); schedule.configure_sets( - QueueMeshes + (QueueMeshes, QueueSweep) + .chain() .in_set(Queue) .after(prepare_assets::), ); diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index e9baccf141dbe..250954f5e66f7 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -823,16 +823,14 @@ where /// Removes all entities not marked as clean from the bins. /// /// During `queue_material_meshes`, we process all visible entities and mark - /// each as clean as we come to it. Then we call this method, which removes - /// entities that aren't marked as clean from the bins. + /// each as clean as we come to it. Then, in [`sweep_old_entities`], we call + /// this method, which removes entities that aren't marked as clean from the + /// bins. pub fn sweep_old_entities(&mut self) { // Search for entities not marked as valid. We have to do this in // reverse order because `swap_remove_index` will potentially invalidate // all indices after the one we remove. for index in ReverseFixedBitSetZeroesIterator::new(&self.valid_cached_entity_bin_keys) { - // If we found an invalid entity, remove it. Note that this - // potentially invalidates later indices, but that's OK because - // we're going in reverse order. let Some((entity, entity_bin_key)) = self.cached_entity_bin_keys.swap_remove_index(index) else { @@ -1068,6 +1066,7 @@ where ), ) .in_set(RenderSet::PrepareResources), + sweep_old_entities::.in_set(RenderSet::QueueSweep), ), ); } @@ -1605,6 +1604,18 @@ where } } +/// Removes entities that became invisible or changed phases from the bins. +/// +/// This must run after queuing. +pub fn sweep_old_entities(mut render_phases: ResMut>) +where + BPI: BinnedPhaseItem, +{ + for phase in render_phases.0.values_mut() { + phase.sweep_old_entities(); + } +} + impl BinnedRenderPhaseType { pub fn mesh( batchable: bool, diff --git a/crates/bevy_sprite/src/mesh2d/material.rs b/crates/bevy_sprite/src/mesh2d/material.rs index 7593b3d85e6ea..c5bc522131a34 100644 --- a/crates/bevy_sprite/src/mesh2d/material.rs +++ b/crates/bevy_sprite/src/mesh2d/material.rs @@ -848,10 +848,6 @@ pub fn queue_material2d_meshes( } } } - - // Remove invalid entities from the bins. - opaque_phase.sweep_old_entities(); - alpha_mask_phase.sweep_old_entities(); } } From 0a0014ccfa321d44c5231dfe66fc1bd41ce2a6a4 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Mon, 10 Feb 2025 14:56:59 -0800 Subject: [PATCH 2/2] Remove commented out code --- crates/bevy_pbr/src/render/light.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index 91c00c00ffa3a..00fb77f62d19f 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1953,9 +1953,6 @@ pub fn queue_shadows( *current_change_tick, ); } - - // Remove invalid entities from the bins. - //shadow_phase.sweep_old_entities(); } } }