From c2abcd5c97935981cf2e7cb07f0a5a5604aa9cdc Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 24 Feb 2025 22:12:28 -0500 Subject: [PATCH 1/9] Use register_dynamic for merging --- crates/bevy_ecs/src/component.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index e2712dcb7b2fb..dcd8b4c8c32b5 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2113,7 +2113,7 @@ impl RequiredComponents { unsafe { self.register_dynamic(component_id, erased, inheritance_depth) }; } - /// Iterates the ids of all required components. This includes recursive required components. + /// Iterates the ids of all required components' ids. This includes recursive required components. pub fn iter_ids(&self) -> impl Iterator + '_ { self.0.keys().copied() } @@ -2131,8 +2131,21 @@ impl RequiredComponents { // Merges `required_components` into this collection. This only inserts a required component // if it _did not already exist_. pub(crate) fn merge(&mut self, required_components: &RequiredComponents) { - for (id, constructor) in &required_components.0 { - self.0.entry(*id).or_insert_with(|| constructor.clone()); + for ( + component_id, + RequiredComponent { + constructor, + inheritance_depth, + }, + ) in required_components + .0 + .iter() + .map(|(id, req)| (*id, req.clone())) + { + // SAFETY: This exact registration must have been done on `required_components`, so safety is ensured by that caller. + unsafe { + self.register_dynamic(component_id, constructor, inheritance_depth); + } } } } From 54613b947d94612cf69b8f175740fdb37c0215dd Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Tue, 25 Feb 2025 11:03:25 -0500 Subject: [PATCH 2/9] Added a test for this behavior --- crates/bevy_ecs/src/lib.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 8d415d7469183..429dea5090a93 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2635,6 +2635,37 @@ mod tests { assert_eq!(to_vec(required_z), vec![(b, 0), (c, 1)]); } + #[test] + fn required_components_inheritance_depth_bias() { + #[derive(Component, PartialEq, Eq, Clone, Copy, Debug)] + struct MyRequired(bool); + + #[derive(Component, Default)] + #[require(MyRequired(|| MyRequired(false)))] + struct MiddleMan; + + #[derive(Component, Default)] + #[require(MiddleMan)] + struct ConflictingRequire; + + #[derive(Component, Default)] + #[require(MyRequired(|| MyRequired(true)))] + struct MyComponent; + + let mut world = World::new(); + let order_a = world + .spawn((ConflictingRequire, MyComponent)) + .get::() + .cloned(); + let order_b = world + .spawn((MyComponent, ConflictingRequire)) + .get::() + .cloned(); + + assert_eq!(order_a, Some(MyRequired(true))); + assert_eq!(order_b, Some(MyRequired(true))); + } + #[test] #[should_panic = "Recursive required components detected: A → B → C → B\nhelp: If this is intentional, consider merging the components."] fn required_components_recursion_errors() { From a8e81f6736e776472249f6e4853d3beaf4eeba38 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Thu, 6 Mar 2025 10:56:06 -0500 Subject: [PATCH 3/9] added `register_dynamic_with` --- crates/bevy_ecs/src/component.rs | 48 +++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index dcd8b4c8c32b5..7b11abb1d6b05 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2008,25 +2008,47 @@ impl RequiredComponents { /// `constructor` _must_ initialize a component for `component_id` in such a way that /// matches the storage type of the component. It must only use the given `table_row` or `Entity` to /// initialize the storage for `component_id` corresponding to the given entity. + pub unsafe fn register_dynamic_with( + &mut self, + component_id: ComponentId, + inheritance_depth: u16, + constructor: impl FnOnce() -> RequiredComponentConstructor, + ) { + let entry = self.0.entry(component_id); + match entry { + bevy_platform_support::collections::hash_map::Entry::Occupied(mut occupied) => { + let current = occupied.get_mut(); + if current.inheritance_depth > inheritance_depth { + *current = RequiredComponent { + constructor: constructor(), + inheritance_depth, + } + } + } + bevy_platform_support::collections::hash_map::Entry::Vacant(vacant) => { + vacant.insert(RequiredComponent { + constructor: constructor(), + inheritance_depth, + }); + } + } + } + + /// Forwards to [`register_dynamic_with`](RequiredComponents::register_dynamic_with). + /// + /// # Safety + /// + /// `component_id` must match the type initialized by `constructor`. + /// `constructor` _must_ initialize a component for `component_id` in such a way that + /// matches the storage type of the component. It must only use the given `table_row` or `Entity` to + /// initialize the storage for `component_id` corresponding to the given entity. pub unsafe fn register_dynamic( &mut self, component_id: ComponentId, constructor: RequiredComponentConstructor, inheritance_depth: u16, ) { - self.0 - .entry(component_id) - .and_modify(|component| { - if component.inheritance_depth > inheritance_depth { - // New registration is more specific than existing requirement - component.constructor = constructor.clone(); - component.inheritance_depth = inheritance_depth; - } - }) - .or_insert(RequiredComponent { - constructor, - inheritance_depth, - }); + self.register_dynamic_with(component_id, inheritance_depth, || constructor); } /// Registers a required component. From 9308cd7b10594b0909374ffa4f6949fed415f231 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Thu, 6 Mar 2025 11:00:05 -0500 Subject: [PATCH 4/9] use register_dynamic_with internally --- crates/bevy_ecs/src/component.rs | 127 +++++++++++++++---------------- 1 file changed, 63 insertions(+), 64 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 7b11abb1d6b05..a71e9a9e9daef 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1368,10 +1368,10 @@ impl Components { // SAFETY: Component ID and constructor match the ones on the original requiree. // The original requiree is responsible for making sure the registration is safe. unsafe { - required_components.register_dynamic( + required_components.register_dynamic_with( *component_id, - component.constructor.clone(), component.inheritance_depth + depth + 1, + || component.constructor.clone(), ); }; } @@ -1423,10 +1423,10 @@ impl Components { // Register the required component for the requiree. // SAFETY: Component ID and constructor match the ones on the original requiree. unsafe { - required_components.register_dynamic( + required_components.register_dynamic_with( component_id, - component.constructor, component.inheritance_depth, + || component.constructor, ); }; @@ -1533,11 +1533,9 @@ impl Components { for (id, component) in required { // Register the inherited required components for the requiree. // The inheritance depth is increased by `1` since this is a component required by the original required component. - required_components.register_dynamic( - id, - component.constructor.clone(), - component.inheritance_depth + 1, - ); + required_components.register_dynamic_with(id, component.inheritance_depth + 1, || { + component.constructor.clone() + }); self.get_required_by_mut(id).unwrap().insert(requiree); } } @@ -2075,64 +2073,66 @@ impl RequiredComponents { constructor: fn() -> C, inheritance_depth: u16, ) { - let erased: RequiredComponentConstructor = RequiredComponentConstructor({ - // `portable-atomic-util` `Arc` is not able to coerce an unsized - // type like `std::sync::Arc` can. Creating a `Box` first does the - // coercion. - // - // This would be resolved by https://github.com/rust-lang/rust/issues/123430 - - #[cfg(not(target_has_atomic = "ptr"))] - use alloc::boxed::Box; - - type Constructor = dyn for<'a, 'b> Fn( - &'a mut Table, - &'b mut SparseSets, - Tick, - TableRow, - Entity, - MaybeLocation, - ); + let erased = || { + RequiredComponentConstructor({ + // `portable-atomic-util` `Arc` is not able to coerce an unsized + // type like `std::sync::Arc` can. Creating a `Box` first does the + // coercion. + // + // This would be resolved by https://github.com/rust-lang/rust/issues/123430 + + #[cfg(not(target_has_atomic = "ptr"))] + use alloc::boxed::Box; + + type Constructor = dyn for<'a, 'b> Fn( + &'a mut Table, + &'b mut SparseSets, + Tick, + TableRow, + Entity, + MaybeLocation, + ); - #[cfg(not(target_has_atomic = "ptr"))] - type Intermediate = Box; - - #[cfg(target_has_atomic = "ptr")] - type Intermediate = Arc; - - let boxed: Intermediate = Intermediate::new( - move |table, sparse_sets, change_tick, table_row, entity, caller| { - OwningPtr::make(constructor(), |ptr| { - // SAFETY: This will only be called in the context of `BundleInfo::write_components`, which will - // pass in a valid table_row and entity requiring a C constructor - // C::STORAGE_TYPE is the storage type associated with `component_id` / `C` - // `ptr` points to valid `C` data, which matches the type associated with `component_id` - unsafe { - BundleInfo::initialize_required_component( - table, - sparse_sets, - change_tick, - table_row, - entity, - component_id, - C::STORAGE_TYPE, - ptr, - caller, - ); - } - }); - }, - ); + #[cfg(not(target_has_atomic = "ptr"))] + type Intermediate = Box; + + #[cfg(target_has_atomic = "ptr")] + type Intermediate = Arc; + + let boxed: Intermediate = Intermediate::new( + move |table, sparse_sets, change_tick, table_row, entity, caller| { + OwningPtr::make(constructor(), |ptr| { + // SAFETY: This will only be called in the context of `BundleInfo::write_components`, which will + // pass in a valid table_row and entity requiring a C constructor + // C::STORAGE_TYPE is the storage type associated with `component_id` / `C` + // `ptr` points to valid `C` data, which matches the type associated with `component_id` + unsafe { + BundleInfo::initialize_required_component( + table, + sparse_sets, + change_tick, + table_row, + entity, + component_id, + C::STORAGE_TYPE, + ptr, + caller, + ); + } + }); + }, + ); - Arc::from(boxed) - }); + Arc::from(boxed) + }) + }; // SAFETY: // `component_id` matches the type initialized by the `erased` constructor above. // `erased` initializes a component for `component_id` in such a way that // matches the storage type of the component. It only uses the given `table_row` or `Entity` to // initialize the storage corresponding to the given entity. - unsafe { self.register_dynamic(component_id, erased, inheritance_depth) }; + unsafe { self.register_dynamic_with(component_id, inheritance_depth, erased) }; } /// Iterates the ids of all required components' ids. This includes recursive required components. @@ -2159,14 +2159,13 @@ impl RequiredComponents { constructor, inheritance_depth, }, - ) in required_components - .0 - .iter() - .map(|(id, req)| (*id, req.clone())) + ) in required_components.0.iter() { // SAFETY: This exact registration must have been done on `required_components`, so safety is ensured by that caller. unsafe { - self.register_dynamic(component_id, constructor, inheritance_depth); + self.register_dynamic_with(*component_id, *inheritance_depth, || { + constructor.clone() + }); } } } From 8211c9f55de632371108c511d3b6068b28a89d21 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Thu, 6 Mar 2025 11:01:40 -0500 Subject: [PATCH 5/9] removed unneeded cloning elsewhere --- crates/bevy_ecs/src/component.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index a71e9a9e9daef..025570d33e61f 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1413,7 +1413,7 @@ impl Components { .collect(); // Register the new required components. - for (component_id, component) in inherited_requirements.iter().cloned() { + for (component_id, component) in inherited_requirements.iter() { // SAFETY: The caller ensures that the `requiree` is valid. let required_components = unsafe { self.get_required_components_mut(requiree) @@ -1424,16 +1424,16 @@ impl Components { // SAFETY: Component ID and constructor match the ones on the original requiree. unsafe { required_components.register_dynamic_with( - component_id, + *component_id, component.inheritance_depth, - || component.constructor, + || component.constructor.clone(), ); }; // Add the requiree to the list of components that require the required component. // SAFETY: The caller ensures that the required components are valid. let required_by = unsafe { - self.get_required_by_mut(component_id) + self.get_required_by_mut(*component_id) .debug_checked_unwrap() }; required_by.insert(requiree); From 58055633d32a83112dfc4c4f709a786de568f7ba Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Thu, 6 Mar 2025 11:07:32 -0500 Subject: [PATCH 6/9] fixed formmating from merge --- crates/bevy_ecs/src/component.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index e4bc6d225e598..8ecb38a2832e2 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1523,7 +1523,6 @@ impl Components { let required_by = unsafe { self.get_required_by_mut(required).debug_checked_unwrap() }; required_by.insert(requiree); - self.register_inherited_required_components(requiree, required, required_components); } From c32dd3b427ebc70abde00204245938fd8587b785 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Thu, 6 Mar 2025 11:43:01 -0500 Subject: [PATCH 7/9] removed register_dynamic --- crates/bevy_ecs/src/component.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 8ecb38a2832e2..098eafa682d32 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2018,23 +2018,6 @@ impl RequiredComponents { } } - /// Forwards to [`register_dynamic_with`](RequiredComponents::register_dynamic_with). - /// - /// # Safety - /// - /// `component_id` must match the type initialized by `constructor`. - /// `constructor` _must_ initialize a component for `component_id` in such a way that - /// matches the storage type of the component. It must only use the given `table_row` or `Entity` to - /// initialize the storage for `component_id` corresponding to the given entity. - pub unsafe fn register_dynamic( - &mut self, - component_id: ComponentId, - constructor: RequiredComponentConstructor, - inheritance_depth: u16, - ) { - self.register_dynamic_with(component_id, inheritance_depth, || constructor); - } - /// Registers a required component. /// /// If the component is already registered, it will be overwritten if the given inheritance depth From d06e90ca15075b8d32e91cfc9343e79778f08a34 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Sun, 9 Mar 2025 13:43:01 -0700 Subject: [PATCH 8/9] Update crates/bevy_ecs/src/component.rs Co-authored-by: Joona Aalto --- crates/bevy_ecs/src/component.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 098eafa682d32..f1d9ca4c15779 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2104,7 +2104,7 @@ impl RequiredComponents { unsafe { self.register_dynamic_with(component_id, inheritance_depth, erased) }; } - /// Iterates the ids of all required components' ids. This includes recursive required components. + /// Iterates the ids of all required components. This includes recursive required components. pub fn iter_ids(&self) -> impl Iterator + '_ { self.0.keys().copied() } From a3f68ea6b086f03391484b7893ad94d500d21b51 Mon Sep 17 00:00:00 2001 From: Elliott Pierce Date: Mon, 10 Mar 2025 10:03:22 -0400 Subject: [PATCH 9/9] doc clarifications Co-Authored-By: Joona Aalto --- crates/bevy_ecs/src/component.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index f1d9ca4c15779..7f8954b82e114 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2119,8 +2119,11 @@ impl RequiredComponents { } } - // Merges `required_components` into this collection. This only inserts a required component - // if it _did not already exist_. + /// Merges `required_components` into this collection. This only inserts a required component + /// if it _did not already exist_ *or* if the required component is more specific than the existing one + /// (in other words, if the inheritance depth is smaller). + /// + /// See [`register_dynamic_with`](Self::register_dynamic_with) for details. pub(crate) fn merge(&mut self, required_components: &RequiredComponents) { for ( component_id,