From 90e35bc25f2406dbf68f4e24ec32984c0a7caf9e Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 13:26:22 -0500 Subject: [PATCH 01/12] mark `propagate_recursive` as unsafe and add safety comments --- crates/bevy_transform/src/systems.rs | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 2fb9006076012..5fbab3cd713c2 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -46,21 +46,28 @@ pub fn propagate_transforms( changed |= children_changed; for child in children.iter() { - propagate_recursive( - &global_transform, - &transform_query, - &parent_query, - &children_query, - entity, - *child, - changed, - ); + // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` + // will be unique across each invocation of this loop body. + // Since this is the only place where `transform_query` gets used, `child` will not be borrowed anywhere else. + unsafe { + propagate_recursive( + &global_transform, + &transform_query, + &parent_query, + &children_query, + entity, + *child, + changed, + ) + }; } }, ); } -fn propagate_recursive( +/// # Safety +/// `unsafe_transform_query` must not be borrowed for `entity`, nor any of its children. +unsafe fn propagate_recursive( parent: &GlobalTransform, unsafe_transform_query: &Query< (&Transform, Changed, &mut GlobalTransform), From 5fe198cf5c322197c7a015f893bc67a6fe0f0584 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 13:26:30 -0500 Subject: [PATCH 02/12] remove an outdated comment --- crates/bevy_transform/src/systems.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 5fbab3cd713c2..b7a8eb52951e5 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -78,7 +78,6 @@ unsafe fn propagate_recursive( expected_parent: Entity, entity: Entity, mut changed: bool, - // We use a result here to use the `?` operator. Ideally we'd use a try block instead ) { let Ok(actual_parent) = parent_query.get(entity) else { panic!("Propagated child for {:?} has no Parent component!", entity); From 357efbbbb23494d5d11de5b628b3b500a9fa82f0 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 13:27:26 -0500 Subject: [PATCH 03/12] be more specific in a comment --- 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 b7a8eb52951e5..2f830b8735a79 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -46,7 +46,7 @@ pub fn propagate_transforms( changed |= children_changed; for child in children.iter() { - // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` + // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity // will be unique across each invocation of this loop body. // Since this is the only place where `transform_query` gets used, `child` will not be borrowed anywhere else. unsafe { From 5b7d4bdad0dd1f1d2be423556778cbfc9d295777 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 13:31:18 -0500 Subject: [PATCH 04/12] add another safety comment --- crates/bevy_transform/src/systems.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 2f830b8735a79..9f8b1e0262a36 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -132,15 +132,19 @@ unsafe fn propagate_recursive( // If our `Children` has changed, we need to recalculate everything below us changed |= changed_children; for child in children { - propagate_recursive( - &global_matrix, - unsafe_transform_query, - parent_query, - children_query, - entity, - *child, - changed, - ); + // SAFETY: The caller guarantees that `unsafe_transform_query` will not be borrowed + // for any children of `entity`, so it is safe to call `propagate_recursive` for each child. + unsafe { + propagate_recursive( + &global_matrix, + unsafe_transform_query, + parent_query, + children_query, + entity, + *child, + changed, + ) + }; } } From 448f16dc2b16d98130f0db5d81e7265f8d8a238b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 13:40:53 -0500 Subject: [PATCH 05/12] clippy --- crates/bevy_transform/src/systems.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 9f8b1e0262a36..a472ad4d5bc03 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -58,8 +58,8 @@ pub fn propagate_transforms( entity, *child, changed, - ) - }; + ); + } } }, ); @@ -143,8 +143,8 @@ unsafe fn propagate_recursive( entity, *child, changed, - ) - }; + ); + } } } From 9020bff5bca804e08ffce6f881bb6c3ecaf260ff Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 14:25:41 -0500 Subject: [PATCH 06/12] Update systems.rs --- crates/bevy_transform/src/systems.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index a472ad4d5bc03..1ab08789e261c 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -49,6 +49,7 @@ pub fn propagate_transforms( // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity // will be unique across each invocation of this loop body. // Since this is the only place where `transform_query` gets used, `child` will not be borrowed anywhere else. + // Also, since `child` only has one ancestor, none of its children will be borrowed by other invocations of the loop. unsafe { propagate_recursive( &global_transform, From de3cd69356add786bd2e4d1fc823c718cec59318 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 14:26:00 -0500 Subject: [PATCH 07/12] 'children' -> 'descendants' --- 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 1ab08789e261c..7f0029d28be0a 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -134,7 +134,7 @@ unsafe fn propagate_recursive( changed |= changed_children; for child in children { // SAFETY: The caller guarantees that `unsafe_transform_query` will not be borrowed - // for any children of `entity`, so it is safe to call `propagate_recursive` for each child. + // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. unsafe { propagate_recursive( &global_matrix, From 468da7641460a7b52a101a8cb08f0a5e909cf1be Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 14:34:30 -0500 Subject: [PATCH 08/12] fix a comment --- 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 7f0029d28be0a..3492f31b919ba 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -67,7 +67,7 @@ pub fn propagate_transforms( } /// # Safety -/// `unsafe_transform_query` must not be borrowed for `entity`, nor any of its children. +/// `unsafe_transform_query` must not be borrowed for `entity`, nor any of its descendants. unsafe fn propagate_recursive( parent: &GlobalTransform, unsafe_transform_query: &Query< From bf2682da145c9ed12cbccc9db396a6f5f5570353 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 21 Dec 2022 14:43:05 -0500 Subject: [PATCH 09/12] change another 'children' to 'descendants' --- 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 3492f31b919ba..8b907b5511f87 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -49,7 +49,7 @@ pub fn propagate_transforms( // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity // will be unique across each invocation of this loop body. // Since this is the only place where `transform_query` gets used, `child` will not be borrowed anywhere else. - // Also, since `child` only has one ancestor, none of its children will be borrowed by other invocations of the loop. + // Also, since `child` only has one ancestor, none of its descendants will be borrowed by other invocations of the loop. unsafe { propagate_recursive( &global_transform, From ef337e0057c12ce252da6521b8df3605e463b2aa Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 22 Dec 2022 07:29:15 -0500 Subject: [PATCH 10/12] 'borrow' -> 'fetch' --- crates/bevy_transform/src/systems.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 8b907b5511f87..fa52f6c58b1cb 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -48,8 +48,8 @@ pub fn propagate_transforms( for child in children.iter() { // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity // will be unique across each invocation of this loop body. - // Since this is the only place where `transform_query` gets used, `child` will not be borrowed anywhere else. - // Also, since `child` only has one ancestor, none of its descendants will be borrowed by other invocations of the loop. + // Since this is the only place where `transform_query` gets used, `child` will not get fetched elsewhere. + // Also, since `child` only has one ancestor, none of its descendants will get fetched by other invocations of the loop. unsafe { propagate_recursive( &global_transform, @@ -67,7 +67,7 @@ pub fn propagate_transforms( } /// # Safety -/// `unsafe_transform_query` must not be borrowed for `entity`, nor any of its descendants. +/// `unsafe_transform_query` must not have any existing fetches for `entity`, nor any of its descendants. unsafe fn propagate_recursive( parent: &GlobalTransform, unsafe_transform_query: &Query< @@ -133,7 +133,7 @@ unsafe fn propagate_recursive( // If our `Children` has changed, we need to recalculate everything below us changed |= changed_children; for child in children { - // SAFETY: The caller guarantees that `unsafe_transform_query` will not be borrowed + // SAFETY: The caller guarantees that `unsafe_transform_query` will not be fetched // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. unsafe { propagate_recursive( From 7c17e2f6bcbd775a1ef35f9d31a9edab58e19225 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Fri, 23 Dec 2022 22:43:31 -0500 Subject: [PATCH 11/12] rewrite docs and comment --- crates/bevy_transform/src/systems.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index fa52f6c58b1cb..66281889027b6 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -46,10 +46,12 @@ pub fn propagate_transforms( changed |= children_changed; for child in children.iter() { - // SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity - // will be unique across each invocation of this loop body. - // Since this is the only place where `transform_query` gets used, `child` will not get fetched elsewhere. - // Also, since `child` only has one ancestor, none of its descendants will get fetched by other invocations of the loop. + // 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. + // - 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. unsafe { propagate_recursive( &global_transform, @@ -66,8 +68,17 @@ pub fn propagate_transforms( ); } +/// Recursively propagates the transforms for `entity` and all of its descendants. +/// +/// # Panics +/// +/// If `entity` or any of its descendants have a malformed hierarchy. +/// The panic will occur propagating the transforms of any malformed entities and their descendants. +/// /// # Safety -/// `unsafe_transform_query` must not have any existing fetches for `entity`, nor any of its descendants. +/// +/// While this function is running, `unsafe_transform_query` must not have any fetches for `entity`, +/// nor any of its descendants. unsafe fn propagate_recursive( parent: &GlobalTransform, unsafe_transform_query: &Query< From 189e1bd344e511cd6bd20c171e3bda2514f9932f Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 24 Dec 2022 09:26:47 -0500 Subject: [PATCH 12/12] add a word --- 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 66281889027b6..53de43a51357f 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -73,7 +73,7 @@ pub fn propagate_transforms( /// # Panics /// /// If `entity` or any of its descendants have a malformed hierarchy. -/// The panic will occur propagating the transforms of any malformed entities and their descendants. +/// The panic will occur before propagating the transforms of any malformed entities and their descendants. /// /// # Safety ///