Skip to content

Commit

Permalink
enum Visibility component (bevyengine#6320)
Browse files Browse the repository at this point in the history
Consolidation of all the feedback about bevyengine#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>
  • Loading branch information
2 people authored and alradish committed Jan 22, 2023
1 parent 0e0d6ac commit 87630a0
Show file tree
Hide file tree
Showing 7 changed files with 136 additions and 50 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_gltf/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
14 changes: 7 additions & 7 deletions crates/bevy_render/src/spatial_bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
}

Expand Down
162 changes: 124 additions & 38 deletions crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand All @@ -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(),
};

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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>>());
}
}
2 changes: 1 addition & 1 deletion examples/2d/mesh2d_manual.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion examples/animation/animated_transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
))
Expand Down
2 changes: 1 addition & 1 deletion examples/shader/shader_instancing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down
2 changes: 1 addition & 1 deletion examples/stress_tests/many_foxes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
))
Expand Down

0 comments on commit 87630a0

Please sign in to comment.