Split ComputedVisibility into two components to allow for accurate change detection and speed up visibility propagation

[JoJoJet]

Objective

Fix #8267.
Fixes half of #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.

// 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(),
});

// 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() {

// 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() {

// 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();

[alice-i-cecile]

Could you run some benchmarks? I expect this to be quite performance sensitive, but I'd put even odds on improvements vs regressions.

[alice-i-cecile]

Given that we have a trivial getter, and a public way to set values as both true and false, I have a hard time seeing why we shouldn't just make this field pub.

The docs about how to use this are excellent, but they could just be moved to the struct level.

[JoJoJet]

In previous discussion about components like Parent, I think it was decided that we should avoid the .0 syntax in public APIs, which is why I opted to derive deref and add a getter.

I decided not to provide a .set(false) option, as marking an entity as visible in view is meant to be irreversible during a given frame (this is a property I carried over from the old API, which had .set_visible_in_view()). Of course you could still reset the view visibility by just overwriting the entire component, but at least this API nudges users in the right direction.

Agreed about the documentation.

[alice-i-cecile]

Non-blocking: this may benefit from parallel query iteration: it's a trivial loop with a ton of entities in it.

[JoJoJet]

I ran cargo run --release --example many_foxes

1. Without par_iter: avg fps 288.0 over 5285 frames.
2. With par_iter: avg fps 281.8 over 5073.

Parallel iteration doesn't seem to have much of an effect, though it's possible that it would scale better for scenes with a huge number of entities.

[alice-i-cecile]

From an architectural and usability perspective I really like these proposed changes. I am nervous about performance costs here: I often see these systems as among the most expensive in real applications.

[JoJoJet]

I ran the stress test cargo run --release --example many_foxes:

1. Main - avg fps of 285.2 over 5239 frames.
2. This PR - avg FPS of 287.9 over 5475 frames.

[JoJoJet]

With cargo run --release --example bevymark with ~8000 birds, the FPS stays in the high 600s with and without this PR. I don't think this PR adds or takes away performance.

[mockersf]

Would it be possible to keep it as one component and check if the value changes before updating it? Or triggering change manually when needed?

[JoJoJet]

Would it be possible to keep it as one component and check if the value changes before updating it?

That's the "naive" solution I mentioned in #8267. In order for that approach to work, we'd have to overhaul how view visibility is calculated.

Each frame, the view visibility of every entity is reset. Then any number of systems belonging to the CheckVisibility set will mark entities that are visible in any view. Systems belonging to this set can live in any crate, not just bevy_render. View calculations are done in bevy_pbr, and are allowed to be performed in 3rd party. Because of this, it's not as simple as just using set_if_neq.

Furthermore, I think it is good to split it into separate components, even without the change detection issue. Inherited visibility and view visibility are separate concepts, and it's odd to bundle them the way we do now.

[DGriffin91]

@JoJoJet How do I run it single threaded?

[JoJoJet]

You should be able to just disable the multi-threaded cargo feature for bevy.

[DGriffin91]

Without the multi-threaded feature I get 101ms on bevy main and 86ms on this PR. Nice work!

[JoJoJet]

Nice, thanks for testing it!

[superdump]

Just a few minor comments. Looks generally good.

[JoJoJet]

Thanks for the review! I believe that's everything.