[Merged by Bors] - Visibilty Inheritance, universal ComputedVisibility and RenderLayers support #5310

cart · 2022-07-13T19:40:58Z

Objective

Fixes #4907. Fixes #838. Fixes #5089.
Supersedes #5146. Supersedes #2087. Supersedes #865. Supersedes #5114

Visibility is currently entirely local. Set a parent entity to be invisible, and the children are still visible. This makes it hard for users to hide entire hierarchies of entities.

Additionally, the semantics of Visibility vs ComputedVisibility are inconsistent across entity types. 3D meshes use ComputedVisibility as the "definitive" visibility component, with Visibility being just one data source. Sprites just use Visibility, which means they can't feed off of ComputedVisibility data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information.

Solution

Splits ComputedVisibilty::is_visible into ComputedVisibilty::is_visible_in_view and ComputedVisibilty::is_visible_in_hierarchy. For each visible entity, is_visible_in_hierarchy is computed by propagating visibility down the hierarchy. The ComputedVisibility::is_visible() function combines these two booleans for the canonical "is this entity visible" function.

Additionally, all entities that have Visibility now also have ComputedVisibility. Sprites, Lights, and UI entities now use ComputedVisibility when appropriate.

This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support RenderLayers and work properly across multiple views.

Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on main runs in 0.017612 seconds and this runs in 0.01902. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See this thread for more info. Note that #5146 in combination with #5114 are a viable alternative to this PR and would perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice.

Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite.

Note that this builds directly on #5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into ComputedVisibility, then does the additional work of porting everything to ComputedVisibility. See my comments here for rationale.

Follow up work

Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separateVisibleLights collection for both clarity and performance reasons. And maybe even consider scoping VisibleEntities down to VisibleMeshes?.
Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset.
Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the is_visible() function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc).

Changelog

ComputedVisibility now takes hierarchy visibility into account.
2D, UI and Light entities now use the ComputedVisibility component.

Migration Guide

If you were previously reading Visibility::is_visible as the "actual visibility" for sprites or lights, use ComputedVisibilty::is_visible() instead:

// before (0.7)
fn system(query: Query<&Visibility>) {
  for visibility in query.iter() {
    if visibility.is_visible {
       log!("found visible entity");
    }
  }
}

// after (0.8)
fn system(query: Query<&ComputedVisibility>) {
  for visibility in query.iter() {
    if visibility.is_visible() {
       log!("found visible entity");
    }
  }
}

…here

alice-i-cecile · 2022-07-13T19:48:51Z

Does this supercede #5089? I don't think this will fix #5183; do you want to toss that in?

cart · 2022-07-13T19:55:33Z

This fixes #5089. It does not add "renderlayer visibility inheritance", so it does not fix #5183. If we choose to support that (and it seems like we might want to), I think we should probably do that in a separate PR.

alice-i-cecile · 2022-07-13T20:02:37Z

My apologies, I meant to ask if this supercedes #5114.

superdump · 2022-07-13T20:20:02Z

My apologies, I meant to ask if this supercedes #5114.

Yes.

superdump

Basically LGTM, just a few nits.

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

crates/bevy_sprite/src/render/mod.rs

cart · 2022-07-13T22:24:38Z

Hmm some thoughts on "light renderlayers":

Lights are both "visible" to cameras and "see" entities. Naively, porting them to RenderLayers means that value means "both" things. I think this means we need to split out RenderLayers from VisibleRenderLayers. Cameras only "see" things, so they have VisibleRenderLayers. Lights are both "seen" and "see things" so they have both.
We should really split out VisibleEntities from VisibleLights (and maybe even finer granularity) for performance reasons. No sense in iterating over a list that contains both across both contexts. This would require some changes to check_visibility.
This adds enough complexity that it probably deserves a follow up. The current behavior in the PR appears to be equivalent to main.

I did get "per view directional lights using render layers" working though, but I don't think I will push those changes here:

Co-authored-by: Robert Swain <robert.swain@gmail.com>

@james7132

…support (#5310) # Objective Fixes #4907. Fixes #838. Fixes #5089. Supersedes #5146. Supersedes #2087. Supersedes #865. Supersedes #5114 Visibility is currently entirely local. Set a parent entity to be invisible, and the children are still visible. This makes it hard for users to hide entire hierarchies of entities. Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information. ## Solution Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function. Additionally, all entities that have `Visibility` now also have `ComputedVisibility`. Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate. This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views. Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](#5146 (comment)) for more info. Note that #5146 in combination with #5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice. Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite. ![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png) Note that this builds directly on #5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](#5146 (comment)) for rationale. ## Follow up work * Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?. * Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset. * Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc). --- ## Changelog * ComputedVisibility now takes hierarchy visibility into account. * 2D, UI and Light entities now use the ComputedVisibility component. ## Migration Guide If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead: ```rust // before (0.7) fn system(query: Query<&Visibility>) { for visibility in query.iter() { if visibility.is_visible { log!("found visible entity"); } } } // after (0.8) fn system(query: Query<&ComputedVisibility>) { for visibility in query.iter() { if visibility.is_visible() { log!("found visible entity"); } } } ``` Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2022-07-15T23:51:20Z

Pull request successfully merged into main.

Build succeeded:

…5335) # Objective Gltfs, and a few examples were broken by #5310. Fix em. Closes #5334 ## Solution Add `VisibilityBundle` as described here: #5334 (comment) and sprinkle it around where needed.

# Objective UI nodes can be hidden by setting their `Visibility` property. Since #5310 was merged, this is now ergonomic to use, as visibility is now inherited. However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes. ## Solution Fixes #5360. I've updated the `ui_focus_system` to accomplish this in a minimally intrusive way, and updated the docs to match. **NOTE:** I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for `bevy_ui`. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope. ## Paths not taken ### Separate `Disabled` component This is a much larger and more controversial change, and not well-scoped to UI. Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on. Splitting this behavior would be more complex, and substantially violate user expectations. ### A separate limbo world Mentioned in the linked issue. Super cool, but all of the problems of the `Disabled` component solution with a whole new RFC-worth of complexity. ### Using change detection to reduce the amount of redundant work Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system. We simply don't have the tests or benchmarks here to justify this. ## Changelog - UI nodes are now always in an `Interaction::None` state while they are hidden (via the `ComputedVisibility` component).

@james7132

…support (bevyengine#5310) # Objective Fixes bevyengine#4907. Fixes bevyengine#838. Fixes bevyengine#5089. Supersedes bevyengine#5146. Supersedes bevyengine#2087. Supersedes bevyengine#865. Supersedes bevyengine#5114 Visibility is currently entirely local. Set a parent entity to be invisible, and the children are still visible. This makes it hard for users to hide entire hierarchies of entities. Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information. ## Solution Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function. Additionally, all entities that have `Visibility` now also have `ComputedVisibility`. Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate. This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views. Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](bevyengine#5146 (comment)) for more info. Note that bevyengine#5146 in combination with bevyengine#5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice. Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite. ![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png) Note that this builds directly on bevyengine#5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](bevyengine#5146 (comment)) for rationale. ## Follow up work * Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?. * Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset. * Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc). --- ## Changelog * ComputedVisibility now takes hierarchy visibility into account. * 2D, UI and Light entities now use the ComputedVisibility component. ## Migration Guide If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead: ```rust // before (0.7) fn system(query: Query<&Visibility>) { for visibility in query.iter() { if visibility.is_visible { log!("found visible entity"); } } } // after (0.8) fn system(query: Query<&ComputedVisibility>) { for visibility in query.iter() { if visibility.is_visible() { log!("found visible entity"); } } } ``` Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…evyengine#5335) # Objective Gltfs, and a few examples were broken by bevyengine#5310. Fix em. Closes bevyengine#5334 ## Solution Add `VisibilityBundle` as described here: bevyengine#5334 (comment) and sprinkle it around where needed.

…yengine#5361) # Objective UI nodes can be hidden by setting their `Visibility` property. Since bevyengine#5310 was merged, this is now ergonomic to use, as visibility is now inherited. However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes. ## Solution Fixes bevyengine#5360. I've updated the `ui_focus_system` to accomplish this in a minimally intrusive way, and updated the docs to match. **NOTE:** I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for `bevy_ui`. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope. ## Paths not taken ### Separate `Disabled` component This is a much larger and more controversial change, and not well-scoped to UI. Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on. Splitting this behavior would be more complex, and substantially violate user expectations. ### A separate limbo world Mentioned in the linked issue. Super cool, but all of the problems of the `Disabled` component solution with a whole new RFC-worth of complexity. ### Using change detection to reduce the amount of redundant work Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system. We simply don't have the tests or benchmarks here to justify this. ## Changelog - UI nodes are now always in an `Interaction::None` state while they are hidden (via the `ComputedVisibility` component).

@james7132

…support (bevyengine#5310) # Objective Fixes bevyengine#4907. Fixes bevyengine#838. Fixes bevyengine#5089. Supersedes bevyengine#5146. Supersedes bevyengine#2087. Supersedes bevyengine#865. Supersedes bevyengine#5114 Visibility is currently entirely local. Set a parent entity to be invisible, and the children are still visible. This makes it hard for users to hide entire hierarchies of entities. Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information. ## Solution Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function. Additionally, all entities that have `Visibility` now also have `ComputedVisibility`. Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate. This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views. Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](bevyengine#5146 (comment)) for more info. Note that bevyengine#5146 in combination with bevyengine#5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice. Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite. ![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png) Note that this builds directly on bevyengine#5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](bevyengine#5146 (comment)) for rationale. ## Follow up work * Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?. * Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset. * Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc). --- ## Changelog * ComputedVisibility now takes hierarchy visibility into account. * 2D, UI and Light entities now use the ComputedVisibility component. ## Migration Guide If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead: ```rust // before (0.7) fn system(query: Query<&Visibility>) { for visibility in query.iter() { if visibility.is_visible { log!("found visible entity"); } } } // after (0.8) fn system(query: Query<&ComputedVisibility>) { for visibility in query.iter() { if visibility.is_visible() { log!("found visible entity"); } } } ``` Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…evyengine#5335) # Objective Gltfs, and a few examples were broken by bevyengine#5310. Fix em. Closes bevyengine#5334 ## Solution Add `VisibilityBundle` as described here: bevyengine#5334 (comment) and sprinkle it around where needed.

…yengine#5361) # Objective UI nodes can be hidden by setting their `Visibility` property. Since bevyengine#5310 was merged, this is now ergonomic to use, as visibility is now inherited. However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes. ## Solution Fixes bevyengine#5360. I've updated the `ui_focus_system` to accomplish this in a minimally intrusive way, and updated the docs to match. **NOTE:** I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for `bevy_ui`. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope. ## Paths not taken ### Separate `Disabled` component This is a much larger and more controversial change, and not well-scoped to UI. Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on. Splitting this behavior would be more complex, and substantially violate user expectations. ### A separate limbo world Mentioned in the linked issue. Super cool, but all of the problems of the `Disabled` component solution with a whole new RFC-worth of complexity. ### Using change detection to reduce the amount of redundant work Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system. We simply don't have the tests or benchmarks here to justify this. ## Changelog - UI nodes are now always in an `Interaction::None` state while they are hidden (via the `ComputedVisibility` component).

# Objective `ComputedVisibility` could afford to be smaller/faster. Optimizing the size and performance of operations on the component will positively benefit almost all extraction systems. This was listed as one of the potential pieces of future work for #5310. ## Solution Merge both internal booleans into a single `u8` bitflag field. Rely on bitmasks to evaluate local, hierarchical, and general visibility. Pros: - `ComputedVisibility::is_visible` should be a single bitmask test instead of two. - `ComputedVisibility` is now only 1 byte. Should be able to fit 100% more per cache line when using dense iteration. Cons: - Harder to read. - Setting individual values inside `ComputedVisiblity` require bitmask mutations. This should be a non-breaking change. No public API was changed. The only publicly visible effect is that `ComputedVisibility` is now 1 byte instead of 2.

# Objective `ComputedVisibility` could afford to be smaller/faster. Optimizing the size and performance of operations on the component will positively benefit almost all extraction systems. This was listed as one of the potential pieces of future work for bevyengine#5310. ## Solution Merge both internal booleans into a single `u8` bitflag field. Rely on bitmasks to evaluate local, hierarchical, and general visibility. Pros: - `ComputedVisibility::is_visible` should be a single bitmask test instead of two. - `ComputedVisibility` is now only 1 byte. Should be able to fit 100% more per cache line when using dense iteration. Cons: - Harder to read. - Setting individual values inside `ComputedVisiblity` require bitmask mutations. This should be a non-breaking change. No public API was changed. The only publicly visible effect is that `ComputedVisibility` is now 1 byte instead of 2.

@james7132

…support (bevyengine#5310) # Objective Fixes bevyengine#4907. Fixes bevyengine#838. Fixes bevyengine#5089. Supersedes bevyengine#5146. Supersedes bevyengine#2087. Supersedes bevyengine#865. Supersedes bevyengine#5114 Visibility is currently entirely local. Set a parent entity to be invisible, and the children are still visible. This makes it hard for users to hide entire hierarchies of entities. Additionally, the semantics of `Visibility` vs `ComputedVisibility` are inconsistent across entity types. 3D meshes use `ComputedVisibility` as the "definitive" visibility component, with `Visibility` being just one data source. Sprites just use `Visibility`, which means they can't feed off of `ComputedVisibility` data, such as culling information, RenderLayers, and (added in this pr) visibility inheritance information. ## Solution Splits `ComputedVisibilty::is_visible` into `ComputedVisibilty::is_visible_in_view` and `ComputedVisibilty::is_visible_in_hierarchy`. For each visible entity, `is_visible_in_hierarchy` is computed by propagating visibility down the hierarchy. The `ComputedVisibility::is_visible()` function combines these two booleans for the canonical "is this entity visible" function. Additionally, all entities that have `Visibility` now also have `ComputedVisibility`. Sprites, Lights, and UI entities now use `ComputedVisibility` when appropriate. This means that in addition to visibility inheritance, everything using Visibility now also supports RenderLayers. Notably, Sprites (and other 2d objects) now support `RenderLayers` and work properly across multiple views. Also note that this does increase the amount of work done per sprite. Bevymark with 100,000 sprites on `main` runs in `0.017612` seconds and this runs in `0.01902`. That is certainly a gap, but I believe the api consistency and extra functionality this buys us is worth it. See [this thread](bevyengine#5146 (comment)) for more info. Note that bevyengine#5146 in combination with bevyengine#5114 _are_ a viable alternative to this PR and _would_ perform better, but that comes at the cost of api inconsistencies and doing visibility calculations in the "wrong" place. The current visibility system does have potential for performance improvements. I would prefer to evolve that one system as a whole rather than doing custom hacks / different behaviors for each feature slice. Here is a "split screen" example where the left camera uses RenderLayers to filter out the blue sprite. ![image](https://user-images.githubusercontent.com/2694663/178814868-2e9a2173-bf8c-4c79-8815-633899d492c3.png) Note that this builds directly on bevyengine#5146 and that @james7132 deserves the credit for the baseline visibility inheritance work. This pr moves the inherited visibility field into `ComputedVisibility`, then does the additional work of porting everything to `ComputedVisibility`. See my [comments here](bevyengine#5146 (comment)) for rationale. ## Follow up work * Now that lights use ComputedVisibility, VisibleEntities now includes "visible lights" in the entity list. Functionally not a problem as we use queries to filter the list down in the desired context. But we should consider splitting this out into a separate`VisibleLights` collection for both clarity and performance reasons. And _maybe_ even consider scoping `VisibleEntities` down to `VisibleMeshes`?. * Investigate alternative sprite rendering impls (in combination with visibility system tweaks) that avoid re-generating a per-view fixedbitset of visible entities every frame, then checking each ExtractedEntity. This is where most of the performance overhead lives. Ex: we could generate ExtractedEntities per-view using the VisibleEntities list, avoiding the need for the bitset. * Should ComputedVisibility use bitflags under the hood? This would cut down on the size of the component, potentially speed up the `is_visible()` function, and allow us to cheaply expand ComputedVisibility with more data (ex: split out local visibility and parent visibility, add more culling classes, etc). --- ## Changelog * ComputedVisibility now takes hierarchy visibility into account. * 2D, UI and Light entities now use the ComputedVisibility component. ## Migration Guide If you were previously reading `Visibility::is_visible` as the "actual visibility" for sprites or lights, use `ComputedVisibilty::is_visible()` instead: ```rust // before (0.7) fn system(query: Query<&Visibility>) { for visibility in query.iter() { if visibility.is_visible { log!("found visible entity"); } } } // after (0.8) fn system(query: Query<&ComputedVisibility>) { for visibility in query.iter() { if visibility.is_visible() { log!("found visible entity"); } } } ``` Co-authored-by: Carter Anderson <mcanders1@gmail.com>

…evyengine#5335) # Objective Gltfs, and a few examples were broken by bevyengine#5310. Fix em. Closes bevyengine#5334 ## Solution Add `VisibilityBundle` as described here: bevyengine#5334 (comment) and sprinkle it around where needed.

…yengine#5361) # Objective UI nodes can be hidden by setting their `Visibility` property. Since bevyengine#5310 was merged, this is now ergonomic to use, as visibility is now inherited. However, UI nodes still receive (and store) interactions when hidden, resulting in surprising hidden state (and an inability to otherwise disable UI nodes. ## Solution Fixes bevyengine#5360. I've updated the `ui_focus_system` to accomplish this in a minimally intrusive way, and updated the docs to match. **NOTE:** I have not added automated tests to verify this behavior, as we do not currently have a good testing paradigm for `bevy_ui`. I'm not thrilled with that by any means, but I'm not sure fixing it is within scope. ## Paths not taken ### Separate `Disabled` component This is a much larger and more controversial change, and not well-scoped to UI. Furthermore, it is extremely rare that you want hidden UI elements to function: the most common cases are for things like changing tabs, collapsing elements or so on. Splitting this behavior would be more complex, and substantially violate user expectations. ### A separate limbo world Mentioned in the linked issue. Super cool, but all of the problems of the `Disabled` component solution with a whole new RFC-worth of complexity. ### Using change detection to reduce the amount of redundant work Adds a lot of complexity for questionable performance gains. Likely involves a complete refactor of the entire system. We simply don't have the tests or benchmarks here to justify this. ## Changelog - UI nodes are now always in an `Interaction::None` state while they are hidden (via the `ComputedVisibility` component).

# Objective `ComputedVisibility` could afford to be smaller/faster. Optimizing the size and performance of operations on the component will positively benefit almost all extraction systems. This was listed as one of the potential pieces of future work for bevyengine#5310. ## Solution Merge both internal booleans into a single `u8` bitflag field. Rely on bitmasks to evaluate local, hierarchical, and general visibility. Pros: - `ComputedVisibility::is_visible` should be a single bitmask test instead of two. - `ComputedVisibility` is now only 1 byte. Should be able to fit 100% more per cache line when using dense iteration. Cons: - Harder to read. - Setting individual values inside `ComputedVisiblity` require bitmask mutations. This should be a non-breaking change. No public API was changed. The only publicly visible effect is that `ComputedVisibility` is now 1 byte instead of 2.

# Objective bevy-scene does not have a reason to depend on bevy-render except to include the `Visibility` and `ComputedVisibility` components. Including that in the dependency chain is unnecessary for people not using `bevy_render`. Also fixed a problem where compilation fails when the `serialize` feature was not enabled. ## Solution This was added in #5335 to address some of the problems caused by #5310. Imo the user just always have to remember to include `VisibilityBundle` when they spawn `SceneBundle` or `DynamicSceneBundle`, but that will be a breaking change. This PR makes `bevy_render` an optional dependency of `bevy_scene` instead to respect the existing behavior.

# Objective bevy-scene does not have a reason to depend on bevy-render except to include the `Visibility` and `ComputedVisibility` components. Including that in the dependency chain is unnecessary for people not using `bevy_render`. Also fixed a problem where compilation fails when the `serialize` feature was not enabled. ## Solution This was added in bevyengine#5335 to address some of the problems caused by bevyengine#5310. Imo the user just always have to remember to include `VisibilityBundle` when they spawn `SceneBundle` or `DynamicSceneBundle`, but that will be a breaking change. This PR makes `bevy_render` an optional dependency of `bevy_scene` instead to respect the existing behavior.

james7132 and others added 11 commits June 29, 2022 23:57

Inherit visibility

3d27c97

Fix docs

d6d8c19

Merge branch 'main' into inherited-visibility

abeca0e

Avoid triggering change detection where possible

a0968bd

Merge branch 'main' into inherited-visibility

a0b7db7

Adopt ComputedVisibility everywhere and move "inherited visibility" t…

b4d4f95

…here

default to false

2aa616b

separate aabb visibility from non-aabb

9bfc1d7

Merge remote-tracking branch 'origin/main' into pr/james7132/5146

cb75f45

fix lighting ordering

97062fc

use bitset for per-view visibility

af25b22

cart added this to the Bevy 0.8 milestone Jul 13, 2022

cart mentioned this pull request Jul 13, 2022

Allow Visibility to be inherited #5146

Closed

cart added 2 commits July 13, 2022 14:02

better visibility propagation test (and remove old tests)

c9c5c84

Encapsulate ComputedVisibility fields and improve docs

9869fbf

superdump requested changes Jul 13, 2022

View reviewed changes

cart and others added 5 commits July 13, 2022 16:21

typo fix

80f3c8f

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

08554d9

Co-authored-by: Robert Swain <robert.swain@gmail.com>

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

d3630bd

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Update crates/bevy_sprite/src/render/mod.rs

0043b0d

Co-authored-by: Robert Swain <robert.swain@gmail.com>

remove unrelated comment

bb95756

bors bot changed the title ~~Visibilty Inheritance, universal ComputedVisibility and RenderLayers support~~ [Merged by Bors] - Visibilty Inheritance, universal ComputedVisibility and RenderLayers support Jul 15, 2022

bors bot closed this Jul 15, 2022

james7132 mentioned this pull request Jul 15, 2022

Make sprites respect RenderLayers #5114

Closed

This was referenced Jul 16, 2022

Children are invisible unless parent has Visibility and ComputedInvisibility #5334

Closed

[Merged by Bors] - Add VisibilityBundle and use it to fix gltfs, scenes, and examples #5335

Closed

alice-i-cecile mentioned this pull request Jul 18, 2022

[Merged by Bors] - Disable UI node Interaction when ComputedVisibility is false #5361

Closed

ManevilleF mentioned this pull request Jul 18, 2022

Adding a Prepare stage for the sprite pipeline #5247

Closed

TethysSvensson mentioned this pull request Jul 19, 2022

Make sprites and text2d work with RenderLayers (add ComputedVisibility) #4007

Closed

james7132 mentioned this pull request Oct 20, 2022

[Merged by Bors] - Shrink ComputedVisibility #6305

Closed

Neo-Zhixing mentioned this pull request Mar 20, 2023

Make bevy_render an optional dependency of bevy_scene #8136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Visibilty Inheritance, universal ComputedVisibility and RenderLayers support #5310

[Merged by Bors] - Visibilty Inheritance, universal ComputedVisibility and RenderLayers support #5310

cart commented Jul 13, 2022 •

edited

Loading

alice-i-cecile commented Jul 13, 2022

cart commented Jul 13, 2022

alice-i-cecile commented Jul 13, 2022

superdump commented Jul 13, 2022

superdump left a comment

cart commented Jul 13, 2022

bors bot commented Jul 15, 2022

[Merged by Bors] - Visibilty Inheritance, universal ComputedVisibility and RenderLayers support #5310

[Merged by Bors] - Visibilty Inheritance, universal ComputedVisibility and RenderLayers support #5310

Conversation

cart commented Jul 13, 2022 • edited Loading

Objective

Solution

Follow up work

Changelog

Migration Guide

alice-i-cecile commented Jul 13, 2022

cart commented Jul 13, 2022

alice-i-cecile commented Jul 13, 2022

superdump commented Jul 13, 2022

superdump left a comment

Choose a reason for hiding this comment

cart commented Jul 13, 2022

bors bot commented Jul 15, 2022

cart commented Jul 13, 2022 •

edited

Loading