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

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<Entity>);
+pub struct PreviousParent(pub Entity);
 
 impl Deref for Parent {
     type Target = Entity;

crates/bevy_transform/src/hierarchy/child_builder.rs

@@ -15,10 +15,7 @@ impl Command for InsertChildren {
     fn write(self: Box<Self>, world: &mut World, _resources: &mut Resources) {
         for child in self.children.iter() {
             world
-                .insert(
+                .insert(*child, (Parent(self.parent), PreviousParent(self.parent)))
-                    *child,
-                    (Parent(self.parent), PreviousParent(Some(self.parent))),
-                )
                 .unwrap();
         }
         {
@@ -53,10 +50,7 @@ impl Command for PushChildren {
     fn write(self: Box<Self>, world: &mut World, _resources: &mut Resources) {
         for child in self.children.iter() {
             world
-                .insert(
+                .insert(*child, (Parent(self.parent), PreviousParent(self.parent)))
-                    *child,
-                    (Parent(self.parent), PreviousParent(Some(self.parent))),
-                )
                 .unwrap();
         }
         {
@@ -255,11 +249,11 @@ mod tests {
 
         assert_eq!(
             *world.get::<PreviousParent>(child1).unwrap(),
-            PreviousParent(Some(parent))
+            PreviousParent(parent)
         );
         assert_eq!(
             *world.get::<PreviousParent>(child2).unwrap(),
-            PreviousParent(Some(parent))
+            PreviousParent(parent)
         );
     }
 
@@ -291,11 +285,11 @@ mod tests {
 
         assert_eq!(
             *world.get::<PreviousParent>(child1).unwrap(),
-            PreviousParent(Some(parent))
+            PreviousParent(parent)
         );
         assert_eq!(
             *world.get::<PreviousParent>(child2).unwrap(),
-            PreviousParent(Some(parent))
+            PreviousParent(parent)
         );
 
         commands.insert_children(parent, 1, &entities[3..]);
@@ -310,11 +304,11 @@ mod tests {
         assert_eq!(*world.get::<Parent>(child4).unwrap(), Parent(parent));
         assert_eq!(
             *world.get::<PreviousParent>(child3).unwrap(),
-            PreviousParent(Some(parent))
+            PreviousParent(parent)
         );
         assert_eq!(
             *world.get::<PreviousParent>(child4).unwrap(),
-            PreviousParent(Some(parent))
+            PreviousParent(parent)
         );
     }
 }

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<Without<PreviousParent, (Entity, &Parent)>>,
-) {
-    // 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<Without<Parent, (Entity, &PreviousParent)>>,
     // 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) =
-            if let Ok(mut previous_parent_children) =
+            children_query.get_mut::<Children>(previous_parent.0)
-                children_query.get_mut::<Children>(previous_parent_entity)
+        {
-            {
+            log::trace!(" > Removing {:?} from it's prev parent's children", entity);
-                log::trace!(" > Removing {:?} from it's prev parent's children", entity);
+            previous_parent_children.0.retain(|e| *e != entity);
-                previous_parent_children.0.retain(|e| *e != entity);
+            commands.remove_one::<PreviousParent>(entity);
-            }
         }
     }
 
@@ -39,28 +27,29 @@ pub fn parent_update_system(
     let mut children_additions = HashMap::<Entity, SmallVec<[Entity; 8]>>::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 let Some(mut previous_parent) = possible_previous_parent {
-        // If the `PreviousParent` is not None.
-        if let Some(previous_parent_entity) = previous_parent.0 {
             // 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::<Children>(previous_parent_entity)
+                children_query.get_mut::<Children>(previous_parent.0)
             {
                 log::trace!(" > Removing {:?} from prev parent's children", entity);
                 (*previous_parent_children).0.retain(|e| *e != entity);
             }
-        }
 
-        // Set `PreviousParent = Parent`.
+            // Set `PreviousParent = Parent`.
-        *previous_parent = PreviousParent(Some(parent.0));
+            *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<Box<dyn System>> {
-    vec![
+    vec![parent_update_system.system()]
-        missing_previous_parent_system.system(),
-        parent_update_system.system(),
-    ]
 }
 
 #[cfg(test)]

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;