From 44a5207de84c91802eba948e22b26f8cda21b4da Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 14 Nov 2022 23:34:52 +0000 Subject: [PATCH] Shrink ComputedVisibility (#6305) # 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 #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. --- crates/bevy_render/src/view/visibility/mod.rs | 50 ++++++++++++------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 2d5acddcac5cf..c08df608985d1 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -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 { @@ -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`] @@ -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. @@ -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 @@ -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 @@ -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() + }; } } @@ -280,13 +297,12 @@ 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; // 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, @@ -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() { @@ -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); @@ -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); @@ -518,7 +534,7 @@ mod test { .entity(e) .get::() .unwrap() - .is_visible_in_hierarchy + .is_visible_in_hierarchy() }; assert!( !is_visible(root1),