From 6b75589e2c91429eb6c35574840d47d39b2379b9 Mon Sep 17 00:00:00 2001 From: Gabriel Bourgeois Date: Thu, 6 Oct 2022 21:39:34 +0000 Subject: [PATCH] Fix inconsistent children removal behavior (#6017) # Objective Fixes #6010 ## Solution As discussed in #6010, this makes it so the `Children` component is removed from the entity whenever all of its children are removed. The behavior is now consistent between all of the commands that may remove children from a parent, and this is tested via two new test functions (one for world functions and one for commands). Documentation was also added to `insert_children`, `push_children`, `add_child` and `remove_children` commands to make this behavior clearer for users. ## Changelog - Fixed `Children` component not getting removed from entity when all its children are moved to a new parent. ## Migration Guide - Queries with `Changed` will no longer match entities that had all of their children removed using `remove_children`. - `RemovedComponents` will now contain entities that had all of their children remove using `remove_children`. --- crates/bevy_hierarchy/src/child_builder.rs | 111 ++++++++++++++++++++- crates/bevy_ui/src/flex/mod.rs | 13 ++- 2 files changed, 122 insertions(+), 2 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index bc66b1ba6c..e759121daa 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -77,10 +77,15 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { } push_events(world, events); - if let Some(mut parent_children) = world.get_mut::(parent) { + let mut parent = world.entity_mut(parent); + if let Some(mut parent_children) = parent.get_mut::() { parent_children .0 .retain(|parent_child| !children.contains(parent_child)); + + if parent_children.is_empty() { + parent.remove::(); + } } } @@ -258,12 +263,26 @@ pub trait BuildChildren { /// ``` fn add_children(&mut self, f: impl FnOnce(&mut ChildBuilder) -> T) -> T; /// Pushes children to the back of the builder's children + /// + /// If the children were previously children of another parent, that parent's [`Children`] component + /// will have those children removed from its list. Removing all children from a parent causes its + /// [`Children`] component to be removed from the entity. fn push_children(&mut self, children: &[Entity]) -> &mut Self; /// Inserts children at the given index + /// + /// If the children were previously children of another parent, that parent's [`Children`] component + /// will have those children removed from its list. Removing all children from a parent causes its + /// [`Children`] component to be removed from the entity. fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self; /// Removes the given children + /// + /// Removing all children from a parent causes its [`Children`] component to be removed from the entity. fn remove_children(&mut self, children: &[Entity]) -> &mut Self; /// Adds a single child + /// + /// If the children were previously children of another parent, that parent's [`Children`] component + /// will have those children removed from its list. Removing all children from a parent causes its + /// [`Children`] component to be removed from the entity. fn add_child(&mut self, child: Entity) -> &mut Self; } @@ -672,6 +691,96 @@ mod tests { assert!(world.get::(child4).is_none()); } + /// Tests what happens when all children are removed from a parent using world functions + #[test] + fn children_removed_when_empty_world() { + let mut world = World::default(); + let entities = world + .spawn_batch(vec![(C(1),), (C(2),), (C(3),)]) + .collect::>(); + + let parent1 = entities[0]; + let parent2 = entities[1]; + let child = entities[2]; + + // push child into parent1 + world.entity_mut(parent1).push_children(&[child]); + assert_eq!( + world.get::(parent1).unwrap().0.as_slice(), + &[child] + ); + + // move only child from parent1 with `push_children` + world.entity_mut(parent2).push_children(&[child]); + assert!(world.get::(parent1).is_none()); + + // move only child from parent2 with `insert_children` + world.entity_mut(parent1).insert_children(0, &[child]); + assert!(world.get::(parent2).is_none()); + + // remove only child from parent1 with `remove_children` + world.entity_mut(parent1).remove_children(&[child]); + assert!(world.get::(parent1).is_none()); + } + + /// Tests what happens when all children are removed form a parent using commands + #[test] + fn children_removed_when_empty_commands() { + let mut world = World::default(); + let entities = world + .spawn_batch(vec![(C(1),), (C(2),), (C(3),)]) + .collect::>(); + + let parent1 = entities[0]; + let parent2 = entities[1]; + let child = entities[2]; + + let mut queue = CommandQueue::default(); + + // push child into parent1 + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent1).push_children(&[child]); + queue.apply(&mut world); + } + assert_eq!( + world.get::(parent1).unwrap().0.as_slice(), + &[child] + ); + + // move only child from parent1 with `push_children` + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent2).push_children(&[child]); + queue.apply(&mut world); + } + assert!(world.get::(parent1).is_none()); + + // move only child from parent2 with `insert_children` + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent1).insert_children(0, &[child]); + queue.apply(&mut world); + } + assert!(world.get::(parent2).is_none()); + + // move only child from parent1 with `add_child` + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent2).add_child(child); + queue.apply(&mut world); + } + assert!(world.get::(parent1).is_none()); + + // remove only child from parent2 with `remove_children` + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent2).remove_children(&[child]); + queue.apply(&mut world); + } + assert!(world.get::(parent2).is_none()); + } + #[test] fn regression_push_children_same_archetype() { let mut world = World::new(); diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 952fb54606..471d8e3ead 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -130,6 +130,13 @@ without UI components as a child of an entity with UI components, results may be .unwrap(); } + /// Removes children from the entity's taffy node if it exists. Does nothing otherwise. + pub fn try_remove_children(&mut self, entity: Entity) { + if let Some(taffy_node) = self.entity_to_taffy.get(&entity) { + self.taffy.set_children(*taffy_node, &[]).unwrap(); + } + } + pub fn update_window(&mut self, window: &Window) { let taffy = &mut self.taffy; let node = self.window_nodes.entry(window.id()).or_insert_with(|| { @@ -216,6 +223,7 @@ pub fn flex_node_system( (With, Changed), >, children_query: Query<(Entity, &Children), (With, Changed)>, + removed_children: RemovedComponents, mut node_transform_query: Query<(Entity, &mut Node, &mut Transform, Option<&Parent>)>, removed_nodes: RemovedComponents, ) { @@ -262,7 +270,10 @@ pub fn flex_node_system( flex_surface.set_window_children(primary_window.id(), root_node_query.iter()); } - // update children + // update and remove children + for entity in &removed_children { + flex_surface.try_remove_children(entity); + } for (entity, children) in &children_query { flex_surface.update_children(entity, children); }