From 41f22df49dd7adb185fb50e103c4bd86297af9ac Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Wed, 18 Jan 2023 10:30:36 +0100 Subject: [PATCH 1/4] Fix transform propagation for orphaned hierarchies Co-authored-by: vyb --- crates/bevy_transform/src/systems.rs | 83 +++++++++++++++++++++++++--- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 27e52cbc06bc4..661fa33f3cfcf 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -2,6 +2,8 @@ use crate::components::{GlobalTransform, Transform}; use bevy_ecs::{ change_detection::Ref, prelude::{Changed, DetectChanges, Entity, Query, With, Without}, + removal_detection::RemovedComponents, + system::ParamSet, }; use bevy_hierarchy::{Children, Parent}; @@ -9,16 +11,29 @@ use bevy_hierarchy::{Children, Parent}; /// /// Third party plugins should ensure that this is used in concert with [`propagate_transforms`]. pub fn sync_simple_transforms( - mut query: Query< - (&Transform, &mut GlobalTransform), - (Changed, Without, Without), - >, + mut query: ParamSet<( + Query< + (&Transform, &mut GlobalTransform), + (Changed, Without, Without), + >, + Query<(Ref, &mut GlobalTransform), Without>, + )>, + mut orphaned: RemovedComponents, ) { query + .p0() .par_iter_mut() .for_each_mut(|(transform, mut global_transform)| { *global_transform = GlobalTransform::from(*transform); }); + // updated orphaned entities + let mut query = query.p1(); + let mut iter = query.iter_many_mut(orphaned.iter()); + while let Some((transform, mut global_transform)) = iter.fetch_next() { + if !transform.is_changed() { + *global_transform = GlobalTransform::from(*transform); + } + } } /// Update [`GlobalTransform`] component of entities based on entity hierarchy and @@ -30,12 +45,15 @@ pub fn propagate_transforms( (Entity, &Children, Ref, &mut GlobalTransform), Without, >, + mut orphaned: RemovedComponents, transform_query: Query<(Ref, &mut GlobalTransform, Option<&Children>), With>, parent_query: Query<(Entity, Ref)>, ) { + let mut orphaned = orphaned.iter().collect::>(); + orphaned.sort_unstable(); root_query.par_iter_mut().for_each_mut( |(entity, children, transform, mut global_transform)| { - let changed = transform.is_changed(); + let changed = transform.is_changed() || orphaned.binary_search(&entity).is_ok(); if changed { *global_transform = GlobalTransform::from(*transform); } @@ -161,10 +179,61 @@ mod test { use bevy_tasks::{ComputeTaskPool, TaskPool}; use crate::components::{GlobalTransform, Transform}; - use crate::systems::*; - use crate::TransformBundle; + use crate::{propagate_transforms, sync_simple_transforms, TransformBundle}; use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; + #[test] + fn correct_parent_removed() { + ComputeTaskPool::init(TaskPool::default); + let mut world = World::default(); + let offset_global_transform = + |offset| GlobalTransform::from(Transform::from_xyz(offset, offset, offset)); + let offset_transform = + |offset| TransformBundle::from_transform(Transform::from_xyz(offset, offset, offset)); + + let mut schedule = Schedule::new(); + schedule.add_systems((sync_simple_transforms, propagate_transforms)); + + let mut command_queue = CommandQueue::default(); + let mut commands = Commands::new(&mut command_queue, &world); + let root = commands.spawn(offset_transform(3.3)).id(); + let parent = commands.spawn(offset_transform(4.4)).id(); + let child = commands.spawn(offset_transform(5.5)).id(); + commands.entity(parent).set_parent(root); + commands.entity(child).set_parent(parent); + command_queue.apply(&mut world); + schedule.run(&mut world); + + assert_ne!( + world.get::(parent).unwrap(), + &GlobalTransform::from(Transform::IDENTITY) + ); + + // Remove parent of `parent` + let mut command_queue = CommandQueue::default(); + let mut commands = Commands::new(&mut command_queue, &world); + commands.entity(parent).remove_parent(); + command_queue.apply(&mut world); + schedule.run(&mut world); + + assert_eq!( + world.get::(parent).unwrap(), + &offset_global_transform(4.4) + ); + + // Remove parent of `child` + let mut command_queue = CommandQueue::default(); + let mut commands = Commands::new(&mut command_queue, &world); + commands.entity(child).remove_parent(); + command_queue.apply(&mut world); + schedule.run(&mut world); + + assert_eq!( + world.get::(child).unwrap(), + &offset_global_transform(5.5) + ); + } + #[test] fn did_propagate() { ComputeTaskPool::init(TaskPool::default); From 3985aaf49210df1d13ee9fa5e6c97991e977dcd4 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Tue, 7 Feb 2023 10:54:08 +0100 Subject: [PATCH 2/4] Fix rebase mishaps --- crates/bevy_transform/src/systems.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 661fa33f3cfcf..6eafac37a2a67 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -179,7 +179,8 @@ mod test { use bevy_tasks::{ComputeTaskPool, TaskPool}; use crate::components::{GlobalTransform, Transform}; - use crate::{propagate_transforms, sync_simple_transforms, TransformBundle}; + use crate::systems::*; + use crate::TransformBundle; use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; #[test] From d1af08a7b4edd0e5230d0b61448b9b93c41b3548 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Mon, 3 Apr 2023 16:18:00 +0200 Subject: [PATCH 3/4] Include jojojet feedback --- crates/bevy_transform/src/systems.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 6eafac37a2a67..e362c982bebb6 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -3,7 +3,7 @@ use bevy_ecs::{ change_detection::Ref, prelude::{Changed, DetectChanges, Entity, Query, With, Without}, removal_detection::RemovedComponents, - system::ParamSet, + system::{Local, ParamSet}, }; use bevy_hierarchy::{Children, Parent}; @@ -48,12 +48,14 @@ pub fn propagate_transforms( mut orphaned: RemovedComponents, transform_query: Query<(Ref, &mut GlobalTransform, Option<&Children>), With>, parent_query: Query<(Entity, Ref)>, + mut orphaned_entities: Local>, ) { - let mut orphaned = orphaned.iter().collect::>(); - orphaned.sort_unstable(); + orphaned_entities.clear(); + orphaned_entities.extend(orphaned.iter()); + orphaned_entities.sort_unstable(); root_query.par_iter_mut().for_each_mut( |(entity, children, transform, mut global_transform)| { - let changed = transform.is_changed() || orphaned.binary_search(&entity).is_ok(); + let changed = transform.is_changed() || orphaned_entities.binary_search(&entity).is_ok(); if changed { *global_transform = GlobalTransform::from(*transform); } @@ -205,9 +207,10 @@ mod test { command_queue.apply(&mut world); schedule.run(&mut world); - assert_ne!( + assert_eq!( world.get::(parent).unwrap(), - &GlobalTransform::from(Transform::IDENTITY) + &offset_global_transform(4.4 + 3.3), + "The transform systems didn't run, ie: `GlobalTransform` wasn't updated", ); // Remove parent of `parent` @@ -219,7 +222,8 @@ mod test { assert_eq!( world.get::(parent).unwrap(), - &offset_global_transform(4.4) + &offset_global_transform(4.4), + "The global transform of an orphaned entity wasn't updated properly", ); // Remove parent of `child` @@ -231,7 +235,8 @@ mod test { assert_eq!( world.get::(child).unwrap(), - &offset_global_transform(5.5) + &offset_global_transform(5.5), + "The global transform of an orphaned entity wasn't updated properly", ); } From 1964a63771c30e30edbbf2798affb60d30763c4b Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Tue, 4 Apr 2023 17:28:39 +0200 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com> --- crates/bevy_transform/src/systems.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index e362c982bebb6..46629eca57db9 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -20,13 +20,14 @@ pub fn sync_simple_transforms( )>, mut orphaned: RemovedComponents, ) { + // Update changed entities. query .p0() .par_iter_mut() .for_each_mut(|(transform, mut global_transform)| { *global_transform = GlobalTransform::from(*transform); }); - // updated orphaned entities + // Update orphaned entities. let mut query = query.p1(); let mut iter = query.iter_many_mut(orphaned.iter()); while let Some((transform, mut global_transform)) = iter.fetch_next() {