From 7734b1ea6d323721d2f80d3f03175ee9c29267f1 Mon Sep 17 00:00:00 2001 From: SvenTS <73037851+SvenTS@users.noreply.github.com> Date: Thu, 29 Oct 2020 21:54:29 +0100 Subject: [PATCH] Fix PreviousParent lag by merging parent update systems (#713) * Sync previous parent in parent_update_system * Previous parent does not need to be an option now * Remove previous parent after parent deletion --- .../bevy_transform/src/components/parent.rs | 2 +- .../src/hierarchy/child_builder.rs | 22 +++----- .../hierarchy/hierarchy_maintenance_system.rs | 50 +++++++------------ .../src/hierarchy/world_child_builder.rs | 2 +- 4 files changed, 28 insertions(+), 48 deletions(-) diff --git a/crates/bevy_transform/src/components/parent.rs b/crates/bevy_transform/src/components/parent.rs index c18b5342bfbec..bf9bdab9a67b2 100644 --- a/crates/bevy_transform/src/components/parent.rs +++ b/crates/bevy_transform/src/components/parent.rs @@ -26,7 +26,7 @@ impl MapEntities for Parent { } #[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub struct PreviousParent(pub Option); +pub struct PreviousParent(pub Entity); impl Deref for Parent { type Target = Entity; diff --git a/crates/bevy_transform/src/hierarchy/child_builder.rs b/crates/bevy_transform/src/hierarchy/child_builder.rs index db77fe474527e..74bff7c2e1f3e 100644 --- a/crates/bevy_transform/src/hierarchy/child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/child_builder.rs @@ -15,10 +15,7 @@ impl Command for InsertChildren { fn write(self: Box, world: &mut World, _resources: &mut Resources) { for child in self.children.iter() { world - .insert( - *child, - (Parent(self.parent), PreviousParent(Some(self.parent))), - ) + .insert(*child, (Parent(self.parent), PreviousParent(self.parent))) .unwrap(); } { @@ -53,10 +50,7 @@ impl Command for PushChildren { fn write(self: Box, world: &mut World, _resources: &mut Resources) { for child in self.children.iter() { world - .insert( - *child, - (Parent(self.parent), PreviousParent(Some(self.parent))), - ) + .insert(*child, (Parent(self.parent), PreviousParent(self.parent))) .unwrap(); } { @@ -255,11 +249,11 @@ mod tests { assert_eq!( *world.get::(child1).unwrap(), - PreviousParent(Some(parent)) + PreviousParent(parent) ); assert_eq!( *world.get::(child2).unwrap(), - PreviousParent(Some(parent)) + PreviousParent(parent) ); } @@ -291,11 +285,11 @@ mod tests { assert_eq!( *world.get::(child1).unwrap(), - PreviousParent(Some(parent)) + PreviousParent(parent) ); assert_eq!( *world.get::(child2).unwrap(), - PreviousParent(Some(parent)) + PreviousParent(parent) ); commands.insert_children(parent, 1, &entities[3..]); @@ -310,11 +304,11 @@ mod tests { assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); assert_eq!( *world.get::(child3).unwrap(), - PreviousParent(Some(parent)) + PreviousParent(parent) ); assert_eq!( *world.get::(child4).unwrap(), - PreviousParent(Some(parent)) + PreviousParent(parent) ); } } diff --git a/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs b/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs index a844e886babc3..c3f2f338d7982 100644 --- a/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs +++ b/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs @@ -3,35 +3,23 @@ use bevy_ecs::{Commands, Entity, IntoQuerySystem, Query, System, Without}; use bevy_utils::HashMap; use smallvec::SmallVec; -pub fn missing_previous_parent_system( - mut commands: Commands, - mut query: Query>, -) { - // Add missing `PreviousParent` components - for (entity, _parent) in &mut query.iter() { - log::trace!("Adding missing PreviousParent to {:?}", entity); - commands.insert_one(entity, PreviousParent(None)); - } -} - pub fn parent_update_system( mut commands: Commands, mut removed_parent_query: Query>, // TODO: ideally this only runs when the Parent component has changed - mut changed_parent_query: Query<(Entity, &Parent, &mut PreviousParent)>, + mut changed_parent_query: Query<(Entity, &Parent, Option<&mut PreviousParent>)>, 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 &mut removed_parent_query.iter() { log::trace!("Parent was removed from {:?}", entity); - if let Some(previous_parent_entity) = previous_parent.0 { - if let Ok(mut previous_parent_children) = - children_query.get_mut::(previous_parent_entity) - { - log::trace!(" > Removing {:?} from it's prev parent's children", entity); - previous_parent_children.0.retain(|e| *e != entity); - } + if let Ok(mut previous_parent_children) = + children_query.get_mut::(previous_parent.0) + { + log::trace!(" > Removing {:?} from it's prev parent's children", entity); + previous_parent_children.0.retain(|e| *e != entity); + commands.remove_one::(entity); } } @@ -39,28 +27,29 @@ pub fn parent_update_system( let mut children_additions = HashMap::>::default(); // Entities with a changed Parent (that also have a PreviousParent, even if None) - for (entity, parent, mut previous_parent) in &mut changed_parent_query.iter() { + for (entity, parent, possible_previous_parent) in &mut changed_parent_query.iter() { log::trace!("Parent changed for {:?}", entity); - - // If the `PreviousParent` is not None. - if let Some(previous_parent_entity) = previous_parent.0 { + 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_entity == parent.0 { + if previous_parent.0 == parent.0 { log::trace!(" > But the previous parent is the same, ignoring..."); continue; } // Remove from `PreviousParent.Children`. if let Ok(mut previous_parent_children) = - children_query.get_mut::(previous_parent_entity) + children_query.get_mut::(previous_parent.0) { log::trace!(" > Removing {:?} from prev parent's children", entity); (*previous_parent_children).0.retain(|e| *e != entity); } - } - // Set `PreviousParent = Parent`. - *previous_parent = PreviousParent(Some(parent.0)); + // Set `PreviousParent = Parent`. + *previous_parent = PreviousParent(parent.0); + } else { + log::trace!("Adding missing PreviousParent to {:?}", entity); + commands.insert_one(entity, PreviousParent(parent.0)); + }; // Add to the parent's `Children` (either the real component, or // `children_additions`). @@ -99,10 +88,7 @@ pub fn parent_update_system( } pub fn hierarchy_maintenance_systems() -> Vec> { - vec![ - missing_previous_parent_system.system(), - parent_update_system.system(), - ] + vec![parent_update_system.system()] } #[cfg(test)] diff --git a/crates/bevy_transform/src/hierarchy/world_child_builder.rs b/crates/bevy_transform/src/hierarchy/world_child_builder.rs index 98fa07ee0ce75..a658956c06e69 100644 --- a/crates/bevy_transform/src/hierarchy/world_child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/world_child_builder.rs @@ -16,7 +16,7 @@ impl<'a, 'b> WorldChildBuilder<'a, 'b> { .expect("There should always be a parent at this point."); self.world_builder .spawn(components) - .with_bundle((Parent(parent_entity), PreviousParent(Some(parent_entity)))); + .with_bundle((Parent(parent_entity), PreviousParent(parent_entity))); let entity = self.world_builder.current_entity.unwrap(); { let world = &mut self.world_builder.world;