From 0977aba8bdbe09c10cb506ce3aa18c6473c25854 Mon Sep 17 00:00:00 2001 From: Yilin Wei Date: Wed, 15 Dec 2021 16:16:14 +0000 Subject: [PATCH 1/6] 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_hierarchy/src/components/parent.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index ddc0e2a634ee6..7264ab38d9c5d 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -62,3 +62,6 @@ impl FromWorld for PreviousParent { PreviousParent(Entity::from_raw(u32::MAX)) } } + +#[derive(Component, Debug, Copy, Clone, Eq, PartialEq, Reflect)] +pub struct DirtyParent; From 471cc926943fea8a3986f3a465e4f35bd087f426 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 26 Apr 2022 20:47:10 +0100 Subject: [PATCH 2/6] Move to updating when `Children` have changed --- crates/bevy_hierarchy/src/components/parent.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 7264ab38d9c5d..ddc0e2a634ee6 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -62,6 +62,3 @@ impl FromWorld for PreviousParent { PreviousParent(Entity::from_raw(u32::MAX)) } } - -#[derive(Component, Debug, Copy, Clone, Eq, PartialEq, Reflect)] -pub struct DirtyParent; From bed40916eda16c405f155fe5d79fd15c4e1c5138 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 26 Apr 2022 21:29:50 +0100 Subject: [PATCH 3/6] Implement the changes to check if Children changed --- crates/bevy_transform/src/systems.rs | 106 ++++++++++++++++++++------- 1 file changed, 80 insertions(+), 26 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index bc40d58bb61df..d77c8e9659e84 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -13,6 +13,7 @@ pub fn transform_propagate_system( mut root_query: Query< ( Option<&Children>, + Option>, &Transform, Changed, &mut GlobalTransform, @@ -23,10 +24,13 @@ pub fn transform_propagate_system( (&Transform, Changed, &mut GlobalTransform), With, >, - children_query: Query, (With, With)>, + children_query: Query<(&Children, Changed), (With, With)>, ) { - for (children, transform, transform_changed, mut global_transform) in root_query.iter_mut() { - let mut changed = false; + for (children, maybe_changed_children, transform, transform_changed, mut global_transform) in + root_query.iter_mut() + { + // TODO: Use `Option::contains` when stable + let mut changed = maybe_changed_children == Some(true); if transform_changed { *global_transform = GlobalTransform::from(*transform); changed = true; @@ -34,7 +38,7 @@ pub fn transform_propagate_system( if let Some(children) = children { for child in children.iter() { - propagate_recursive( + let _ = propagate_recursive( &global_transform, &mut transform_query, &children_query, @@ -52,35 +56,34 @@ fn propagate_recursive( (&Transform, Changed, &mut GlobalTransform), With, >, - children_query: &Query, (With, With)>, + children_query: &Query<(&Children, Changed), (With, With)>, entity: Entity, mut changed: bool, -) { + // We use a result here to use the `?` operator. Ideally we'd use a try block instead +) -> Result<(), ()> { let global_matrix = { - if let Ok((transform, transform_changed, mut global_transform)) = - transform_query.get_mut(entity) - { - changed |= transform_changed; - if changed { - *global_transform = parent.mul_transform(*transform); - } - *global_transform - } else { - return; + let (transform, transform_changed, mut global_transform) = + transform_query.get_mut(entity).map_err(drop)?; + + changed |= transform_changed; + if changed { + *global_transform = parent.mul_transform(*transform); } + *global_transform }; - if let Ok(Some(children)) = children_query.get(entity) { - for child in children.iter() { - propagate_recursive( - &global_matrix, - transform_query, - children_query, - *child, - changed, - ); - } + let (children, changed_children) = children_query.get(entity).map_err(drop)?; + changed |= changed_children; + for child in children.iter() { + let _ = propagate_recursive( + &global_matrix, + transform_query, + children_query, + *child, + changed, + ); } + Ok(()) } #[cfg(test)] @@ -90,6 +93,7 @@ mod test { system::{CommandQueue, Commands}, world::World, }; + use bevy_math::vec3; use crate::components::{GlobalTransform, Transform}; use crate::systems::transform_propagate_system; @@ -271,4 +275,54 @@ mod test { vec![children[1]] ); } + + #[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); + + 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_bundle(( + Transform::identity(), + GlobalTransform::default(), + Parent(parent), + )) + .id(); + + let children = &[child]; + + command_queue.apply(&mut world); + schedule.run(&mut world); + + // check the `Children` structure is spawned + assert_eq!(&**world.get::(parent).unwrap(), &*children); + + schedule.run(&mut world); + + assert_eq!( + world.get::(child).unwrap(), + &GlobalTransform { + translation, + ..Default::default() + }, + ); + } } From ef087539c10ad27547fbf4b2ddc8ddef8251ed5b Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 26 Apr 2022 21:35:48 +0100 Subject: [PATCH 4/6] Make the test multi-layered and use App instead --- crates/bevy_transform/src/systems.rs | 73 ++++++++++++++-------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index d77c8e9659e84..41b2b655e7b2a 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -1,10 +1,5 @@ use crate::components::{GlobalTransform, Transform}; -use bevy_ecs::{ - entity::Entity, - prelude::Changed, - query::{With, Without}, - system::Query, -}; +use bevy_ecs::prelude::*; use bevy_hierarchy::{Children, Parent}; /// Update [`GlobalTransform`] component of entities based on entity hierarchy and @@ -88,11 +83,9 @@ fn propagate_recursive( #[cfg(test)] mod test { - use bevy_ecs::{ - schedule::{Schedule, Stage, SystemStage}, - system::{CommandQueue, Commands}, - world::World, - }; + use bevy_app::prelude::*; + use bevy_ecs::prelude::*; + use bevy_ecs::system::CommandQueue; use bevy_math::vec3; use crate::components::{GlobalTransform, Transform}; @@ -278,27 +271,22 @@ mod test { #[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); - - let mut schedule = Schedule::default(); - schedule.add_stage("update", update_stage); + let mut app = App::new(); - let mut command_queue = CommandQueue::default(); - let mut commands = Commands::new(&mut command_queue, &world); + app.add_system(parent_update_system); + app.add_system(transform_propagate_system); let translation = vec3(1.0, 0.0, 0.0); - let parent = commands + let parent = app + .world .spawn() .insert(Transform::from_translation(translation)) .insert(GlobalTransform::default()) .id(); - let child = commands + let child = app + .world .spawn() .insert_bundle(( Transform::identity(), @@ -307,22 +295,33 @@ mod test { )) .id(); - let children = &[child]; + let grandchild = app + .world + .spawn() + .insert_bundle(( + Transform::identity(), + GlobalTransform::default(), + Parent(child), + )) + .id(); - command_queue.apply(&mut world); - schedule.run(&mut world); + app.update(); // check the `Children` structure is spawned - assert_eq!(&**world.get::(parent).unwrap(), &*children); - - schedule.run(&mut world); - - assert_eq!( - world.get::(child).unwrap(), - &GlobalTransform { - translation, - ..Default::default() - }, - ); + assert_eq!(&**app.world.get::(parent).unwrap(), &[child]); + assert_eq!(&**app.world.get::(child).unwrap(), &[grandchild]); + // Note that at this point, the `GlobalTransform`s will not have updated yet, due to `Commands` delay + app.update(); + + let mut state = app.world.query::<&GlobalTransform>(); + for global in state.iter(&app.world) { + assert_eq!( + global, + &GlobalTransform { + translation, + ..Default::default() + }, + ); + } } } From a5509de81de8f310df242fb13fbf8a8cc1901201 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sun, 1 May 2022 13:40:28 +0100 Subject: [PATCH 5/6] Make import worse As requsted by @mockersf --- crates/bevy_transform/src/systems.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 41b2b655e7b2a..66ab332e45d90 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -1,5 +1,5 @@ use crate::components::{GlobalTransform, Transform}; -use bevy_ecs::prelude::*; +use bevy_ecs::prelude::{Changed, Entity, Query, With, Without}; use bevy_hierarchy::{Children, Parent}; /// Update [`GlobalTransform`] component of entities based on entity hierarchy and From 9591e784fd4e5455cb9e95a9478baa9a101b02d7 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Sun, 1 May 2022 21:23:41 +0100 Subject: [PATCH 6/6] Only use one Option --- crates/bevy_transform/src/systems.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 66ab332e45d90..da063f8241597 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -7,8 +7,7 @@ use bevy_hierarchy::{Children, Parent}; pub fn transform_propagate_system( mut root_query: Query< ( - Option<&Children>, - Option>, + Option<(&Children, Changed)>, &Transform, Changed, &mut GlobalTransform, @@ -21,17 +20,15 @@ pub fn transform_propagate_system( >, children_query: Query<(&Children, Changed), (With, With)>, ) { - for (children, maybe_changed_children, transform, transform_changed, mut global_transform) in - root_query.iter_mut() - { - // TODO: Use `Option::contains` when stable - let mut changed = maybe_changed_children == Some(true); + for (children, transform, transform_changed, mut global_transform) in root_query.iter_mut() { + let mut changed = transform_changed; if transform_changed { *global_transform = GlobalTransform::from(*transform); - changed = true; } - if let Some(children) = children { + if let Some((children, changed_children)) = children { + // If our `Children` has changed, we need to recalculate everything below us + changed |= changed_children; for child in children.iter() { let _ = propagate_recursive( &global_transform, @@ -68,6 +65,7 @@ fn propagate_recursive( }; let (children, changed_children) = children_query.get(entity).map_err(drop)?; + // If our `Children` has changed, we need to recalculate everything below us changed |= changed_children; for child in children.iter() { let _ = propagate_recursive(