From 8eb0440f1ed0f075ed488e7efa36ba0f8bb37cad Mon Sep 17 00:00:00 2001 From: James Liu Date: Sun, 10 Jul 2022 20:29:06 +0000 Subject: [PATCH] Hierarchy commandization (#4197) ## Objective Implement absolute minimum viable product for the changes proposed in bevyengine/rfcs#53. ## Solution - Remove public mutative access to `Parent` (Children is already publicly read-only). This includes public construction methods like `Copy`, `Clone`, and `Default`. - Remove `PreviousParent` - Remove `parent_update_system` - Update all hierarchy related commands to immediately update both `Parent` and `Children` references. ## Remaining TODOs - [ ] Update documentation for both `Parent` and `Children`. Discourage using `EntityCommands::remove` - [x] Add `HierarchyEvent` to notify listeners of hierarchy updates. This is meant to replace listening on `PreviousParent` ## Followup - These changes should be best moved to the hooks mentioned in #3742. - Backing storage for both might be best moved to indexes mentioned in the same relations. --- crates/bevy_animation/src/lib.rs | 6 +- crates/bevy_hierarchy/src/child_builder.rs | 357 ++++++++---------- .../bevy_hierarchy/src/components/children.rs | 14 +- crates/bevy_hierarchy/src/components/mod.rs | 2 +- .../bevy_hierarchy/src/components/parent.rs | 46 +-- crates/bevy_hierarchy/src/events.rs | 32 ++ crates/bevy_hierarchy/src/lib.rs | 22 +- crates/bevy_hierarchy/src/systems.rs | 72 ---- crates/bevy_transform/src/lib.rs | 11 +- crates/bevy_transform/src/systems.rs | 169 +++++---- crates/bevy_ui/src/flex/mod.rs | 2 +- examples/animation/gltf_skinned_mesh.rs | 2 +- examples/ecs/hierarchy.rs | 21 -- 13 files changed, 315 insertions(+), 441 deletions(-) create mode 100644 crates/bevy_hierarchy/src/events.rs delete mode 100644 crates/bevy_hierarchy/src/systems.rs diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs index ebacc5d8ee..92ecf16c25 100644 --- a/crates/bevy_animation/src/lib.rs +++ b/crates/bevy_animation/src/lib.rs @@ -15,7 +15,7 @@ use bevy_ecs::{ schedule::ParallelSystemDescriptorCoercion, system::{Query, Res}, }; -use bevy_hierarchy::{Children, HierarchySystem}; +use bevy_hierarchy::Children; use bevy_math::{Quat, Vec3}; use bevy_reflect::{Reflect, TypeUuid}; use bevy_time::Time; @@ -295,9 +295,7 @@ impl Plugin for AnimationPlugin { .register_type::() .add_system_to_stage( CoreStage::PostUpdate, - animation_player - .before(TransformSystem::TransformPropagate) - .after(HierarchySystem::ParentUpdate), + animation_player.before(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 4d5687c66a..1063cfbdda 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -1,13 +1,88 @@ -use smallvec::SmallVec; - +use crate::{ + prelude::{Children, Parent}, + HierarchyEvent, +}; use bevy_ecs::{ bundle::Bundle, entity::Entity, + event::Events, system::{Command, Commands, EntityCommands}, world::{EntityMut, World}, }; +use smallvec::SmallVec; -use crate::prelude::{Children, Parent, PreviousParent}; +fn push_events(world: &mut World, events: SmallVec<[HierarchyEvent; 8]>) { + if let Some(mut moved) = world.get_resource_mut::>() { + for evt in events { + moved.send(evt); + } + } +} + +fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) { + let mut parent = world.entity_mut(parent); + if let Some(mut children) = parent.get_mut::() { + children.0.push(child); + } else { + parent.insert(Children(smallvec::smallvec![child])); + } +} + +fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option { + let mut child = world.entity_mut(child); + if let Some(mut parent) = child.get_mut::() { + let previous = parent.0; + *parent = Parent(new_parent); + Some(previous) + } else { + child.insert(Parent(new_parent)); + None + } +} + +fn remove_from_children(world: &mut World, parent: Entity, child: Entity) { + let mut parent = world.entity_mut(parent); + if let Some(mut children) = parent.get_mut::() { + children.0.retain(|x| *x != child); + if children.is_empty() { + parent.remove::(); + } + } +} + +fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { + let mut moved: SmallVec<[HierarchyEvent; 8]> = SmallVec::with_capacity(children.len()); + for child in children { + if let Some(previous) = update_parent(world, *child, parent) { + debug_assert!(parent != previous); + remove_from_children(world, previous, *child); + moved.push(HierarchyEvent::ChildMoved { + child: *child, + previous_parent: previous, + new_parent: parent, + }); + } + } + push_events(world, moved); +} + +fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { + let mut events: SmallVec<[HierarchyEvent; 8]> = SmallVec::new(); + for child in children { + world.entity_mut(*child).remove::(); + events.push(HierarchyEvent::ChildRemoved { + child: *child, + parent, + }); + } + push_events(world, events); + + if let Some(mut parent_children) = world.get_mut::(parent) { + parent_children + .0 + .retain(|parent_child| !children.contains(parent_child)); + } +} /// Command that adds a child to an entity #[derive(Debug)] @@ -20,16 +95,32 @@ pub struct AddChild { impl Command for AddChild { fn write(self, world: &mut World) { - world - .entity_mut(self.child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(self.parent), PreviousParent(self.parent))); - if let Some(mut children) = world.get_mut::(self.parent) { - children.0.push(self.child); + let previous = update_parent(world, self.child, self.parent); + if let Some(previous) = previous { + if previous == self.parent { + return; + } + remove_from_children(world, previous, self.child); + if let Some(mut events) = world.get_resource_mut::>() { + events.send(HierarchyEvent::ChildMoved { + child: self.child, + previous_parent: previous, + new_parent: self.parent, + }); + } + } else if let Some(mut events) = world.get_resource_mut::>() { + events.send(HierarchyEvent::ChildAdded { + child: self.child, + parent: self.parent, + }); + } + let mut parent = world.entity_mut(self.parent); + if let Some(mut children) = parent.get_mut::() { + if !children.contains(&self.child) { + children.0.push(self.child); + } } else { - world - .entity_mut(self.parent) - .insert(Children(smallvec::smallvec![self.child])); + parent.insert(Children(smallvec::smallvec![self.child])); } } } @@ -44,20 +135,13 @@ pub struct InsertChildren { impl Command for InsertChildren { fn write(self, world: &mut World) { - for child in self.children.iter() { - world - .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(self.parent), PreviousParent(self.parent))); - } - { - if let Some(mut children) = world.get_mut::(self.parent) { - children.0.insert_from_slice(self.index, &self.children); - } else { - world - .entity_mut(self.parent) - .insert(Children(self.children)); - } + update_old_parents(world, self.parent, &self.children); + let mut parent = world.entity_mut(self.parent); + if let Some(mut children) = parent.get_mut::() { + children.0.retain(|value| !self.children.contains(value)); + children.0.insert_from_slice(self.index, &self.children); + } else { + parent.insert(Children(self.children)); } } } @@ -70,27 +154,14 @@ pub struct PushChildren { } impl Command for PushChildren { - fn write(self, world: &mut World) { - for child in self.children.iter() { - world - .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(self.parent), PreviousParent(self.parent))); - } - { - let mut added = false; - if let Some(mut children) = world.get_mut::(self.parent) { - children.0.extend(self.children.iter().cloned()); - added = true; - } - - // NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails - // borrow-checking - if !added { - world - .entity_mut(self.parent) - .insert(Children(self.children)); - } + fn write(mut self, world: &mut World) { + update_old_parents(world, self.parent, &self.children); + let mut parent = world.entity_mut(self.parent); + if let Some(mut children) = parent.get_mut::() { + children.0.retain(|child| !self.children.contains(child)); + children.0.append(&mut self.children); + } else { + parent.insert(Children(self.children)); } } } @@ -101,32 +172,8 @@ pub struct RemoveChildren { children: SmallVec<[Entity; 8]>, } -fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { - for child in children.iter() { - let mut child = world.entity_mut(*child); - let mut remove_parent = false; - if let Some(child_parent) = child.get_mut::() { - if child_parent.0 == parent { - remove_parent = true; - } - } - if remove_parent { - if let Some(parent) = child.remove::() { - child.insert(PreviousParent(parent.0)); - } - } - } - // Remove the children from the parents. - if let Some(mut parent_children) = world.get_mut::(parent) { - parent_children - .0 - .retain(|parent_child| !children.contains(parent_child)); - } -} - impl Command for RemoveChildren { fn write(self, world: &mut World) { - // Remove any matching Parent components from the children remove_children(self.parent, &self.children, world); } } @@ -285,15 +332,15 @@ impl<'w> WorldChildBuilder<'w> { .world .spawn() .insert_bundle(bundle) - .insert_bundle((Parent(parent_entity), PreviousParent(parent_entity))) + .insert(Parent(parent_entity)) .id(); + push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); - if let Some(mut parent) = self.world.get_entity_mut(parent_entity) { - if let Some(mut children) = parent.get_mut::() { - children.0.push(entity); - } else { - parent.insert(Children(smallvec::smallvec![entity])); - } + if let Some(mut added) = self.world.get_resource_mut::>() { + added.send(HierarchyEvent::ChildAdded { + child: entity, + parent: parent_entity, + }); } self.world.entity_mut(entity) } @@ -301,18 +348,14 @@ impl<'w> WorldChildBuilder<'w> { /// Spawns an [`Entity`] with no components and inserts it into the children defined by the [`WorldChildBuilder`] which adds the [`Parent`] component to it. pub fn spawn(&mut self) -> EntityMut<'_> { let parent_entity = self.parent_entity(); - let entity = self - .world - .spawn() - .insert_bundle((Parent(parent_entity), PreviousParent(parent_entity))) - .id(); + let entity = self.world.spawn().insert(Parent(parent_entity)).id(); + push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); - if let Some(mut parent) = self.world.get_entity_mut(parent_entity) { - if let Some(mut children) = parent.get_mut::() { - children.0.push(entity); - } else { - parent.insert(Children(smallvec::smallvec![entity])); - } + if let Some(mut added) = self.world.get_resource_mut::>() { + added.send(HierarchyEvent::ChildAdded { + child: entity, + parent: parent_entity, + }); } self.world.entity_mut(entity) } @@ -361,16 +404,14 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { { // SAFETY: parent entity is not modified and its location is updated manually let world = unsafe { self.world_mut() }; - for child in children.iter() { - world - .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(parent), PreviousParent(parent))); - } + update_old_parents(world, parent, children); // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); } if let Some(mut children_component) = self.get_mut::() { + children_component + .0 + .retain(|value| !children.contains(value)); children_component.0.extend(children.iter().cloned()); } else { self.insert(Children::with(children)); @@ -383,17 +424,15 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { { // SAFETY: parent entity is not modified and its location is updated manually let world = unsafe { self.world_mut() }; - for child in children.iter() { - world - .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(parent), PreviousParent(parent))); - } + update_old_parents(world, parent, children); // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); } if let Some(mut children_component) = self.get_mut::() { + children_component + .0 + .retain(|value| !children.contains(value)); children_component.0.insert_from_slice(index, children); } else { self.insert(Children::with(children)); @@ -405,26 +444,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { let parent = self.id(); // SAFETY: This doesn't change the parent's location let world = unsafe { self.world_mut() }; - for child in children.iter() { - let mut child = world.entity_mut(*child); - let mut remove_parent = false; - if let Some(child_parent) = child.get_mut::() { - if child_parent.0 == parent { - remove_parent = true; - } - } - if remove_parent { - if let Some(parent) = child.remove::() { - child.insert(PreviousParent(parent.0)); - } - } - } - // Remove the children from the parents. - if let Some(mut parent_children) = world.get_mut::(parent) { - parent_children - .0 - .retain(|parent_child| !children.contains(parent_child)); - } + remove_children(parent, children, world); self } } @@ -450,13 +470,11 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { let parent = self .current_entity .expect("Cannot add children without a parent. Try creating an entity first."); - for child in children.iter() { - self.world - .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(parent), PreviousParent(parent))); - } + update_old_parents(self.world, parent, children); if let Some(mut children_component) = self.world.get_mut::(parent) { + children_component + .0 + .retain(|value| !children.contains(value)); children_component.0.extend(children.iter().cloned()); } else { self.world @@ -470,14 +488,11 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { let parent = self .current_entity .expect("Cannot add children without a parent. Try creating an entity first."); - - for child in children.iter() { - self.world - .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(parent), PreviousParent(parent))); - } + update_old_parents(self.world, parent, children); if let Some(mut children_component) = self.world.get_mut::(parent) { + children_component + .0 + .retain(|value| !children.contains(value)); children_component.0.insert_from_slice(index, children); } else { self.world @@ -499,6 +514,8 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { #[cfg(test)] mod tests { + use super::{BuildChildren, BuildWorldChildren}; + use crate::prelude::{Children, Parent}; use smallvec::{smallvec, SmallVec}; use bevy_ecs::{ @@ -508,10 +525,6 @@ mod tests { world::World, }; - use crate::prelude::{Children, Parent, PreviousParent}; - - use super::{BuildChildren, BuildWorldChildren}; - #[derive(Component)] struct C(u32); @@ -538,20 +551,13 @@ mod tests { assert_eq!(*world.get::(children[0]).unwrap(), Parent(parent)); assert_eq!(*world.get::(children[1]).unwrap(), Parent(parent)); - assert_eq!( - *world.get::(children[0]).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(children[1]).unwrap(), - PreviousParent(parent) - ); + assert_eq!(*world.get::(children[0]).unwrap(), Parent(parent)); + assert_eq!(*world.get::(children[1]).unwrap(), Parent(parent)); } #[test] fn push_and_insert_and_remove_children_commands() { let mut world = World::default(); - let entities = world .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) .collect::>(); @@ -577,14 +583,8 @@ mod tests { assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); - assert_eq!( - *world.get::(child1).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(child2).unwrap(), - PreviousParent(parent) - ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); { let mut commands = Commands::new(&mut queue, &world); @@ -599,14 +599,8 @@ mod tests { ); assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); - assert_eq!( - *world.get::(child3).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(child4).unwrap(), - PreviousParent(parent) - ); + assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); let remove_children = [child1, child4]; { @@ -622,20 +616,11 @@ mod tests { ); assert!(world.get::(child1).is_none()); assert!(world.get::(child4).is_none()); - assert_eq!( - *world.get::(child1).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(child4).unwrap(), - PreviousParent(parent) - ); } #[test] fn push_and_insert_and_remove_children_world() { let mut world = World::default(); - let entities = world .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) .collect::>(); @@ -656,14 +641,8 @@ mod tests { assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); - assert_eq!( - *world.get::(child1).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(child2).unwrap(), - PreviousParent(parent) - ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); world.entity_mut(parent).insert_children(1, &entities[3..]); let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child3, child4, child2]; @@ -673,14 +652,8 @@ mod tests { ); assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); - assert_eq!( - *world.get::(child3).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(child4).unwrap(), - PreviousParent(parent) - ); + assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); let remove_children = [child1, child4]; world.entity_mut(parent).remove_children(&remove_children); @@ -691,14 +664,6 @@ mod tests { ); assert!(world.get::(child1).is_none()); assert!(world.get::(child4).is_none()); - assert_eq!( - *world.get::(child1).unwrap(), - PreviousParent(parent) - ); - assert_eq!( - *world.get::(child4).unwrap(), - PreviousParent(parent) - ); } #[test] diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index d6c1e42604..0413a4b079 100644 --- a/crates/bevy_hierarchy/src/components/children.rs +++ b/crates/bevy_hierarchy/src/components/children.rs @@ -1,7 +1,9 @@ use bevy_ecs::{ component::Component, entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + prelude::FromWorld, reflect::{ReflectComponent, ReflectMapEntities}, + world::World, }; use bevy_reflect::Reflect; use core::slice; @@ -9,7 +11,7 @@ use smallvec::SmallVec; use std::ops::Deref; /// Contains references to the child entities of this entity -#[derive(Component, Default, Clone, Debug, Reflect)] +#[derive(Component, Debug, Reflect)] #[reflect(Component, MapEntities)] pub struct Children(pub(crate) SmallVec<[Entity; 8]>); @@ -23,6 +25,16 @@ impl MapEntities for Children { } } +// TODO: We need to impl either FromWorld or Default so Children can be registered as Reflect. +// This is because Reflect deserialize by creating an instance and apply a patch on top. +// However Children should only ever be set with a real user-defined entities. Its worth looking +// into better ways to handle cases like this. +impl FromWorld for Children { + fn from_world(_world: &mut World) -> Self { + Children(SmallVec::new()) + } +} + impl Children { /// Builds and returns a [`Children`] component with the given entities pub fn with(entity: &[Entity]) -> Self { diff --git a/crates/bevy_hierarchy/src/components/mod.rs b/crates/bevy_hierarchy/src/components/mod.rs index 3d6928ea3f..3c8b544850 100644 --- a/crates/bevy_hierarchy/src/components/mod.rs +++ b/crates/bevy_hierarchy/src/components/mod.rs @@ -2,4 +2,4 @@ mod children; mod parent; pub use children::Children; -pub use parent::{Parent, PreviousParent}; +pub use parent::Parent; diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 525e56bdec..d4a5e2f3be 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -5,16 +5,23 @@ use bevy_ecs::{ world::{FromWorld, World}, }; use bevy_reflect::Reflect; -use std::ops::{Deref, DerefMut}; +use std::ops::Deref; /// Holds a reference to the parent entity of this entity. /// This component should only be present on entities that actually have a parent entity. -#[derive(Component, Debug, Copy, Clone, Eq, PartialEq, Reflect)] +#[derive(Component, Debug, Eq, PartialEq, Reflect)] #[reflect(Component, MapEntities, PartialEq)] -pub struct Parent(pub Entity); +pub struct Parent(pub(crate) Entity); -// TODO: We need to impl either FromWorld or Default so Parent can be registered as Properties. -// This is because Properties deserialize by creating an instance and apply a patch on top. +impl Parent { + /// Gets the [`Entity`] ID of the parent. + pub fn get(&self) -> Entity { + self.0 + } +} + +// TODO: We need to impl either FromWorld or Default so Parent can be registered as Reflect. +// This is because Reflect deserialize by creating an instance and apply a patch on top. // However Parent should only ever be set with a real user-defined entity. Its worth looking into // better ways to handle cases like this. impl FromWorld for Parent { @@ -41,32 +48,3 @@ impl Deref for Parent { &self.0 } } - -impl DerefMut for Parent { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} - -/// Component that holds the [`Parent`] this entity had previously -#[derive(Component, Debug, Copy, Clone, Eq, PartialEq, Reflect)] -#[reflect(Component, MapEntities, PartialEq)] -pub struct PreviousParent(pub(crate) Entity); - -impl MapEntities for PreviousParent { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { - // PreviousParent of an entity in the new world can be in outside world, in which - // case it should not be mapped. - if let Ok(mapped_entity) = entity_map.get(self.0) { - self.0 = mapped_entity; - } - Ok(()) - } -} - -// TODO: Better handle this case see `impl FromWorld for Parent` -impl FromWorld for PreviousParent { - fn from_world(_world: &mut World) -> Self { - PreviousParent(Entity::from_raw(u32::MAX)) - } -} diff --git a/crates/bevy_hierarchy/src/events.rs b/crates/bevy_hierarchy/src/events.rs new file mode 100644 index 0000000000..5fd3555de9 --- /dev/null +++ b/crates/bevy_hierarchy/src/events.rs @@ -0,0 +1,32 @@ +use bevy_ecs::prelude::Entity; + +/// A [`Event`] that is fired whenever there is a change in the world's +/// hierarchy. +/// +/// [`Event`]: bevy_ecs::event::Event +#[derive(Debug, Clone)] +pub enum HierarchyEvent { + /// Fired whenever an [`Entity`] is added as a child to a new parent. + ChildAdded { + /// The child that added + child: Entity, + /// The parent the child was added to + parent: Entity, + }, + /// Fired whenever an child [`Entity`] is removed from is parent. + ChildRemoved { + /// The child that removed + child: Entity, + /// The parent the child was removed from + parent: Entity, + }, + /// Fired whenever an child [`Entity`] is moved to a new parent. + ChildMoved { + /// The child that moved + child: Entity, + /// The parent the child was removed from + previous_parent: Entity, + /// The parent the child was added to + new_parent: Entity, + }, +} diff --git a/crates/bevy_hierarchy/src/lib.rs b/crates/bevy_hierarchy/src/lib.rs index 60fa42f525..60d55f7b8c 100644 --- a/crates/bevy_hierarchy/src/lib.rs +++ b/crates/bevy_hierarchy/src/lib.rs @@ -13,8 +13,8 @@ pub use hierarchy::*; mod child_builder; pub use child_builder::*; -mod systems; -pub use systems::*; +mod events; +pub use events::*; #[doc(hidden)] pub mod prelude { @@ -23,31 +23,15 @@ pub mod prelude { } use bevy_app::prelude::*; -use bevy_ecs::prelude::*; /// The base plugin for handling [`Parent`] and [`Children`] components #[derive(Default)] pub struct HierarchyPlugin; -/// Label enum for the systems relating to hierarchy upkeep -#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)] -pub enum HierarchySystem { - /// Updates [`Parent`] when changes in the hierarchy occur - ParentUpdate, -} - impl Plugin for HierarchyPlugin { fn build(&self, app: &mut App) { app.register_type::() .register_type::() - .register_type::() - .add_startup_system_to_stage( - StartupStage::PostStartup, - parent_update_system.label(HierarchySystem::ParentUpdate), - ) - .add_system_to_stage( - CoreStage::PostUpdate, - parent_update_system.label(HierarchySystem::ParentUpdate), - ); + .add_event::(); } } diff --git a/crates/bevy_hierarchy/src/systems.rs b/crates/bevy_hierarchy/src/systems.rs deleted file mode 100644 index 194f0da0c2..0000000000 --- a/crates/bevy_hierarchy/src/systems.rs +++ /dev/null @@ -1,72 +0,0 @@ -use crate::components::*; -use bevy_ecs::{ - entity::Entity, - prelude::Changed, - query::Without, - system::{Commands, Query}, -}; -use bevy_utils::HashMap; -use smallvec::SmallVec; - -/// Updates parents when the hierarchy is changed -pub fn parent_update_system( - mut commands: Commands, - removed_parent_query: Query<(Entity, &PreviousParent), Without>, - mut parent_query: Query<(Entity, &Parent, Option<&mut PreviousParent>), Changed>, - mut children_query: Query<&mut Children>, -) { - // Entities with a missing `Parent` (ie. ones that have a `PreviousParent`), remove - // them from the `Children` of the `PreviousParent`. - for (entity, previous_parent) in removed_parent_query.iter() { - if let Ok(mut previous_parent_children) = children_query.get_mut(previous_parent.0) { - previous_parent_children.0.retain(|e| *e != entity); - commands.entity(entity).remove::(); - } - } - - // Tracks all newly created `Children` Components this frame. - let mut children_additions = HashMap::>::default(); - - // Entities with a changed Parent (that also have a PreviousParent, even if None) - for (entity, parent, possible_previous_parent) in parent_query.iter_mut() { - if let Some(mut previous_parent) = possible_previous_parent { - // New and previous point to the same Entity, carry on, nothing to see here. - if previous_parent.0 == parent.0 { - continue; - } - - // Remove from `PreviousParent.Children`. - if let Ok(mut previous_parent_children) = children_query.get_mut(previous_parent.0) { - (*previous_parent_children).0.retain(|e| *e != entity); - } - - // Set `PreviousParent = Parent`. - *previous_parent = PreviousParent(parent.0); - } else { - commands.entity(entity).insert(PreviousParent(parent.0)); - }; - - // Add to the parent's `Children` (either the real component, or - // `children_additions`). - if let Ok(mut new_parent_children) = children_query.get_mut(parent.0) { - // This is the parent - // PERF: Ideally we shouldn't need to check for duplicates - if !(*new_parent_children).0.contains(&entity) { - (*new_parent_children).0.push(entity); - } - } else { - // The parent doesn't have a children entity, lets add it - children_additions - .entry(parent.0) - .or_insert_with(Default::default) - .push(entity); - } - } - - // Flush the `children_additions` to the command buffer. It is stored separate to - // collect multiple new children that point to the same parent into the same - // SmallVec, and to prevent redundant add+remove operations. - children_additions.iter().for_each(|(e, v)| { - commands.entity(*e).insert(Children::with(v)); - }); -} diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index a88017c7d7..1488278398 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -14,7 +14,6 @@ pub mod prelude { use bevy_app::prelude::*; use bevy_ecs::prelude::*; -use bevy_hierarchy::HierarchySystem; use prelude::{GlobalTransform, Transform}; /// A [`Bundle`] of the [`Transform`] and [`GlobalTransform`] @@ -92,18 +91,14 @@ impl Plugin for TransformPlugin { fn build(&self, app: &mut App) { app.register_type::() .register_type::() - // Adding these to startup ensures the first update is "correct" + // add transform systems to startup so the first update is "correct" .add_startup_system_to_stage( StartupStage::PostStartup, - systems::transform_propagate_system - .label(TransformSystem::TransformPropagate) - .after(HierarchySystem::ParentUpdate), + systems::transform_propagate_system.label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::transform_propagate_system - .label(TransformSystem::TransformPropagate) - .after(HierarchySystem::ParentUpdate), + systems::transform_propagate_system.label(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 672e67b513..d87fa5e7e8 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -67,7 +67,7 @@ fn propagate_recursive( transform_query.get_mut(entity).map_err(drop)?; // Note that for parallelising, this check cannot occur here, since there is an `&mut GlobalTransform` (in global_transform) assert_eq!( - child_parent.0, expected_parent, + child_parent.get(), expected_parent, "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" ); changed |= transform_changed; @@ -103,17 +103,14 @@ mod test { use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; use crate::TransformBundle; - use bevy_hierarchy::{ - parent_update_system, BuildChildren, BuildWorldChildren, Children, Parent, - }; + use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; #[test] fn did_propagate() { let mut world = World::default(); let mut update_stage = SystemStage::parallel(); - update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system.after(parent_update_system)); + update_stage.add_system(transform_propagate_system); let mut schedule = Schedule::default(); schedule.add_stage("update", update_stage); @@ -155,10 +152,8 @@ mod test { #[test] fn did_propagate_command_buffer() { let mut world = World::default(); - let mut update_stage = SystemStage::parallel(); - update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system.after(parent_update_system)); + update_stage.add_system(transform_propagate_system); let mut schedule = Schedule::default(); schedule.add_stage("update", update_stage); @@ -200,36 +195,38 @@ mod test { let mut world = World::default(); let mut update_stage = SystemStage::parallel(); - update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system.after(parent_update_system)); + update_stage.add_system(transform_propagate_system); let mut schedule = Schedule::default(); schedule.add_stage("update", update_stage); // Add parent entities - let mut command_queue = CommandQueue::default(); - let mut commands = Commands::new(&mut command_queue, &world); let mut children = Vec::new(); - let parent = commands - .spawn() - .insert(Transform::from_xyz(1.0, 0.0, 0.0)) - .id(); - commands.entity(parent).with_children(|parent| { - children.push( - parent - .spawn() - .insert(Transform::from_xyz(0.0, 2.0, 0.0)) - .id(), - ); - children.push( - parent - .spawn() - .insert(Transform::from_xyz(0.0, 3.0, 0.0)) - .id(), - ); - }); - command_queue.apply(&mut world); - schedule.run(&mut world); + let parent = { + let mut command_queue = CommandQueue::default(); + let mut commands = Commands::new(&mut command_queue, &world); + let parent = commands + .spawn() + .insert(Transform::from_xyz(1.0, 0.0, 0.0)) + .id(); + commands.entity(parent).with_children(|parent| { + children.push( + parent + .spawn() + .insert(Transform::from_xyz(0.0, 2.0, 0.0)) + .id(), + ); + children.push( + parent + .spawn() + .insert(Transform::from_xyz(0.0, 3.0, 0.0)) + .id(), + ); + }); + command_queue.apply(&mut world); + schedule.run(&mut world); + parent + }; assert_eq!( world @@ -242,9 +239,13 @@ mod test { ); // Parent `e1` to `e2`. - (*world.get_mut::(children[0]).unwrap()).0 = children[1]; - - schedule.run(&mut world); + { + let mut command_queue = CommandQueue::default(); + let mut commands = Commands::new(&mut command_queue, &world); + commands.entity(children[1]).add_child(children[0]); + command_queue.apply(&mut world); + schedule.run(&mut world); + } assert_eq!( world @@ -285,36 +286,28 @@ mod test { fn correct_transforms_when_no_children() { let mut app = App::new(); - app.add_system(parent_update_system); - app.add_system(transform_propagate_system.after(parent_update_system)); + app.add_system(transform_propagate_system); let translation = vec3(1.0, 0.0, 0.0); + // These will be overwritten. + let mut child = Entity::from_raw(0); + let mut grandchild = Entity::from_raw(1); let parent = app .world .spawn() .insert(Transform::from_translation(translation)) .insert(GlobalTransform::default()) - .id(); - - let child = app - .world - .spawn() - .insert_bundle(( - Transform::identity(), - GlobalTransform::default(), - Parent(parent), - )) - .id(); - - let grandchild = app - .world - .spawn() - .insert_bundle(( - Transform::identity(), - GlobalTransform::default(), - Parent(child), - )) + .with_children(|builder| { + child = builder + .spawn_bundle((Transform::identity(), GlobalTransform::default())) + .with_children(|builder| { + grandchild = builder + .spawn_bundle((Transform::identity(), GlobalTransform::default())) + .id(); + }) + .id(); + }) .id(); app.update(); @@ -336,37 +329,47 @@ mod test { ); } } + #[test] #[should_panic] fn panic_when_hierarchy_cycle() { - let mut world = World::default(); - // This test is run on a single thread in order to avoid breaking the global task pool by panicking - // This fixes the flaky tests reported in https://github.com/bevyengine/bevy/issues/4996 - let mut update_stage = SystemStage::single_threaded(); + // We cannot directly edit Parent and Children, so we use a temp world to break + // the hierarchy's invariants. + let mut temp = World::new(); + let mut app = App::new(); - update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system.after(parent_update_system)); + app.add_system(transform_propagate_system); - let child = world + fn setup_world(world: &mut World) -> (Entity, Entity) { + let mut grandchild = Entity::from_raw(0); + let child = world + .spawn() + .insert_bundle((Transform::identity(), GlobalTransform::default())) + .with_children(|builder| { + grandchild = builder + .spawn() + .insert_bundle((Transform::identity(), GlobalTransform::default())) + .id(); + }) + .id(); + (child, grandchild) + } + + let (temp_child, temp_grandchild) = setup_world(&mut temp); + let (child, grandchild) = setup_world(&mut app.world); + + assert_eq!(temp_child, child); + assert_eq!(temp_grandchild, grandchild); + + app.world .spawn() - .insert_bundle((Transform::identity(), GlobalTransform::default())) - .id(); + .insert_bundle((Transform::default(), GlobalTransform::default())) + .push_children(&[child]); + std::mem::swap( + &mut *app.world.get_mut::(child).unwrap(), + &mut *temp.get_mut::(grandchild).unwrap(), + ); - let grandchild = world - .spawn() - .insert_bundle(( - Transform::identity(), - GlobalTransform::default(), - Parent(child), - )) - .id(); - world.spawn().insert_bundle(( - Transform::default(), - GlobalTransform::default(), - Children::with(&[child]), - )); - world.entity_mut(child).insert(Parent(grandchild)); - - update_stage.run(&mut world); + app.update(); } } diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 149ecae13c..58509b691f 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -279,7 +279,7 @@ pub fn flex_node_system( new_position.x = to_logical(layout.location.x + layout.size.width / 2.0); new_position.y = to_logical(layout.location.y + layout.size.height / 2.0); if let Some(parent) = parent { - if let Ok(parent_layout) = flex_surface.get_layout(parent.0) { + if let Ok(parent_layout) = flex_surface.get_layout(**parent) { new_position.x -= to_logical(parent_layout.size.width / 2.0); new_position.y -= to_logical(parent_layout.size.height / 2.0); } diff --git a/examples/animation/gltf_skinned_mesh.rs b/examples/animation/gltf_skinned_mesh.rs index aea6e6a4fa..76fe5753c1 100644 --- a/examples/animation/gltf_skinned_mesh.rs +++ b/examples/animation/gltf_skinned_mesh.rs @@ -52,7 +52,7 @@ fn joint_animation( // Iter skinned mesh entity for skinned_mesh_parent in parent_query.iter() { // Mesh node is the parent of the skinned mesh entity. - let mesh_node_entity = skinned_mesh_parent.0; + let mesh_node_entity = skinned_mesh_parent.get(); // Get `Children` in the mesh node. let mesh_node_children = children_query.get(mesh_node_entity).unwrap(); diff --git a/examples/ecs/hierarchy.rs b/examples/ecs/hierarchy.rs index dbd9175079..eaa483fe01 100644 --- a/examples/ecs/hierarchy.rs +++ b/examples/ecs/hierarchy.rs @@ -41,27 +41,6 @@ fn setup(mut commands: Commands, asset_server: Res) { // Store parent entity for next sections .id(); - // Another way to create a hierarchy is to add a Parent component to an entity, - // which would be added automatically to parents with other methods. - // Similarly, adding a Parent component will automatically add a Children component to the - // parent. - commands - .spawn_bundle(SpriteBundle { - transform: Transform { - translation: Vec3::new(-250.0, 0.0, 0.0), - scale: Vec3::splat(0.75), - ..default() - }, - texture: texture.clone(), - sprite: Sprite { - color: Color::RED, - ..default() - }, - ..default() - }) - // Using the entity from the previous section as the parent: - .insert(Parent(parent)); - // Another way is to use the push_children function to add children after the parent // entity has already been spawned. let child = commands