From 643620514b0b6edb9e06c510eb8ba8e88bb77215 Mon Sep 17 00:00:00 2001 From: ickk Date: Sun, 25 Dec 2022 00:39:29 +0000 Subject: [PATCH] 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 --- crates/bevy_gltf/src/loader.rs | 2 +- crates/bevy_render/src/spatial_bundle.rs | 14 +- crates/bevy_render/src/view/visibility/mod.rs | 162 ++++++++++++++---- examples/2d/mesh2d_manual.rs | 2 +- examples/animation/animated_transform.rs | 2 +- examples/shader/shader_instancing.rs | 2 +- examples/stress_tests/many_foxes.rs | 2 +- 7 files changed, 136 insertions(+), 50 deletions(-) diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs index 79d19e2898ef49..81d793993ac7c5 100644 --- a/crates/bevy_gltf/src/loader.rs +++ b/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( diff --git a/crates/bevy_render/src/spatial_bundle.rs b/crates/bevy_render/src/spatial_bundle.rs index 6eedd7cbf59924..b2d50da6097f6c 100644 --- a/crates/bevy_render/src/spatial_bundle.rs +++ b/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 }; } diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index cfa85157a62703..e4a57217c1bc1d 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/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 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,10 +468,7 @@ mod test { let root1 = app .world - .spawn(( - Visibility { is_visible: false }, - ComputedVisibility::default(), - )) + .spawn((Visibility::Hidden, ComputedVisibility::default())) .id(); let root1_child1 = app .world @@ -469,10 +476,7 @@ mod test { .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::() + .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::()); + assert_eq!(1, mem::size_of::>()); + } } diff --git a/examples/2d/mesh2d_manual.rs b/examples/2d/mesh2d_manual.rs index 977095e87f9f03..4147a4a4966f41 100644 --- a/examples/2d/mesh2d_manual.rs +++ b/examples/2d/mesh2d_manual.rs @@ -103,7 +103,7 @@ fn star( // The `Handle` 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 diff --git a/examples/animation/animated_transform.rs b/examples/animation/animated_transform.rs index faa090be04c642..ce8d9c7bc468db 100644 --- a/examples/animation/animated_transform.rs +++ b/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, )) diff --git a/examples/shader/shader_instancing.rs b/examples/shader/shader_instancing.rs index 5d8c8b816f4733..10fc65ea61c23e 100644 --- a/examples/shader/shader_instancing.rs +++ b/examples/shader/shader_instancing.rs @@ -35,7 +35,7 @@ fn main() { fn setup(mut commands: Commands, mut meshes: ResMut>) { 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))) diff --git a/examples/stress_tests/many_foxes.rs b/examples/stress_tests/many_foxes.rs index 4011ddec374f5f..9d5ae3d398b8c8 100644 --- a/examples/stress_tests/many_foxes.rs +++ b/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 }, ))