diff --git a/crates/bevy_camera/src/visibility/mod.rs b/crates/bevy_camera/src/visibility/mod.rs index 9a4d496693823..769937a5b9326 100644 --- a/crates/bevy_camera/src/visibility/mod.rs +++ b/crates/bevy_camera/src/visibility/mod.rs @@ -3,7 +3,7 @@ mod render_layers; use core::any::TypeId; -use bevy_ecs::entity::{EntityHashMap, EntityHashSet}; +use bevy_ecs::entity::EntityHashMap; use bevy_ecs::lifecycle::HookContext; use bevy_ecs::world::DeferredWorld; use derive_more::derive::{Deref, DerefMut}; @@ -169,12 +169,15 @@ pub struct VisibilityClass(pub SmallVec<[TypeId; 1]>); /// Later in the frame, systems in [`CheckVisibility`] will mark any visible entities using [`ViewVisibility::set`]. /// Because of this, values of this type will be marked as changed every frame, even when they do not change. /// -/// If you wish to add custom visibility system that sets this value, make sure you add it to the [`CheckVisibility`] set. +/// If you wish to add custom visibility system that sets this value, make sure you add it to the +/// [`CheckVisibility`] set, and be sure to set [`WasVisibleNowHidden`] to false when `set`ting the +/// [`ViewVisibility`] /// /// [`VisibilityPropagate`]: VisibilitySystems::VisibilityPropagate /// [`CheckVisibility`]: VisibilitySystems::CheckVisibility #[derive(Component, Deref, Debug, Default, Clone, Copy, Reflect, PartialEq, Eq)] #[reflect(Component, Default, Debug, PartialEq, Clone)] +#[require(WasVisibleNowHidden)] pub struct ViewVisibility(bool); impl ViewVisibility { @@ -369,7 +372,6 @@ impl Plugin for VisibilityPlugin { .ambiguous_with(CalculateBounds) .ambiguous_with(mark_3d_meshes_as_changed_if_their_assets_changed), ) - .init_resource::() .add_systems( PostUpdate, ( @@ -522,29 +524,34 @@ fn propagate_recursive( Ok(()) } -/// Stores all entities that were visible in the previous frame. +/// Used as a scratch space to ensure that [`ViewVisibility`] is only mutated (triggering change +/// detection) when necessary. /// -/// 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); - -/// Resets the view visibility of every entity. -/// Entities that are visible will be marked as such later this frame -/// by a [`VisibilitySystems::CheckVisibility`] system. -fn reset_view_visibility( - mut query: Query<(Entity, &ViewVisibility)>, - mut previous_visible_entities: ResMut, -) { - previous_visible_entities.clear(); - - query.iter_mut().for_each(|(entity, view_visibility)| { - // Record the entities that were previously visible. - if view_visibility.get() { - previous_visible_entities.insert(entity); - } - }); +/// This is needed because an entity might be seen by many views (cameras, lights that cast shadows, +/// etc.), so it is easy to know if an entity is visible to something, but hard to know if it is +/// *globally* non-visible to any view. To solve this, we set this component to `true` every frame +/// that the entity's `ViewVisibility` was also `true` the previous frame. Then, during the +/// [`VisibilitySystems::CheckVisibility`] system set, it is the responsibility of those systems to +/// mark this component `false` if the entity is currently visible. Once this is done, the only +/// entities with this entity set to `true` are entities that were visible last frame, but are not +/// visible this frame. Finally, we can use this to set the `ViewVisibility` of these entities to +/// hidden, ensuring that we have only triggered change detection where necessary. +/// +/// Consider if we did the simplest approach of setting all entities to hidden, then marking visible +/// entities. Every single [`ViewVisibility`] would trigger change detection. +#[derive(Component, Default, Reflect, Deref, DerefMut)] +#[reflect(Component)] +pub struct WasVisibleNowHidden(bool); + +/// Track entities that were visible last frame, used to granularly update [`ViewVisibility`] this +/// frame. See [`WasVisibleNowHidden`] for details. +fn reset_view_visibility(mut query: Query<(&ViewVisibility, &mut WasVisibleNowHidden)>) { + query + .par_iter_mut() + .for_each(|(view_visibility, mut previously_visible)| { + // Record the entities that were previously visible. + **previously_visible = view_visibility.get(); + }); } /// System updating the visibility of entities each frame. @@ -569,6 +576,7 @@ pub fn check_visibility( Entity, &InheritedVisibility, &mut ViewVisibility, + &mut WasVisibleNowHidden, Option<&VisibilityClass>, Option<&RenderLayers>, Option<&Aabb>, @@ -577,7 +585,6 @@ pub fn check_visibility( Has, )>, visible_entity_ranges: Option>, - mut previous_visible_entities: ResMut, ) { let visible_entity_ranges = visible_entity_ranges.as_deref(); @@ -597,6 +604,7 @@ pub fn check_visibility( entity, inherited_visibility, mut view_visibility, + mut was_visible_now_hidden, visibility_class, maybe_entity_mask, maybe_model_aabb, @@ -651,11 +659,13 @@ pub fn check_visibility( if !**view_visibility { view_visibility.set(); } + // The entity is visible, so we must set this to false. + **was_visible_now_hidden = false; // The visibility class may be None here because AABB gizmos can be enabled via // config without a renderable component being added to the entity. This workaround - // allows view visibility to be set for entities without a renderable component but - // that still need to render gizmos. + // allows view visibility to be set for entities without a renderable component, but + // still need to render gizmos. if let Some(visibility_class) = visibility_class { // Add the entity to the queue for all visibility classes the entity is in. for visibility_class_id in visibility_class.iter() { @@ -670,40 +680,26 @@ pub fn check_visibility( // Drain all the thread queues into the `visible_entities` list. for class_queues in thread_queues.iter_mut() { for (class, entities) in class_queues { - let visible_entities_for_class = visible_entities.get_mut(*class); - for entity in entities.drain(..) { - // As we mark entities as visible, we remove them from the - // `previous_visible_entities` list. At the end, all of the - // entities remaining in `previous_visible_entities` will be - // entities that were visible last frame but are no longer - // visible this frame. - previous_visible_entities.remove(&entity); - - visible_entities_for_class.push(entity); - } + visible_entities.get_mut(*class).append(entities); } } } } -/// 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). +/// The last step in the visibility pipeline. Looks at entities that were visible last frame but not +/// marked as visible this frame ([`WasVisibleNowHidden`]), and marks them as hidden by setting the +/// [`ViewVisibility`]. This process is needed to ensure we only trigger change detection on +/// [`ViewVisibility`] when necessary. See the docs on [`WasVisibleNowHidden`] for more details. fn mark_newly_hidden_entities_invisible( - mut view_visibilities: Query<&mut ViewVisibility>, - mut previous_visible_entities: ResMut, + mut view_visibilities: Query<(&mut ViewVisibility, &WasVisibleNowHidden)>, ) { - // 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) = view_visibilities.get_mut(entity) { - *view_visibility = ViewVisibility::HIDDEN; - } - } + view_visibilities + .par_iter_mut() + .for_each(|(mut view_visibility, previously_visible)| { + if **previously_visible { + *view_visibility = ViewVisibility::HIDDEN; + } + }); } /// A generic component add hook that automatically adds the appropriate diff --git a/crates/bevy_light/src/lib.rs b/crates/bevy_light/src/lib.rs index 416a5565169e2..37b07cbb092cd 100644 --- a/crates/bevy_light/src/lib.rs +++ b/crates/bevy_light/src/lib.rs @@ -5,8 +5,8 @@ use bevy_camera::{ primitives::{Aabb, CascadesFrusta, CubemapFrusta, Frustum, Sphere}, visibility::{ CascadesVisibleEntities, CubemapVisibleEntities, InheritedVisibility, NoFrustumCulling, - PreviousVisibleEntities, RenderLayers, ViewVisibility, VisibilityRange, VisibilitySystems, - VisibleEntityRanges, VisibleMeshEntities, + RenderLayers, ViewVisibility, VisibilityRange, VisibilitySystems, VisibleEntityRanges, + VisibleMeshEntities, }, CameraUpdateSystems, }; @@ -26,6 +26,8 @@ use cluster::{ }; mod ambient_light; pub use ambient_light::{AmbientLight, GlobalAmbientLight}; +use bevy_camera::visibility::WasVisibleNowHidden; + mod probe; pub use probe::{ AtmosphereEnvironmentMapLight, EnvironmentMapLight, GeneratedEnvironmentMapLight, @@ -450,23 +452,17 @@ 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| { - world.resource_scope::( - |world, mut previous_visible_entities| { - let mut query = world.query::<(Entity, &mut ViewVisibility)>(); - for entities in defer_queue.iter_mut() { - let mut iter = query.iter_many_mut(world, entities.iter()); - 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); - } + let mut query = world.query::<(&mut ViewVisibility, &mut WasVisibleNowHidden)>(); + for entities in defer_queue.iter_mut() { + let mut iter = query.iter_many_mut(world, entities.iter()); + while let Some((mut view_visibility, mut was_visible_now_hidden)) = iter.fetch_next() { + if !**view_visibility { + view_visibility.set(); } - }, - ); + // Unset any entities that were discovered to be visible + **was_visible_now_hidden = false; + } + } }); } @@ -491,6 +487,7 @@ pub fn check_point_light_mesh_visibility( Entity, &InheritedVisibility, &mut ViewVisibility, + &mut WasVisibleNowHidden, Option<&RenderLayers>, Option<&Aabb>, Option<&GlobalTransform>, @@ -504,7 +501,6 @@ pub fn check_point_light_mesh_visibility( ), >, visible_entity_ranges: Option>, - mut previous_visible_entities: ResMut, mut cubemap_visible_entities_queue: Local; 6]>>, mut spot_visible_entities_queue: Local>>, mut checked_lights: Local, @@ -549,6 +545,7 @@ pub fn check_point_light_mesh_visibility( entity, inherited_visibility, mut view_visibility, + mut was_visible_now_hidden, maybe_entity_mask, maybe_aabb, maybe_transform, @@ -590,6 +587,7 @@ pub fn check_point_light_mesh_visibility( if !**view_visibility { view_visibility.set(); } + **was_visible_now_hidden = false; visible_entities.push(entity); } } @@ -597,6 +595,7 @@ pub fn check_point_light_mesh_visibility( if !**view_visibility { view_visibility.set(); } + **was_visible_now_hidden = false; for visible_entities in cubemap_visible_entities_local_queue.iter_mut() { visible_entities.push(entity); @@ -609,12 +608,6 @@ pub fn check_point_light_mesh_visibility( for (dst, source) in cubemap_visible_entities.iter_mut().zip(entities.iter_mut()) { - // 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); } } @@ -648,6 +641,7 @@ pub fn check_point_light_mesh_visibility( entity, inherited_visibility, mut view_visibility, + mut was_visible_now_hidden, maybe_entity_mask, maybe_aabb, maybe_transform, @@ -686,12 +680,14 @@ pub fn check_point_light_mesh_visibility( if !**view_visibility { view_visibility.set(); } + **was_visible_now_hidden = false; spot_visible_entities_local_queue.push(entity); } } else { if !**view_visibility { view_visibility.set(); } + **was_visible_now_hidden = false; spot_visible_entities_local_queue.push(entity); } }, @@ -699,12 +695,6 @@ 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());