Filter UI traversal to only Node and GhostNode (#15746)
# 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<Node>, With<GhostNode>)>` 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>
This commit is contained in:
		
							parent
							
								
									0e30b68b20
								
							
						
					
					
						commit
						5989a845f0
					
				| @ -44,7 +44,12 @@ impl<'w, 's> UiRootNodes<'w, 's> { | |||||||
| /// System param that gives access to UI children utilities, skipping over [`GhostNode`].
 | /// System param that gives access to UI children utilities, skipping over [`GhostNode`].
 | ||||||
| #[derive(SystemParam)] | #[derive(SystemParam)] | ||||||
| pub struct UiChildren<'w, 's> { | 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<GhostNode>), | ||||||
|  |         Or<(With<Node>, With<GhostNode>)>, | ||||||
|  |     >, | ||||||
|     changed_children_query: Query<'w, 's, Entity, Changed<Children>>, |     changed_children_query: Query<'w, 's, Entity, Changed<Children>>, | ||||||
|     children_query: Query<'w, 's, &'static Children>, |     children_query: Query<'w, 's, &'static Children>, | ||||||
|     ghost_nodes_query: Query<'w, 's, Entity, With<GhostNode>>, |     ghost_nodes_query: Query<'w, 's, Entity, With<GhostNode>>, | ||||||
| @ -101,11 +106,21 @@ impl<'w, 's> UiChildren<'w, 's> { | |||||||
|                 .iter_ghost_nodes(entity) |                 .iter_ghost_nodes(entity) | ||||||
|                 .any(|entity| self.changed_children_query.contains(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> { | pub struct UiChildrenIter<'w, 's> { | ||||||
|     stack: SmallVec<[Entity; 8]>, |     stack: SmallVec<[Entity; 8]>, | ||||||
|     query: &'s Query<'w, 's, (Option<&'static Children>, Option<&'static GhostNode>)>, |     query: &'s Query< | ||||||
|  |         'w, | ||||||
|  |         's, | ||||||
|  |         (Option<&'static Children>, Has<GhostNode>), | ||||||
|  |         Or<(With<Node>, With<GhostNode>)>, | ||||||
|  |     >, | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { | impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { | ||||||
| @ -113,8 +128,8 @@ impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { | |||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         loop { |         loop { | ||||||
|             let entity = self.stack.pop()?; |             let entity = self.stack.pop()?; | ||||||
|             let (children, ghost_node) = self.query.get(entity).ok()?; |             if let Ok((children, has_ghost_node)) = self.query.get(entity) { | ||||||
|             if ghost_node.is_none() { |                 if !has_ghost_node { | ||||||
|                     return Some(entity); |                     return Some(entity); | ||||||
|                 } |                 } | ||||||
|                 if let Some(children) = children { |                 if let Some(children) = children { | ||||||
| @ -123,6 +138,7 @@ impl<'w, 's> Iterator for UiChildrenIter<'w, 's> { | |||||||
|             } |             } | ||||||
|         } |         } | ||||||
|     } |     } | ||||||
|  | } | ||||||
| 
 | 
 | ||||||
| #[cfg(test)] | #[cfg(test)] | ||||||
| mod tests { | mod tests { | ||||||
| @ -186,10 +202,12 @@ mod tests { | |||||||
|         let n9 = world.spawn((A(9), GhostNode)).id(); |         let n9 = world.spawn((A(9), GhostNode)).id(); | ||||||
|         let n10 = world.spawn((A(10), NodeBundle::default())).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(n1).add_children(&[n2, n3, n4, n6]); | ||||||
|         world.entity_mut(n2).add_children(&[n5]); |         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(n7).add_children(&[n8]); | ||||||
|         world.entity_mut(n9).add_children(&[n10]); |         world.entity_mut(n9).add_children(&[n10]); | ||||||
| 
 | 
 | ||||||
|  | |||||||
| @ -11,7 +11,7 @@ use bevy_ecs::{ | |||||||
|     system::{Commands, Local, Query, Res, ResMut, SystemParam}, |     system::{Commands, Local, Query, Res, ResMut, SystemParam}, | ||||||
|     world::Ref, |     world::Ref, | ||||||
| }; | }; | ||||||
| use bevy_hierarchy::Children; | use bevy_hierarchy::{Children, Parent}; | ||||||
| use bevy_math::{UVec2, Vec2}; | use bevy_math::{UVec2, Vec2}; | ||||||
| use bevy_render::camera::{Camera, NormalizedRenderTarget}; | use bevy_render::camera::{Camera, NormalizedRenderTarget}; | ||||||
| use bevy_sprite::BorderRect; | use bevy_sprite::BorderRect; | ||||||
| @ -114,7 +114,7 @@ pub fn ui_layout_system( | |||||||
|         ), |         ), | ||||||
|         With<Node>, |         With<Node>, | ||||||
|     >, |     >, | ||||||
|     node_query: Query<Entity, With<Node>>, |     node_query: Query<(Entity, Option<Ref<Parent>>), With<Node>>, | ||||||
|     ui_children: UiChildren, |     ui_children: UiChildren, | ||||||
|     mut removed_components: UiLayoutSystemRemovedComponentParam, |     mut removed_components: UiLayoutSystemRemovedComponentParam, | ||||||
|     mut node_transform_query: Query<( |     mut node_transform_query: Query<( | ||||||
| @ -249,7 +249,19 @@ pub fn ui_layout_system( | |||||||
|         ui_surface.try_remove_children(entity); |         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) { |         if ui_children.is_changed(entity) { | ||||||
|             ui_surface.update_children(entity, ui_children.iter_ui_children(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()); |     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
 |     // 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) { |         if ui_children.is_changed(entity) { | ||||||
|             ui_surface.update_children(entity, ui_children.iter_ui_children(entity)); |             ui_surface.update_children(entity, ui_children.iter_ui_children(entity)); | ||||||
|         } |         } | ||||||
|  | |||||||
| @ -7,7 +7,7 @@ use bevy_ecs::{ | |||||||
|     prelude::Resource, |     prelude::Resource, | ||||||
| }; | }; | ||||||
| use bevy_math::UVec2; | use bevy_math::UVec2; | ||||||
| use bevy_utils::{default, tracing::warn}; | use bevy_utils::default; | ||||||
| 
 | 
 | ||||||
| use crate::{ | use crate::{ | ||||||
|     layout::convert, LayoutContext, LayoutError, Measure, MeasureArgs, NodeMeasure, Style, |     layout::convert, LayoutContext, LayoutError, Measure, MeasureArgs, NodeMeasure, Style, | ||||||
| @ -289,10 +289,6 @@ impl UiSurface { | |||||||
|                 .layout(*taffy_node) |                 .layout(*taffy_node) | ||||||
|                 .map_err(LayoutError::TaffyError) |                 .map_err(LayoutError::TaffyError) | ||||||
|         } else { |         } 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) |             Err(LayoutError::InvalidHierarchy) | ||||||
|         } |         } | ||||||
|     } |     } | ||||||
|  | |||||||
		Loading…
	
		Reference in New Issue
	
	Block a user
	 Viktor Gustavsson
						Viktor Gustavsson