Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hiding parent entity does not hide children #838

Closed
AlexCMueller opened this issue Nov 11, 2020 · 4 comments
Closed

Hiding parent entity does not hide children #838

AlexCMueller opened this issue Nov 11, 2020 · 4 comments
Labels
A-Hierarchy Parent-child entity hierarchies A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@AlexCMueller
Copy link

When I first tested this, I expected hiding a parent entity to also hide its children. This would be a useful behavior: an example would be making a rudimentary state system by having each state be represented by a root entity, and hiding the root entity of inactive states so the entire states aren't visible.

Below is a minimal example of a child still being visible after the parent is hidden, using bevy 0.3.0.

use bevy::prelude::*;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins) 
        .add_startup_system(setup.system())
        .run();
}

fn setup(
    mut commands: Commands,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    commands
        .spawn(Camera2dComponents::default())
        .spawn(())
        .with(Draw {
            is_visible: false,
            ..Default::default()
        })
        .with_children(|parent| {
            parent.spawn(SpriteComponents {
                material: materials.add(Color::BLUE.into()),
                sprite: Sprite::new(Vec2::new(50.0, 50.0)),
                ..Default::default()
            });
        });
}

image

@memoryruins memoryruins added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Nov 11, 2020
@razaamir

This comment has been minimized.

@cart
Copy link
Member

cart commented Nov 12, 2020

Totally agreed. Hiding a parent should hide all descendants

@kumorig
Copy link

kumorig commented Nov 22, 2020

Similar is true for Ui components using structure:

  • NodeBundle
    • ButtonBundle
      • TextBundle
        • Text

Setting the NodeBundle.display to Display::None hides the button, but the text is still rendered, but in the wrong position.

Tested using current latest: 1e9a054

Example with NodeBundle using Display::None

use bevy::prelude::*;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .run();
}

fn setup(
    commands: &mut Commands,
    asset_server: Res<AssetServer>,
    mut materials: ResMut<Assets<ColorMaterial>>,
) {
    commands
        // ui camera
        .spawn(UiCameraBundle::default())
        .spawn(NodeBundle {
            material: materials.add(ColorMaterial::color(Color::rgb(0.5, 0.5, 0.5).into())),
            style: Style {
                // display: Display::Flex,
                display: Display::None,
                ..Default::default()
            },
            ..Default::default()
        })
        .with_children(|p| {
            p.spawn(ButtonBundle {
                style: Style {
                    size: Size::new(Val::Px(150.0), Val::Px(65.0)),
                    margin: Rect::all(Val::Auto),
                    justify_content: JustifyContent::Center,
                    align_items: AlignItems::Center,
                    ..Default::default()
                },
                material: materials.add(ColorMaterial::color(Color::rgb(0.7, 0.7, 0.7).into())),
                ..Default::default()
            })
            .with_children(|p| {
                p.spawn(TextBundle {
                    text: Text {
                        value: "Button".to_string(),
                        font: asset_server.load("fonts/FiraSans-Bold.ttf"),
                        style: TextStyle {
                            font_size: 40.0,
                            color: Color::rgb(0.9, 0.9, 0.9),
                            ..Default::default()
                        },
                    },
                    ..Default::default()
                });
            });
        });
}

Display::Flex

button_N7OzkYyGo8

Display::None

button_nFVsoo4u2G

(I just assume it's the same issue) Especially since the button disappears, you would also assume the button text (or bundle?) to be hidden.

@rparrett
Copy link
Contributor

rparrett commented Feb 2, 2021

This workaround seems to be working for me. Posting here in case it's useful to anyone else.

fn set_visible_recursive(
    is_visible: bool,
    entity: Entity,
    visible_query: &mut Query<&mut Visible>,
    children_query: &Query<&Children>,
) {
    if let Ok(mut visible) = visible_query.get_mut(entity) {
        visible.is_visible = is_visible;
    }

    if let Ok(children) = children_query.get(entity) {
        for child in children.iter() {
            set_visible_recursive(is_visible, *child, visible_query, children_query);
        }
    }
}

fn some_system(
  children_query: Query<&Children>,
  mut visible_query: Query<&mut Visible>,
  // ...
) {
    // ...
    set_visible_recursive(visible, entity, &mut visible_query, &children_query);
}

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior and removed C-Feature A new feature, making something new possible labels Feb 17, 2021
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets and removed A-ECS Entities, components, systems, and events labels Dec 12, 2021
@alice-i-cecile alice-i-cecile added the A-Hierarchy Parent-child entity hierarchies label Apr 4, 2022
@bors bors bot closed this as completed in 40d4992 Jul 15, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this issue Aug 8, 2022
…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 <[email protected]>
james7132 added a commit to james7132/bevy that referenced this issue Oct 28, 2022
…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 <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
7 participants