From 6732003ac572df84d4290b3899d851f8b9c5c708 Mon Sep 17 00:00:00 2001 From: Yilin Wei Date: Wed, 15 Dec 2021 16:16:14 +0000 Subject: [PATCH 1/3] Closes #3329 `parent_update` and `transform_propagate` run in the same stage but `parent_update` can spawn `Children`. This means that the `system` can enter an inconsistent state where the `GlobalTransform` has not been updated on `Children` when spawning an `Entity` where the `Parent` does not have an existing `Children` component. Introduce a marker trait, `DirtyParent`, so that the system will revert to the correct state the next time the systems run. --- crates/bevy_transform/src/components/mod.rs | 2 +- .../bevy_transform/src/components/parent.rs | 3 + .../hierarchy/hierarchy_maintenance_system.rs | 80 ++++++++++++++++++- crates/bevy_transform/src/lib.rs | 8 +- .../src/transform_propagate_system.rs | 9 ++- 5 files changed, 95 insertions(+), 7 deletions(-) diff --git a/crates/bevy_transform/src/components/mod.rs b/crates/bevy_transform/src/components/mod.rs index 67720a2b4e08e..b57f77450a9ff 100644 --- a/crates/bevy_transform/src/components/mod.rs +++ b/crates/bevy_transform/src/components/mod.rs @@ -5,5 +5,5 @@ mod transform; pub use children::Children; pub use global_transform::*; -pub use parent::{Parent, PreviousParent}; +pub use parent::{DirtyParent, Parent, PreviousParent}; pub use transform::*; diff --git a/crates/bevy_transform/src/components/parent.rs b/crates/bevy_transform/src/components/parent.rs index c594d4c4dae2d..3290c7153f7a4 100644 --- a/crates/bevy_transform/src/components/parent.rs +++ b/crates/bevy_transform/src/components/parent.rs @@ -59,3 +59,6 @@ impl FromWorld for PreviousParent { PreviousParent(Entity::new(u32::MAX)) } } + +#[derive(Component, Debug, Copy, Clone, Eq, PartialEq, Reflect)] +pub struct DirtyParent; diff --git a/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs b/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs index 6fde5d52ebfbb..31f55e3a8e9cf 100644 --- a/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs +++ b/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs @@ -2,7 +2,7 @@ use crate::components::*; use bevy_ecs::{ entity::Entity, prelude::Changed, - query::Without, + query::{With, Without}, system::{Commands, Query}, }; use bevy_utils::HashMap; @@ -67,9 +67,23 @@ pub fn parent_update_system( // 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)); + // Mark the entity with `Dirty` so that systems which run + // in the same stage have a way to `Query` for a missed update + commands + .entity(*e) + .insert_bundle((Children::with(v), DirtyParent)); }); } + +pub fn clean_dirty_parents( + mut commands: Commands, + dirty_parent_query: Query>, +) { + for entity in dirty_parent_query.iter() { + commands.entity(entity).remove::(); + } +} + #[cfg(test)] mod test { use bevy_ecs::{ @@ -77,10 +91,72 @@ mod test { system::CommandQueue, world::World, }; + use bevy_math::vec3; use super::*; use crate::{hierarchy::BuildChildren, transform_propagate_system::transform_propagate_system}; + #[test] + fn correct_transforms_when_no_children() { + 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); + update_stage.add_system(clean_dirty_parents); + + let mut schedule = Schedule::default(); + schedule.add_stage("update", update_stage); + + let mut command_queue = CommandQueue::default(); + let mut commands = Commands::new(&mut command_queue, &world); + + let translation = vec3(1.0, 0.0, 0.0); + + let parent = commands + .spawn() + .insert(Transform::from_translation(translation)) + .insert(GlobalTransform::default()) + .id(); + + let child = commands + .spawn() + .insert(Transform::default()) + .insert(GlobalTransform::default()) + .insert(Parent(parent)) + .id(); + + let children = vec![child]; + + command_queue.apply(&mut world); + schedule.run(&mut world); + + // check the `Children` structure is spawned + assert_eq!( + world + .get::(parent) + .unwrap() + .0 + .iter() + .cloned() + .collect::>(), + children, + ); + assert!(world.get::(parent).is_some()); + + schedule.run(&mut world); + + assert_eq!( + world.get::(child).unwrap(), + &GlobalTransform { + translation, + ..Default::default() + }, + ); + + assert!(world.get::(parent).is_none()); + } + #[test] fn correct_children() { let mut world = World::default(); diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index f255c613db814..769ec60bbc9d3 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -9,7 +9,10 @@ pub mod prelude { use bevy_app::prelude::*; use bevy_ecs::schedule::{ParallelSystemDescriptorCoercion, SystemLabel}; -use prelude::{parent_update_system, Children, GlobalTransform, Parent, PreviousParent, Transform}; +use prelude::{ + clean_dirty_parents, parent_update_system, Children, GlobalTransform, Parent, PreviousParent, + Transform, +}; #[derive(Default)] pub struct TransformPlugin; @@ -47,6 +50,7 @@ impl Plugin for TransformPlugin { transform_propagate_system::transform_propagate_system .label(TransformSystem::TransformPropagate) .after(TransformSystem::ParentUpdate), - ); + ) + .add_system_to_stage(CoreStage::PostUpdate, clean_dirty_parents); } } diff --git a/crates/bevy_transform/src/transform_propagate_system.rs b/crates/bevy_transform/src/transform_propagate_system.rs index a4ab7aadcdf89..d4eb4574df3bb 100644 --- a/crates/bevy_transform/src/transform_propagate_system.rs +++ b/crates/bevy_transform/src/transform_propagate_system.rs @@ -1,4 +1,4 @@ -use crate::components::{Children, GlobalTransform, Parent, Transform}; +use crate::components::{Children, DirtyParent, GlobalTransform, Parent, Transform}; use bevy_ecs::{ entity::Entity, query::{Changed, With, Without}, @@ -12,13 +12,14 @@ pub fn transform_propagate_system( (Entity, Option<&Children>, &Transform, &mut GlobalTransform), Without, >, + dirty_parent_query: Query>, mut transform_query: Query<(&Transform, &mut GlobalTransform), With>, changed_transform_query: Query>, children_query: Query, (With, With)>, ) { for (entity, children, transform, mut global_transform) in root_query.iter_mut() { let mut changed = false; - if changed_transform_query.get(entity).is_ok() { + if changed_transform_query.get(entity).is_ok() || dirty_parent_query.get(entity).is_ok() { *global_transform = GlobalTransform::from(*transform); changed = true; } @@ -27,6 +28,7 @@ pub fn transform_propagate_system( for child in children.0.iter() { propagate_recursive( &global_transform, + &dirty_parent_query, &changed_transform_query, &mut transform_query, &children_query, @@ -40,6 +42,7 @@ pub fn transform_propagate_system( fn propagate_recursive( parent: &GlobalTransform, + dirty_parent_query: &Query>, changed_transform_query: &Query>, transform_query: &mut Query<(&Transform, &mut GlobalTransform), With>, children_query: &Query, (With, With)>, @@ -47,6 +50,7 @@ fn propagate_recursive( mut changed: bool, ) { changed |= changed_transform_query.get(entity).is_ok(); + changed |= dirty_parent_query.get(entity).is_ok(); let global_matrix = { if let Ok((transform, mut global_transform)) = transform_query.get_mut(entity) { @@ -63,6 +67,7 @@ fn propagate_recursive( for child in children.0.iter() { propagate_recursive( &global_matrix, + &dirty_parent_query, changed_transform_query, transform_query, children_query, From 5adb2855c1a20e84b6ede11ca52e6b7d331673a1 Mon Sep 17 00:00:00 2001 From: Yilin Wei Date: Wed, 15 Dec 2021 16:39:13 +0000 Subject: [PATCH 2/3] fix clippy warnings --- crates/bevy_transform/src/transform_propagate_system.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_transform/src/transform_propagate_system.rs b/crates/bevy_transform/src/transform_propagate_system.rs index d4eb4574df3bb..04dc874d0e479 100644 --- a/crates/bevy_transform/src/transform_propagate_system.rs +++ b/crates/bevy_transform/src/transform_propagate_system.rs @@ -67,7 +67,7 @@ fn propagate_recursive( for child in children.0.iter() { propagate_recursive( &global_matrix, - &dirty_parent_query, + dirty_parent_query, changed_transform_query, transform_query, children_query, From 46cead4a3ea846ae4042548774fcb63941fbb0e3 Mon Sep 17 00:00:00 2001 From: Yilin Wei Date: Fri, 17 Dec 2021 01:16:36 +0000 Subject: [PATCH 3/3] address comments. --- .../src/hierarchy/hierarchy_maintenance_system.rs | 4 ++-- .../bevy_transform/src/transform_propagate_system.rs | 12 ++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs b/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs index 31f55e3a8e9cf..82fd7ac315dd6 100644 --- a/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs +++ b/crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs @@ -67,8 +67,8 @@ pub fn parent_update_system( // 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)| { - // Mark the entity with `Dirty` so that systems which run - // in the same stage have a way to `Query` for a missed update + // Mark the entity with a `DirtyParent` component so that systems which run + // in the same stage have a way to query for a missed update. commands .entity(*e) .insert_bundle((Children::with(v), DirtyParent)); diff --git a/crates/bevy_transform/src/transform_propagate_system.rs b/crates/bevy_transform/src/transform_propagate_system.rs index 04dc874d0e479..860902ec5c415 100644 --- a/crates/bevy_transform/src/transform_propagate_system.rs +++ b/crates/bevy_transform/src/transform_propagate_system.rs @@ -1,6 +1,7 @@ use crate::components::{Children, DirtyParent, GlobalTransform, Parent, Transform}; use bevy_ecs::{ entity::Entity, + prelude::Or, query::{Changed, With, Without}, system::Query, }; @@ -12,14 +13,13 @@ pub fn transform_propagate_system( (Entity, Option<&Children>, &Transform, &mut GlobalTransform), Without, >, - dirty_parent_query: Query>, mut transform_query: Query<(&Transform, &mut GlobalTransform), With>, - changed_transform_query: Query>, + changed_transform_query: Query, With)>>, children_query: Query, (With, With)>, ) { for (entity, children, transform, mut global_transform) in root_query.iter_mut() { let mut changed = false; - if changed_transform_query.get(entity).is_ok() || dirty_parent_query.get(entity).is_ok() { + if changed_transform_query.get(entity).is_ok() { *global_transform = GlobalTransform::from(*transform); changed = true; } @@ -28,7 +28,6 @@ pub fn transform_propagate_system( for child in children.0.iter() { propagate_recursive( &global_transform, - &dirty_parent_query, &changed_transform_query, &mut transform_query, &children_query, @@ -42,15 +41,13 @@ pub fn transform_propagate_system( fn propagate_recursive( parent: &GlobalTransform, - dirty_parent_query: &Query>, - changed_transform_query: &Query>, + changed_transform_query: &Query, With)>>, transform_query: &mut Query<(&Transform, &mut GlobalTransform), With>, children_query: &Query, (With, With)>, entity: Entity, mut changed: bool, ) { changed |= changed_transform_query.get(entity).is_ok(); - changed |= dirty_parent_query.get(entity).is_ok(); let global_matrix = { if let Ok((transform, mut global_transform)) = transform_query.get_mut(entity) { @@ -67,7 +64,6 @@ fn propagate_recursive( for child in children.0.iter() { propagate_recursive( &global_matrix, - dirty_parent_query, changed_transform_query, transform_query, children_query,