From 7d0edbc4d65c483d3e73c574b93a4a36560c0d07 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 17 Jan 2023 22:26:51 +0000 Subject: [PATCH] Improve change detection behavior for transform propagation (#6870) # Objective Fix #4647. If any child is changed, or even reordered, `Changed` is true, which causes transform propagation to propagate changes to all siblings of a changed child, even if they don't need to be. ## Solution As `Parent` and `Children` are updated in tandem in hierarchy commands after #4800. `Changed` is true on the child when `Changed` is true on the parent. However, unlike checking children, checking `Changed` is only localized to the current entity and will not force propagation to the siblings. Also took the opportunity to change propagation to use `Query::iter_many` instead of repeated `Query::get` calls. Should cut a bit of the overhead out of propagation. This means we won't panic when there isn't a `Parent` on the child, just skip over it. The tests from #4608 still pass, so the change detection here still works just fine under this approach. --- crates/bevy_transform/src/systems.rs | 90 +++++++++++++--------------- 1 file changed, 43 insertions(+), 47 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 21eb7cbc51..a5ca2f7612 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -27,12 +27,11 @@ pub fn sync_simple_transforms( /// to propagate transforms correctly. pub fn propagate_transforms( mut root_query: Query< - (Entity, Ref, Ref, &mut GlobalTransform), + (Entity, &Children, Ref, &mut GlobalTransform), Without, >, - transform_query: Query<(Ref, &mut GlobalTransform), With>, - parent_query: Query<&Parent>, - children_query: Query, (With, With)>, + transform_query: Query<(Ref, &mut GlobalTransform, Option<&Children>), With>, + parent_query: Query<(Entity, Ref)>, ) { root_query.par_for_each_mut( // The differing depths and sizes of hierarchy trees causes the work for each root to be @@ -40,18 +39,21 @@ pub fn propagate_transforms( // large trees are not clumped together. 1, |(entity, children, transform, mut global_transform)| { - let mut changed = transform.is_changed(); + let changed = transform.is_changed(); if changed { *global_transform = GlobalTransform::from(*transform); } - // If our `Children` has changed, we need to recalculate everything below us - changed |= children.is_changed(); - - for child in children.iter() { + for (child, actual_parent) in parent_query.iter_many(children) { + assert_eq!( + actual_parent.get(), entity, + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" + ); // SAFETY: - // - We may operate as if the hierarchy is consistent, since `propagate_recursive` will panic before continuing - // to propagate if it encounters an entity with inconsistent parentage. + // - `child` must have consistent parentage, or the above assertion would panic. + // Since `child` is parented to a root entity, the entire hierarchy leading to it is consistent. + // - We may operate as if all descendants are consistent, since `propagate_recursive` will panic before + // continuing to propagate if it encounters an entity with inconsistent parentage. // - Since each root entity is unique and the hierarchy is consistent and forest-like, // other root entities' `propagate_recursive` calls will not conflict with this one. // - Since this is the only place where `transform_query` gets used, there will be no conflicting fetches elsewhere. @@ -60,10 +62,8 @@ pub fn propagate_transforms( &global_transform, &transform_query, &parent_query, - &children_query, - entity, - *child, - changed, + child, + changed || actual_parent.is_changed(), ); } } @@ -75,35 +75,30 @@ pub fn propagate_transforms( /// /// # Panics /// -/// If `entity` or any of its descendants have a malformed hierarchy. -/// The panic will occur before propagating the transforms of any malformed entities and their descendants. +/// If `entity`'s descendants have a malformed hierarchy, this function will panic occur before propagating +/// the transforms of any malformed entities and their descendants. /// /// # Safety /// -/// While this function is running, `unsafe_transform_query` must not have any fetches for `entity`, +/// - While this function is running, `transform_query` must not have any fetches for `entity`, /// nor any of its descendants. +/// - The caller must ensure that the hierarchy leading to `entity` +/// is well-formed and must remain as a tree or a forest. Each entity must have at most one parent. unsafe fn propagate_recursive( parent: &GlobalTransform, - unsafe_transform_query: &Query<(Ref, &mut GlobalTransform), With>, - parent_query: &Query<&Parent>, - children_query: &Query, (With, With)>, - expected_parent: Entity, + transform_query: &Query< + (Ref, &mut GlobalTransform, Option<&Children>), + With, + >, + parent_query: &Query<(Entity, Ref)>, entity: Entity, mut changed: bool, ) { - let Ok(actual_parent) = parent_query.get(entity) else { - panic!("Propagated child for {entity:?} has no Parent component!"); - }; - assert_eq!( - actual_parent.get(), expected_parent, - "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" - ); - - let global_matrix = { - let Ok((transform, mut global_transform)) = + let (global_matrix, children) = { + let Ok((transform, mut global_transform, children)) = // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. - // - The above assertion ensures that each child has one and only one unique parent throughout the entire + // - The caller ensures that each child has one and only one unique parent throughout the entire // hierarchy. // // For example, consider the following malformed hierarchy: @@ -127,7 +122,7 @@ unsafe fn propagate_recursive( // // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting // to mutably access E. - (unsafe { unsafe_transform_query.get_unchecked(entity) }) else { + (unsafe { transform_query.get_unchecked(entity) }) else { return; }; @@ -135,26 +130,27 @@ unsafe fn propagate_recursive( if changed { *global_transform = parent.mul_transform(*transform); } - *global_transform + (*global_transform, children) }; - let Ok(children) = children_query.get(entity) else { - return - }; - // If our `Children` has changed, we need to recalculate everything below us - changed |= children.is_changed(); - for child in &children { - // SAFETY: The caller guarantees that `unsafe_transform_query` will not be fetched + let Some(children) = children else { return }; + for (child, actual_parent) in parent_query.iter_many(children) { + assert_eq!( + actual_parent.get(), entity, + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" + ); + // SAFETY: The caller guarantees that `transform_query` will not be fetched // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. + // + // The above assertion ensures that each child has one and only one unique parent throughout the + // entire hierarchy. unsafe { propagate_recursive( &global_matrix, - unsafe_transform_query, + transform_query, parent_query, - children_query, - entity, - *child, - changed, + child, + changed || actual_parent.is_changed(), ); } }