From d79339ea62718c63973cae6e679a363956b8b6fc Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Thu, 23 Jul 2020 17:32:53 -0700 Subject: [PATCH] transform: add more control parent/child ordering and make parent/children insertion atomic --- crates/bevy_ecs/src/system/commands.rs | 40 ++- crates/bevy_ecs/src/world/world_builder.rs | 2 +- .../bevy_transform/src/components/parent.rs | 12 +- .../src/hierarchy/child_builder.rs | 292 ++++++++++++++++-- .../src/hierarchy/world_child_builder.rs | 23 +- crates/bevy_transform/src/lib.rs | 3 +- 6 files changed, 338 insertions(+), 34 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands.rs index f715759ba8..65f379e4ac 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands.rs @@ -177,6 +177,21 @@ impl CommandsInternal { }))); self } + + pub fn write_world(&mut self, world_writer: W) -> &mut Self { + self.commands + .push(Command::WriteWorld(Box::new(world_writer))); + self + } + + pub fn write_resources( + &mut self, + resources_writer: W, + ) -> &mut Self { + self.commands + .push(Command::WriteResources(Box::new(resources_writer))); + self + } } #[derive(Default, Clone)] @@ -260,11 +275,7 @@ impl Commands { } pub fn write_world(&mut self, world_writer: W) -> &mut Self { - self.commands - .lock() - .unwrap() - .commands - .push(Command::WriteWorld(Box::new(world_writer))); + self.commands.lock().unwrap().write_world(world_writer); self } @@ -275,8 +286,7 @@ impl Commands { self.commands .lock() .unwrap() - .commands - .push(Command::WriteResources(Box::new(resources_writer))); + .write_resources(resources_writer); self } @@ -291,6 +301,22 @@ impl Commands { } } } + + pub fn current_entity(&self) -> Option { + let commands = self.commands.lock().unwrap(); + commands.current_entity + } + + pub fn for_current_entity(&mut self, mut func: impl FnMut(Entity)) -> &mut Self { + { + let commands = self.commands.lock().unwrap(); + let current_entity = commands + .current_entity + .expect("The 'current entity' is not set. You should spawn an entity first."); + func(current_entity); + } + self + } } #[cfg(test)] diff --git a/crates/bevy_ecs/src/world/world_builder.rs b/crates/bevy_ecs/src/world/world_builder.rs index a026836726..9891ed275f 100644 --- a/crates/bevy_ecs/src/world/world_builder.rs +++ b/crates/bevy_ecs/src/world/world_builder.rs @@ -14,7 +14,7 @@ impl WorldBuilderSource for World { } pub struct WorldBuilder<'a> { - world: &'a mut World, + pub world: &'a mut World, pub current_entity: Option, } diff --git a/crates/bevy_transform/src/components/parent.rs b/crates/bevy_transform/src/components/parent.rs index c846c06704..ab0a455d87 100644 --- a/crates/bevy_transform/src/components/parent.rs +++ b/crates/bevy_transform/src/components/parent.rs @@ -1,10 +1,20 @@ -use bevy_ecs::Entity; +use bevy_ecs::{FromResources, Entity}; use bevy_property::Properties; use std::ops::{DerefMut, Deref}; #[derive(Debug, Copy, Clone, Eq, PartialEq, Properties)] pub struct Parent(pub Entity); +// TODO: We need to impl either FromResources or Default so Parent can be registered as Properties. +// This is because Properties 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 FromResources for Parent { + fn from_resources(_resources: &bevy_ecs::Resources) -> Self { + Parent(Entity::from_id(u32::MAX)) + } +} + #[derive(Debug, Copy, Clone, Eq, PartialEq)] pub struct PreviousParent(pub Option); diff --git a/crates/bevy_transform/src/hierarchy/child_builder.rs b/crates/bevy_transform/src/hierarchy/child_builder.rs index 66c0f8115e..02d29c492a 100644 --- a/crates/bevy_transform/src/hierarchy/child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/child_builder.rs @@ -1,9 +1,83 @@ -use crate::prelude::{LocalTransform, Parent}; -use bevy_ecs::{Commands, CommandsInternal, Component, DynamicBundle, Entity}; +use crate::prelude::{Children, LocalTransform, Parent, PreviousParent}; +use bevy_ecs::{Commands, CommandsInternal, Component, DynamicBundle, Entity, WorldWriter}; +use smallvec::SmallVec; + +pub struct InsertChildren { + parent: Entity, + children: SmallVec<[Entity; 8]>, + index: usize, +} + +impl WorldWriter for InsertChildren { + fn write(self: Box, world: &mut bevy_ecs::World) { + for child in self.children.iter() { + world + .insert( + *child, + ( + Parent(self.parent), + PreviousParent(None), + LocalTransform::default(), + ), + ) + .unwrap(); + } + { + let mut added = false; + if let Ok(mut children) = world.get_mut::(self.parent) { + children.insert_from_slice(self.index, &self.children); + added = true; + } + + // NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails borrow-checking + if !added { + world + .insert_one(self.parent, Children(SmallVec::from(self.children))) + .unwrap(); + } + } + } +} + +pub struct PushChildren { + parent: Entity, + children: SmallVec<[Entity; 8]>, +} pub struct ChildBuilder<'a> { commands: &'a mut CommandsInternal, - parent_entities: Vec, + push_children: PushChildren, +} + +impl WorldWriter for PushChildren { + fn write(self: Box, world: &mut bevy_ecs::World) { + for child in self.children.iter() { + world + .insert( + *child, + ( + Parent(self.parent), + PreviousParent(None), + LocalTransform::default(), + ), + ) + .unwrap(); + } + { + let mut added = false; + if let Ok(mut children) = world.get_mut::(self.parent) { + children.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 + .insert_one(self.parent, Children(SmallVec::from(self.children))) + .unwrap(); + } + } + } } impl<'a> ChildBuilder<'a> { @@ -16,14 +90,8 @@ impl<'a> ChildBuilder<'a> { entity: Entity, components: impl DynamicBundle + Send + Sync + 'static, ) -> &mut Self { - let parent_entity = self - .parent_entities - .last() - .cloned() - .expect("There should always be a parent at this point."); - self.commands - .spawn_as_entity(entity, components) - .with_bundle((Parent(parent_entity), LocalTransform::default())); + self.commands.spawn_as_entity(entity, components); + self.push_children.children.push(entity); self } @@ -39,10 +107,21 @@ impl<'a> ChildBuilder<'a> { self.commands.with(component); self } + + pub fn for_current_entity(&mut self, mut func: impl FnMut(Entity)) -> &mut Self { + let current_entity = self + .commands + .current_entity + .expect("The 'current entity' is not set. You should spawn an entity first."); + func(current_entity); + self + } } pub trait BuildChildren { fn with_children(&mut self, parent: impl FnMut(&mut ChildBuilder)) -> &mut Self; + fn push_children(&mut self, parent: Entity, children: &[Entity]) -> &mut Self; + fn insert_children(&mut self, parent: Entity, index: usize, children: &[Entity]) -> &mut Self; } impl BuildChildren for Commands { @@ -50,12 +129,42 @@ impl BuildChildren for Commands { { let mut commands = self.commands.lock().unwrap(); let current_entity = commands.current_entity.expect("Cannot add children because the 'current entity' is not set. You should spawn an entity first."); - let mut builder = ChildBuilder { - commands: &mut commands, - parent_entities: vec![current_entity], + commands.current_entity = None; + let push_children = { + let mut builder = ChildBuilder { + commands: &mut commands, + push_children: PushChildren { + children: SmallVec::default(), + parent: current_entity, + }, + }; + parent(&mut builder); + builder.push_children }; - parent(&mut builder); + commands.current_entity = Some(current_entity); + commands.write_world(push_children); + } + self + } + fn push_children(&mut self, parent: Entity, children: &[Entity]) -> &mut Self { + { + let mut commands = self.commands.lock().unwrap(); + commands.write_world(PushChildren { + children: SmallVec::from(children), + parent, + }); + } + self + } + fn insert_children(&mut self, parent: Entity, index: usize, children: &[Entity]) -> &mut Self { + { + let mut commands = self.commands.lock().unwrap(); + commands.write_world(InsertChildren { + children: SmallVec::from(children), + index, + parent, + }); } self } @@ -63,16 +172,155 @@ impl BuildChildren for Commands { impl<'a> BuildChildren for ChildBuilder<'a> { fn with_children(&mut self, mut spawn_children: impl FnMut(&mut ChildBuilder)) -> &mut Self { - let current_entity = self - .commands - .current_entity - .expect("Cannot add children without a parent. Try creating an entity first."); - self.parent_entities.push(current_entity); + let current_entity = self.commands.current_entity.expect("Cannot add children because the 'current entity' is not set. You should spawn an entity first."); self.commands.current_entity = None; + let push_children = { + let mut builder = ChildBuilder { + commands: self.commands, + push_children: PushChildren { + children: SmallVec::default(), + parent: current_entity, + }, + }; - spawn_children(self); + spawn_children(&mut builder); + builder.push_children + }; - self.commands.current_entity = self.parent_entities.pop(); + self.commands.current_entity = Some(current_entity); + self.commands.write_world(push_children); + self + } + + fn push_children(&mut self, parent: Entity, children: &[Entity]) -> &mut Self { + self.commands.write_world(PushChildren { + children: SmallVec::from(children), + parent, + }); + self + } + fn insert_children(&mut self, parent: Entity, index: usize, children: &[Entity]) -> &mut Self { + self.commands.write_world(InsertChildren { + children: SmallVec::from(children), + index, + parent, + }); self } } + +#[cfg(test)] +mod tests { + use super::BuildChildren; + use crate::prelude::{Children, LocalTransform, Parent, PreviousParent}; + use bevy_ecs::{Commands, Entity, Resources, World}; + use smallvec::{smallvec, SmallVec}; + + #[test] + fn build_children() { + let mut world = World::default(); + let mut resources = Resources::default(); + let mut commands = Commands::default(); + + let mut parent = None; + let mut child1 = None; + let mut child2 = None; + + commands + .spawn((1,)) + .for_current_entity(|e| parent = Some(e)) + .with_children(|parent| { + parent + .spawn((2,)) + .for_current_entity(|e| child1 = Some(e)) + .spawn((3,)) + .for_current_entity(|e| child2 = Some(e)); + }); + + commands.apply(&mut world, &mut resources); + let parent = parent.expect("parent should exist"); + let child1 = child1.expect("child1 should exist"); + let child2 = child2.expect("child2 should exist"); + let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child2]; + + assert_eq!( + world.get::(parent).unwrap().0.clone(), + expected_children + ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + + assert_eq!( + *world.get::(child1).unwrap(), + PreviousParent(None) + ); + assert_eq!( + *world.get::(child2).unwrap(), + PreviousParent(None) + ); + + assert!(world.get::(child1).is_ok()); + assert!(world.get::(child2).is_ok()); + } + + #[test] + fn push_and_insert_children() { + let mut world = World::default(); + let mut resources = Resources::default(); + let mut commands = Commands::default(); + let entities = world + .spawn_batch(vec![(1,), (2,), (3,), (4,), (5,)]) + .collect::>(); + + commands.push_children(entities[0], &entities[1..3]); + commands.apply(&mut world, &mut resources); + + let parent = entities[0]; + let child1 = entities[1]; + let child2 = entities[2]; + let child3 = entities[3]; + let child4 = entities[4]; + + let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child2]; + assert_eq!( + world.get::(parent).unwrap().0.clone(), + expected_children + ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + + assert_eq!( + *world.get::(child1).unwrap(), + PreviousParent(None) + ); + assert_eq!( + *world.get::(child2).unwrap(), + PreviousParent(None) + ); + + assert!(world.get::(child1).is_ok()); + assert!(world.get::(child2).is_ok()); + + commands.insert_children(parent, 1, &entities[3..]); + commands.apply(&mut world, &mut resources); + + let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child3, child4, child2]; + assert_eq!( + world.get::(parent).unwrap().0.clone(), + expected_children + ); + assert_eq!(*world.get::(child3).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); + assert_eq!( + *world.get::(child3).unwrap(), + PreviousParent(None) + ); + assert_eq!( + *world.get::(child4).unwrap(), + PreviousParent(None) + ); + + assert!(world.get::(child3).is_ok()); + assert!(world.get::(child4).is_ok()); + } +} diff --git a/crates/bevy_transform/src/hierarchy/world_child_builder.rs b/crates/bevy_transform/src/hierarchy/world_child_builder.rs index f6842116f3..74af43dbcc 100644 --- a/crates/bevy_transform/src/hierarchy/world_child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/world_child_builder.rs @@ -1,4 +1,4 @@ -use crate::prelude::{LocalTransform, Parent}; +use crate::prelude::{LocalTransform, Parent, PreviousParent, Children}; use bevy_ecs::{Component, DynamicBundle, Entity, WorldBuilder}; pub struct WorldChildBuilder<'a, 'b> { @@ -23,7 +23,26 @@ impl<'a, 'b> WorldChildBuilder<'a, 'b> { .expect("There should always be a parent at this point."); self.world_builder .spawn_as_entity(entity, components) - .with_bundle((Parent(parent_entity), LocalTransform::default())); + .with_bundle(( + Parent(parent_entity), + PreviousParent(None), + LocalTransform::default(), + )); + { + let world = &mut self.world_builder.world; + let mut added = false; + if let Ok(mut children) = world.get_mut::(parent_entity) { + children.push(entity); + added = true; + } + + // NOTE: ideally this is just an else statement, but currently that _incorrectly_ fails borrow-checking + if !added { + world + .insert_one(parent_entity, Children(smallvec::smallvec![entity])) + .unwrap(); + } + } self } diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index ab2231c0e2..b8dd9ad756 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -11,7 +11,7 @@ pub mod prelude { use bevy_app::prelude::*; use bevy_ecs::prelude::*; use bevy_type_registry::RegisterType; -use prelude::{Children, LocalTransform, NonUniformScale, Rotation, Scale, Transform, Translation}; +use prelude::{Children, LocalTransform, NonUniformScale, Rotation, Scale, Transform, Translation, Parent}; pub(crate) fn transform_systems() -> Vec> { let mut systems = Vec::with_capacity(5); @@ -30,6 +30,7 @@ pub struct TransformPlugin; impl AppPlugin for TransformPlugin { fn build(&self, app: &mut AppBuilder) { app.register_component::() + .register_component::() .register_component::() .register_component::() .register_component::()