From 9c2478289fde288cd416ece5cd09ba45ae1b3eb4 Mon Sep 17 00:00:00 2001 From: ickk Date: Mon, 17 Oct 2022 02:49:27 +1100 Subject: [PATCH 1/3] Turn the `Visibility` struct into an enum add `Eq`, `PartialEq` derives to `Visibility` impl `Copy` for `Visibility` add a test ensuring that `Visibility` is one byte and that null pointer optimisation is doing its thing Document the variants clarify `Visibility::Inherited` behaviour for root-level entities add unconditional `Visibility::Visible` variant add tests remove controversial `toggle` method since it may be confusing with 3 variants impl `PartialEq` for `&Visibility`/`Visibility` pairs --- crates/bevy_gltf/src/loader.rs | 2 +- crates/bevy_render/src/spatial_bundle.rs | 14 +- crates/bevy_render/src/view/visibility/mod.rs | 167 ++++++++++++++---- 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, 142 insertions(+), 49 deletions(-) diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs index ce80600020b1f..cfa4592399611 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 6eedd7cbf5992..b2d50da6097f6 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 d6979ac5e0882..1b6f954282abe 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -23,34 +23,47 @@ 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, 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, Debug, PartialEq, Eq)] #[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. + 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 }; +// Allows `Visibility == &Visibility` +impl std::cmp::PartialEq<&Visibility> for Visibility { + #[inline] + fn eq(&self, other: &&Visibility) -> bool { + *self == **other + } +} - /// Toggle the visibility. - pub fn toggle(&mut self) { - self.is_visible = !self.is_visible; +impl Default for Visibility { + fn default() -> Self { + Self::Inherited } } @@ -64,13 +77,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 { is_visible_in_hierarchy: false, is_visible_in_view: false, }; @@ -271,7 +284,9 @@ fn visibility_propagate_system( children_query: Query<&Children, (With, With, With)>, ) { for (children, visibility, mut computed_visibility, entity) in root_query.iter_mut() { - computed_visibility.is_visible_in_hierarchy = visibility.is_visible; + // Setting `Visibility::Inherited` on the root entity is the same as setting `Visibility::Visible`. + computed_visibility.is_visible_in_hierarchy = + 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.is_visible_in_view = false; if let Some(children) = children { @@ -304,7 +319,9 @@ fn propagate_recursive( child_parent.get(), expected_parent, "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" ); - computed_visibility.is_visible_in_hierarchy = visibility.is_visible && parent_visible; + computed_visibility.is_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.is_visible_in_view = false; computed_visibility.is_visible_in_hierarchy @@ -433,10 +450,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 @@ -444,10 +458,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 @@ -478,10 +489,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 @@ -553,4 +561,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 in 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 5eb72f3a19d88..602ce568efd32 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 19f284aa02519..cf82eb4e72c7e 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 71e68566b40e5..4012473a5b879 100644 --- a/examples/shader/shader_instancing.rs +++ b/examples/shader/shader_instancing.rs @@ -32,7 +32,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 d67d05e5b50e0..b9ec2314e7752 100644 --- a/examples/stress_tests/many_foxes.rs +++ b/examples/stress_tests/many_foxes.rs @@ -111,7 +111,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 }, )) From 84abd88f48fcf66ad99f9f686c4d333dd688c460 Mon Sep 17 00:00:00 2001 From: ickk Date: Tue, 25 Oct 2022 17:38:15 +1100 Subject: [PATCH 2/3] derive default enum variant --- crates/bevy_render/src/view/visibility/mod.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 1b6f954282abe..d71a7289d6597 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -29,12 +29,13 @@ use crate::{ /// /// 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, Debug, PartialEq, Eq)] +#[derive(Component, Clone, Copy, Reflect, Debug, PartialEq, Eq, Default)] #[reflect(Component, Default)] 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, @@ -61,12 +62,6 @@ impl std::cmp::PartialEq<&Visibility> for Visibility { } } -impl Default for Visibility { - fn default() -> Self { - Self::Inherited - } -} - /// Algorithmically-computed indication of whether an entity is visible and should be extracted for rendering #[derive(Component, Clone, Reflect, Debug, Eq, PartialEq)] #[reflect(Component, Default)] @@ -636,7 +631,7 @@ mod test { is_visible(root1_child2_grandchild1), "an unconditionally visible child of a hidden parent is visible" ); - assert!(is_visible(root2), "an inheriting root in visible"); + assert!(is_visible(root2), "an inheriting root is visible"); assert!(!is_visible(root3), "a hidden root is hidden"); } From eca598a3214d5edc8da3d1733a56e2f1473793c0 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Tue, 20 Dec 2022 16:13:29 -0800 Subject: [PATCH 3/3] fix test --- crates/bevy_render/src/view/visibility/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 04946e967b7db..e4a57217c1bc1 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -632,7 +632,7 @@ mod test { .entity(e) .get::() .unwrap() - .is_visible_in_hierarchy + .is_visible_in_hierarchy() }; assert!( is_visible(root1),