Don't mark newly-hidden meshes invisible until all visibility-determining systems run. (#17922)

The `check_visibility` system currently follows this algorithm:

1. Store all meshes that were visible last frame in the
`PreviousVisibleMeshes` set.

2. Determine which meshes are visible. For each such visible mesh,
remove it from `PreviousVisibleMeshes`.

3. Mark all meshes that remain in `PreviousVisibleMeshes` as invisible.

This algorithm would be correct if the `check_visibility` were the only
system that marked meshes visible. However, it's not: the shadow-related
systems `check_dir_light_mesh_visibility` and
`check_point_light_mesh_visibility` can as well. This results in the
following sequence of events for meshes that are in a shadow map but
*not* visible from a camera:

A. `check_visibility` runs, finds that no camera contains these meshes,
   and marks them hidden, which sets the changed flag.

B. `check_dir_light_mesh_visibility` and/or
   `check_point_light_mesh_visibility` run, discover that these meshes
   are visible in the shadow map, and marks them as visible, again
   setting the `ViewVisibility` changed flag.

C. During the extraction phase, the mesh extraction system sees that
   `ViewVisibility` is changed and re-extracts the mesh.

This is inefficient and results in needless work during rendering.

This patch fixes the issue in two ways:

* The `check_dir_light_mesh_visibility` and
`check_point_light_mesh_visibility` systems now remove meshes that they
discover from `PreviousVisibleMeshes`.

* Step (3) above has been moved from `check_visibility` to a separate
system, `mark_newly_hidden_entities_invisible`. This system runs after
all visibility-determining systems, ensuring that
`PreviousVisibleMeshes` contains only those meshes that truly became
invisible on this frame.

This fix dramatically improves the performance of [the Caldera
benchmark], when combined with several other patches I've submitted.

[the Caldera benchmark]:
https://github.com/DGriffin91/bevy_caldera_scene

crates/bevy_pbr/src/lib.rs

@@ -429,7 +429,8 @@ impl Plugin for PbrPlugin {
                         // NOTE: This MUST be scheduled AFTER the core renderer visibility check
                         // because that resets entity `ViewVisibility` for the first view
                         // which would override any results from this otherwise
-                        .after(VisibilitySystems::CheckVisibility),
+                        .after(VisibilitySystems::CheckVisibility)
+                        .before(VisibilitySystems::MarkNewlyHiddenEntitiesInvisible),
                 ),
             );
 

crates/bevy_pbr/src/light/mod.rs

@@ -13,8 +13,8 @@ use bevy_render::{
     mesh::Mesh3d,
     primitives::{Aabb, CascadesFrusta, CubemapFrusta, Frustum, Sphere},
     view::{
-        InheritedVisibility, NoFrustumCulling, RenderLayers, ViewVisibility, VisibilityClass,
+        InheritedVisibility, NoFrustumCulling, PreviousVisibleEntities, RenderLayers,
-        VisibilityRange, VisibleEntityRanges,
+        ViewVisibility, VisibilityClass, VisibilityRange, VisibleEntityRanges,
     },
 };
 use bevy_transform::components::{GlobalTransform, Transform};
@@ -814,15 +814,23 @@ pub fn check_dir_light_mesh_visibility(
     // TODO: use resource to avoid unnecessary memory alloc
     let mut defer_queue = core::mem::take(defer_visible_entities_queue.deref_mut());
     commands.queue(move |world: &mut World| {
-        let mut query = world.query::<&mut ViewVisibility>();
+        world.resource_scope::<PreviousVisibleEntities, _>(
-        for entities in defer_queue.iter_mut() {
+            |world, mut previous_visible_entities| {
-            let mut iter = query.iter_many_mut(world, entities.iter());
+                let mut query = world.query::<(Entity, &mut ViewVisibility)>();
-            while let Some(mut view_visibility) = iter.fetch_next() {
+                for entities in defer_queue.iter_mut() {
-                if !**view_visibility {
+                    let mut iter = query.iter_many_mut(world, entities.iter());
-                    view_visibility.set();
+                    while let Some((entity, mut view_visibility)) = iter.fetch_next() {
+                        if !**view_visibility {
+                            view_visibility.set();
+                        }
+
+                        // Remove any entities that were discovered to be
+                        // visible from the `PreviousVisibleEntities` resource.
+                        previous_visible_entities.remove(&entity);
+                    }
                 }
-            }
+            },
-        }
+        );
     });
 }
 
@@ -860,6 +868,7 @@ pub fn check_point_light_mesh_visibility(
         ),
     >,
     visible_entity_ranges: Option<Res<VisibleEntityRanges>>,
+    mut previous_visible_entities: ResMut<PreviousVisibleEntities>,
     mut cubemap_visible_entities_queue: Local<Parallel<[Vec<Entity>; 6]>>,
     mut spot_visible_entities_queue: Local<Parallel<Vec<Entity>>>,
     mut checked_lights: Local<EntityHashSet>,
@@ -961,10 +970,17 @@ pub fn check_point_light_mesh_visibility(
                 );
 
                 for entities in cubemap_visible_entities_queue.iter_mut() {
-                    cubemap_visible_entities
+                    for (dst, source) in
-                        .iter_mut()
+                        cubemap_visible_entities.iter_mut().zip(entities.iter_mut())
-                        .zip(entities.iter_mut())
+                    {
-                        .for_each(|(dst, source)| dst.entities.append(source));
+                        // Remove any entities that were discovered to be
+                        // visible from the `PreviousVisibleEntities` resource.
+                        for entity in source.iter() {
+                            previous_visible_entities.remove(entity);
+                        }
+
+                        dst.entities.append(source);
+                    }
                 }
 
                 for visible_entities in cubemap_visible_entities.iter_mut() {
@@ -1047,6 +1063,12 @@ pub fn check_point_light_mesh_visibility(
 
                 for entities in spot_visible_entities_queue.iter_mut() {
                     visible_entities.append(entities);
+
+                    // Remove any entities that were discovered to be visible
+                    // from the `PreviousVisibleEntities` resource.
+                    for entity in entities {
+                        previous_visible_entities.remove(entity);
+                    }
                 }
 
                 shrink_entities(visible_entities.deref_mut());

crates/bevy_render/src/view/visibility/mod.rs

@@ -325,6 +325,10 @@ pub enum VisibilitySystems {
     /// the order of systems within this set is irrelevant, as [`check_visibility`]
     /// assumes that its operations are irreversible during the frame.
     CheckVisibility,
+    /// Label for the `mark_newly_hidden_entities_invisible` system, which sets
+    /// [`ViewVisibility`] to [`ViewVisibility::HIDDEN`] for entities that no
+    /// view has marked as visible.
+    MarkNewlyHiddenEntitiesInvisible,
 }
 
 pub struct VisibilityPlugin;
@@ -340,6 +344,10 @@ impl Plugin for VisibilityPlugin {
                     .before(CheckVisibility)
                     .after(TransformSystem::TransformPropagate),
             )
+            .configure_sets(
+                PostUpdate,
+                MarkNewlyHiddenEntitiesInvisible.after(CheckVisibility),
+            )
             .init_resource::<PreviousVisibleEntities>()
             .add_systems(
                 PostUpdate,
@@ -348,6 +356,7 @@ impl Plugin for VisibilityPlugin {
                     (visibility_propagate_system, reset_view_visibility)
                         .in_set(VisibilityPropagate),
                     check_visibility.in_set(CheckVisibility),
+                    mark_newly_hidden_entities_invisible.in_set(MarkNewlyHiddenEntitiesInvisible),
                 ),
             );
     }
@@ -456,6 +465,10 @@ fn propagate_recursive(
 }
 
 /// Stores all entities that were visible in the previous frame.
+///
+/// As systems that check visibility judge entities visible, they remove them
+/// from this set. Afterward, the `mark_newly_hidden_entities_invisible` system
+/// runs and marks every mesh still remaining in this set as hidden.
 #[derive(Resource, Default, Deref, DerefMut)]
 pub struct PreviousVisibleEntities(EntityHashSet);
 
@@ -607,13 +620,23 @@ pub fn check_visibility(
             }
         }
     }
+}
 
-    // Now whatever previous visible entities are left are entities that were
+/// Marks any entities that weren't judged visible this frame as invisible.
+///
+/// As visibility-determining systems run, they remove entities that they judge
+/// visible from [`PreviousVisibleEntities`]. At the end of visibility
+/// determination, all entities that remain in [`PreviousVisibleEntities`] must
+/// be invisible. This system goes through those entities and marks them newly
+/// invisible (which sets the change flag for them).
+fn mark_newly_hidden_entities_invisible(
+    mut view_visibilities: Query<&mut ViewVisibility>,
+    mut previous_visible_entities: ResMut<PreviousVisibleEntities>,
+) {
+    // Whatever previous visible entities are left are entities that were
     // visible last frame but just became invisible.
     for entity in previous_visible_entities.drain() {
-        if let Ok((_, _, mut view_visibility, _, _, _, _, _, _)) =
+        if let Ok(mut view_visibility) = view_visibilities.get_mut(entity) {
-            visible_aabb_query.get_mut(entity)
-        {
             *view_visibility = ViewVisibility::HIDDEN;
         }
     }