Skip to content

Commit

Permalink
Shrink ComputedVisibility (bevyengine#6305)
Browse files Browse the repository at this point in the history
# Objective
`ComputedVisibility` could afford to be smaller/faster. Optimizing the size and performance of operations on the component will positively benefit almost all extraction systems.

This was listed as one of the potential pieces of future work for bevyengine#5310.

## Solution
Merge both internal booleans into a single `u8` bitflag field. Rely on bitmasks to evaluate local, hierarchical, and general visibility.

Pros:

 - `ComputedVisibility::is_visible` should be a single bitmask test instead of two.
 - `ComputedVisibility` is now only 1 byte. Should be able to fit 100% more per cache line when using dense iteration.

Cons:

 - Harder to read.
 - Setting individual values inside `ComputedVisiblity` require bitmask mutations. 

This should be a non-breaking change. No public API was changed. The only publicly visible effect is that `ComputedVisibility` is now 1 byte instead of 2.
  • Loading branch information
james7132 authored and taiyoungjang committed Dec 15, 2022
1 parent d3cc8dc commit 44a5207
Showing 1 changed file with 33 additions and 17 deletions.
50 changes: 33 additions & 17 deletions crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,19 @@ impl Visibility {
}
}

bitflags::bitflags! {
#[derive(Reflect)]
struct ComputedVisibilityFlags: u8 {
const VISIBLE_IN_VIEW = 1 << 0;
const VISIBLE_IN_HIERARCHY = 1 << 1;
}
}

/// 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)]
pub struct ComputedVisibility {
is_visible_in_hierarchy: bool,
is_visible_in_view: bool,
flags: ComputedVisibilityFlags,
}

impl Default for ComputedVisibility {
Expand All @@ -71,8 +78,7 @@ impl Default for ComputedVisibility {
impl ComputedVisibility {
/// A [`ComputedVisibility`], set as invisible.
pub const INVISIBLE: Self = ComputedVisibility {
is_visible_in_hierarchy: false,
is_visible_in_view: false,
flags: ComputedVisibilityFlags::empty(),
};

/// Whether this entity is visible to something this frame. This is true if and only if [`Self::is_visible_in_hierarchy`] and [`Self::is_visible_in_view`]
Expand All @@ -81,7 +87,7 @@ impl ComputedVisibility {
/// [`CoreStage::Update`] stage will yield the value from the previous frame.
#[inline]
pub fn is_visible(&self) -> bool {
self.is_visible_in_hierarchy && self.is_visible_in_view
self.flags.bits == ComputedVisibilityFlags::all().bits
}

/// Whether this entity is visible in the entity hierarchy, which is determined by the [`Visibility`] component.
Expand All @@ -90,7 +96,8 @@ impl ComputedVisibility {
/// [`VisibilitySystems::VisibilityPropagate`] system label.
#[inline]
pub fn is_visible_in_hierarchy(&self) -> bool {
self.is_visible_in_hierarchy
self.flags
.contains(ComputedVisibilityFlags::VISIBLE_IN_HIERARCHY)
}

/// Whether this entity is visible in _any_ view (Cameras, Lights, etc). Each entity type (and view type) should choose how to set this
Expand All @@ -102,7 +109,8 @@ impl ComputedVisibility {
/// Other entities might just set this to `true` every frame.
#[inline]
pub fn is_visible_in_view(&self) -> bool {
self.is_visible_in_view
self.flags
.contains(ComputedVisibilityFlags::VISIBLE_IN_VIEW)
}

/// Sets `is_visible_in_view` to `true`. This is not reversible for a given frame, as it encodes whether or not this is visible in
Expand All @@ -111,7 +119,16 @@ impl ComputedVisibility {
/// label. Don't call this unless you are defining a custom visibility system. For normal user-defined entity visibility, see [`Visibility`].
#[inline]
pub fn set_visible_in_view(&mut self) {
self.is_visible_in_view = true;
self.flags.insert(ComputedVisibilityFlags::VISIBLE_IN_VIEW);
}

#[inline]
fn reset(&mut self, visible_in_hierarchy: bool) {
self.flags = if visible_in_hierarchy {
ComputedVisibilityFlags::VISIBLE_IN_HIERARCHY
} else {
ComputedVisibilityFlags::empty()
};
}
}

Expand Down Expand Up @@ -280,13 +297,12 @@ fn visibility_propagate_system(
children_query: Query<&Children, (With<Parent>, With<Visibility>, With<ComputedVisibility>)>,
) {
for (children, visibility, mut computed_visibility, entity) in root_query.iter_mut() {
computed_visibility.is_visible_in_hierarchy = visibility.is_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.reset(visibility.is_visible);
if let Some(children) = children {
for child in children.iter() {
let _ = propagate_recursive(
computed_visibility.is_visible_in_hierarchy,
computed_visibility.is_visible_in_hierarchy(),
&mut visibility_query,
&children_query,
*child,
Expand All @@ -313,10 +329,10 @@ 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;
let visible_in_hierarchy = visibility.is_visible && parent_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
computed_visibility.reset(visible_in_hierarchy);
visible_in_hierarchy
};

for child in children_query.get(entity).map_err(drop)?.iter() {
Expand Down Expand Up @@ -390,7 +406,7 @@ pub fn check_visibility(
}
}

computed_visibility.is_visible_in_view = true;
computed_visibility.set_visible_in_view();
let cell = thread_queues.get_or_default();
let mut queue = cell.take();
queue.push(entity);
Expand All @@ -412,7 +428,7 @@ pub fn check_visibility(
return;
}

computed_visibility.is_visible_in_view = true;
computed_visibility.set_visible_in_view();
let cell = thread_queues.get_or_default();
let mut queue = cell.take();
queue.push(entity);
Expand Down Expand Up @@ -518,7 +534,7 @@ mod test {
.entity(e)
.get::<ComputedVisibility>()
.unwrap()
.is_visible_in_hierarchy
.is_visible_in_hierarchy()
};
assert!(
!is_visible(root1),
Expand Down

0 comments on commit 44a5207

Please sign in to comment.