enum Visibility component (#6320)

Consolidation of all the feedback about #6271 as well as the addition of an "unconditionally visible" mode.

# Objective

The current implementation of the `Visibility` struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching. 

Additionally as it stands Bevy only has two settings for visibility of an entity: 
- "unconditionally hidden" `Visibility { is_visible: false }`, 
- "inherit visibility from parent" `Visibility { is_visible: true }`
   where a root level entity set to "inherit" is visible. 

Note that given the behaviour, the current naming of the inner field is a little deceptive or unclear.

Using an enum for `Visibility` opens the door for adding an extra behaviour mode. This PR adds a new "unconditionally visible" mode, which causes an entity to be visible even if its Parent entity is hidden. There should not really be any performance cost to the addition of this new mode.

--
The recently added `toggle` method is removed in this PR, as its semantics could be confusing with 3 variants.

## Solution

Change the Visibility component into
```rust
enum Visibility {
  Hidden,    // unconditionally hidden
  Visible,   // unconditionally visible
  Inherited, // inherit visibility from parent
}
```

---

## Changelog

### Changed

`Visibility` is now an enum

## Migration Guide

- evaluation of the `visibility.is_visible` field should now check for `visibility == Visibility::Inherited`.
- setting the `visibility.is_visible` field should now directly set the value: `*visibility = Visibility::Inherited`.
- usage of `Visibility::VISIBLE` or `Visibility::INVISIBLE` should now use `Visibility::Inherited` or `Visibility::Hidden` respectively.
- `ComputedVisibility::INVISIBLE` and `SpatialBundle::VISIBLE_IDENTITY` have been renamed to `ComputedVisibility::HIDDEN` and `SpatialBundle::INHERITED_IDENTITY` respectively.






Co-authored-by: Carter Anderson <mcanders1@gmail.com>

Author: ickk

crates/bevy_gltf/src/loader.rs

@@ -465,7 +465,7 @@ async fn load_gltf<'a, 'b>(
         let mut entity_to_skin_index_map = HashMap::new();
 
         world
-            .spawn(SpatialBundle::VISIBLE_IDENTITY)
+            .spawn(SpatialBundle::INHERITED_IDENTITY)
             .with_children(|parent| {
                 for node in scene.nodes() {
                     let result = load_node(

crates/bevy_render/src/spatial_bundle.rs

@@ -33,22 +33,22 @@ impl SpatialBundle {
     pub const fn from_transform(transform: Transform) -> Self {
         SpatialBundle {
             transform,
-            ..Self::VISIBLE_IDENTITY
+            ..Self::INHERITED_IDENTITY
         }
     }
 
     /// A visible [`SpatialBundle`], with no translation, rotation, and a scale of 1 on all axes.
-    pub const VISIBLE_IDENTITY: Self = SpatialBundle {
-        visibility: Visibility::VISIBLE,
-        computed: ComputedVisibility::INVISIBLE,
+    pub const INHERITED_IDENTITY: Self = SpatialBundle {
+        visibility: Visibility::Inherited,
+        computed: ComputedVisibility::HIDDEN,
         transform: Transform::IDENTITY,
         global_transform: GlobalTransform::IDENTITY,
     };
 
     /// An invisible [`SpatialBundle`], with no translation, rotation, and a scale of 1 on all axes.
-    pub const INVISIBLE_IDENTITY: Self = SpatialBundle {
-        visibility: Visibility::INVISIBLE,
-        ..Self::VISIBLE_IDENTITY
+    pub const HIDDEN_IDENTITY: Self = SpatialBundle {
+        visibility: Visibility::Hidden,
+        ..Self::INHERITED_IDENTITY
     };
 }
 

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

@@ -23,34 +23,42 @@ use crate::{
 };
 
 /// User indication of whether an entity is visible. Propagates down the entity hierarchy.
-
-/// If an entity is hidden in this way,  all [`Children`] (and all of their children and so on) will also be hidden.
-/// This is done by setting the values of their [`ComputedVisibility`] component.
-#[derive(Component, Clone, Reflect, FromReflect, Debug)]
+///
+/// If an entity is hidden in this way, all [`Children`] (and all of their children and so on) who
+/// are set to `Inherited` will also be hidden.
+///
+/// This is done by the `visibility_propagate_system` which uses the entity hierarchy and
+/// `Visibility` to set the values of each entity's [`ComputedVisibility`] component.
+#[derive(Component, Clone, Copy, Reflect, FromReflect, Debug, PartialEq, Eq, Default)]
 #[reflect(Component, Default)]
-pub struct Visibility {
-    /// Indicates whether this entity is visible. Hidden values will propagate down the entity hierarchy.
-    /// If this entity is hidden, all of its descendants will be hidden as well. See [`Children`] and [`Parent`] for
-    /// hierarchy info.
-    pub is_visible: bool,
+pub enum Visibility {
+    /// An entity with `Visibility::Inherited` will inherit the Visibility of its [`Parent`].
+    ///
+    /// A root-level entity that is set to `Inherited` will be visible.
+    #[default]
+    Inherited,
+    /// An entity with `Visibility::Hidden` will be unconditionally hidden.
+    Hidden,
+    /// An entity with `Visibility::Visible` will be unconditionally visible.
+    ///
+    /// Note that an entity with `Visibility::Visible` will be visible regardless of whether the
+    /// [`Parent`] entity is hidden.
+    Visible,
 }
 
-impl Default for Visibility {
-    fn default() -> Self {
-        Visibility::VISIBLE
+// Allows `&Visibility == Visibility`
+impl std::cmp::PartialEq<Visibility> for &Visibility {
+    #[inline]
+    fn eq(&self, other: &Visibility) -> bool {
+        **self == *other
     }
 }
 
-impl Visibility {
-    /// A [`Visibility`], set as visible.
-    pub const VISIBLE: Self = Visibility { is_visible: true };
-
-    /// A [`Visibility`], set as invisible.
-    pub const INVISIBLE: Self = Visibility { is_visible: false };
-
-    /// Toggle the visibility.
-    pub fn toggle(&mut self) {
-        self.is_visible = !self.is_visible;
+// Allows `Visibility == &Visibility`
+impl std::cmp::PartialEq<&Visibility> for Visibility {
+    #[inline]
+    fn eq(&self, other: &&Visibility) -> bool {
+        *self == **other
     }
 }
 
@@ -71,13 +79,13 @@ pub struct ComputedVisibility {
 
 impl Default for ComputedVisibility {
     fn default() -> Self {
-        Self::INVISIBLE
+        Self::HIDDEN
     }
 }
 
 impl ComputedVisibility {
     /// A [`ComputedVisibility`], set as invisible.
-    pub const INVISIBLE: Self = ComputedVisibility {
+    pub const HIDDEN: Self = ComputedVisibility {
         flags: ComputedVisibilityFlags::empty(),
     };
 
@@ -298,7 +306,8 @@ fn visibility_propagate_system(
 ) {
     for (children, visibility, mut computed_visibility, entity) in root_query.iter_mut() {
         // reset "view" visibility here ... if this entity should be drawn a future system should set this to true
-        computed_visibility.reset(visibility.is_visible);
+        computed_visibility
+            .reset(visibility == Visibility::Inherited || visibility == Visibility::Visible);
         if let Some(children) = children {
             for child in children.iter() {
                 let _ = propagate_recursive(
@@ -329,7 +338,8 @@ fn propagate_recursive(
             child_parent.get(), expected_parent,
             "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle"
         );
-        let visible_in_hierarchy = visibility.is_visible && parent_visible;
+        let visible_in_hierarchy = (parent_visible && visibility == Visibility::Inherited)
+            || visibility == Visibility::Visible;
         // reset "view" visibility here ... if this entity should be drawn a future system should set this to true
         computed_visibility.reset(visible_in_hierarchy);
         visible_in_hierarchy
@@ -458,21 +468,15 @@ mod test {
 
         let root1 = app
             .world
-            .spawn((
-                Visibility { is_visible: false },
-                ComputedVisibility::default(),
-            ))
+            .spawn((Visibility::Hidden, ComputedVisibility::default()))
             .id();
         let root1_child1 = app
             .world
             .spawn((Visibility::default(), ComputedVisibility::default()))
             .id();
         let root1_child2 = app
             .world
-            .spawn((
-                Visibility { is_visible: false },
-                ComputedVisibility::default(),
-            ))
+            .spawn((Visibility::Hidden, ComputedVisibility::default()))
             .id();
         let root1_child1_grandchild1 = app
             .world
@@ -503,10 +507,7 @@ mod test {
             .id();
         let root2_child2 = app
             .world
-            .spawn((
-                Visibility { is_visible: false },
-                ComputedVisibility::default(),
-            ))
+            .spawn((Visibility::Hidden, ComputedVisibility::default()))
             .id();
         let root2_child1_grandchild1 = app
             .world
@@ -578,4 +579,89 @@ mod test {
             "child's invisibility propagates down to grandchild"
         );
     }
+
+    #[test]
+    fn visibility_propagation_unconditional_visible() {
+        let mut app = App::new();
+        app.add_system(visibility_propagate_system);
+
+        let root1 = app
+            .world
+            .spawn((Visibility::Visible, ComputedVisibility::default()))
+            .id();
+        let root1_child1 = app
+            .world
+            .spawn((Visibility::Inherited, ComputedVisibility::default()))
+            .id();
+        let root1_child2 = app
+            .world
+            .spawn((Visibility::Hidden, ComputedVisibility::default()))
+            .id();
+        let root1_child1_grandchild1 = app
+            .world
+            .spawn((Visibility::Visible, ComputedVisibility::default()))
+            .id();
+        let root1_child2_grandchild1 = app
+            .world
+            .spawn((Visibility::Visible, ComputedVisibility::default()))
+            .id();
+
+        let root2 = app
+            .world
+            .spawn((Visibility::Inherited, ComputedVisibility::default()))
+            .id();
+        let root3 = app
+            .world
+            .spawn((Visibility::Hidden, ComputedVisibility::default()))
+            .id();
+
+        app.world
+            .entity_mut(root1)
+            .push_children(&[root1_child1, root1_child2]);
+        app.world
+            .entity_mut(root1_child1)
+            .push_children(&[root1_child1_grandchild1]);
+        app.world
+            .entity_mut(root1_child2)
+            .push_children(&[root1_child2_grandchild1]);
+
+        app.update();
+
+        let is_visible = |e: Entity| {
+            app.world
+                .entity(e)
+                .get::<ComputedVisibility>()
+                .unwrap()
+                .is_visible_in_hierarchy()
+        };
+        assert!(
+            is_visible(root1),
+            "an unconditionally visible root is visible"
+        );
+        assert!(
+            is_visible(root1_child1),
+            "an inheriting child of an unconditionally visible parent is visible"
+        );
+        assert!(
+            !is_visible(root1_child2),
+            "a hidden child on an unconditionally visible parent is hidden"
+        );
+        assert!(
+            is_visible(root1_child1_grandchild1),
+            "an unconditionally visible child of an inheriting parent is visible"
+        );
+        assert!(
+            is_visible(root1_child2_grandchild1),
+            "an unconditionally visible child of a hidden parent is visible"
+        );
+        assert!(is_visible(root2), "an inheriting root is visible");
+        assert!(!is_visible(root3), "a hidden root is hidden");
+    }
+
+    #[test]
+    fn ensure_visibility_enum_size() {
+        use std::mem;
+        assert_eq!(1, mem::size_of::<Visibility>());
+        assert_eq!(1, mem::size_of::<Option<Visibility>>());
+    }
 }

examples/2d/mesh2d_manual.rs

@@ -103,7 +103,7 @@ fn star(
         // The `Handle<Mesh>` needs to be wrapped in a `Mesh2dHandle` to use 2d rendering instead of 3d
         Mesh2dHandle(meshes.add(star)),
         // This bundle's components are needed for something to be rendered
-        SpatialBundle::VISIBLE_IDENTITY,
+        SpatialBundle::INHERITED_IDENTITY,
     ));
 
     // Spawn the camera

examples/animation/animated_transform.rs

@@ -129,7 +129,7 @@ fn setup(
         .with_children(|p| {
             // This entity is just used for animation, but doesn't display anything
             p.spawn((
-                SpatialBundle::VISIBLE_IDENTITY,
+                SpatialBundle::INHERITED_IDENTITY,
                 // Add the Name component
                 orbit_controller,
             ))

examples/shader/shader_instancing.rs

@@ -35,7 +35,7 @@ fn main() {
 fn setup(mut commands: Commands, mut meshes: ResMut<Assets<Mesh>>) {
     commands.spawn((
         meshes.add(Mesh::from(shape::Cube { size: 0.5 })),
-        SpatialBundle::VISIBLE_IDENTITY,
+        SpatialBundle::INHERITED_IDENTITY,
         InstanceMaterialData(
             (1..=10)
                 .flat_map(|x| (1..=10).map(move |y| (x as f32 / 10.0, y as f32 / 10.0)))

examples/stress_tests/many_foxes.rs

@@ -113,7 +113,7 @@ fn setup(
         let (base_rotation, ring_direction) = ring_directions[ring_index % 2];
         let ring_parent = commands
             .spawn((
-                SpatialBundle::VISIBLE_IDENTITY,
+                SpatialBundle::INHERITED_IDENTITY,
                 ring_direction,
                 Ring { radius },
             ))