Skip to content

Commit

Permalink
Split ComputedVisibility into two components to allow for accurate …
Browse files Browse the repository at this point in the history
…change detection and speed up visibility propagation (bevyengine#9497)

# Objective

Fix bevyengine#8267.
Fixes half of bevyengine#7840.

The `ComputedVisibility` component contains two flags: hierarchy
visibility, and view visibility (whether its visible to any cameras).
Due to the modular and open-ended way that view visibility is computed,
it triggers change detection every single frame, even when the value
does not change. Since hierarchy visibility is stored in the same
component as view visibility, this means that change detection for
inherited visibility is completely broken.

At the company I work for, this has become a real issue. We are using
change detection to only re-render scenes when necessary. The broken
state of change detection for computed visibility means that we have to
to rely on the non-inherited `Visibility` component for now. This is
workable in the early stages of our project, but since we will
inevitably want to use the hierarchy, we will have to either:

1. Roll our own solution for computed visibility.
2. Fix the issue for everyone.

## Solution

Split the `ComputedVisibility` component into two: `InheritedVisibilty`
and `ViewVisibility`.
This allows change detection to behave properly for
`InheritedVisibility`.
View visiblity is still erratic, although it is less useful to be able
to detect changes
for this flavor of visibility.

Overall, this actually simplifies the API. Since the visibility system
consists of
self-explaining components, it is much easier to document the behavior
and usage.
This approach is more modular and "ECS-like" -- one could
strip out the `ViewVisibility` component entirely if it's not needed,
and rely only on inherited visibility.

---

## Changelog

- `ComputedVisibility` has been removed in favor of:
`InheritedVisibility` and `ViewVisiblity`.

## Migration Guide

The `ComputedVisibilty` component has been split into
`InheritedVisiblity` and
`ViewVisibility`. Replace any usages of
`ComputedVisibility::is_visible_in_hierarchy`
with `InheritedVisibility::get`, and replace
`ComputedVisibility::is_visible_in_view`
 with `ViewVisibility::get`.
 
 ```rust
 // Before:
 commands.spawn(VisibilityBundle {
     visibility: Visibility::Inherited,
     computed_visibility: ComputedVisibility::default(),
 });
 
 // After:
 commands.spawn(VisibilityBundle {
     visibility: Visibility::Inherited,
     inherited_visibility: InheritedVisibility::default(),
     view_visibility: ViewVisibility::default(),
 });
 ```
 
 ```rust
 // Before:
 fn my_system(q: Query<&ComputedVisibilty>) {
     for vis in &q {
         if vis.is_visible_in_hierarchy() {
     
 // After:
 fn my_system(q: Query<&InheritedVisibility>) {
     for inherited_visibility in &q {
         if inherited_visibility.get() {
 ```
 
 ```rust
 // Before:
 fn my_system(q: Query<&ComputedVisibilty>) {
     for vis in &q {
         if vis.is_visible_in_view() {
     
 // After:
 fn my_system(q: Query<&ViewVisibility>) {
     for view_visibility in &q {
         if view_visibility.get() {
 ```
 
 ```rust
 // Before:
 fn my_system(mut q: Query<&mut ComputedVisibilty>) {
     for vis in &mut q {
         vis.set_visible_in_view();
     
 // After:
 fn my_system(mut q: Query<&mut ViewVisibility>) {
     for view_visibility in &mut q {
         view_visibility.set();
 ```

---------

Co-authored-by: Robert Swain <[email protected]>
  • Loading branch information
JoJoJet and superdump authored Sep 1, 2023
1 parent 02025ef commit 02b520b
Show file tree
Hide file tree
Showing 24 changed files with 472 additions and 317 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_hierarchy/src/valid_parent_check_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl<T> Default for ReportHierarchyIssue<T> {
/// which parent hasn't a `T` component.
///
/// Hierarchy propagations are top-down, and limited only to entities
/// with a specific component (such as `ComputedVisibility` and `GlobalTransform`).
/// with a specific component (such as `InheritedVisibility` and `GlobalTransform`).
/// This means that entities with one of those component
/// and a parent without the same component is probably a programming error.
/// (See B0004 explanation linked in warning message)
Expand Down
21 changes: 15 additions & 6 deletions crates/bevy_pbr/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use bevy_reflect::Reflect;
use bevy_render::{
mesh::Mesh,
primitives::{CascadesFrusta, CubemapFrusta, Frustum},
view::{ComputedVisibility, Visibility, VisibleEntities},
view::{InheritedVisibility, ViewVisibility, Visibility, VisibleEntities},
};
use bevy_transform::components::{GlobalTransform, Transform};
use bevy_utils::HashMap;
Expand All @@ -25,8 +25,10 @@ pub struct MaterialMeshBundle<M: Material> {
pub global_transform: GlobalTransform,
/// User indication of whether an entity is visible
pub visibility: Visibility,
/// Inherited visibility of an entity.
pub inherited_visibility: InheritedVisibility,
/// Algorithmically-computed indication of whether an entity is visible and should be extracted for rendering
pub computed_visibility: ComputedVisibility,
pub view_visibility: ViewVisibility,
}

impl<M: Material> Default for MaterialMeshBundle<M> {
Expand All @@ -37,7 +39,8 @@ impl<M: Material> Default for MaterialMeshBundle<M> {
transform: Default::default(),
global_transform: Default::default(),
visibility: Default::default(),
computed_visibility: Default::default(),
inherited_visibility: Default::default(),
view_visibility: Default::default(),
}
}
}
Expand Down Expand Up @@ -85,8 +88,10 @@ pub struct PointLightBundle {
pub global_transform: GlobalTransform,
/// Enables or disables the light
pub visibility: Visibility,
/// Inherited visibility of an entity.
pub inherited_visibility: InheritedVisibility,
/// Algorithmically-computed indication of whether an entity is visible and should be extracted for rendering
pub computed_visibility: ComputedVisibility,
pub view_visibility: ViewVisibility,
}

/// A component bundle for spot light entities
Expand All @@ -99,8 +104,10 @@ pub struct SpotLightBundle {
pub global_transform: GlobalTransform,
/// Enables or disables the light
pub visibility: Visibility,
/// Inherited visibility of an entity.
pub inherited_visibility: InheritedVisibility,
/// Algorithmically-computed indication of whether an entity is visible and should be extracted for rendering
pub computed_visibility: ComputedVisibility,
pub view_visibility: ViewVisibility,
}

/// A component bundle for [`DirectionalLight`] entities.
Expand All @@ -115,6 +122,8 @@ pub struct DirectionalLightBundle {
pub global_transform: GlobalTransform,
/// Enables or disables the light
pub visibility: Visibility,
/// Inherited visibility of an entity.
pub visible_in_hieararchy: InheritedVisibility,
/// Algorithmically-computed indication of whether an entity is visible and should be extracted for rendering
pub computed_visibility: ComputedVisibility,
pub view_visibility: ViewVisibility,
}
4 changes: 2 additions & 2 deletions crates/bevy_pbr/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ impl Plugin for PbrPlugin {
.after(CameraUpdateSystem),
update_directional_light_frusta
.in_set(SimulationLightSystems::UpdateLightFrusta)
// This must run after CheckVisibility because it relies on ComputedVisibility::is_visible()
// This must run after CheckVisibility because it relies on `ViewVisibility`
.after(VisibilitySystems::CheckVisibility)
.after(TransformSystem::TransformPropagate)
.after(SimulationLightSystems::UpdateDirectionalLightCascades)
Expand All @@ -241,7 +241,7 @@ impl Plugin for PbrPlugin {
.after(TransformSystem::TransformPropagate)
.after(SimulationLightSystems::UpdateLightFrusta)
// NOTE: This MUST be scheduled AFTER the core renderer visibility check
// because that resets entity ComputedVisibility for the first view
// because that resets entity `ViewVisibility` for the first view
// which would override any results from this otherwise
.after(VisibilitySystems::CheckVisibility),
),
Expand Down
64 changes: 34 additions & 30 deletions crates/bevy_pbr/src/light.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use bevy_render::{
primitives::{Aabb, CascadesFrusta, CubemapFrusta, Frustum, HalfSpace, Sphere},
render_resource::BufferBindingType,
renderer::RenderDevice,
view::{ComputedVisibility, RenderLayers, VisibleEntities},
view::{InheritedVisibility, RenderLayers, ViewVisibility, VisibleEntities},
};
use bevy_transform::{components::GlobalTransform, prelude::Transform};
use bevy_utils::{tracing::warn, HashMap};
Expand Down Expand Up @@ -1178,8 +1178,8 @@ pub(crate) fn assign_lights_to_clusters(
&mut Clusters,
Option<&mut VisiblePointLights>,
)>,
point_lights_query: Query<(Entity, &GlobalTransform, &PointLight, &ComputedVisibility)>,
spot_lights_query: Query<(Entity, &GlobalTransform, &SpotLight, &ComputedVisibility)>,
point_lights_query: Query<(Entity, &GlobalTransform, &PointLight, &ViewVisibility)>,
spot_lights_query: Query<(Entity, &GlobalTransform, &SpotLight, &ViewVisibility)>,
mut lights: Local<Vec<PointLightAssignmentData>>,
mut cluster_aabb_spheres: Local<Vec<Option<Sphere>>>,
mut max_point_lights_warning_emitted: Local<bool>,
Expand All @@ -1196,7 +1196,7 @@ pub(crate) fn assign_lights_to_clusters(
lights.extend(
point_lights_query
.iter()
.filter(|(.., visibility)| visibility.is_visible())
.filter(|(.., visibility)| visibility.get())
.map(
|(entity, transform, point_light, _visibility)| PointLightAssignmentData {
entity,
Expand All @@ -1210,7 +1210,7 @@ pub(crate) fn assign_lights_to_clusters(
lights.extend(
spot_lights_query
.iter()
.filter(|(.., visibility)| visibility.is_visible())
.filter(|(.., visibility)| visibility.get())
.map(
|(entity, transform, spot_light, _visibility)| PointLightAssignmentData {
entity,
Expand Down Expand Up @@ -1797,7 +1797,7 @@ pub fn update_directional_light_frusta(
(
&Cascades,
&DirectionalLight,
&ComputedVisibility,
&ViewVisibility,
&mut CascadesFrusta,
),
(
Expand All @@ -1810,7 +1810,7 @@ pub fn update_directional_light_frusta(
// The frustum is used for culling meshes to the light for shadow mapping
// so if shadow mapping is disabled for this light, then the frustum is
// not needed.
if !directional_light.shadows_enabled || !visibility.is_visible() {
if !directional_light.shadows_enabled || !visibility.get() {
continue;
}

Expand Down Expand Up @@ -1931,14 +1931,15 @@ pub fn check_light_mesh_visibility(
&CascadesFrusta,
&mut CascadesVisibleEntities,
Option<&RenderLayers>,
&mut ComputedVisibility,
&mut ViewVisibility,
),
Without<SpotLight>,
>,
mut visible_entity_query: Query<
(
Entity,
&mut ComputedVisibility,
&InheritedVisibility,
&mut ViewVisibility,
Option<&RenderLayers>,
Option<&Aabb>,
Option<&GlobalTransform>,
Expand All @@ -1963,13 +1964,8 @@ pub fn check_light_mesh_visibility(
}

// Directional lights
for (
directional_light,
frusta,
mut visible_entities,
maybe_view_mask,
light_computed_visibility,
) in &mut directional_lights
for (directional_light, frusta, mut visible_entities, maybe_view_mask, light_view_visibility) in
&mut directional_lights
{
// Re-use already allocated entries where possible.
let mut views_to_remove = Vec::new();
Expand All @@ -1995,16 +1991,22 @@ pub fn check_light_mesh_visibility(
}

// NOTE: If shadow mapping is disabled for the light then it must have no visible entities
if !directional_light.shadows_enabled || !light_computed_visibility.is_visible() {
if !directional_light.shadows_enabled || !light_view_visibility.get() {
continue;
}

let view_mask = maybe_view_mask.copied().unwrap_or_default();

for (entity, mut computed_visibility, maybe_entity_mask, maybe_aabb, maybe_transform) in
&mut visible_entity_query
for (
entity,
inherited_visibility,
mut view_visibility,
maybe_entity_mask,
maybe_aabb,
maybe_transform,
) in &mut visible_entity_query
{
if !computed_visibility.is_visible_in_hierarchy() {
if !inherited_visibility.get() {
continue;
}

Expand All @@ -2029,12 +2031,12 @@ pub fn check_light_mesh_visibility(
continue;
}

computed_visibility.set_visible_in_view();
view_visibility.set();
frustum_visible_entities.entities.push(entity);
}
}
} else {
computed_visibility.set_visible_in_view();
view_visibility.set();
for view in frusta.frusta.keys() {
let view_visible_entities = visible_entities
.entities
Expand Down Expand Up @@ -2081,13 +2083,14 @@ pub fn check_light_mesh_visibility(

for (
entity,
mut computed_visibility,
inherited_visibility,
mut view_visibility,
maybe_entity_mask,
maybe_aabb,
maybe_transform,
) in &mut visible_entity_query
{
if !computed_visibility.is_visible_in_hierarchy() {
if !inherited_visibility.get() {
continue;
}

Expand All @@ -2109,12 +2112,12 @@ pub fn check_light_mesh_visibility(
.zip(cubemap_visible_entities.iter_mut())
{
if frustum.intersects_obb(aabb, &model_to_world, true, true) {
computed_visibility.set_visible_in_view();
view_visibility.set();
visible_entities.entities.push(entity);
}
}
} else {
computed_visibility.set_visible_in_view();
view_visibility.set();
for visible_entities in cubemap_visible_entities.iter_mut() {
visible_entities.entities.push(entity);
}
Expand Down Expand Up @@ -2145,13 +2148,14 @@ pub fn check_light_mesh_visibility(

for (
entity,
mut computed_visibility,
inherited_visibility,
mut view_visibility,
maybe_entity_mask,
maybe_aabb,
maybe_transform,
) in visible_entity_query.iter_mut()
{
if !computed_visibility.is_visible_in_hierarchy() {
if !inherited_visibility.get() {
continue;
}

Expand All @@ -2169,11 +2173,11 @@ pub fn check_light_mesh_visibility(
}

if frustum.intersects_obb(aabb, &model_to_world, true, true) {
computed_visibility.set_visible_in_view();
view_visibility.set();
visible_entities.entities.push(entity);
}
} else {
computed_visibility.set_visible_in_view();
view_visibility.set();
visible_entities.entities.push(entity);
}
}
Expand Down
24 changes: 13 additions & 11 deletions crates/bevy_pbr/src/render/light.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use bevy_render::{
render_resource::*,
renderer::{RenderContext, RenderDevice, RenderQueue},
texture::*,
view::{ComputedVisibility, ExtractedView, VisibleEntities},
view::{ExtractedView, ViewVisibility, VisibleEntities},
Extract,
};
use bevy_transform::{components::GlobalTransform, prelude::Transform};
Expand Down Expand Up @@ -299,15 +299,15 @@ pub fn extract_lights(
&PointLight,
&CubemapVisibleEntities,
&GlobalTransform,
&ComputedVisibility,
&ViewVisibility,
)>,
>,
spot_lights: Extract<
Query<(
&SpotLight,
&VisibleEntities,
&GlobalTransform,
&ComputedVisibility,
&ViewVisibility,
)>,
>,
directional_lights: Extract<
Expand All @@ -319,7 +319,7 @@ pub fn extract_lights(
&Cascades,
&CascadeShadowConfig,
&GlobalTransform,
&ComputedVisibility,
&ViewVisibility,
),
Without<SpotLight>,
>,
Expand All @@ -346,10 +346,10 @@ pub fn extract_lights(

let mut point_lights_values = Vec::with_capacity(*previous_point_lights_len);
for entity in global_point_lights.iter().copied() {
if let Ok((point_light, cubemap_visible_entities, transform, visibility)) =
if let Ok((point_light, cubemap_visible_entities, transform, view_visibility)) =
point_lights.get(entity)
{
if !visibility.is_visible() {
if !view_visibility.get() {
continue;
}
// TODO: This is very much not ideal. We should be able to re-use the vector memory.
Expand Down Expand Up @@ -385,8 +385,10 @@ pub fn extract_lights(

let mut spot_lights_values = Vec::with_capacity(*previous_spot_lights_len);
for entity in global_point_lights.iter().copied() {
if let Ok((spot_light, visible_entities, transform, visibility)) = spot_lights.get(entity) {
if !visibility.is_visible() {
if let Ok((spot_light, visible_entities, transform, view_visibility)) =
spot_lights.get(entity)
{
if !view_visibility.get() {
continue;
}
// TODO: This is very much not ideal. We should be able to re-use the vector memory.
Expand Down Expand Up @@ -433,10 +435,10 @@ pub fn extract_lights(
cascades,
cascade_config,
transform,
visibility,
) in directional_lights.iter()
view_visibility,
) in &directional_lights
{
if !visibility.is_visible() {
if !view_visibility.get() {
continue;
}

Expand Down
Loading

0 comments on commit 02b520b

Please sign in to comment.