From 8c99a1cd5acd59565df91b9ae2725a4f41155ab3 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 00:28:56 +0800 Subject: [PATCH 01/14] Added `QueryRecursive`. --- crates/bevy_hierarchy/src/lib.rs | 9 +- crates/bevy_hierarchy/src/recursive.rs | 259 +++++++++++++++++++++++++ 2 files changed, 266 insertions(+), 2 deletions(-) create mode 100644 crates/bevy_hierarchy/src/recursive.rs diff --git a/crates/bevy_hierarchy/src/lib.rs b/crates/bevy_hierarchy/src/lib.rs index 0be21f6d16bbf..937d7e6b62ad3 100644 --- a/crates/bevy_hierarchy/src/lib.rs +++ b/crates/bevy_hierarchy/src/lib.rs @@ -1,5 +1,5 @@ #![cfg_attr(docsrs, feature(doc_auto_cfg))] -#![forbid(unsafe_code)] +#![deny(unsafe_code)] #![doc( html_logo_url = "https://bevyengine.org/assets/icon.png", html_favicon_url = "https://bevyengine.org/assets/icon.png" @@ -70,10 +70,15 @@ pub use valid_parent_check_plugin::*; mod query_extension; pub use query_extension::*; +mod recursive; +pub use recursive::QueryRecursive; + #[doc(hidden)] pub mod prelude { #[doc(hidden)] - pub use crate::{child_builder::*, components::*, hierarchy::*, query_extension::*}; + pub use crate::{ + child_builder::*, components::*, hierarchy::*, query_extension::*, QueryRecursive, + }; #[doc(hidden)] #[cfg(feature = "bevy_app")] diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs new file mode 100644 index 0000000000000..780022d359f27 --- /dev/null +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -0,0 +1,259 @@ +use bevy_ecs::{ + entity::Entity, + query::{QueryData, QueryFilter, With, WorldQuery}, + system::{Query, SystemParam}, +}; + +use crate::{Children, Parent}; + +/// [`SystemParam`] that provide mutable hierarchical access to data stored in the world. +/// +/// When queried, we recursively look for entities from parent to child, along the queries defined by the user. +/// +/// [`QueryRecursive`] is a generic data structure that accepts `5` type parameters: +/// +/// * `QShared` is present in all entities, a read only version of this item is passed down from parent to child during iteration. +/// * `QRoot` is present on root entities only. +/// * `QChild` is present on child entities only. +/// * `FRoot` is a [`QueryFilter`] for root entities. +/// * `FChild` is a [`QueryFilter`] for child entities, [`With`] is automatically added. +/// +/// The user is responsible for excluding the all child entities in `FRoot` (for example use `Without`). +/// +/// # Example +/// +/// A naive transform pipeline implementation. +/// ``` +/// # use bevy_ecs::prelude::*; +/// # use bevy_hierarchy::prelude::*; +/// # #[derive(Clone, Copy, Component)] pub struct Transform; +/// # #[derive(Clone, Copy, Component)] pub struct GlobalTransform; +/// +/// # impl Transform { +/// # fn into(self) -> GlobalTransform { +/// # GlobalTransform +/// # } +/// # } +/// +/// # impl GlobalTransform { +/// # fn mul_transform(self, global: Transform) -> GlobalTransform { +/// # GlobalTransform +/// # } +/// # } +/// fn propagate_transforms(mut query: QueryRecursive< +/// (&Transform, &mut GlobalTransform), +/// (), +/// (), +/// Without, +/// () +/// >) { +/// query.for_each_mut( +/// |(transform, mut global_transform), ()| +/// *global_transform = (*transform).into(), +/// |(_, parent), (transform, mut global_transform), ()| +/// *global_transform = parent.mul_transform(*transform), +/// ); +/// } +/// ``` +/// +/// # Panics +/// +/// If hierarchy is malformed, for example if a parent child mismatch is found. +#[derive(SystemParam)] +pub struct QueryRecursive< + 'w, + 's, + QShared: QueryData + 'static, + QRoot: QueryData + 'static, + QChild: QueryData + 'static, + FRoot: QueryFilter + 'static, + FChild: QueryFilter + 'static, +> { + root: Query<'w, 's, (QShared, QRoot, Option<&'static Children>), FRoot>, + children: Query<'w, 's, (QShared, QChild, Option<&'static Children>), (FChild, With)>, + parent: Query<'w, 's, (Entity, &'static Parent)>, +} + +type Item<'t, T> = ::Item<'t>; +type ReadItem<'t, T> = <::ReadOnly as WorldQuery>::Item<'t>; + +impl< + QShared: QueryData + 'static, + QRoot: QueryData + 'static, + QChild: QueryData + 'static, + FRoot: QueryFilter + 'static, + FChild: QueryFilter + 'static, + > QueryRecursive<'_, '_, QShared, QRoot, QChild, FRoot, FChild> +{ + /// Iterate through the [`QueryRecursive`] hierarchy. + /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + pub fn for_each( + &self, + root_fn: impl FnMut(&ReadItem, ReadItem), + mut child_fn: impl FnMut(&ReadItem, ReadItem, ReadItem), + ) { + self.for_each_with(root_fn, |a, _, b, c| child_fn(a, b, c)); + } + + /// Iterate through the [`QueryRecursive`] hierarchy. + /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + pub fn for_each_mut( + &mut self, + root_fn: impl FnMut(Item, Item), + mut child_fn: impl FnMut(&ReadItem, Item, Item), + ) { + self.for_each_mut_with(root_fn, |a, _, b, c| child_fn(a, b, c)); + } + + /// Iterate through a the [`QueryRecursive`] hierarchy while passing down an evaluation result from parent to child. + /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + #[allow(unsafe_code)] + pub fn for_each_with( + &self, + mut root_fn: impl FnMut(&ReadItem, ReadItem) -> T, + mut child_fn: impl FnMut(&ReadItem, &T, ReadItem, ReadItem) -> T, + ) { + for (shared, owned, children) in self.root.iter() { + let info = root_fn(&shared, owned); + let Some(children) = children else { + continue; + }; + for entity in children { + // Safety: `self.children` is not fetched while this is running. + unsafe { + propagate( + &shared, + &info, + &self.children.to_readonly(), + &self.parent, + *entity, + &mut child_fn, + ); + }; + } + } + } + + /// Iterate through a the [`QueryRecursive`] hierarchy while passing down an evaluation result from parent to child. + /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + #[allow(unsafe_code)] + pub fn for_each_mut_with( + &mut self, + mut root_fn: impl FnMut(Item, Item) -> T, + mut child_fn: impl FnMut(&ReadItem, &T, Item, Item) -> T, + ) { + let infos: Vec<_> = self + .root + .iter_mut() + .map(|(shared, root, _)| root_fn(shared, root)) + .collect(); + for ((shared, _, children), info) in self.root.iter().zip(infos) { + let Some(children) = children else { + continue; + }; + for entity in children { + // Safety: `self.children` is not fetched while this is running. + unsafe { + propagate( + &shared, + &info, + &self.children, + &self.parent, + *entity, + &mut child_fn, + ); + }; + } + } + } +} + +/// Recursively run a function on descendants, passing immutable references of parent to child. +/// +/// # Panics +/// +/// If `entity`'s descendants have a malformed hierarchy, this function will panic. +/// +/// # Safety +/// +/// - While this function is running, `main_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. +#[allow(unsafe_code)] +unsafe fn propagate< + QShared: QueryData + 'static, + QMain: QueryData + 'static, + Filter: QueryFilter + 'static, + Info: 'static, +>( + parent: &ReadItem, + parent_info: &Info, + main_query: &Query<(QShared, QMain, Option<&'static Children>), (Filter, With)>, + parent_query: &Query<(Entity, &Parent)>, + entity: Entity, + mut function: impl FnMut(&ReadItem, &Info, Item, Item) -> Info, +) { + // SAFETY: This call cannot create aliased mutable references. + // - The top level iteration parallelizes on the roots of the hierarchy. + // - The caller ensures that each child has one and only one unique parent throughout the entire + // hierarchy. + // + // For example, consider the following malformed hierarchy: + // + // A + // / \ + // B C + // \ / + // D + // + // D has two parents, B and C. If the propagation passes through C, but the Parent component on D points to B, + // the above check will panic as the origin parent does match the recorded parent. + // + // Also consider the following case, where A and B are roots: + // + // A B + // \ / + // C D + // \ / + // E + // + // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting + // to mutably access E. + let info = { + let Ok((shared, owned, _)) = + // Safety: see above. + (unsafe { main_query.get_unchecked(entity) }) else { + return; + }; + + function(parent, parent_info, shared, owned) + }; + + let Ok((shared, _, children)) = main_query.get(entity) else { + return; + }; + + 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 `main_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( + &shared, + &info, + main_query, + parent_query, + child, + &mut function, + ); + } + } +} From 49c570f7ff19b7cb51a0ca61c04b73ea9eb74fa7 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 00:43:49 +0800 Subject: [PATCH 02/14] Update recursive.rs --- crates/bevy_hierarchy/src/recursive.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 780022d359f27..e95900f2aae5f 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -19,7 +19,7 @@ use crate::{Children, Parent}; /// * `FChild` is a [`QueryFilter`] for child entities, [`With`] is automatically added. /// /// The user is responsible for excluding the all child entities in `FRoot` (for example use `Without`). -/// +/// /// # Example /// /// A naive transform pipeline implementation. From 30af92bb0eb8bf16b60474565f94bbb31c2837d5 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 01:57:18 +0800 Subject: [PATCH 03/14] Soundness fix: now panics if a cycle is found. --- crates/bevy_hierarchy/src/recursive.rs | 45 ++++++++++++++++++++------ 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index e95900f2aae5f..be71f553f78ba 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -28,13 +28,13 @@ use crate::{Children, Parent}; /// # use bevy_hierarchy::prelude::*; /// # #[derive(Clone, Copy, Component)] pub struct Transform; /// # #[derive(Clone, Copy, Component)] pub struct GlobalTransform; -/// +/// # /// # impl Transform { /// # fn into(self) -> GlobalTransform { /// # GlobalTransform /// # } /// # } -/// +/// # /// # impl GlobalTransform { /// # fn mul_transform(self, global: Transform) -> GlobalTransform { /// # GlobalTransform @@ -58,7 +58,7 @@ use crate::{Children, Parent}; /// /// # Panics /// -/// If hierarchy is malformed, for example if a parent child mismatch is found. +/// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. #[derive(SystemParam)] pub struct QueryRecursive< 'w, @@ -87,6 +87,10 @@ impl< { /// Iterate through the [`QueryRecursive`] hierarchy. /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + /// + /// # Panics + /// + /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each( &self, root_fn: impl FnMut(&ReadItem, ReadItem), @@ -95,8 +99,12 @@ impl< self.for_each_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } - /// Iterate through the [`QueryRecursive`] hierarchy. + /// Mutably iterate through the [`QueryRecursive`] hierarchy. /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + /// + /// # Panics + /// + /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each_mut( &mut self, root_fn: impl FnMut(Item, Item), @@ -105,8 +113,12 @@ impl< self.for_each_mut_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } - /// Iterate through a the [`QueryRecursive`] hierarchy while passing down an evaluation result from parent to child. - /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + /// Iterate through the [`QueryRecursive`] hierarchy while passing down an evaluation result from parent to child. + /// Children also receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + /// + /// # Panics + /// + /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. #[allow(unsafe_code)] pub fn for_each_with( &self, @@ -122,6 +134,7 @@ impl< // Safety: `self.children` is not fetched while this is running. unsafe { propagate( + *entity, &shared, &info, &self.children.to_readonly(), @@ -134,8 +147,12 @@ impl< } } - /// Iterate through a the [`QueryRecursive`] hierarchy while passing down an evaluation result from parent to child. - /// Children receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + /// Mutably iterate through the [`QueryRecursive`] hierarchy while passing down an evaluation result from parent to child. + /// Children also receives a readonly reference to their parent's `QShared` [`QueryData`] as the first argument. + /// + /// # Panics + /// + /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. #[allow(unsafe_code)] pub fn for_each_mut_with( &mut self, @@ -155,6 +172,7 @@ impl< // Safety: `self.children` is not fetched while this is running. unsafe { propagate( + *entity, &shared, &info, &self.children, @@ -187,6 +205,7 @@ unsafe fn propagate< Filter: QueryFilter + 'static, Info: 'static, >( + actual_root: Entity, parent: &ReadItem, parent_info: &Info, main_query: &Query<(QShared, QMain, Option<&'static Children>), (Filter, With)>, @@ -236,9 +255,16 @@ unsafe fn propagate< let Some(children) = children else { return }; for (child, actual_parent) in parent_query.iter_many(children) { + // Check if entities are chained properly. assert_eq!( actual_parent.get(), entity, - "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained." + ); + // Since entities are chained properly, The only error that can occur is forming a circle with the root node. + // This was not needed in `propagate_transform` since the root node did not have parents. + assert_ne!( + actual_root, entity, + "Malformed hierarchy. Your hierarchy contains a cycle" ); // SAFETY: The caller guarantees that `main_query` will not be fetched // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. @@ -247,6 +273,7 @@ unsafe fn propagate< // entire hierarchy. unsafe { propagate( + actual_root, &shared, &info, main_query, From 60afdf706fca3f7ad47a8d37fc224f88bba0883c Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 02:12:39 +0800 Subject: [PATCH 04/14] Reworked access patterns. --- crates/bevy_hierarchy/src/recursive.rs | 49 ++++++++++---------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index be71f553f78ba..7c7d92331cffc 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -93,8 +93,8 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each( &self, - root_fn: impl FnMut(&ReadItem, ReadItem), - mut child_fn: impl FnMut(&ReadItem, ReadItem, ReadItem), + root_fn: impl FnMut(&ReadItem, &ReadItem), + mut child_fn: impl FnMut(&ReadItem, &ReadItem, &ReadItem), ) { self.for_each_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } @@ -107,8 +107,8 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each_mut( &mut self, - root_fn: impl FnMut(Item, Item), - mut child_fn: impl FnMut(&ReadItem, Item, Item), + root_fn: impl FnMut(&mut Item, &mut Item), + mut child_fn: impl FnMut(&Item, &mut Item, &mut Item), ) { self.for_each_mut_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } @@ -122,11 +122,11 @@ impl< #[allow(unsafe_code)] pub fn for_each_with( &self, - mut root_fn: impl FnMut(&ReadItem, ReadItem) -> T, - mut child_fn: impl FnMut(&ReadItem, &T, ReadItem, ReadItem) -> T, + mut root_fn: impl FnMut(&ReadItem, &ReadItem) -> T, + mut child_fn: impl FnMut(&ReadItem, &T, &ReadItem, &ReadItem) -> T, ) { for (shared, owned, children) in self.root.iter() { - let info = root_fn(&shared, owned); + let info = root_fn(&shared, &owned); let Some(children) = children else { continue; }; @@ -140,7 +140,7 @@ impl< &self.children.to_readonly(), &self.parent, *entity, - &mut child_fn, + |a, b, c, d| child_fn(a, b, c, d), ); }; } @@ -156,15 +156,11 @@ impl< #[allow(unsafe_code)] pub fn for_each_mut_with( &mut self, - mut root_fn: impl FnMut(Item, Item) -> T, - mut child_fn: impl FnMut(&ReadItem, &T, Item, Item) -> T, + mut root_fn: impl FnMut(&mut Item, &mut Item) -> T, + mut child_fn: impl FnMut(&Item, &T, &mut Item, &mut Item) -> T, ) { - let infos: Vec<_> = self - .root - .iter_mut() - .map(|(shared, root, _)| root_fn(shared, root)) - .collect(); - for ((shared, _, children), info) in self.root.iter().zip(infos) { + for (mut shared, mut root, children) in self.root.iter_mut() { + let info = root_fn(&mut shared, &mut root); let Some(children) = children else { continue; }; @@ -206,12 +202,12 @@ unsafe fn propagate< Info: 'static, >( actual_root: Entity, - parent: &ReadItem, + parent: &Item, parent_info: &Info, main_query: &Query<(QShared, QMain, Option<&'static Children>), (Filter, With)>, parent_query: &Query<(Entity, &Parent)>, entity: Entity, - mut function: impl FnMut(&ReadItem, &Info, Item, Item) -> Info, + mut function: impl FnMut(&Item, &Info, &mut Item, &mut Item) -> Info, ) { // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. @@ -239,20 +235,13 @@ unsafe fn propagate< // // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting // to mutably access E. - let info = { - let Ok((shared, owned, _)) = - // Safety: see above. - (unsafe { main_query.get_unchecked(entity) }) else { - return; - }; - - function(parent, parent_info, shared, owned) - }; - - let Ok((shared, _, children)) = main_query.get(entity) else { + let Ok((mut shared, mut owned, children)) = (unsafe { main_query.get_unchecked(entity) }) + else { return; }; + let info = function(parent, parent_info, &mut shared, &mut owned); + let Some(children) = children else { return }; for (child, actual_parent) in parent_query.iter_many(children) { // Check if entities are chained properly. @@ -264,7 +253,7 @@ unsafe fn propagate< // This was not needed in `propagate_transform` since the root node did not have parents. assert_ne!( actual_root, entity, - "Malformed hierarchy. Your hierarchy contains a cycle" + "Malformed hierarchy. The hierarchy contains a cycle" ); // SAFETY: The caller guarantees that `main_query` will not be fetched // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. From 1a6e90cc478f8789f84992d7b2f2c1660c242230 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 02:52:23 +0800 Subject: [PATCH 05/14] Revert "Reworked access patterns." This reverts commit 60afdf706fca3f7ad47a8d37fc224f88bba0883c. --- crates/bevy_hierarchy/src/recursive.rs | 49 ++++++++++++++++---------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 7c7d92331cffc..be71f553f78ba 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -93,8 +93,8 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each( &self, - root_fn: impl FnMut(&ReadItem, &ReadItem), - mut child_fn: impl FnMut(&ReadItem, &ReadItem, &ReadItem), + root_fn: impl FnMut(&ReadItem, ReadItem), + mut child_fn: impl FnMut(&ReadItem, ReadItem, ReadItem), ) { self.for_each_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } @@ -107,8 +107,8 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each_mut( &mut self, - root_fn: impl FnMut(&mut Item, &mut Item), - mut child_fn: impl FnMut(&Item, &mut Item, &mut Item), + root_fn: impl FnMut(Item, Item), + mut child_fn: impl FnMut(&ReadItem, Item, Item), ) { self.for_each_mut_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } @@ -122,11 +122,11 @@ impl< #[allow(unsafe_code)] pub fn for_each_with( &self, - mut root_fn: impl FnMut(&ReadItem, &ReadItem) -> T, - mut child_fn: impl FnMut(&ReadItem, &T, &ReadItem, &ReadItem) -> T, + mut root_fn: impl FnMut(&ReadItem, ReadItem) -> T, + mut child_fn: impl FnMut(&ReadItem, &T, ReadItem, ReadItem) -> T, ) { for (shared, owned, children) in self.root.iter() { - let info = root_fn(&shared, &owned); + let info = root_fn(&shared, owned); let Some(children) = children else { continue; }; @@ -140,7 +140,7 @@ impl< &self.children.to_readonly(), &self.parent, *entity, - |a, b, c, d| child_fn(a, b, c, d), + &mut child_fn, ); }; } @@ -156,11 +156,15 @@ impl< #[allow(unsafe_code)] pub fn for_each_mut_with( &mut self, - mut root_fn: impl FnMut(&mut Item, &mut Item) -> T, - mut child_fn: impl FnMut(&Item, &T, &mut Item, &mut Item) -> T, + mut root_fn: impl FnMut(Item, Item) -> T, + mut child_fn: impl FnMut(&ReadItem, &T, Item, Item) -> T, ) { - for (mut shared, mut root, children) in self.root.iter_mut() { - let info = root_fn(&mut shared, &mut root); + let infos: Vec<_> = self + .root + .iter_mut() + .map(|(shared, root, _)| root_fn(shared, root)) + .collect(); + for ((shared, _, children), info) in self.root.iter().zip(infos) { let Some(children) = children else { continue; }; @@ -202,12 +206,12 @@ unsafe fn propagate< Info: 'static, >( actual_root: Entity, - parent: &Item, + parent: &ReadItem, parent_info: &Info, main_query: &Query<(QShared, QMain, Option<&'static Children>), (Filter, With)>, parent_query: &Query<(Entity, &Parent)>, entity: Entity, - mut function: impl FnMut(&Item, &Info, &mut Item, &mut Item) -> Info, + mut function: impl FnMut(&ReadItem, &Info, Item, Item) -> Info, ) { // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. @@ -235,12 +239,19 @@ unsafe fn propagate< // // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting // to mutably access E. - let Ok((mut shared, mut owned, children)) = (unsafe { main_query.get_unchecked(entity) }) - else { - return; + let info = { + let Ok((shared, owned, _)) = + // Safety: see above. + (unsafe { main_query.get_unchecked(entity) }) else { + return; + }; + + function(parent, parent_info, shared, owned) }; - let info = function(parent, parent_info, &mut shared, &mut owned); + let Ok((shared, _, children)) = main_query.get(entity) else { + return; + }; let Some(children) = children else { return }; for (child, actual_parent) in parent_query.iter_many(children) { @@ -253,7 +264,7 @@ unsafe fn propagate< // This was not needed in `propagate_transform` since the root node did not have parents. assert_ne!( actual_root, entity, - "Malformed hierarchy. The hierarchy contains a cycle" + "Malformed hierarchy. Your hierarchy contains a cycle" ); // SAFETY: The caller guarantees that `main_query` will not be fetched // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. From b1a84b29c04f275c633172849ec75a64f9e94407 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 02:58:46 +0800 Subject: [PATCH 06/14] Update recursive.rs --- crates/bevy_hierarchy/src/recursive.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index be71f553f78ba..b112bcd716888 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -93,7 +93,7 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each( &self, - root_fn: impl FnMut(&ReadItem, ReadItem), + root_fn: impl FnMut(ReadItem, ReadItem), mut child_fn: impl FnMut(&ReadItem, ReadItem, ReadItem), ) { self.for_each_with(root_fn, |a, _, b, c| child_fn(a, b, c)); @@ -122,11 +122,15 @@ impl< #[allow(unsafe_code)] pub fn for_each_with( &self, - mut root_fn: impl FnMut(&ReadItem, ReadItem) -> T, + mut root_fn: impl FnMut(ReadItem, ReadItem) -> T, mut child_fn: impl FnMut(&ReadItem, &T, ReadItem, ReadItem) -> T, ) { - for (shared, owned, children) in self.root.iter() { - let info = root_fn(&shared, owned); + let infos: Vec<_> = self + .root + .iter() + .map(|(shared, root, _)| root_fn(shared, root)) + .collect(); + for ((shared, _, children), info) in self.root.iter().zip(infos) { let Some(children) = children else { continue; }; From ef6ad29b25970fcbe8d13f346cc26240b9295481 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 03:20:05 +0800 Subject: [PATCH 07/14] Update recursive.rs --- crates/bevy_hierarchy/src/recursive.rs | 52 +++++++++----------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index b112bcd716888..7a3a2d75fb698 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -93,8 +93,8 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each( &self, - root_fn: impl FnMut(ReadItem, ReadItem), - mut child_fn: impl FnMut(&ReadItem, ReadItem, ReadItem), + root_fn: impl FnMut(&ReadItem, ReadItem), + mut child_fn: impl FnMut(&ReadItem, &ReadItem, ReadItem), ) { self.for_each_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } @@ -107,8 +107,8 @@ impl< /// If hierarchy is malformed, for example if a parent child mismatch or a cycle is found. pub fn for_each_mut( &mut self, - root_fn: impl FnMut(Item, Item), - mut child_fn: impl FnMut(&ReadItem, Item, Item), + root_fn: impl FnMut(&mut Item, Item), + mut child_fn: impl FnMut(&Item, &mut Item, Item), ) { self.for_each_mut_with(root_fn, |a, _, b, c| child_fn(a, b, c)); } @@ -122,15 +122,11 @@ impl< #[allow(unsafe_code)] pub fn for_each_with( &self, - mut root_fn: impl FnMut(ReadItem, ReadItem) -> T, - mut child_fn: impl FnMut(&ReadItem, &T, ReadItem, ReadItem) -> T, + mut root_fn: impl FnMut(&ReadItem, ReadItem) -> T, + mut child_fn: impl FnMut(&ReadItem, &T, &ReadItem, ReadItem) -> T, ) { - let infos: Vec<_> = self - .root - .iter() - .map(|(shared, root, _)| root_fn(shared, root)) - .collect(); - for ((shared, _, children), info) in self.root.iter().zip(infos) { + for (shared, owned, children) in self.root.iter() { + let info = root_fn(&shared, owned); let Some(children) = children else { continue; }; @@ -144,7 +140,7 @@ impl< &self.children.to_readonly(), &self.parent, *entity, - &mut child_fn, + |a, b, c, d| child_fn(a, b, c, d), ); }; } @@ -160,15 +156,11 @@ impl< #[allow(unsafe_code)] pub fn for_each_mut_with( &mut self, - mut root_fn: impl FnMut(Item, Item) -> T, - mut child_fn: impl FnMut(&ReadItem, &T, Item, Item) -> T, + mut root_fn: impl FnMut(&mut Item, Item) -> T, + mut child_fn: impl FnMut(&Item, &T, &mut Item, Item) -> T, ) { - let infos: Vec<_> = self - .root - .iter_mut() - .map(|(shared, root, _)| root_fn(shared, root)) - .collect(); - for ((shared, _, children), info) in self.root.iter().zip(infos) { + for (mut shared, owned, children) in self.root.iter_mut() { + let info = root_fn(&mut shared, owned); let Some(children) = children else { continue; }; @@ -210,12 +202,12 @@ unsafe fn propagate< Info: 'static, >( actual_root: Entity, - parent: &ReadItem, + parent: &Item, parent_info: &Info, main_query: &Query<(QShared, QMain, Option<&'static Children>), (Filter, With)>, parent_query: &Query<(Entity, &Parent)>, entity: Entity, - mut function: impl FnMut(&ReadItem, &Info, Item, Item) -> Info, + mut function: impl FnMut(&Item, &Info, &mut Item, Item) -> Info, ) { // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. @@ -243,20 +235,12 @@ unsafe fn propagate< // // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting // to mutably access E. - let info = { - let Ok((shared, owned, _)) = - // Safety: see above. - (unsafe { main_query.get_unchecked(entity) }) else { - return; - }; - - function(parent, parent_info, shared, owned) - }; - - let Ok((shared, _, children)) = main_query.get(entity) else { + let Ok((mut shared, owned, children)) = (unsafe { main_query.get_unchecked(entity) }) else { return; }; + let info = function(parent, parent_info, &mut shared, owned); + let Some(children) = children else { return }; for (child, actual_parent) in parent_query.iter_many(children) { // Check if entities are chained properly. From dc23f685be73a6520449b46e059ed9493445130d Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 03:21:05 +0800 Subject: [PATCH 08/14] Update recursive.rs --- crates/bevy_hierarchy/src/recursive.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 7a3a2d75fb698..86d08c59a176d 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -48,10 +48,10 @@ use crate::{Children, Parent}; /// () /// >) { /// query.for_each_mut( -/// |(transform, mut global_transform), ()| -/// *global_transform = (*transform).into(), -/// |(_, parent), (transform, mut global_transform), ()| -/// *global_transform = parent.mul_transform(*transform), +/// |(transform, global_transform), ()| +/// **global_transform = (**transform).into(), +/// |(_, parent), (transform, global_transform), ()| +/// **global_transform = parent.mul_transform(**transform), /// ); /// } /// ``` From 0d16950ba853e877e8c606c0baec886938f793e7 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 13:48:47 +0800 Subject: [PATCH 09/14] Added test. --- crates/bevy_hierarchy/src/recursive.rs | 188 +++++++++++++++++++++++-- 1 file changed, 176 insertions(+), 12 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 86d08c59a176d..dfd55ca027225 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -12,13 +12,15 @@ use crate::{Children, Parent}; /// /// [`QueryRecursive`] is a generic data structure that accepts `5` type parameters: /// -/// * `QShared` is present in all entities, a read only version of this item is passed down from parent to child during iteration. -/// * `QRoot` is present on root entities only. -/// * `QChild` is present on child entities only. +/// * `QShared` is a [`QueryData`] present on all entities, a read only reference of this item is passed down from parent to child during iteration. +/// * `QRoot` is a [`QueryData`] present on root entities only. +/// * `QChild` is a [`QueryData`] present on child entities only. /// * `FRoot` is a [`QueryFilter`] for root entities. /// * `FChild` is a [`QueryFilter`] for child entities, [`With`] is automatically added. /// -/// The user is responsible for excluding the all child entities in `FRoot` (for example use `Without`). +/// The user is responsible for excluding all child entities in `root` +/// and make sure entities in `root` are not ancestors of each other, +/// for example using `FRoot` `Without`. /// /// # Example /// @@ -69,7 +71,7 @@ pub struct QueryRecursive< FRoot: QueryFilter + 'static, FChild: QueryFilter + 'static, > { - root: Query<'w, 's, (QShared, QRoot, Option<&'static Children>), FRoot>, + root: Query<'w, 's, (Entity, QShared, QRoot, Option<&'static Children>), FRoot>, children: Query<'w, 's, (QShared, QChild, Option<&'static Children>), (FChild, With)>, parent: Query<'w, 's, (Entity, &'static Parent)>, } @@ -125,7 +127,7 @@ impl< mut root_fn: impl FnMut(&ReadItem, ReadItem) -> T, mut child_fn: impl FnMut(&ReadItem, &T, &ReadItem, ReadItem) -> T, ) { - for (shared, owned, children) in self.root.iter() { + for (actual_root, shared, owned, children) in self.root.iter() { let info = root_fn(&shared, owned); let Some(children) = children else { continue; @@ -134,13 +136,13 @@ impl< // Safety: `self.children` is not fetched while this is running. unsafe { propagate( - *entity, + actual_root, &shared, &info, &self.children.to_readonly(), &self.parent, *entity, - |a, b, c, d| child_fn(a, b, c, d), + &mut |a, b, c, d| child_fn(a, b, c, d), ); }; } @@ -159,7 +161,7 @@ impl< mut root_fn: impl FnMut(&mut Item, Item) -> T, mut child_fn: impl FnMut(&Item, &T, &mut Item, Item) -> T, ) { - for (mut shared, owned, children) in self.root.iter_mut() { + for (actual_root, mut shared, owned, children) in self.root.iter_mut() { let info = root_fn(&mut shared, owned); let Some(children) = children else { continue; @@ -168,7 +170,7 @@ impl< // Safety: `self.children` is not fetched while this is running. unsafe { propagate( - *entity, + actual_root, &shared, &info, &self.children, @@ -180,6 +182,11 @@ impl< } } } + + // Note: if to be implemented in the future, + // `par_for_each_mut` is always unsafe unless we + // can guarantee root nodes are not ancestors of each other. + // This can be guaranteed if `Without` is forced or verified to exist. } /// Recursively run a function on descendants, passing immutable references of parent to child. @@ -207,7 +214,7 @@ unsafe fn propagate< main_query: &Query<(QShared, QMain, Option<&'static Children>), (Filter, With)>, parent_query: &Query<(Entity, &Parent)>, entity: Entity, - mut function: impl FnMut(&Item, &Info, &mut Item, Item) -> Info, + function: &mut impl FnMut(&Item, &Info, &mut Item, Item) -> Info, ) { // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. @@ -267,8 +274,165 @@ unsafe fn propagate< main_query, parent_query, child, - &mut function, + function, ); } } } + +#[cfg(test)] +mod test { + use crate::prelude::*; + use bevy_ecs::{prelude::*, system::RunSystemOnce}; + + #[derive(Component)] + pub struct ShouldBe(u32); + #[derive(Component)] + pub struct Transform(u32); + #[derive(Component, Default)] + pub struct GlobalTransform(u32); + + #[derive(Component, Default)] + pub struct Trim; + + #[derive(Component, Default)] + pub struct Trimmed; + + fn test_world() -> World { + let mut world = World::new(); + world + .spawn((Transform(1), GlobalTransform::default(), ShouldBe(1))) + .with_children(|s| { + s.spawn((Transform(1), GlobalTransform::default(), ShouldBe(2))) + .with_children(|s| { + s.spawn((Transform(2), GlobalTransform::default(), ShouldBe(4))); + s.spawn(( + Transform(3), + GlobalTransform::default(), + ShouldBe(5), + Trim, + Trimmed, + )) + .with_children(|s| { + s.spawn(( + Transform(0), + GlobalTransform::default(), + ShouldBe(5), + Trimmed, + )); + s.spawn(( + Transform(2), + GlobalTransform::default(), + ShouldBe(7), + Trimmed, + )); + }); + }); + s.spawn((Transform(2), GlobalTransform::default(), ShouldBe(3))); + s.spawn(( + Transform(3), + GlobalTransform::default(), + ShouldBe(4), + Trim, + Trimmed, + )) + .with_children(|s| { + s.spawn(( + Transform(1), + GlobalTransform::default(), + ShouldBe(5), + Trimmed, + )); + s.spawn(( + Transform(4), + GlobalTransform::default(), + ShouldBe(8), + Trimmed, + )); + }); + s.spawn(( + Transform(4), + GlobalTransform::default(), + ShouldBe(5), + Trim, + Trimmed, + )); + }); + world + } + + #[test] + pub fn test() { + let mut world = test_world(); + world.run_system_once( + |mut query: QueryRecursive< + (&Transform, &mut GlobalTransform), + (), + (), + Without, + (), + >| { + query.for_each_mut( + |(transform, global), ()| global.0 = transform.0, + |(_, parent), (transform, global), ()| global.0 = transform.0 + parent.0, + ); + }, + ); + world + .query::<(&GlobalTransform, &ShouldBe)>() + .iter(&world) + .for_each(|(a, b)| { + assert_eq!(a.0, b.0); + }); + + let mut world = test_world(); + world.run_system_once( + |mut query: QueryRecursive< + &mut GlobalTransform, + &Transform, + &Transform, + Without, + (), + >| { + query.for_each_mut( + |global, transform| global.0 = transform.0, + |parent, global, transform| global.0 = transform.0 + parent.0, + ); + }, + ); + world + .query::<(&GlobalTransform, &ShouldBe)>() + .iter(&world) + .for_each(|(a, b)| { + assert_eq!(a.0, b.0); + }); + + let mut world = test_world(); + world.run_system_once( + |mut query: QueryRecursive< + &mut GlobalTransform, + &Transform, + &Transform, + (Without, Without), + Without, + >| { + query.for_each_mut( + |global, transform| global.0 = transform.0, + |parent, global, transform| global.0 = transform.0 + parent.0, + ); + }, + ); + world + .query_filtered::<(&GlobalTransform, &ShouldBe), Without>() + .iter(&world) + .for_each(|(a, b)| { + assert_eq!(a.0, b.0); + }); + world + .query_filtered::<&GlobalTransform, With>() + .iter(&world) + .for_each(|a| { + assert_eq!(a.0, 0); + }); + } +} From 761922c8b51cba288b12f0d093a5ca83767abf54 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 13:57:12 +0800 Subject: [PATCH 10/14] More tests. --- crates/bevy_hierarchy/src/recursive.rs | 70 ++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index dfd55ca027225..c902b0791920a 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -400,6 +400,35 @@ mod test { ); }, ); + world + .query::<(&GlobalTransform, &ShouldBe)>() + .iter(&world) + .for_each(|(a, b)| { + assert_eq!(a.0, b.0); + }); + + let mut world = test_world(); + world.run_system_once( + |mut query: QueryRecursive< + &mut GlobalTransform, + &Transform, + &Transform, + Without, + (), + >| { + query.for_each_mut_with( + |global, transform| { + global.0 = transform.0; + global.0 + }, + |_, parent, global, transform| { + global.0 = transform.0 + *parent; + global.0 + }, + ); + }, + ); + world .query::<(&GlobalTransform, &ShouldBe)>() .iter(&world) @@ -434,5 +463,46 @@ mod test { .for_each(|a| { assert_eq!(a.0, 0); }); + + world + .query::<(&GlobalTransform, &ShouldBe)>() + .iter(&world) + .for_each(|(a, b)| { + assert_eq!(a.0, b.0); + }); + + let mut world = test_world(); + world.run_system_once( + |mut query: QueryRecursive< + &mut GlobalTransform, + &Transform, + &Transform, + (Without, Without), + Without, + >| { + query.for_each_mut_with( + |global, transform| { + global.0 = transform.0; + global.0 + }, + |_, parent, global, transform| { + global.0 = transform.0 + *parent; + global.0 + }, + ); + }, + ); + world + .query_filtered::<(&GlobalTransform, &ShouldBe), Without>() + .iter(&world) + .for_each(|(a, b)| { + assert_eq!(a.0, b.0); + }); + world + .query_filtered::<&GlobalTransform, With>() + .iter(&world) + .for_each(|a| { + assert_eq!(a.0, 0); + }); } } From a8f7c7a3e8e0fa44c2720409df37d941730a5add Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sat, 1 Jun 2024 14:06:59 +0800 Subject: [PATCH 11/14] Update recursive.rs --- crates/bevy_hierarchy/src/recursive.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index c902b0791920a..2c71f670f5469 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -464,13 +464,6 @@ mod test { assert_eq!(a.0, 0); }); - world - .query::<(&GlobalTransform, &ShouldBe)>() - .iter(&world) - .for_each(|(a, b)| { - assert_eq!(a.0, b.0); - }); - let mut world = test_world(); world.run_system_once( |mut query: QueryRecursive< From 1a116a66626346ed3fa0e46fc20dea77e05b1b90 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sun, 2 Jun 2024 17:56:01 +0800 Subject: [PATCH 12/14] Soundness: Deny self referencing nodes root nodes. --- crates/bevy_hierarchy/src/recursive.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 2c71f670f5469..7ac71a9090939 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -133,7 +133,16 @@ impl< continue; }; for entity in children { + assert_ne!( + *entity, actual_root, + "Malformed hierarchy. Self-referencing entity detected." + ); + assert_eq!( + self.parent.get(*entity).map(|(_, p)| p.get()).ok(), Some(actual_root), + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained." + ); // Safety: `self.children` is not fetched while this is running. + // actual root is the parent of entity and is not self referencing. unsafe { propagate( actual_root, @@ -167,6 +176,14 @@ impl< continue; }; for entity in children { + assert_ne!( + *entity, actual_root, + "Malformed hierarchy. Self-referencing entity detected." + ); + assert_eq!( + self.parent.get(*entity).map(|(_, p)| p.get()).ok(), Some(actual_root), + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained." + ); // Safety: `self.children` is not fetched while this is running. unsafe { propagate( @@ -201,6 +218,8 @@ impl< /// 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. +/// - When called externally, `actual_root` must be the parent of `entity` and not self referencing, +/// when called recursively, `actual_root` must be the ancestor of `entity`. #[allow(unsafe_code)] unsafe fn propagate< QShared: QueryData + 'static, From cbcfedaa52eb8d9ddb14999062a9a18a378b47fd Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sun, 2 Jun 2024 18:16:49 +0800 Subject: [PATCH 13/14] Made the unsafe fn slightly safer. --- crates/bevy_hierarchy/src/recursive.rs | 36 ++++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 7ac71a9090939..00a1e4b38f80a 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -133,16 +133,13 @@ impl< continue; }; for entity in children { - assert_ne!( - *entity, actual_root, - "Malformed hierarchy. Self-referencing entity detected." - ); + // Self referencing root node is checked by `propagate`. assert_eq!( self.parent.get(*entity).map(|(_, p)| p.get()).ok(), Some(actual_root), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained." ); // Safety: `self.children` is not fetched while this is running. - // actual root is the parent of entity and is not self referencing. + // `actual_root` is the parent of entity. unsafe { propagate( actual_root, @@ -176,15 +173,13 @@ impl< continue; }; for entity in children { - assert_ne!( - *entity, actual_root, - "Malformed hierarchy. Self-referencing entity detected." - ); + // Self referencing root node is checked by `propagate`. assert_eq!( self.parent.get(*entity).map(|(_, p)| p.get()).ok(), Some(actual_root), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained." ); // Safety: `self.children` is not fetched while this is running. + // `actual_root` is the parent of entity. unsafe { propagate( actual_root, @@ -208,6 +203,18 @@ impl< /// Recursively run a function on descendants, passing immutable references of parent to child. /// +/// We are iterating through a tree in a DFS, at each point in iteration, we are holding a chain of distinct items, +/// having a loop would be UB. +/// +/// They could be caused by +/// +/// - A broken parent-child chain, detected by a parent-child mismatch. +/// - A self referencing root node. +/// - A parent-child loop back to the root node. +/// +/// These are in general checked by this function, but the parent-child relation between the root and its immediate +/// child must be validated by the caller. +/// /// # Panics /// /// If `entity`'s descendants have a malformed hierarchy, this function will panic. @@ -218,8 +225,9 @@ impl< /// 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. -/// - When called externally, `actual_root` must be the parent of `entity` and not self referencing, -/// when called recursively, `actual_root` must be the ancestor of `entity`. +/// - `parent` must be fetched from `entity`'s parent. +/// - When called externally, `actual_root` must be the parent of `entity`. +/// - When called recursively, `actual_root` must be an ancestor of `entity`. #[allow(unsafe_code)] unsafe fn propagate< QShared: QueryData + 'static, @@ -235,6 +243,12 @@ unsafe fn propagate< entity: Entity, function: &mut impl FnMut(&Item, &Info, &mut Item, Item) -> Info, ) { + // detects self referencing node, proceeding would cause UB. + assert_ne!( + entity, actual_root, + "Malformed hierarchy. Self-referencing entity detected." + ); + // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. // - The caller ensures that each child has one and only one unique parent throughout the entire From fb54ea38374d0f427d6a3ced73b662c1463666b0 Mon Sep 17 00:00:00 2001 From: mintlu8 Date: Sun, 2 Jun 2024 18:32:58 +0800 Subject: [PATCH 14/14] Removed redundant comment. --- crates/bevy_hierarchy/src/recursive.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/bevy_hierarchy/src/recursive.rs b/crates/bevy_hierarchy/src/recursive.rs index 00a1e4b38f80a..47932418e0af1 100644 --- a/crates/bevy_hierarchy/src/recursive.rs +++ b/crates/bevy_hierarchy/src/recursive.rs @@ -133,7 +133,6 @@ impl< continue; }; for entity in children { - // Self referencing root node is checked by `propagate`. assert_eq!( self.parent.get(*entity).map(|(_, p)| p.get()).ok(), Some(actual_root), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained." @@ -173,7 +172,6 @@ impl< continue; }; for entity in children { - // Self referencing root node is checked by `propagate`. assert_eq!( self.parent.get(*entity).map(|(_, p)| p.get()).ok(), Some(actual_root), "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained."