From b2b302bdfd8395a071fc18b3cb942d5f0c83eb1f Mon Sep 17 00:00:00 2001 From: Brett Striker Date: Mon, 25 Mar 2024 15:11:50 -0400 Subject: [PATCH] Restore pre 0.13.1 Root Node Layout behavior (#12698) # Objective Fix the regression for Root Node's Layout behavior introduced in https://github.com/bevyengine/bevy/pull/12268 - Add regression test for Root Node Layout's behaving as they did before 0.13.1 - Restore pre 0.13.1 Root Node Layout behavior (fixes https://github.com/bevyengine/bevy/issues/12624) ## Solution This implements [@nicoburns suggestion ](https://discord.com/channels/691052431525675048/743663673393938453/1221593626476548146), where instead of adding the camera to the taffy node tree, we revert back to adding a new "parent" node for each root node while maintaining their relationship with the camera. > If you can do the ecs change detection to move the node to the correct Taffy instance for the camera then you should also be able to move it to a `Vec` of root nodes for that camera. --- ## Changelog Fixed https://github.com/bevyengine/bevy/issues/12624 - Restores pre 0.13.1 Root Node Layout behavior ## Migration Guide If you were affected by the 0.13.1 regression and added `position_type: Absolute` to all your root nodes you might be able to reclaim some LOC by removing them now that the 0.13 behavior is restored. --- crates/bevy_ui/src/layout/mod.rs | 112 +++++++++++++++++++++++++++---- 1 file changed, 98 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs index 6b7940e1484ec..7ff8a59229fca 100644 --- a/crates/bevy_ui/src/layout/mod.rs +++ b/crates/bevy_ui/src/layout/mod.rs @@ -52,7 +52,7 @@ struct RootNodePair { #[derive(Resource)] pub struct UiSurface { entity_to_taffy: EntityHashMap, - camera_entity_to_taffy: EntityHashMap, + camera_entity_to_taffy: EntityHashMap>, camera_roots: EntityHashMap>, taffy: Taffy, } @@ -168,10 +168,7 @@ without UI components as a child of an entity with UI components, results may be ..default() }; - let camera_node = *self - .camera_entity_to_taffy - .entry(camera_id) - .or_insert_with(|| self.taffy.new_leaf(viewport_style.clone()).unwrap()); + let camera_root_node_map = self.camera_entity_to_taffy.entry(camera_id).or_default(); let existing_roots = self.camera_roots.entry(camera_id).or_default(); let mut new_roots = Vec::new(); for entity in children { @@ -186,10 +183,13 @@ without UI components as a child of an entity with UI components, results may be self.taffy.remove_child(previous_parent, node).unwrap(); } - self.taffy.add_child(camera_node, node).unwrap(); + let viewport_node = *camera_root_node_map + .entry(entity) + .or_insert_with(|| self.taffy.new_leaf(viewport_style.clone()).unwrap()); + self.taffy.add_child(viewport_node, node).unwrap(); RootNodePair { - implicit_viewport_node: camera_node, + implicit_viewport_node: viewport_node, user_root_node: node, } }); @@ -219,8 +219,10 @@ without UI components as a child of an entity with UI components, results may be /// Removes each camera entity from the internal map and then removes their associated node from taffy pub fn remove_camera_entities(&mut self, entities: impl IntoIterator) { for entity in entities { - if let Some(node) = self.camera_entity_to_taffy.remove(&entity) { - self.taffy.remove(node).unwrap(); + if let Some(camera_root_node_map) = self.camera_entity_to_taffy.remove(&entity) { + for (_, node) in camera_root_node_map.iter() { + self.taffy.remove(*node).unwrap(); + } } } } @@ -543,20 +545,19 @@ mod tests { use bevy_ecs::entity::Entity; use bevy_ecs::event::Events; use bevy_ecs::prelude::{Commands, Component, In, Query, With}; + use bevy_ecs::query::Without; use bevy_ecs::schedule::apply_deferred; use bevy_ecs::schedule::IntoSystemConfigs; use bevy_ecs::schedule::Schedule; use bevy_ecs::system::RunSystemOnce; use bevy_ecs::world::World; - use bevy_hierarchy::despawn_with_children_recursive; - use bevy_hierarchy::BuildWorldChildren; - use bevy_hierarchy::Children; - use bevy_math::Vec2; - use bevy_math::{vec2, UVec2}; + use bevy_hierarchy::{despawn_with_children_recursive, BuildWorldChildren, Children, Parent}; + use bevy_math::{vec2, Rect, UVec2, Vec2}; use bevy_render::camera::ManualTextureViews; use bevy_render::camera::OrthographicProjection; use bevy_render::prelude::Camera; use bevy_render::texture::Image; + use bevy_transform::prelude::{GlobalTransform, Transform}; use bevy_utils::prelude::default; use bevy_utils::HashMap; use bevy_window::PrimaryWindow; @@ -857,6 +858,89 @@ mod tests { assert!(ui_surface.entity_to_taffy.is_empty()); } + /// regression test for >=0.13.1 root node layouts + /// ensure root nodes act like they are absolutely positioned + /// without explicitly declaring it. + #[test] + fn ui_root_node_should_act_like_position_absolute() { + let (mut world, mut ui_schedule) = setup_ui_test_world(); + + let mut size = 150.; + + world.spawn(NodeBundle { + style: Style { + // test should pass without explicitly requiring position_type to be set to Absolute + // position_type: PositionType::Absolute, + width: Val::Px(size), + height: Val::Px(size), + ..default() + }, + ..default() + }); + + size -= 50.; + + world.spawn(NodeBundle { + style: Style { + // position_type: PositionType::Absolute, + width: Val::Px(size), + height: Val::Px(size), + ..default() + }, + ..default() + }); + + size -= 50.; + + world.spawn(NodeBundle { + style: Style { + // position_type: PositionType::Absolute, + width: Val::Px(size), + height: Val::Px(size), + ..default() + }, + ..default() + }); + + ui_schedule.run(&mut world); + + let overlap_check = world + .query_filtered::<(Entity, &Node, &mut GlobalTransform, &Transform), Without>() + .iter_mut(&mut world) + .fold( + Option::<(Rect, bool)>::None, + |option_rect, (entity, node, mut global_transform, transform)| { + // fix global transform - for some reason the global transform isn't populated yet. + // might be related to how these specific tests are working directly with World instead of App + *global_transform = GlobalTransform::from(transform.compute_affine()); + let global_transform = &*global_transform; + let current_rect = node.logical_rect(global_transform); + assert!( + current_rect.height().abs() + current_rect.width().abs() > 0., + "root ui node {entity:?} doesn't have a logical size" + ); + assert_ne!( + global_transform.affine(), + GlobalTransform::default().affine(), + "root ui node {entity:?} global transform is not populated" + ); + let Some((rect, is_overlapping)) = option_rect else { + return Some((current_rect, false)); + }; + if rect.contains(current_rect.center()) { + Some((current_rect, true)) + } else { + Some((current_rect, is_overlapping)) + } + }, + ); + + let Some((_rect, is_overlapping)) = overlap_check else { + unreachable!("test not setup properly"); + }; + assert!(is_overlapping, "root ui nodes are expected to behave like they have absolute position and be independent from each other"); + } + #[test] fn ui_node_should_properly_update_when_changing_target_camera() { #[derive(Component)]