From 613756a275140e1421e4bca82560cd63e3ed68c7 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 12 Mar 2022 16:29:50 -0800 Subject: [PATCH 01/21] Make Parent immutable, remove PreviousParent, parent_update_system --- crates/bevy_hierarchy/src/child_builder.rs | 31 ++++---- crates/bevy_hierarchy/src/components/mod.rs | 2 +- .../bevy_hierarchy/src/components/parent.rs | 27 +------ crates/bevy_hierarchy/src/lib.rs | 21 +----- crates/bevy_hierarchy/src/systems.rs | 72 ------------------- crates/bevy_transform/src/hierarchy/mod.rs | 6 ++ crates/bevy_transform/src/lib.rs | 7 +- crates/bevy_ui/src/flex/mod.rs | 2 +- 8 files changed, 24 insertions(+), 144 deletions(-) delete mode 100644 crates/bevy_hierarchy/src/systems.rs create mode 100644 crates/bevy_transform/src/hierarchy/mod.rs diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 3dd47da3878a6..0b63e6e58122e 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -1,4 +1,4 @@ -use crate::prelude::{Children, Parent, PreviousParent}; +use crate::prelude::{Children, Parent}; use bevy_ecs::{ bundle::Bundle, entity::Entity, @@ -20,8 +20,7 @@ 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))); + .insert(Parent(self.parent)); if let Some(mut children) = world.get_mut::(self.parent) { children.0.push(self.child); } else { @@ -45,8 +44,7 @@ impl Command for InsertChildren { 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))); + .insert(Parent(self.parent)); } { if let Some(mut children) = world.get_mut::(self.parent) { @@ -72,8 +70,7 @@ impl Command for PushChildren { 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))); + .insert(Parent(self.parent)); } { let mut added = false; @@ -110,7 +107,7 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { } if remove_parent { if let Some(parent) = child.remove::() { - child.insert(PreviousParent(parent.0)); + // child.insert(PreviousParent(parent.0)); } } } @@ -246,7 +243,7 @@ impl<'w> WorldChildBuilder<'w> { .world .spawn() .insert_bundle(bundle) - .insert_bundle((Parent(parent_entity), PreviousParent(parent_entity))) + .insert(Parent(parent_entity)) .id(); self.current_entity = Some(entity); if let Some(mut parent) = self.world.get_entity_mut(parent_entity) { @@ -265,7 +262,7 @@ impl<'w> WorldChildBuilder<'w> { let entity = self .world .spawn() - .insert_bundle((Parent(parent_entity), PreviousParent(parent_entity))) + .insert(Parent(parent_entity)) .id(); self.current_entity = Some(entity); if let Some(mut parent) = self.world.get_entity_mut(parent_entity) { @@ -325,8 +322,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { for child in children.iter() { world .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(parent), PreviousParent(parent))); + .insert(Parent(parent)); } // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); @@ -347,8 +343,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { for child in children.iter() { world .entity_mut(*child) - // FIXME: don't erase the previous parent (see #1545) - .insert_bundle((Parent(parent), PreviousParent(parent))); + .insert(Parent(parent)); } // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); @@ -376,7 +371,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { } if remove_parent { if let Some(parent) = child.remove::() { - child.insert(PreviousParent(parent.0)); + // child.insert(PreviousParent(parent.0)); } } } @@ -414,8 +409,7 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { 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))); + .insert(Parent(parent)); } if let Some(mut children_component) = self.world.get_mut::(parent) { children_component.0.extend(children.iter().cloned()); @@ -435,8 +429,7 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { 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))); + .insert(Parent(parent)); } if let Some(mut children_component) = self.world.get_mut::(parent) { children_component.0.insert_from_slice(index, children); diff --git a/crates/bevy_hierarchy/src/components/mod.rs b/crates/bevy_hierarchy/src/components/mod.rs index 3d6928ea3f0ae..3c8b544850382 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 ddc0e2a634ee6..c359a44e21d0c 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -11,7 +11,7 @@ use std::ops::{Deref, DerefMut}; /// This component should only be present on entities that actually have a parent entity. #[derive(Component, Debug, Copy, Clone, 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. @@ -37,28 +37,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> { - self.0 = entity_map.get(self.0)?; - 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/lib.rs b/crates/bevy_hierarchy/src/lib.rs index 60fa42f525e97..600ce2e21e63c 100644 --- a/crates/bevy_hierarchy/src/lib.rs +++ b/crates/bevy_hierarchy/src/lib.rs @@ -13,9 +13,6 @@ pub use hierarchy::*; mod child_builder; pub use child_builder::*; -mod systems; -pub use systems::*; - #[doc(hidden)] pub mod prelude { #[doc(hidden)] @@ -29,25 +26,9 @@ use bevy_ecs::prelude::*; #[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), - ); + .register_type::(); } } diff --git a/crates/bevy_hierarchy/src/systems.rs b/crates/bevy_hierarchy/src/systems.rs deleted file mode 100644 index 194f0da0c2af4..0000000000000 --- 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/hierarchy/mod.rs b/crates/bevy_transform/src/hierarchy/mod.rs new file mode 100644 index 0000000000000..ab3609b624fec --- /dev/null +++ b/crates/bevy_transform/src/hierarchy/mod.rs @@ -0,0 +1,6 @@ +mod child_builder; +#[allow(clippy::module_inception)] +mod hierarchy; + +pub use child_builder::*; +pub use hierarchy::*; \ No newline at end of file diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index a88017c7d7d60..9007aeb50b338 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`] @@ -96,14 +95,12 @@ impl Plugin for TransformPlugin { .add_startup_system_to_stage( StartupStage::PostStartup, systems::transform_propagate_system - .label(TransformSystem::TransformPropagate) - .after(HierarchySystem::ParentUpdate), + .label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, systems::transform_propagate_system - .label(TransformSystem::TransformPropagate) - .after(HierarchySystem::ParentUpdate), + .label(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 63ccf94a2ca13..5fd63c5578826 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -287,7 +287,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(**tparent) { new_position.x -= to_logical(parent_layout.size.width / 2.0); new_position.y -= to_logical(parent_layout.size.height / 2.0); } From 838de433197258556218c161a227975907b7b180 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 12 Mar 2022 18:54:20 -0800 Subject: [PATCH 02/21] Make hierarchy commands "atomic" --- crates/bevy_hierarchy/src/child_builder.rs | 145 ++++++++---------- .../bevy_hierarchy/src/components/parent.rs | 2 +- crates/bevy_transform/src/systems.rs | 2 +- crates/bevy_ui/src/flex/mod.rs | 2 +- examples/ecs/hierarchy.rs | 21 --- 5 files changed, 68 insertions(+), 104 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 0b63e6e58122e..18eb67ee96bab 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -7,6 +7,33 @@ use bevy_ecs::{ }; use smallvec::SmallVec; +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; + *parent = Parent(new_parent); + Some(*previous) + } else { + child.insert(Parent(new_parent)); + None + } +} + +fn remove_from_children(world: &mut World, parent: Entity, child: Entity) -> Option { + let mut remove = false; + let mut parent = world.get_entity_mut(parent)?; + if let Some(mut children) = parent.get_mut::() { + if let Some(idx) = children.iter().position(|x| *x == child) { + children.0.remove(idx); + remove = children.is_empty(); + } + } + if remove { + parent.remove::(); + } + Some(parent.id()) +} + /// Command that adds a child to an entity #[derive(Debug)] pub struct AddChild { @@ -18,15 +45,20 @@ pub struct AddChild { impl Command for AddChild { fn write(self, world: &mut World) { - world - .entity_mut(self.child) - .insert(Parent(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 previous == Some(self.parent) { + return; + } + if let Some(previous) = previous { + remove_from_children(world, previous, self.child); + } + let mut parent = world.entity_mut(self.parent); + if let Some(mut children) = parent.get_mut::() { + if children.iter().position(|x| *x == self.child).is_none() { + children.0.push(self.child); + } } else { - world - .entity_mut(self.parent) - .insert(Children(smallvec::smallvec![self.child])); + parent.insert(Children(smallvec::smallvec![self.child])); } } } @@ -42,19 +74,17 @@ pub struct InsertChildren { impl Command for InsertChildren { fn write(self, world: &mut World) { for child in self.children.iter() { - world - .entity_mut(*child) - .insert(Parent(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)); + if let Some(previous) = update_parent(world, *child, self.parent) { + remove_from_children(world, previous, *child); } } + 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)); + } } } @@ -68,25 +98,16 @@ pub struct PushChildren { impl Command for PushChildren { fn write(self, world: &mut World) { for child in self.children.iter() { - world - .entity_mut(*child) - .insert(Parent(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)); + if let Some(previous) = update_parent(world, *child, self.parent) { + remove_from_children(world, previous, *child); } } + let mut parent = world.entity_mut(self.parent); + if let Some(mut children) = parent.get_mut::() { + children.0.extend(self.children.iter().cloned()); + } else { + parent.insert(Children(self.children)); + } } } @@ -98,20 +119,8 @@ pub struct RemoveChildren { 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)); - } - } + world.entity_mut(*child).remove::(); } - // Remove the children from the parents. if let Some(mut parent_children) = world.get_mut::(parent) { parent_children .0 @@ -121,7 +130,6 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { 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); } } @@ -259,11 +267,7 @@ 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(Parent(parent_entity)) - .id(); + let entity = self.world.spawn().insert(Parent(parent_entity)).id(); 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::() { @@ -320,9 +324,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { // SAFE: 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) - .insert(Parent(parent)); + world.entity_mut(*child).insert(Parent(parent)); } // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); @@ -341,9 +343,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { // SAFE: 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) - .insert(Parent(parent)); + world.entity_mut(*child).insert(Parent(parent)); } // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype self.update_location(); @@ -362,18 +362,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { // SAFE: 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)); - } - } + world.entity_mut(*child).remove::(); } // Remove the children from the parents. if let Some(mut parent_children) = world.get_mut::(parent) { @@ -407,9 +396,7 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { .current_entity .expect("Cannot add children without a parent. Try creating an entity first."); for child in children.iter() { - self.world - .entity_mut(*child) - .insert(Parent(parent)); + self.world.entity_mut(*child).insert(Parent(parent)); } if let Some(mut children_component) = self.world.get_mut::(parent) { children_component.0.extend(children.iter().cloned()); @@ -427,9 +414,7 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { .expect("Cannot add children without a parent. Try creating an entity first."); for child in children.iter() { - self.world - .entity_mut(*child) - .insert(Parent(parent)); + self.world.entity_mut(*child).insert(Parent(parent)); } if let Some(mut children_component) = self.world.get_mut::(parent) { children_component.0.insert_from_slice(index, children); @@ -454,7 +439,7 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { #[cfg(test)] mod tests { use super::{BuildChildren, BuildWorldChildren}; - use crate::prelude::{Children, Parent, PreviousParent}; + use crate::prelude::{Children, Parent}; use bevy_ecs::{ component::Component, entity::Entity, diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index c359a44e21d0c..b42ef77d6f0e9 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -5,7 +5,7 @@ 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. diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 9fb2e9a2d0ef9..7705ee6e5f84c 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -86,7 +86,7 @@ mod test { use crate::systems::transform_propagate_system; use crate::TransformBundle; use bevy_hierarchy::{ - parent_update_system, BuildChildren, BuildWorldChildren, Children, Parent, + BuildChildren, BuildWorldChildren, Children, Parent, }; #[test] diff --git a/crates/bevy_ui/src/flex/mod.rs b/crates/bevy_ui/src/flex/mod.rs index 5fd63c5578826..44d8fabd94391 100644 --- a/crates/bevy_ui/src/flex/mod.rs +++ b/crates/bevy_ui/src/flex/mod.rs @@ -287,7 +287,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(**tparent) { + 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/ecs/hierarchy.rs b/examples/ecs/hierarchy.rs index c67dfac85e4a5..881a7fc81e95c 100644 --- a/examples/ecs/hierarchy.rs +++ b/examples/ecs/hierarchy.rs @@ -39,27 +39,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 From 9b3a8f08b6016fb7f566582d207f190a64bb3588 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 12 Mar 2022 19:05:38 -0800 Subject: [PATCH 03/21] Fix tests --- crates/bevy_hierarchy/src/child_builder.rs | 56 ++++++++-------------- crates/bevy_transform/src/systems.rs | 2 - 2 files changed, 20 insertions(+), 38 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 18eb67ee96bab..ad2985074ec11 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -474,12 +474,12 @@ mod tests { assert_eq!(*world.get::(children[1]).unwrap(), Parent(parent)); assert_eq!( - *world.get::(children[0]).unwrap(), - PreviousParent(parent) + *world.get::(children[0]).unwrap(), + Parent(parent) ); assert_eq!( - *world.get::(children[1]).unwrap(), - PreviousParent(parent) + *world.get::(children[1]).unwrap(), + Parent(parent) ); } @@ -513,12 +513,12 @@ mod tests { assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); assert_eq!( - *world.get::(child1).unwrap(), - PreviousParent(parent) + *world.get::(child1).unwrap(), + Parent(parent) ); assert_eq!( - *world.get::(child2).unwrap(), - PreviousParent(parent) + *world.get::(child2).unwrap(), + Parent(parent) ); { @@ -535,12 +535,12 @@ 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) + *world.get::(child3).unwrap(), + Parent(parent) ); assert_eq!( - *world.get::(child4).unwrap(), - PreviousParent(parent) + *world.get::(child4).unwrap(), + Parent(parent) ); let remove_children = [child1, child4]; @@ -557,14 +557,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] @@ -592,12 +584,12 @@ mod tests { assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); assert_eq!( - *world.get::(child1).unwrap(), - PreviousParent(parent) + *world.get::(child1).unwrap(), + Parent(parent) ); assert_eq!( - *world.get::(child2).unwrap(), - PreviousParent(parent) + *world.get::(child2).unwrap(), + Parent(parent) ); world.entity_mut(parent).insert_children(1, &entities[3..]); @@ -609,12 +601,12 @@ 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) + *world.get::(child3).unwrap(), + Parent(parent) ); assert_eq!( - *world.get::(child4).unwrap(), - PreviousParent(parent) + *world.get::(child4).unwrap(), + Parent(parent) ); let remove_children = [child1, child4]; @@ -626,14 +618,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_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 7705ee6e5f84c..66431649ba513 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -94,7 +94,6 @@ 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); let mut schedule = Schedule::default(); @@ -139,7 +138,6 @@ 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); let mut schedule = Schedule::default(); From d471ba3e386f7915f3189c555f6580bc838b9baf Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 12 Mar 2022 19:25:46 -0800 Subject: [PATCH 04/21] Formatting and clippy --- crates/bevy_hierarchy/src/child_builder.rs | 52 +++++----------------- 1 file changed, 11 insertions(+), 41 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index ad2985074ec11..9ff546d0aa573 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -54,7 +54,7 @@ impl Command for AddChild { } let mut parent = world.entity_mut(self.parent); if let Some(mut children) = parent.get_mut::() { - if children.iter().position(|x| *x == self.child).is_none() { + if children.iter().any(|x| *x != self.child) { children.0.push(self.child); } } else { @@ -473,14 +473,8 @@ 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(), - Parent(parent) - ); - assert_eq!( - *world.get::(children[1]).unwrap(), - Parent(parent) - ); + assert_eq!(*world.get::(children[0]).unwrap(), Parent(parent)); + assert_eq!(*world.get::(children[1]).unwrap(), Parent(parent)); } #[test] @@ -512,14 +506,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(), - Parent(parent) - ); - assert_eq!( - *world.get::(child2).unwrap(), - Parent(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); @@ -534,14 +522,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(), - Parent(parent) - ); - assert_eq!( - *world.get::(child4).unwrap(), - Parent(parent) - ); + assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); let remove_children = [child1, child4]; { @@ -583,14 +565,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(), - Parent(parent) - ); - assert_eq!( - *world.get::(child2).unwrap(), - Parent(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]; @@ -600,14 +576,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(), - Parent(parent) - ); - assert_eq!( - *world.get::(child4).unwrap(), - Parent(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); From 9a35a57d1286e31f92df57f6e475a9e8fe888731 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 03:19:54 -0700 Subject: [PATCH 05/21] More complete and correct updates --- crates/bevy_hierarchy/src/child_builder.rs | 38 ++++++++++++---------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 9ff546d0aa573..64dc6701a68dc 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -7,6 +7,16 @@ use bevy_ecs::{ }; use smallvec::SmallVec; +fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) { + if let Some(mut parent) = world.get_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::() { @@ -104,6 +114,7 @@ impl Command for PushChildren { } 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.extend(self.children.iter().cloned()); } else { parent.insert(Children(self.children)); @@ -251,16 +262,9 @@ impl<'w> WorldChildBuilder<'w> { .world .spawn() .insert_bundle(bundle) - .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])); - } - } self.world.entity_mut(entity) } @@ -268,14 +272,8 @@ impl<'w> WorldChildBuilder<'w> { pub fn spawn(&mut self) -> EntityMut<'_> { let parent_entity = self.parent_entity(); 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])); - } - } self.world.entity_mut(entity) } @@ -324,12 +322,15 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { // SAFE: 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).insert(Parent(parent)); + if let Some(previous) = update_parent(world, *child, parent) { + remove_from_children(world, previous, *child); + } } // 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)); @@ -343,13 +344,16 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { // SAFE: 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).insert(Parent(parent)); + if let Some(previous) = update_parent(world, *child, parent) { + remove_from_children(world, previous, *child); + } } // 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)); From 2e9a9a474962a5be3b144ed1f40abdcfb744d614 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 04:15:53 -0700 Subject: [PATCH 06/21] Hierarchy events --- crates/bevy_hierarchy/src/child_builder.rs | 146 ++++++++++++++------- crates/bevy_hierarchy/src/events.rs | 20 +++ crates/bevy_hierarchy/src/lib.rs | 8 +- crates/bevy_transform/src/hierarchy/mod.rs | 6 - crates/bevy_transform/src/lib.rs | 8 +- crates/bevy_transform/src/systems.rs | 4 +- 6 files changed, 133 insertions(+), 59 deletions(-) create mode 100644 crates/bevy_hierarchy/src/events.rs delete mode 100644 crates/bevy_transform/src/hierarchy/mod.rs diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 64dc6701a68dc..eacd57e216b83 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -1,12 +1,43 @@ -use crate::prelude::{Children, Parent}; +use crate::{ + prelude::{Children, Parent}, + ChildAdded, ChildMoved, ChildRemoved, +}; use bevy_ecs::{ bundle::Bundle, entity::Entity, + event::Events, system::{Command, Commands, EntityCommands}, world::{EntityMut, World}, }; use smallvec::SmallVec; +fn push_move_events(world: &mut World, events: SmallVec<[ChildMoved; 8]>) { + { + let mut removed = world.resource_mut::>(); + for evt in events.iter() { + removed.send(ChildRemoved { + child: evt.child, + parent: evt.previous_parent, + }); + } + } + + let mut moved_events = world.resource_mut::>(); + for evt in events { + moved_events.send(evt); + } +} + +fn push_add_events(world: &mut World, parent: Entity, children: &[Entity]) { + let mut added = world.resource_mut::>(); + for child in children.iter() { + added.send(ChildAdded { + child: *child, + parent, + }); + } +} + fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) { if let Some(mut parent) = world.get_entity_mut(parent) { if let Some(mut children) = parent.get_mut::() { @@ -44,6 +75,22 @@ fn remove_from_children(world: &mut World, parent: Entity, child: Entity) -> Opt Some(parent.id()) } +fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { + let mut moved: SmallVec<[ChildMoved; 8]> = SmallVec::with_capacity(children.len()); + for child in children.iter() { + if let Some(previous) = update_parent(world, *child, parent) { + remove_from_children(world, previous, *child); + moved.push(ChildMoved { + child: *child, + previous_parent: previous, + new_parent: parent, + }); + } + } + push_move_events(world, moved); + push_add_events(world, parent, children); +} + /// Command that adds a child to an entity #[derive(Debug)] pub struct AddChild { @@ -61,6 +108,17 @@ impl Command for AddChild { } if let Some(previous) = previous { remove_from_children(world, previous, self.child); + world + .resource_mut::>() + .send(ChildRemoved { + child: self.child, + parent: previous, + }); + world.resource_mut::>().send(ChildMoved { + child: self.child, + previous_parent: previous, + new_parent: self.parent, + }); } let mut parent = world.entity_mut(self.parent); if let Some(mut children) = parent.get_mut::() { @@ -70,6 +128,10 @@ impl Command for AddChild { } else { parent.insert(Children(smallvec::smallvec![self.child])); } + world.resource_mut::>().send(ChildAdded { + child: self.child, + parent: self.parent, + }); } } @@ -83,11 +145,7 @@ pub struct InsertChildren { impl Command for InsertChildren { fn write(self, world: &mut World) { - for child in self.children.iter() { - if let Some(previous) = update_parent(world, *child, self.parent) { - remove_from_children(world, previous, *child); - } - } + 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)); @@ -107,11 +165,7 @@ pub struct PushChildren { impl Command for PushChildren { fn write(self, world: &mut World) { - for child in self.children.iter() { - if let Some(previous) = update_parent(world, *child, self.parent) { - remove_from_children(world, previous, *child); - } - } + 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)); @@ -129,9 +183,22 @@ pub struct RemoveChildren { } fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { + let mut events: SmallVec<[ChildRemoved; 8]> = SmallVec::new(); for child in children.iter() { world.entity_mut(*child).remove::(); + events.push(ChildRemoved { + child: *child, + parent, + }); } + + { + let mut removed = world.resource_mut::>(); + for evt in events { + removed.send(evt); + } + } + if let Some(mut parent_children) = world.get_mut::(parent) { parent_children .0 @@ -258,13 +325,15 @@ impl<'w> WorldChildBuilder<'w> { /// Spawns an entity with the given bundle and inserts it into the children defined by the [`WorldChildBuilder`] pub fn spawn_bundle(&mut self, bundle: impl Bundle + Send + Sync + 'static) -> EntityMut<'_> { let parent_entity = self.parent_entity(); - let entity = self - .world - .spawn() - .insert_bundle(bundle) - .id(); + let entity = self.world.spawn().insert_bundle(bundle).id(); push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); + self.world + .resource_mut::>() + .send(ChildAdded { + child: entity, + parent: parent_entity, + }); self.world.entity_mut(entity) } @@ -274,6 +343,12 @@ impl<'w> WorldChildBuilder<'w> { let entity = self.world.spawn().insert(Parent(parent_entity)).id(); push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); + self.world + .resource_mut::>() + .send(ChildAdded { + child: entity, + parent: parent_entity, + }); self.world.entity_mut(entity) } @@ -321,16 +396,14 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { { // SAFE: parent entity is not modified and its location is updated manually let world = unsafe { self.world_mut() }; - for child in children.iter() { - if let Some(previous) = update_parent(world, *child, parent) { - remove_from_children(world, previous, *child); - } - } + 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 + .retain(|value| !children.contains(value)); children_component.0.extend(children.iter().cloned()); } else { self.insert(Children::with(children)); @@ -343,17 +416,15 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { { // SAFE: parent entity is not modified and its location is updated manually let world = unsafe { self.world_mut() }; - for child in children.iter() { - if let Some(previous) = update_parent(world, *child, parent) { - remove_from_children(world, previous, *child); - } - } + 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 + .retain(|value| !children.contains(value)); children_component.0.insert_from_slice(index, children); } else { self.insert(Children::with(children)); @@ -365,15 +436,7 @@ impl<'w> BuildWorldChildren for EntityMut<'w> { let parent = self.id(); // SAFE: This doesn't change the parent's location let world = unsafe { self.world_mut() }; - for child in children.iter() { - world.entity_mut(*child).remove::(); - } - // 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 } } @@ -399,9 +462,7 @@ 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).insert(Parent(parent)); - } + update_old_parents(self.world, parent, children); if let Some(mut children_component) = self.world.get_mut::(parent) { children_component.0.extend(children.iter().cloned()); } else { @@ -416,10 +477,7 @@ 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).insert(Parent(parent)); - } + update_old_parents(self.world, parent, children); if let Some(mut children_component) = self.world.get_mut::(parent) { children_component.0.insert_from_slice(index, children); } else { diff --git a/crates/bevy_hierarchy/src/events.rs b/crates/bevy_hierarchy/src/events.rs new file mode 100644 index 0000000000000..85e006bd8ad7b --- /dev/null +++ b/crates/bevy_hierarchy/src/events.rs @@ -0,0 +1,20 @@ +use bevy_ecs::prelude::Entity; + +#[derive(Clone)] +pub struct ChildAdded { + pub parent: Entity, + pub child: Entity, +} + +#[derive(Clone)] +pub struct ChildRemoved { + pub parent: Entity, + pub child: Entity, +} + +#[derive(Clone)] +pub struct ChildMoved { + pub child: Entity, + pub previous_parent: Entity, + pub new_parent: Entity, +} diff --git a/crates/bevy_hierarchy/src/lib.rs b/crates/bevy_hierarchy/src/lib.rs index 600ce2e21e63c..908b79898f221 100644 --- a/crates/bevy_hierarchy/src/lib.rs +++ b/crates/bevy_hierarchy/src/lib.rs @@ -13,6 +13,9 @@ pub use hierarchy::*; mod child_builder; pub use child_builder::*; +mod events; +pub use events::*; + #[doc(hidden)] pub mod prelude { #[doc(hidden)] @@ -29,6 +32,9 @@ pub struct HierarchyPlugin; impl Plugin for HierarchyPlugin { fn build(&self, app: &mut App) { app.register_type::() - .register_type::(); + .register_type::() + .add_event::() + .add_event::() + .add_event::(); } } diff --git a/crates/bevy_transform/src/hierarchy/mod.rs b/crates/bevy_transform/src/hierarchy/mod.rs deleted file mode 100644 index ab3609b624fec..0000000000000 --- a/crates/bevy_transform/src/hierarchy/mod.rs +++ /dev/null @@ -1,6 +0,0 @@ -mod child_builder; -#[allow(clippy::module_inception)] -mod hierarchy; - -pub use child_builder::*; -pub use hierarchy::*; \ No newline at end of file diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index 9007aeb50b338..1488278398d44 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -91,16 +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), + systems::transform_propagate_system.label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::transform_propagate_system - .label(TransformSystem::TransformPropagate), + systems::transform_propagate_system.label(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 66431649ba513..4b870e55d55f6 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -85,9 +85,7 @@ mod test { use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; use crate::TransformBundle; - use bevy_hierarchy::{ - BuildChildren, BuildWorldChildren, Children, Parent, - }; + use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; #[test] fn did_propagate() { From 42d0ad328b0e49b9c6ec95d0e3b474f27afba889 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 04:19:44 -0700 Subject: [PATCH 07/21] Document new events --- crates/bevy_hierarchy/src/events.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/bevy_hierarchy/src/events.rs b/crates/bevy_hierarchy/src/events.rs index 85e006bd8ad7b..2fcb827a3b6d3 100644 --- a/crates/bevy_hierarchy/src/events.rs +++ b/crates/bevy_hierarchy/src/events.rs @@ -1,20 +1,33 @@ use bevy_ecs::prelude::Entity; +/// An event that is fired whenever an [`Entity`] is added as a child +/// to a new parent. #[derive(Clone)] pub struct ChildAdded { - pub parent: Entity, + /// The child that added pub child: Entity, + /// The parent the child was added to + pub parent: Entity, } +/// An event that is fired whenever an child [`Entity`] is removed from +/// to parent. #[derive(Clone)] pub struct ChildRemoved { - pub parent: Entity, + /// The child that removed pub child: Entity, + /// The parent the child was removed from + pub parent: Entity, } +/// An event that is fired whenever an child [`Entity`] is moved to +/// a new parent. #[derive(Clone)] pub struct ChildMoved { + /// The child that moved pub child: Entity, + /// The parent the child was removed from pub previous_parent: Entity, + /// The parent the child was added to pub new_parent: Entity, } From 70a96fcdee290cf42d5f9430cbd48b39adfb2ae9 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 04:21:31 -0700 Subject: [PATCH 08/21] Remove return type --- crates/bevy_hierarchy/src/child_builder.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index eacd57e216b83..3379acafd7bee 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -60,19 +60,19 @@ fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option } } -fn remove_from_children(world: &mut World, parent: Entity, child: Entity) -> Option { +fn remove_from_children(world: &mut World, parent: Entity, child: Entity) { let mut remove = false; - let mut parent = world.get_entity_mut(parent)?; - if let Some(mut children) = parent.get_mut::() { - if let Some(idx) = children.iter().position(|x| *x == child) { - children.0.remove(idx); - remove = children.is_empty(); + if let Some(mut parent) = world.get_entity_mut(parent) { + if let Some(mut children) = parent.get_mut::() { + if let Some(idx) = children.iter().position(|x| *x == child) { + children.0.remove(idx); + remove = children.is_empty(); + } + } + if remove { + parent.remove::(); } } - if remove { - parent.remove::(); - } - Some(parent.id()) } fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { From 4d7dea3358e35b3bca057cbba4d9f768d516bf9e Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 04:33:16 -0700 Subject: [PATCH 09/21] Panic if invariant broken --- crates/bevy_hierarchy/src/child_builder.rs | 34 ++++++++++++---------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 3379acafd7bee..ebfb42c03361b 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -39,12 +39,11 @@ fn push_add_events(world: &mut World, parent: Entity, children: &[Entity]) { } fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) { - if let Some(mut parent) = world.get_entity_mut(parent) { - if let Some(mut children) = parent.get_mut::() { - children.0.push(child); - } else { - parent.insert(Children(smallvec::smallvec![child])); - } + 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])); } } @@ -62,17 +61,16 @@ fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option fn remove_from_children(world: &mut World, parent: Entity, child: Entity) { let mut remove = false; - if let Some(mut parent) = world.get_entity_mut(parent) { - if let Some(mut children) = parent.get_mut::() { - if let Some(idx) = children.iter().position(|x| *x == child) { - children.0.remove(idx); - remove = children.is_empty(); - } - } - if remove { - parent.remove::(); + let mut parent = world.entity_mut(parent); + if let Some(mut children) = parent.get_mut::() { + if let Some(idx) = children.iter().position(|x| *x == child) { + children.0.remove(idx); + remove = children.is_empty(); } } + if remove { + parent.remove::(); + } } fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { @@ -464,6 +462,9 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { .expect("Cannot add children without a parent. Try creating an entity first."); 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 @@ -479,6 +480,9 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { .expect("Cannot add children without a parent. Try creating an entity first."); 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 From 88958a14827ec1e2d547aaa60ecfdf1f8edd8997 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 22:34:11 -0700 Subject: [PATCH 10/21] fix up CI --- crates/bevy_hierarchy/src/child_builder.rs | 27 ++++++++++++++++++++-- crates/bevy_hierarchy/src/hierarchy.rs | 8 ++++++- crates/bevy_transform/src/systems.rs | 11 ++++++++- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index ebfb42c03361b..563b807005161 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -323,7 +323,11 @@ impl<'w> WorldChildBuilder<'w> { /// Spawns an entity with the given bundle and inserts it into the children defined by the [`WorldChildBuilder`] pub fn spawn_bundle(&mut self, bundle: impl Bundle + Send + Sync + 'static) -> EntityMut<'_> { let parent_entity = self.parent_entity(); - let entity = self.world.spawn().insert_bundle(bundle).id(); + let entity = self.world + .spawn() + .insert_bundle(bundle) + .insert(Parent(parent_entity)) + .id(); push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); self.world @@ -505,10 +509,11 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { #[cfg(test)] mod tests { use super::{BuildChildren, BuildWorldChildren}; - use crate::prelude::{Children, Parent}; + use crate::prelude::{ChildAdded, ChildMoved, ChildRemoved, Children, Parent}; use bevy_ecs::{ component::Component, entity::Entity, + event::Events, system::{CommandQueue, Commands}, world::World, }; @@ -520,6 +525,11 @@ mod tests { #[test] fn build_children() { let mut world = World::default(); + + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let mut queue = CommandQueue::default(); let mut commands = Commands::new(&mut queue, &world); @@ -547,6 +557,10 @@ mod tests { fn push_and_insert_and_remove_children_commands() { let mut world = World::default(); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let entities = world .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) .collect::>(); @@ -611,6 +625,10 @@ mod tests { fn push_and_insert_and_remove_children_world() { let mut world = World::default(); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let entities = world .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) .collect::>(); @@ -659,6 +677,11 @@ mod tests { #[test] fn regression_push_children_same_archetype() { let mut world = World::new(); + + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let child = world.spawn().id(); world.spawn().push_children(&[child]); } diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 56c9b0f8ad938..98a86761a123e 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -111,12 +111,13 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> { mod tests { use bevy_ecs::{ component::Component, + event::Events, system::{CommandQueue, Commands}, world::World, }; use super::DespawnRecursiveExt; - use crate::{child_builder::BuildChildren, components::Children}; + use crate::{child_builder::BuildChildren, components::Children, ChildAdded, ChildMoved, ChildRemoved}; #[derive(Component, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Debug)] struct Idx(u32); @@ -127,6 +128,11 @@ mod tests { #[test] fn despawn_recursive() { let mut world = World::default(); + + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let mut queue = CommandQueue::default(); let grandparent_entity; { diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 4b870e55d55f6..ee45de99e00aa 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -77,6 +77,7 @@ fn propagate_recursive( #[cfg(test)] mod test { use bevy_ecs::{ + event::Events, schedule::{Schedule, Stage, SystemStage}, system::{CommandQueue, Commands}, world::World, @@ -85,12 +86,16 @@ mod test { use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; use crate::TransformBundle; - use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; + use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent, ChildAdded, ChildMoved, ChildRemoved}; #[test] fn did_propagate() { let mut world = World::default(); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let mut update_stage = SystemStage::parallel(); update_stage.add_system(transform_propagate_system); @@ -135,6 +140,10 @@ mod test { fn did_propagate_command_buffer() { let mut world = World::default(); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let mut update_stage = SystemStage::parallel(); update_stage.add_system(transform_propagate_system); From 51368c1f76bdb91d409bb3ce206463dabd0e0085 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 22:40:38 -0700 Subject: [PATCH 11/21] fix formatting --- crates/bevy_hierarchy/src/child_builder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 563b807005161..8682286f534f5 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -323,7 +323,8 @@ impl<'w> WorldChildBuilder<'w> { /// Spawns an entity with the given bundle and inserts it into the children defined by the [`WorldChildBuilder`] pub fn spawn_bundle(&mut self, bundle: impl Bundle + Send + Sync + 'static) -> EntityMut<'_> { let parent_entity = self.parent_entity(); - let entity = self.world + let entity = self + .world .spawn() .insert_bundle(bundle) .insert(Parent(parent_entity)) From 01b0d48ee40790318f39cf9c31bcc5150e167790 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 23:02:41 -0700 Subject: [PATCH 12/21] Fix UI tests --- crates/bevy_ui/src/update.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ui/src/update.rs b/crates/bevy_ui/src/update.rs index 8af7d72caca62..097ff1994e5d8 100644 --- a/crates/bevy_ui/src/update.rs +++ b/crates/bevy_ui/src/update.rs @@ -135,11 +135,12 @@ fn update_clipping( mod tests { use bevy_ecs::{ component::Component, + event::Events, schedule::{Schedule, Stage, SystemStage}, system::{CommandQueue, Commands}, world::World, }; - use bevy_hierarchy::BuildChildren; + use bevy_hierarchy::{BuildChildren, ChildAdded, ChildMoved, ChildRemoved}; use bevy_transform::components::Transform; use crate::Node; @@ -164,6 +165,11 @@ mod tests { #[test] fn test_ui_z_system() { let mut world = World::default(); + + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + world.insert_resource(Events::::default()); + let mut queue = CommandQueue::default(); let mut commands = Commands::new(&mut queue, &world); commands.spawn_bundle(node_with_transform("0")); From c07b2ee7b4a1155b77425328180592fab654eec9 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 23:25:03 -0700 Subject: [PATCH 13/21] Remove copying/constructing traits from Parent/Children --- crates/bevy_hierarchy/src/child_builder.rs | 4 ++-- crates/bevy_hierarchy/src/components/children.rs | 14 +++++++++++++- crates/bevy_hierarchy/src/components/parent.rs | 6 +++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 8682286f534f5..4a67ad1729ca1 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -50,9 +50,9 @@ fn push_child_unchecked(world: &mut World, parent: Entity, child: Entity) { 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; + let previous = parent.0; *parent = Parent(new_parent); - Some(*previous) + Some(previous) } else { child.insert(Parent(new_parent)); None diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index 96097a1da8a59..4a27ba446c5b7 100644 --- a/crates/bevy_hierarchy/src/components/children.rs +++ b/crates/bevy_hierarchy/src/components/children.rs @@ -1,14 +1,16 @@ use bevy_ecs::{ component::Component, entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + prelude::FromWorld, reflect::{ReflectComponent, ReflectMapEntities}, + world::World, }; use bevy_reflect::Reflect; 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]>); @@ -22,6 +24,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/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index b42ef77d6f0e9..f3605f379cd52 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -9,12 +9,12 @@ 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(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. +// 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 { From e58b0870f1a8010b423fce4a6360cea5295b52f2 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 13 Mar 2022 23:51:21 -0700 Subject: [PATCH 14/21] Scenes are Worlds too --- crates/bevy_hierarchy/src/child_builder.rs | 89 ++++++++----------- .../bevy_hierarchy/src/components/children.rs | 2 +- crates/bevy_hierarchy/src/hierarchy.rs | 10 +-- crates/bevy_transform/src/systems.rs | 18 ++-- crates/bevy_ui/src/update.rs | 6 -- 5 files changed, 49 insertions(+), 76 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 4a67ad1729ca1..9a0a467d52b65 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -12,8 +12,7 @@ use bevy_ecs::{ use smallvec::SmallVec; fn push_move_events(world: &mut World, events: SmallVec<[ChildMoved; 8]>) { - { - let mut removed = world.resource_mut::>(); + if let Some(mut removed) = world.get_resource_mut::>() { for evt in events.iter() { removed.send(ChildRemoved { child: evt.child, @@ -22,19 +21,21 @@ fn push_move_events(world: &mut World, events: SmallVec<[ChildMoved; 8]>) { } } - let mut moved_events = world.resource_mut::>(); - for evt in events { - moved_events.send(evt); + if let Some(mut moved) = world.get_resource_mut::>() { + for evt in events { + moved.send(evt); + } } } fn push_add_events(world: &mut World, parent: Entity, children: &[Entity]) { - let mut added = world.resource_mut::>(); - for child in children.iter() { - added.send(ChildAdded { - child: *child, - parent, - }); + if let Some(mut added) = world.get_resource_mut::>() { + for child in children.iter() { + added.send(ChildAdded { + child: *child, + parent, + }); + } } } @@ -106,17 +107,19 @@ impl Command for AddChild { } if let Some(previous) = previous { remove_from_children(world, previous, self.child); - world - .resource_mut::>() - .send(ChildRemoved { + if let Some(mut removed) = world.get_resource_mut::>() { + removed.send(ChildRemoved { child: self.child, parent: previous, }); - world.resource_mut::>().send(ChildMoved { - child: self.child, - previous_parent: previous, - new_parent: self.parent, - }); + } + if let Some(mut removed) = world.get_resource_mut::>() { + removed.send(ChildMoved { + child: self.child, + previous_parent: previous, + new_parent: self.parent, + }); + } } let mut parent = world.entity_mut(self.parent); if let Some(mut children) = parent.get_mut::() { @@ -126,10 +129,12 @@ impl Command for AddChild { } else { parent.insert(Children(smallvec::smallvec![self.child])); } - world.resource_mut::>().send(ChildAdded { - child: self.child, - parent: self.parent, - }); + if let Some(mut added) = world.get_resource_mut::>() { + added.send(ChildAdded { + child: self.child, + parent: self.parent, + }); + } } } @@ -190,8 +195,7 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { }); } - { - let mut removed = world.resource_mut::>(); + if let Some(mut removed) = world.get_resource_mut::>() { for evt in events { removed.send(evt); } @@ -331,12 +335,12 @@ impl<'w> WorldChildBuilder<'w> { .id(); push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); - self.world - .resource_mut::>() - .send(ChildAdded { + if let Some(mut added) = self.world.get_resource_mut::>() { + added.send(ChildAdded { child: entity, parent: parent_entity, }); + } self.world.entity_mut(entity) } @@ -346,12 +350,12 @@ impl<'w> WorldChildBuilder<'w> { let entity = self.world.spawn().insert(Parent(parent_entity)).id(); push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); - self.world - .resource_mut::>() - .send(ChildAdded { + if let Some(mut added) = self.world.get_resource_mut::>() { + added.send(ChildAdded { child: entity, parent: parent_entity, }); + } self.world.entity_mut(entity) } @@ -510,11 +514,10 @@ impl<'w> BuildWorldChildren for WorldChildBuilder<'w> { #[cfg(test)] mod tests { use super::{BuildChildren, BuildWorldChildren}; - use crate::prelude::{ChildAdded, ChildMoved, ChildRemoved, Children, Parent}; + use crate::prelude::{Children, Parent}; use bevy_ecs::{ component::Component, entity::Entity, - event::Events, system::{CommandQueue, Commands}, world::World, }; @@ -526,11 +529,6 @@ mod tests { #[test] fn build_children() { let mut world = World::default(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let mut queue = CommandQueue::default(); let mut commands = Commands::new(&mut queue, &world); @@ -557,11 +555,6 @@ mod tests { #[test] fn push_and_insert_and_remove_children_commands() { let mut world = World::default(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let entities = world .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) .collect::>(); @@ -625,11 +618,6 @@ mod tests { #[test] fn push_and_insert_and_remove_children_world() { let mut world = World::default(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let entities = world .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) .collect::>(); @@ -678,11 +666,6 @@ mod tests { #[test] fn regression_push_children_same_archetype() { let mut world = World::new(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let child = world.spawn().id(); world.spawn().push_children(&[child]); } diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index 4a27ba446c5b7..edf3ec201d924 100644 --- a/crates/bevy_hierarchy/src/components/children.rs +++ b/crates/bevy_hierarchy/src/components/children.rs @@ -26,7 +26,7 @@ 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 +// 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 { diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 98a86761a123e..3916804ac61ed 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -111,13 +111,16 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> { mod tests { use bevy_ecs::{ component::Component, - event::Events, system::{CommandQueue, Commands}, world::World, }; use super::DespawnRecursiveExt; +<<<<<<< HEAD:crates/bevy_hierarchy/src/hierarchy.rs use crate::{child_builder::BuildChildren, components::Children, ChildAdded, ChildMoved, ChildRemoved}; +======= + use crate::{components::Children, hierarchy::BuildChildren}; +>>>>>>> e7ef4a51 (Scenes are Worlds too):crates/bevy_transform/src/hierarchy/hierarchy.rs #[derive(Component, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Debug)] struct Idx(u32); @@ -128,11 +131,6 @@ mod tests { #[test] fn despawn_recursive() { let mut world = World::default(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let mut queue = CommandQueue::default(); let grandparent_entity; { diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index ee45de99e00aa..457b41f8c04af 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -77,25 +77,28 @@ fn propagate_recursive( #[cfg(test)] mod test { use bevy_ecs::{ - event::Events, schedule::{Schedule, Stage, SystemStage}, system::{CommandQueue, Commands}, world::World, }; +<<<<<<< HEAD:crates/bevy_transform/src/systems.rs use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; use crate::TransformBundle; use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent, ChildAdded, ChildMoved, ChildRemoved}; +======= + use super::*; + use crate::{ + hierarchy::{BuildChildren, BuildWorldChildren}, + TransformBundle, + }; +>>>>>>> e7ef4a51 (Scenes are Worlds too):crates/bevy_transform/src/transform_propagate_system.rs #[test] fn did_propagate() { let mut world = World::default(); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let mut update_stage = SystemStage::parallel(); update_stage.add_system(transform_propagate_system); @@ -139,11 +142,6 @@ mod test { #[test] fn did_propagate_command_buffer() { let mut world = World::default(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let mut update_stage = SystemStage::parallel(); update_stage.add_system(transform_propagate_system); diff --git a/crates/bevy_ui/src/update.rs b/crates/bevy_ui/src/update.rs index 097ff1994e5d8..4d5e075a7afdc 100644 --- a/crates/bevy_ui/src/update.rs +++ b/crates/bevy_ui/src/update.rs @@ -135,7 +135,6 @@ fn update_clipping( mod tests { use bevy_ecs::{ component::Component, - event::Events, schedule::{Schedule, Stage, SystemStage}, system::{CommandQueue, Commands}, world::World, @@ -165,11 +164,6 @@ mod tests { #[test] fn test_ui_z_system() { let mut world = World::default(); - - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - world.insert_resource(Events::::default()); - let mut queue = CommandQueue::default(); let mut commands = Commands::new(&mut queue, &world); commands.spawn_bundle(node_with_transform("0")); From 82febea5bade0b63562798e0a7724877754958d3 Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 15 Mar 2022 02:22:03 -0700 Subject: [PATCH 15/21] fix rebase errors --- .../bevy_hierarchy/src/components/parent.rs | 7 ++ crates/bevy_hierarchy/src/hierarchy.rs | 6 +- crates/bevy_hierarchy/src/lib.rs | 1 - crates/bevy_transform/src/systems.rs | 68 +++++++++---------- crates/bevy_ui/src/update.rs | 2 +- 5 files changed, 42 insertions(+), 42 deletions(-) diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index f3605f379cd52..83e40ac855047 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -13,6 +13,13 @@ use std::ops::Deref; #[reflect(Component, MapEntities, PartialEq)] pub struct Parent(pub(crate) Entity); +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 diff --git a/crates/bevy_hierarchy/src/hierarchy.rs b/crates/bevy_hierarchy/src/hierarchy.rs index 3916804ac61ed..56c9b0f8ad938 100644 --- a/crates/bevy_hierarchy/src/hierarchy.rs +++ b/crates/bevy_hierarchy/src/hierarchy.rs @@ -116,11 +116,7 @@ mod tests { }; use super::DespawnRecursiveExt; -<<<<<<< HEAD:crates/bevy_hierarchy/src/hierarchy.rs - use crate::{child_builder::BuildChildren, components::Children, ChildAdded, ChildMoved, ChildRemoved}; -======= - use crate::{components::Children, hierarchy::BuildChildren}; ->>>>>>> e7ef4a51 (Scenes are Worlds too):crates/bevy_transform/src/hierarchy/hierarchy.rs + use crate::{child_builder::BuildChildren, components::Children}; #[derive(Component, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Debug)] struct Idx(u32); diff --git a/crates/bevy_hierarchy/src/lib.rs b/crates/bevy_hierarchy/src/lib.rs index 908b79898f221..1cbb2257450b3 100644 --- a/crates/bevy_hierarchy/src/lib.rs +++ b/crates/bevy_hierarchy/src/lib.rs @@ -23,7 +23,6 @@ pub mod prelude { } use bevy_app::prelude::*; -use bevy_ecs::prelude::*; /// The base plugin for handling [`Parent`] and [`Children`] components #[derive(Default)] diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 457b41f8c04af..214af65ff44da 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -82,18 +82,10 @@ mod test { world::World, }; -<<<<<<< HEAD:crates/bevy_transform/src/systems.rs use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; use crate::TransformBundle; - use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent, ChildAdded, ChildMoved, ChildRemoved}; -======= - use super::*; - use crate::{ - hierarchy::{BuildChildren, BuildWorldChildren}, - TransformBundle, - }; ->>>>>>> e7ef4a51 (Scenes are Worlds too):crates/bevy_transform/src/transform_propagate_system.rs + use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children}; #[test] fn did_propagate() { @@ -185,36 +177,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); 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 @@ -227,9 +221,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 diff --git a/crates/bevy_ui/src/update.rs b/crates/bevy_ui/src/update.rs index 4d5e075a7afdc..8af7d72caca62 100644 --- a/crates/bevy_ui/src/update.rs +++ b/crates/bevy_ui/src/update.rs @@ -139,7 +139,7 @@ mod tests { system::{CommandQueue, Commands}, world::World, }; - use bevy_hierarchy::{BuildChildren, ChildAdded, ChildMoved, ChildRemoved}; + use bevy_hierarchy::BuildChildren; use bevy_transform::components::Transform; use crate::Node; From b914610f149e11426aab2bb664b4e89ee854b289 Mon Sep 17 00:00:00 2001 From: james7132 Date: Tue, 24 May 2022 02:51:35 -0700 Subject: [PATCH 16/21] Update to reflect changes to the RFC --- crates/bevy_animation/src/lib.rs | 3 +- crates/bevy_hierarchy/src/child_builder.rs | 75 ++++++---------------- crates/bevy_hierarchy/src/events.rs | 59 +++++++++-------- crates/bevy_hierarchy/src/lib.rs | 4 +- 4 files changed, 52 insertions(+), 89 deletions(-) diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs index 0251d69948289..1c8143135f114 100644 --- a/crates/bevy_animation/src/lib.rs +++ b/crates/bevy_animation/src/lib.rs @@ -294,8 +294,7 @@ impl Plugin for AnimationPlugin { .register_type::() .add_system_to_stage( CoreStage::PostUpdate, - animation_player - .before(TransformSystem::TransformPropagate), + animation_player.before(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 12f4ef47b152c..650586a208e80 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -1,8 +1,7 @@ use crate::{ prelude::{Children, Parent}, - ChildAdded, ChildMoved, ChildRemoved, + HierarchyEvent, }; -use smallvec::SmallVec; use bevy_ecs::{ bundle::Bundle, entity::Entity, @@ -10,35 +9,16 @@ use bevy_ecs::{ system::{Command, Commands, EntityCommands}, world::{EntityMut, World}, }; +use smallvec::SmallVec; -fn push_move_events(world: &mut World, events: SmallVec<[ChildMoved; 8]>) { - if let Some(mut removed) = world.get_resource_mut::>() { - for evt in events.iter() { - removed.send(ChildRemoved { - child: evt.child, - parent: evt.previous_parent, - }); - } - } - - if let Some(mut moved) = world.get_resource_mut::>() { +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_add_events(world: &mut World, parent: Entity, children: &[Entity]) { - if let Some(mut added) = world.get_resource_mut::>() { - for child in children.iter() { - added.send(ChildAdded { - child: *child, - parent, - }); - } - } -} - 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::() { @@ -75,19 +55,18 @@ fn remove_from_children(world: &mut World, parent: Entity, child: Entity) { } fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { - let mut moved: SmallVec<[ChildMoved; 8]> = SmallVec::with_capacity(children.len()); + let mut moved: SmallVec<[HierarchyEvent; 8]> = SmallVec::with_capacity(children.len()); for child in children.iter() { if let Some(previous) = update_parent(world, *child, parent) { remove_from_children(world, previous, *child); - moved.push(ChildMoved { + moved.push(HierarchyEvent::ChildMoved { child: *child, previous_parent: previous, new_parent: parent, }); } } - push_move_events(world, moved); - push_add_events(world, parent, children); + push_events(world, moved); } /// Command that adds a child to an entity @@ -107,19 +86,18 @@ impl Command for AddChild { } if let Some(previous) = previous { remove_from_children(world, previous, self.child); - if let Some(mut removed) = world.get_resource_mut::>() { - removed.send(ChildRemoved { - child: self.child, - parent: previous, - }); - } - if let Some(mut removed) = world.get_resource_mut::>() { - removed.send(ChildMoved { + 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::() { @@ -129,12 +107,6 @@ impl Command for AddChild { } else { parent.insert(Children(smallvec::smallvec![self.child])); } - if let Some(mut added) = world.get_resource_mut::>() { - added.send(ChildAdded { - child: self.child, - parent: self.parent, - }); - } } } @@ -186,20 +158,15 @@ pub struct RemoveChildren { } fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { - let mut events: SmallVec<[ChildRemoved; 8]> = SmallVec::new(); + let mut events: SmallVec<[HierarchyEvent; 8]> = SmallVec::new(); for child in children.iter() { world.entity_mut(*child).remove::(); - events.push(ChildRemoved { + events.push(HierarchyEvent::ChildRemoved { child: *child, parent, }); } - - if let Some(mut removed) = world.get_resource_mut::>() { - for evt in events { - removed.send(evt); - } - } + push_events(world, events); if let Some(mut parent_children) = world.get_mut::(parent) { parent_children @@ -372,8 +339,8 @@ impl<'w> WorldChildBuilder<'w> { .id(); push_child_unchecked(self.world, parent_entity, entity); self.current_entity = Some(entity); - if let Some(mut added) = self.world.get_resource_mut::>() { - added.send(ChildAdded { + if let Some(mut added) = self.world.get_resource_mut::>() { + added.send(HierarchyEvent::ChildAdded { child: entity, parent: parent_entity, }); @@ -387,8 +354,8 @@ impl<'w> WorldChildBuilder<'w> { 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 added) = self.world.get_resource_mut::>() { - added.send(ChildAdded { + if let Some(mut added) = self.world.get_resource_mut::>() { + added.send(HierarchyEvent::ChildAdded { child: entity, parent: parent_entity, }); diff --git a/crates/bevy_hierarchy/src/events.rs b/crates/bevy_hierarchy/src/events.rs index 2fcb827a3b6d3..5fd3555de9470 100644 --- a/crates/bevy_hierarchy/src/events.rs +++ b/crates/bevy_hierarchy/src/events.rs @@ -1,33 +1,32 @@ use bevy_ecs::prelude::Entity; -/// An event that is fired whenever an [`Entity`] is added as a child -/// to a new parent. -#[derive(Clone)] -pub struct ChildAdded { - /// The child that added - pub child: Entity, - /// The parent the child was added to - pub parent: Entity, -} - -/// An event that is fired whenever an child [`Entity`] is removed from -/// to parent. -#[derive(Clone)] -pub struct ChildRemoved { - /// The child that removed - pub child: Entity, - /// The parent the child was removed from - pub parent: Entity, -} - -/// An event that is fired whenever an child [`Entity`] is moved to -/// a new parent. -#[derive(Clone)] -pub struct ChildMoved { - /// The child that moved - pub child: Entity, - /// The parent the child was removed from - pub previous_parent: Entity, - /// The parent the child was added to - pub new_parent: 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 1cbb2257450b3..60d55f7b8c7ee 100644 --- a/crates/bevy_hierarchy/src/lib.rs +++ b/crates/bevy_hierarchy/src/lib.rs @@ -32,8 +32,6 @@ impl Plugin for HierarchyPlugin { fn build(&self, app: &mut App) { app.register_type::() .register_type::() - .add_event::() - .add_event::() - .add_event::(); + .add_event::(); } } From 192789d42cabd003807197e6c476e5e5d810c8a6 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 30 May 2022 15:39:22 -0700 Subject: [PATCH 17/21] Fix CI --- crates/bevy_hierarchy/src/child_builder.rs | 4 -- crates/bevy_transform/src/systems.rs | 66 +++++----------------- examples/animation/gltf_skinned_mesh.rs | 2 +- 3 files changed, 14 insertions(+), 58 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 650586a208e80..8145f86ba1aa7 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -528,10 +528,6 @@ mod tests { world::World, }; - use crate::prelude::{Children, Parent, PreviousParent}; - - use super::{BuildChildren, BuildWorldChildren}; - #[derive(Component)] struct C(u32); diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 65510187fa7f1..5a7e2439a2e9e 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -286,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); 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(); @@ -337,36 +329,4 @@ mod test { ); } } - #[test] - #[should_panic] - fn panic_when_hierarchy_cycle() { - let mut app = App::new(); - - app.add_system(parent_update_system); - app.add_system(transform_propagate_system); - - let child = app - .world - .spawn() - .insert_bundle((Transform::identity(), GlobalTransform::default())) - .id(); - - let grandchild = app - .world - .spawn() - .insert_bundle(( - Transform::identity(), - GlobalTransform::default(), - Parent(child), - )) - .id(); - app.world.spawn().insert_bundle(( - Transform::default(), - GlobalTransform::default(), - Children::with(&[child]), - )); - app.world.entity_mut(child).insert(Parent(grandchild)); - - app.update(); - } } diff --git a/examples/animation/gltf_skinned_mesh.rs b/examples/animation/gltf_skinned_mesh.rs index 7cb8f90d113c3..6fc2bf77e1626 100644 --- a/examples/animation/gltf_skinned_mesh.rs +++ b/examples/animation/gltf_skinned_mesh.rs @@ -49,7 +49,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(); From cfeb242ff6aa592c9ab7d102d09f0d6c8f5aedc9 Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 22 Jun 2022 22:02:41 -0700 Subject: [PATCH 18/21] Review comments and some code cleanup --- crates/bevy_hierarchy/src/child_builder.rs | 55 +++++++++++----------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 8145f86ba1aa7..ed7b4d0f530a9 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -41,23 +41,23 @@ fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option } fn remove_from_children(world: &mut World, parent: Entity, child: Entity) { - let mut remove = false; let mut parent = world.entity_mut(parent); - if let Some(mut children) = parent.get_mut::() { - if let Some(idx) = children.iter().position(|x| *x == child) { - children.0.remove(idx); - remove = children.is_empty(); - } - } - if remove { + let empty = if let Some(mut children) = parent.get_mut::() { + children.0.retain(|x| *x != child); + children.0.is_empty() + } else { + return; + }; + if 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.iter() { + 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, @@ -69,6 +69,25 @@ fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { 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.iter() { + 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)] pub struct AddChild { @@ -157,24 +176,6 @@ pub struct RemoveChildren { children: SmallVec<[Entity; 8]>, } -fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { - let mut events: SmallVec<[HierarchyEvent; 8]> = SmallVec::new(); - for child in children.iter() { - 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)); - } -} - impl Command for RemoveChildren { fn write(self, world: &mut World) { remove_children(self.parent, &self.children, world); From e15702b005cf448c4d3191e8cd13f6669c433f9d Mon Sep 17 00:00:00 2001 From: james7132 Date: Wed, 22 Jun 2022 22:24:32 -0700 Subject: [PATCH 19/21] Formatting --- crates/bevy_hierarchy/src/child_builder.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index ed7b4d0f530a9..bfea5cf317f1c 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -87,7 +87,6 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { } } - /// Command that adds a child to an entity #[derive(Debug)] pub struct AddChild { From cc5244a2ef4d159ba79eb40cd61925a63ca72dbf Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 23 Jun 2022 01:35:04 -0700 Subject: [PATCH 20/21] Some code cleanup --- crates/bevy_hierarchy/src/child_builder.rs | 25 ++++++++++------------ 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index bfea5cf317f1c..2977d318a66ed 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -42,14 +42,11 @@ fn update_parent(world: &mut World, child: Entity, new_parent: Entity) -> Option fn remove_from_children(world: &mut World, parent: Entity, child: Entity) { let mut parent = world.entity_mut(parent); - let empty = if let Some(mut children) = parent.get_mut::() { + if let Some(mut children) = parent.get_mut::() { children.0.retain(|x| *x != child); - children.0.is_empty() - } else { - return; - }; - if empty { - parent.remove::(); + if children.is_empty() { + parent.remove::(); + } } } @@ -71,7 +68,7 @@ fn update_old_parents(world: &mut World, parent: Entity, children: &[Entity]) { fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { let mut events: SmallVec<[HierarchyEvent; 8]> = SmallVec::new(); - for child in children.iter() { + for child in children { world.entity_mut(*child).remove::(); events.push(HierarchyEvent::ChildRemoved { child: *child, @@ -99,10 +96,10 @@ pub struct AddChild { impl Command for AddChild { fn write(self, world: &mut World) { let previous = update_parent(world, self.child, self.parent); - if previous == Some(self.parent) { - return; - } 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 { @@ -119,7 +116,7 @@ impl Command for AddChild { } let mut parent = world.entity_mut(self.parent); if let Some(mut children) = parent.get_mut::() { - if children.iter().any(|x| *x != self.child) { + if !children.contains(&self.child) { children.0.push(self.child); } } else { @@ -157,12 +154,12 @@ pub struct PushChildren { } impl Command for PushChildren { - fn write(self, world: &mut World) { + 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.extend(self.children.iter().cloned()); + children.0.append(&mut self.children); } else { parent.insert(Children(self.children)); } From 456c0eaa88e7df14035b52d182e8e65aad35ac53 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 9 Jul 2022 19:14:17 -0700 Subject: [PATCH 21/21] Add back malformed hierarchy test --- crates/bevy_transform/src/systems.rs | 45 +++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 5a7e2439a2e9e..d87fa5e7e812a 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -103,7 +103,7 @@ mod test { use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; use crate::TransformBundle; - use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children}; + use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; #[test] fn did_propagate() { @@ -329,4 +329,47 @@ mod test { ); } } + + #[test] + #[should_panic] + fn panic_when_hierarchy_cycle() { + // 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(); + + app.add_system(transform_propagate_system); + + 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::default(), GlobalTransform::default())) + .push_children(&[child]); + std::mem::swap( + &mut *app.world.get_mut::(child).unwrap(), + &mut *temp.get_mut::(grandchild).unwrap(), + ); + + app.update(); + } }