From 5989a845f0826483b55506e58e0d2e08cc094854 Mon Sep 17 00:00:00 2001 From: Viktor Gustavsson Date: Sun, 13 Oct 2024 19:25:15 +0200 Subject: [PATCH] Filter UI traversal to only Node and GhostNode (#15746) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective With the warning removed in https://github.com/bevyengine/bevy/pull/15736, the rules for the UI tree changes. We no longer need to traverse non `Node`/`GhostNode` entities. ## Solution - Added a filter `Or<(With, With)>` to the child traversal query so we don't unnecessarily traverse nodes that are not part of the UI tree (like text nodes). - Also moved the warning for NoUI->UI entities so it is actually triggered (see comments) ## Testing - Ran unit tests (still passing) - Ran the ghost_nodes and ui examples, still works and looks fine 👍 - Tested the warning by spawning a Node under an empty entity. --- --------- Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com> --- crates/bevy_ui/src/ghost_hierarchy.rs | 36 ++++++++++++++++++------- crates/bevy_ui/src/layout/mod.rs | 20 +++++++++++--- crates/bevy_ui/src/layout/ui_surface.rs | 6 +---- 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/crates/bevy_ui/src/ghost_hierarchy.rs b/crates/bevy_ui/src/ghost_hierarchy.rs index 01ae3bd945..ad5a889480 100644 --- a/crates/bevy_ui/src/ghost_hierarchy.rs +++ b/crates/bevy_ui/src/ghost_hierarchy.rs @@ -44,7 +44,12 @@ impl<'w, 's> UiRootNodes<'w, 's> { /// System param that gives access to UI children utilities, skipping over [`GhostNode`]. #[derive(SystemParam)] pub struct UiChildren<'w, 's> { - ui_children_query: Query<'w, 's, (Option<&'static Children>, Option<&'static GhostNode>)>, + ui_children_query: Query< + 'w, + 's, + (Option<&'static Children>, Has), + Or<(With, With)>, + >, changed_children_query: Query<'w, 's, Entity, Changed>, children_query: Query<'w, 's, &'static Children>, ghost_nodes_query: Query<'w, 's, Entity, With>, @@ -101,11 +106,21 @@ impl<'w, 's> UiChildren<'w, 's> { .iter_ghost_nodes(entity) .any(|entity| self.changed_children_query.contains(entity)) } + + /// Returns `true` if the given entity is either a [`Node`] or a [`GhostNode`]. + pub fn is_ui_node(&'s self, entity: Entity) -> bool { + self.ui_children_query.contains(entity) + } } pub struct UiChildrenIter<'w, 's> { stack: SmallVec<[Entity; 8]>, - query: &'s Query<'w, 's, (Option<&'static Children>, Option<&'static GhostNode>)>, + query: &'s Query< + 'w, + 's, + (Option<&'static Children>, Has), + Or<(With, With)>, + >, } impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { @@ -113,12 +128,13 @@ impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { fn next(&mut self) -> Option { loop { let entity = self.stack.pop()?; - let (children, ghost_node) = self.query.get(entity).ok()?; - if ghost_node.is_none() { - return Some(entity); - } - if let Some(children) = children { - self.stack.extend(children.iter().rev().copied()); + if let Ok((children, has_ghost_node)) = self.query.get(entity) { + if !has_ghost_node { + return Some(entity); + } + if let Some(children) = children { + self.stack.extend(children.iter().rev().copied()); + } } } } @@ -186,10 +202,12 @@ mod tests { let n9 = world.spawn((A(9), GhostNode)).id(); let n10 = world.spawn((A(10), NodeBundle::default())).id(); + let no_ui = world.spawn_empty().id(); + world.entity_mut(n1).add_children(&[n2, n3, n4, n6]); world.entity_mut(n2).add_children(&[n5]); - world.entity_mut(n6).add_children(&[n7, n9]); + world.entity_mut(n6).add_children(&[n7, no_ui, n9]); world.entity_mut(n7).add_children(&[n8]); world.entity_mut(n9).add_children(&[n10]); diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs index 477affba93..d7a2e5fd61 100644 --- a/crates/bevy_ui/src/layout/mod.rs +++ b/crates/bevy_ui/src/layout/mod.rs @@ -11,7 +11,7 @@ use bevy_ecs::{ system::{Commands, Local, Query, Res, ResMut, SystemParam}, world::Ref, }; -use bevy_hierarchy::Children; +use bevy_hierarchy::{Children, Parent}; use bevy_math::{UVec2, Vec2}; use bevy_render::camera::{Camera, NormalizedRenderTarget}; use bevy_sprite::BorderRect; @@ -114,7 +114,7 @@ pub fn ui_layout_system( ), With, >, - node_query: Query>, + node_query: Query<(Entity, Option>), With>, ui_children: UiChildren, mut removed_components: UiLayoutSystemRemovedComponentParam, mut node_transform_query: Query<( @@ -249,7 +249,19 @@ pub fn ui_layout_system( ui_surface.try_remove_children(entity); } - node_query.iter().for_each(|entity| { + node_query.iter().for_each(|(entity, maybe_parent)| { + if let Some(parent) = maybe_parent { + // Note: This does not cover the case where a parent's Node component was removed. + // Users are responsible for fixing hierarchies if they do that (it is not recommended). + // Detecting it here would be a permanent perf burden on the hot path. + if parent.is_changed() && !ui_children.is_ui_node(parent.get()) { + warn!( + "Styled child ({entity}) in a non-UI entity hierarchy. You are using an entity \ +with UI components as a child of an entity without UI components, your UI layout may be broken." + ); + } + } + if ui_children.is_changed(entity) { ui_surface.update_children(entity, ui_children.iter_ui_children(entity)); } @@ -261,7 +273,7 @@ pub fn ui_layout_system( ui_surface.remove_entities(removed_components.removed_nodes.read()); // Re-sync changed children: avoid layout glitches caused by removed nodes that are still set as a child of another node - node_query.iter().for_each(|entity| { + node_query.iter().for_each(|(entity, _)| { if ui_children.is_changed(entity) { ui_surface.update_children(entity, ui_children.iter_ui_children(entity)); } diff --git a/crates/bevy_ui/src/layout/ui_surface.rs b/crates/bevy_ui/src/layout/ui_surface.rs index 71e740f603..4ffd2b4675 100644 --- a/crates/bevy_ui/src/layout/ui_surface.rs +++ b/crates/bevy_ui/src/layout/ui_surface.rs @@ -7,7 +7,7 @@ use bevy_ecs::{ prelude::Resource, }; use bevy_math::UVec2; -use bevy_utils::{default, tracing::warn}; +use bevy_utils::default; use crate::{ layout::convert, LayoutContext, LayoutError, Measure, MeasureArgs, NodeMeasure, Style, @@ -289,10 +289,6 @@ impl UiSurface { .layout(*taffy_node) .map_err(LayoutError::TaffyError) } else { - warn!( - "Styled child ({entity}) in a non-UI entity hierarchy. You are using an entity \ -with UI components as a child of an entity without UI components, your UI layout may be broken." - ); Err(LayoutError::InvalidHierarchy) } }