Restore pre 0.13.1 Root Node Layout behavior

[StrikeForceZero]

Objective

Fix the regression for Root Node's Layout behavior introduced in #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 In bevy 0.13.1 , only a single NodeBundle will work. spawning multiple UI node bundles will destroy the others ones  #12624)

Solution

This implements @nicoburns suggestion , 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 #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.

[StrikeForceZero]

Following @nicoburns suggestion, I was able to reduce the complexity/redundant maps in the unapplied Diff below.

However, I'm not exactly fond of it due to the disjointed remove
//self.taffy.remove(remove_root_node_pair.user_root_node).unwrap(); in remove_camera_entities.

I'm slightly partial to leaving it as is, applying this diff to a new PR, and having that go through some iterations to test/clean it up. Thoughts?

Edit: figured it out, see comment below

Click to show Diff

Index: crates/bevy_ui/src/layout/mod.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs
--- a/crates/bevy_ui/src/layout/mod.rs	(revision Staged)
+++ b/crates/bevy_ui/src/layout/mod.rs	(date 1711341109941)
@@ -52,8 +52,7 @@
 #[derive(Resource)]
 pub struct UiSurface {
     entity_to_taffy: EntityHashMap<taffy::node::Node>,
-    camera_entity_to_taffy: EntityHashMap<EntityHashMap<taffy::node::Node>>,
-    camera_roots: EntityHashMap<Vec<RootNodePair>>,
+    camera_to_root_to_root_node_pair: EntityHashMap<EntityHashMap<RootNodePair>>,
     taffy: Taffy,
 }
 
@@ -68,7 +67,10 @@
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         f.debug_struct("UiSurface")
             .field("entity_to_taffy", &self.entity_to_taffy)
-            .field("camera_roots", &self.camera_roots)
+            .field(
+                "camera_to_root_to_root_node_pair",
+                &self.camera_to_root_to_root_node_pair,
+            )
             .finish()
     }
 }
@@ -79,8 +81,7 @@
         taffy.disable_rounding();
         Self {
             entity_to_taffy: Default::default(),
-            camera_entity_to_taffy: Default::default(),
-            camera_roots: Default::default(),
+            camera_to_root_to_root_node_pair: Default::default(),
             taffy,
         }
     }
@@ -168,40 +169,34 @@
             ..default()
         };
 
-        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();
+        let entity_to_root_node_pair = self
+            .camera_to_root_to_root_node_pair
+            .entry(camera_id)
+            .or_default();
         for entity in children {
-            let node = *self.entity_to_taffy.get(&entity).unwrap();
-            let root_node = existing_roots
-                .iter()
-                .find(|n| n.user_root_node == node)
-                .cloned()
-                .unwrap_or_else(|| {
-                    if let Some(previous_parent) = self.taffy.parent(node) {
-                        // remove the root node from the previous implicit node's children
-                        self.taffy.remove_child(previous_parent, 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: viewport_node,
-                        user_root_node: node,
-                    }
-                });
-            new_roots.push(root_node);
+            let root_node_pair = entity_to_root_node_pair.entry(entity).or_insert_with(|| RootNodePair {
+                implicit_viewport_node: self.taffy.new_leaf(viewport_style.clone()).unwrap(),
+                user_root_node: *self.entity_to_taffy.get(&entity).expect("root node was not created yet or has been deleted from `entity_to_taffy` prematurely"),
+            });
+            if let Some(previous_parent) = self.taffy.parent(root_node_pair.user_root_node) {
+                // remove the root node from the previous implicit node's children
+                self.taffy
+                    .remove_child(previous_parent, root_node_pair.user_root_node)
+                    .unwrap();
+            }
+            self.taffy
+                .add_child(
+                    root_node_pair.implicit_viewport_node,
+                    root_node_pair.user_root_node,
+                )
+                .unwrap();
         }
-
-        self.camera_roots.insert(camera_id, new_roots);
     }
 
     /// Compute the layout for each window entity's corresponding root node in the layout.
     pub fn compute_camera_layout(&mut self, camera: Entity, render_target_resolution: UVec2) {
-        let Some(camera_root_nodes) = self.camera_roots.get(&camera) else {
+        let Some(entity_to_root_node_pair) = self.camera_to_root_to_root_node_pair.get(&camera)
+        else {
             return;
         };
 
@@ -209,9 +204,9 @@
             width: taffy::style::AvailableSpace::Definite(render_target_resolution.x as f32),
             height: taffy::style::AvailableSpace::Definite(render_target_resolution.y as f32),
         };
-        for root_nodes in camera_root_nodes {
+        for (_, root_node_pair) in entity_to_root_node_pair.iter() {
             self.taffy
-                .compute_layout(root_nodes.implicit_viewport_node, available_space)
+                .compute_layout(root_node_pair.implicit_viewport_node, available_space)
                 .unwrap();
         }
     }
@@ -219,9 +214,14 @@
     /// 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<Item = Entity>) {
         for entity in entities {
-            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();
+            if let Some(entity_to_root_node_pair) =
+                self.camera_to_root_to_root_node_pair.remove(&entity)
+            {
+                for (_, remove_root_node_pair) in entity_to_root_node_pair.iter() {
+                    //self.taffy.remove(remove_root_node_pair.user_root_node).unwrap();
+                    self.taffy
+                        .remove(remove_root_node_pair.implicit_viewport_node)
+                        .unwrap();
                 }
             }
         }
@@ -699,7 +699,7 @@
 
         // no UI entities in world, none in UiSurface
         let ui_surface = world.resource::<UiSurface>();
-        assert!(ui_surface.camera_entity_to_taffy.is_empty());
+        assert!(ui_surface.camera_to_root_to_root_node_pair.is_empty());
 
         // respawn camera
         let camera_entity = world.spawn(Camera2dBundle::default()).id();
@@ -713,9 +713,9 @@
 
         let ui_surface = world.resource::<UiSurface>();
         assert!(ui_surface
-            .camera_entity_to_taffy
+            .camera_to_root_to_root_node_pair
             .contains_key(&camera_entity));
-        assert_eq!(ui_surface.camera_entity_to_taffy.len(), 1);
+        assert_eq!(ui_surface.camera_to_root_to_root_node_pair.len(), 1);
 
         world.despawn(ui_entity);
         world.despawn(camera_entity);
@@ -725,9 +725,9 @@
 
         let ui_surface = world.resource::<UiSurface>();
         assert!(!ui_surface
-            .camera_entity_to_taffy
+            .camera_to_root_to_root_node_pair
             .contains_key(&camera_entity));
-        assert!(ui_surface.camera_entity_to_taffy.is_empty());
+        assert!(ui_surface.camera_to_root_to_root_node_pair.is_empty());
     }
 
     #[test]
Index: crates/bevy_ui/src/layout/debug.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/bevy_ui/src/layout/debug.rs b/crates/bevy_ui/src/layout/debug.rs
--- a/crates/bevy_ui/src/layout/debug.rs	(revision Staged)
+++ b/crates/bevy_ui/src/layout/debug.rs	(date 1711341205596)
@@ -12,20 +12,20 @@
         .iter()
         .map(|(entity, node)| (*node, *entity))
         .collect();
-    for (&entity, roots) in &ui_surface.camera_roots {
+    for (&camera_entity, entity_to_root_pair) in &ui_surface.camera_to_root_to_root_node_pair {
         let mut out = String::new();
-        for root in roots {
+        for (root_node_entity, entity_to_root_node_pair) in entity_to_root_pair.iter() {
             print_node(
                 ui_surface,
                 &taffy_to_entity,
-                entity,
-                root.implicit_viewport_node,
+                *root_node_entity,
+                entity_to_root_node_pair.implicit_viewport_node,
                 false,
                 String::new(),
                 &mut out,
             );
         }
-        bevy_utils::tracing::info!("Layout tree for camera entity: {entity:?}\n{out}");
+        bevy_utils::tracing::info!("Layout tree for camera entity: {camera_entity:?}\n{out}");
     }
 }
 
@@ -33,6 +33,7 @@
 fn print_node(
     ui_surface: &UiSurface,
     taffy_to_entity: &HashMap<Node, Entity>,
+    // TODO: unused
     entity: Entity,
     node: Node,
     has_sibling: bool,

[nicoburns]

I haven't tested this, but the code change looks about right to me.

[nicoburns]

Following @nicoburns suggestion, I was able to reduce the complexity/redundant maps in the unapplied Diff below.

Not sure what's going on with that diff, but it's not rendering properly at all.

[alice-i-cecile]

@ethereumdegen @extrawurst @FylmTM: if one of you can test that this fix resolves the regression you saw for your UI, that would be very helpful.

[StrikeForceZero]

Following @nicoburns suggestion, I was able to reduce the complexity/redundant maps in the unapplied Diff below.

Not sure what's going on with that diff, but it's not rendering properly at all.

Whoops. Fixed.

[StrikeForceZero]

@nicoburns I was able to dedupe the mappings here

I was thinking of keeping the RootNodePair and continuing to track the viewport and root Node taffy node with it. Then try removing the entity_to_taffy map to make it more atomic in nature, but I figured the lookup times wouldn't be as good.

But IMO, either way, that should be a follow-up PR with its own discussion on semantics/optimization suggestions, as the scope of this one is to fix the regression.