From e6281e1b72c522c345b941675b0ef340e5f37add Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Sat, 28 Jun 2025 19:51:08 +0200 Subject: [PATCH 01/20] Bundles::register_required_components initial --- crates/bevy_ecs/src/bundle.rs | 113 +++++++++++++++++++++++++++++-- crates/bevy_ecs/src/component.rs | 25 ++++--- crates/bevy_ecs/src/world/mod.rs | 11 ++- 3 files changed, 129 insertions(+), 20 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 8efdc60ad9..840e2d09a5 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -62,8 +62,8 @@ use crate::{ }, change_detection::MaybeLocation, component::{ - Component, ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor, - RequiredComponents, StorageType, Tick, + Component, ComponentId, Components, ComponentsRegistrator, RequiredComponent, + RequiredComponentConstructor, RequiredComponents, StorageType, Tick, }, entity::{Entities, Entity, EntityLocation}, lifecycle::{ADD, INSERT, REMOVE, REPLACE}, @@ -512,7 +512,7 @@ pub struct BundleInfo { /// and the range (0..`explicit_components_len`) must be in the same order as the source bundle /// type writes its components in. component_ids: Vec, - required_components: Vec, + required_components: Vec, explicit_components_len: usize, } @@ -570,14 +570,14 @@ impl BundleInfo { let required_components = required_components .0 .into_iter() - .map(|(component_id, v)| { + .map(|(component_id, required_component)| { // Safety: These ids came out of the passed `components`, so they must be valid. let info = unsafe { components.get_info_unchecked(component_id) }; storages.prepare_component(info); // This adds required components to the component_ids list _after_ using that list to remove explicitly provided // components. This ordering is important! component_ids.push(component_id); - v.constructor + required_component }) .collect(); @@ -828,7 +828,7 @@ impl BundleInfo { for (index, component_id) in self.iter_required_components().enumerate() { if !current_archetype.contains(component_id) { - added_required_components.push(self.required_components[index].clone()); + added_required_components.push(self.required_components[index].constructor.clone()); added.push(component_id); // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; @@ -1804,7 +1804,10 @@ impl<'w> BundleSpawner<'w> { table, sparse_sets, &SpawnBundleStatus, - bundle_info.required_components.iter(), + bundle_info + .required_components + .iter() + .map(|r| &r.constructor), entity, table_row, self.change_tick, @@ -1901,6 +1904,7 @@ pub struct Bundles { /// Cache optimized dynamic [`BundleId`] with single component dynamic_component_bundle_ids: HashMap, dynamic_component_storages: HashMap, + components_in_bundles: Vec>, } impl Bundles { @@ -1943,6 +1947,7 @@ impl Bundles { storages: &mut Storages, ) -> BundleId { let bundle_infos = &mut self.bundle_infos; + *self.bundle_ids.entry(TypeId::of::()).or_insert_with(|| { let mut component_ids= Vec::new(); T::component_ids(components, &mut |id| component_ids.push(id)); @@ -1953,7 +1958,17 @@ impl Bundles { // - appropriate storage for it has been initialized. // - it was created in the same order as the components in T unsafe { BundleInfo::new(core::any::type_name::(), storages, components, component_ids, id) }; + + for component in bundle_info.contributed_components() { + components_in_bundles_push( + &mut self.components_in_bundles, + *component, + id + ); + } + bundle_infos.push(bundle_info); + id }) } @@ -2024,6 +2039,7 @@ impl Bundles { component_ids: &[ComponentId], ) -> BundleId { let bundle_infos = &mut self.bundle_infos; + let components_in_bundles = &mut self.components_in_bundles; // Use `raw_entry_mut` to avoid cloning `component_ids` to access `Entry` let (_, bundle_id) = self @@ -2033,6 +2049,7 @@ impl Bundles { .or_insert_with(|| { let (id, storages) = initialize_dynamic_bundle( bundle_infos, + components_in_bundles, storages, components, Vec::from(component_ids), @@ -2059,12 +2076,14 @@ impl Bundles { component_id: ComponentId, ) -> BundleId { let bundle_infos = &mut self.bundle_infos; + let components_in_bundles = &mut self.components_in_bundles; let bundle_id = self .dynamic_component_bundle_ids .entry(component_id) .or_insert_with(|| { let (id, storage_type) = initialize_dynamic_bundle( bundle_infos, + components_in_bundles, storages, components, vec![component_id], @@ -2074,12 +2093,71 @@ impl Bundles { }); *bundle_id } + + pub(crate) fn register_required_components( + &mut self, + requiree: ComponentId, + required: Vec<(ComponentId, RequiredComponent)>, + ) { + let taken_bundles_with_requiree = self + .components_in_bundles + .get_mut(requiree.index()) + .filter(|bundles| !bundles.is_empty()) + .map(core::mem::take); + + let Some(taken_bundles_with_requiree) = taken_bundles_with_requiree else { + return; + }; + + for bundle_id in taken_bundles_with_requiree.iter() { + let bundle_info = &mut self.bundle_infos[bundle_id.index()]; + + for (newly_required_id, newly_required_component) in required.iter() { + let index = bundle_info + .required_components() + .iter() + .enumerate() + .find_map(|(index, &existing_required_id)| { + (existing_required_id == *newly_required_id).then_some(index) + }); + + match index { + Some(index) => { + let existing_required_component = + &mut bundle_info.required_components[index]; + if existing_required_component.inheritance_depth + > newly_required_component.inheritance_depth + { + *existing_required_component = newly_required_component.clone(); + } + } + None => { + bundle_info.component_ids.push(*newly_required_id); + bundle_info + .required_components + .push(newly_required_component.clone()); + components_in_bundles_push( + &mut self.components_in_bundles, + *newly_required_id, + bundle_info.id, + ); + } + } + } + } + + let replaced = self.components_in_bundles.get_mut(requiree.index()); + // SAFETY: todo + let replaced = unsafe { replaced.debug_checked_unwrap() }; + *replaced = taken_bundles_with_requiree; + } } /// Asserts that all components are part of [`Components`] /// and initializes a [`BundleInfo`]. fn initialize_dynamic_bundle( bundle_infos: &mut Vec, + components_in_bundles: &mut Vec>, storages: &mut Storages, components: &Components, component_ids: Vec, @@ -2097,11 +2175,32 @@ fn initialize_dynamic_bundle( let bundle_info = // SAFETY: `component_ids` are valid as they were just checked unsafe { BundleInfo::new("", storages, components, component_ids, id) }; + + for component in bundle_info.contributed_components() { + components_in_bundles_push(components_in_bundles, *component, id); + } + bundle_infos.push(bundle_info); (id, storage_types) } +fn components_in_bundles_push( + components_in_bundles: &mut Vec>, + component: ComponentId, + bundle: BundleId, +) { + match components_in_bundles.get_mut(component.index()) { + Some(bundles) => bundles.push(bundle), + None => { + let missing_minus_one = component.index() - components_in_bundles.len(); + let iter = core::iter::repeat_n(Vec::new(), missing_minus_one) + .chain(core::iter::once(vec![bundle])); + components_in_bundles.extend(iter); + } + } +} + fn sorted_remove(source: &mut Vec, remove: &[T]) { let mut remove_index = 0; source.retain(|value| { diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 615c5903f8..4779944141 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1986,7 +1986,7 @@ impl Components { requiree: ComponentId, required: ComponentId, constructor: fn() -> R, - ) -> Result<(), RequiredComponentsError> { + ) -> Result, RequiredComponentsError> { // SAFETY: The caller ensures that the `requiree` is valid. let required_components = unsafe { self.get_required_components_mut(requiree) @@ -2006,7 +2006,9 @@ impl Components { // Register the required component for the requiree. // This is a direct requirement with a depth of `0`. - required_components.register_by_id(required, constructor, 0); + let required_component = required_components + .register_by_id(required, constructor, 0) + .clone(); // Add the requiree to the list of components that require the required component. // SAFETY: The component is in the list of required components, so it must exist already. @@ -2015,7 +2017,7 @@ impl Components { let mut required_components_tmp = RequiredComponents::default(); // SAFETY: The caller ensures that the `requiree` and `required` components are valid. - let inherited_requirements = unsafe { + let mut inherited_requirements = unsafe { self.register_inherited_required_components( requiree, required, @@ -2070,7 +2072,9 @@ impl Components { } } - Ok(()) + inherited_requirements.push((required, required_component)); + + Ok(inherited_requirements) } /// Registers the components inherited from `required` for the given `requiree`, @@ -2706,23 +2710,24 @@ impl RequiredComponents { component_id: ComponentId, inheritance_depth: u16, constructor: impl FnOnce() -> RequiredComponentConstructor, - ) { + ) -> &RequiredComponent { let entry = self.0.entry(component_id); match entry { - bevy_platform::collections::hash_map::Entry::Occupied(mut occupied) => { - let current = occupied.get_mut(); + bevy_platform::collections::hash_map::Entry::Occupied(occupied) => { + let current = occupied.into_mut(); if current.inheritance_depth > inheritance_depth { *current = RequiredComponent { constructor: constructor(), inheritance_depth, } } + current } bevy_platform::collections::hash_map::Entry::Vacant(vacant) => { vacant.insert(RequiredComponent { constructor: constructor(), inheritance_depth, - }); + }) } } } @@ -2750,7 +2755,7 @@ impl RequiredComponents { component_id: ComponentId, constructor: fn() -> C, inheritance_depth: u16, - ) { + ) -> &RequiredComponent { let erased = || { RequiredComponentConstructor({ // `portable-atomic-util` `Arc` is not able to coerce an unsized @@ -2810,7 +2815,7 @@ impl RequiredComponents { // `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_with(component_id, inheritance_depth, erased) }; + unsafe { self.register_dynamic_with(component_id, inheritance_depth, erased) } } /// Iterates the ids of all required components. This includes recursive required components. diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 714c5e1eae..5503c8dabc 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -542,10 +542,15 @@ impl World { let required = self.register_component::(); // SAFETY: We just created the `required` and `requiree` components. - unsafe { + let required = unsafe { self.components - .register_required_components::(requiree, required, constructor) - } + .register_required_components::(requiree, required, constructor)? + }; + + self.bundles + .register_required_components(requiree, required); + + Ok(()) } /// Retrieves the [required components](RequiredComponents) for the given component type, if it exists. From 0bac779cec884e3445191e8fab053e20d54384d4 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Sun, 29 Jun 2025 02:38:02 +0200 Subject: [PATCH 02/20] fix bugs, add tests, docs and comments --- crates/bevy_ecs/src/bundle.rs | 38 +++++++- crates/bevy_ecs/src/lib.rs | 153 +++++++++++++++++++++++++++++++ crates/bevy_ecs/src/world/mod.rs | 2 +- 3 files changed, 189 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 840e2d09a5..a8fb4e2de8 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1923,6 +1923,18 @@ impl Bundles { self.bundle_infos.iter() } + /// Iterate over [`BundleInfo`] containing `component`, either explicitly or as required. + pub fn iter_containing(&self, component: ComponentId) -> impl Iterator { + self.components_in_bundles + .get(component.index()) + .into_iter() + .flatten() + .map(|id| unsafe { + // SAFETY: components_in_bundles contains only valid ids + self.bundle_infos.get(id.index()).debug_checked_unwrap() + }) + } + /// Gets the metadata associated with a specific type of bundle. /// Returns `None` if the bundle is not registered with the world. #[inline] @@ -2094,11 +2106,17 @@ impl Bundles { *bundle_id } + /// Updates the required components of bundles that contain `requiree`. + /// + /// Bundles that already contain this required component only update their [`RequiredComponent`] + /// if its [`inheritance_depth`](RequiredComponent::inheritance_depth) is smaller or equal. pub(crate) fn register_required_components( &mut self, requiree: ComponentId, - required: Vec<(ComponentId, RequiredComponent)>, + required: &[(ComponentId, RequiredComponent)], ) { + // take the vector listing the bundles with `requiree` so `components_in_bundles` can be mutated + // while the list is iterated let taken_bundles_with_requiree = self .components_in_bundles .get_mut(requiree.index()) @@ -2109,6 +2127,7 @@ impl Bundles { return; }; + // check each bundle if it needs to be updated for bundle_id in taken_bundles_with_requiree.iter() { let bundle_info = &mut self.bundle_infos[bundle_id.index()]; @@ -2123,15 +2142,19 @@ impl Bundles { match index { Some(index) => { + // required component is already in this bundle + // update `RequiredComponent` if the new one has a smaller or equal `existing_required_component` let existing_required_component = &mut bundle_info.required_components[index]; if existing_required_component.inheritance_depth - > newly_required_component.inheritance_depth + >= newly_required_component.inheritance_depth { *existing_required_component = newly_required_component.clone(); } } None => { + // required component was not yet known to this bundle + // update both `BundleInfo` and `components_in_bundles` bundle_info.component_ids.push(*newly_required_id); bundle_info .required_components @@ -2146,8 +2169,9 @@ impl Bundles { } } + // put the list of bundles with `requiree` back let replaced = self.components_in_bundles.get_mut(requiree.index()); - // SAFETY: todo + // SAFETY: this list has to exist at this index to be taken in the first place let replaced = unsafe { replaced.debug_checked_unwrap() }; *replaced = taken_bundles_with_requiree; } @@ -2185,6 +2209,13 @@ fn initialize_dynamic_bundle( (id, storage_types) } +/// Inserts `bundle` in `components_in_bundles` at [`component.index()`](ComponentId::index). +/// +/// Extends `components_in_bundles` with empty vectors if needed. +/// +/// This method does not check if the bundle is already known to contain this component, +/// so this function should only be called when this was asserted previously or the bundle +/// is new. fn components_in_bundles_push( components_in_bundles: &mut Vec>, component: ComponentId, @@ -2196,6 +2227,7 @@ fn components_in_bundles_push( let missing_minus_one = component.index() - components_in_bundles.len(); let iter = core::iter::repeat_n(Vec::new(), missing_minus_one) .chain(core::iter::once(vec![bundle])); + components_in_bundles.reserve_exact(missing_minus_one + 1); components_in_bundles.extend(iter); } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 86275cd87f..02f292c06c 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2555,6 +2555,159 @@ mod tests { assert_eq!(to_vec(required_z), vec![(b, 0), (c, 1)]); } + #[test] + fn register_required_components_of_explicit_bundle_component_updates_bundle() { + #[derive(Component, Default)] + struct A; + + #[derive(Component, Default)] + #[require(C)] + struct B; + + #[derive(Component, Default)] + struct C; + + let mut world = World::new(); + + world.register_bundle::(); + world.register_required_components::(); + + let entity = world.spawn(A); + assert!(entity.contains::()); + assert!(entity.contains::()); + } + + #[test] + fn register_required_components_of_required_bundle_component_updates_bundle() { + #[derive(Component, Default)] + #[require(B)] + struct A; + + #[derive(Component, Default)] + struct B; + + #[derive(Component, Default)] + #[require(D)] + struct C; + + #[derive(Component, Default)] + struct D; + + let mut world = World::new(); + + world.register_bundle::(); + world.register_required_components::(); + + let entity = world.spawn(A); + assert!(entity.contains::()); + assert!(entity.contains::()); + } + + #[test] + fn register_required_components_with_lower_inheritance_depth_keeps_bundle_unchanged() { + #[derive(Component, Default)] + #[require(C(5))] + struct A; + + #[derive(Component, Default)] + #[require(C(10))] + struct B; + + #[derive(Component, Default, Debug, PartialEq)] + struct C(u8); + + let mut world = World::new(); + + world.register_bundle::(); + world.register_required_components::(); + + let entity = world.spawn(A); + assert_eq!(entity.get::(), Some(&C(5))); + } + + #[test] + fn register_required_components_with_equal_inheritance_depth_updates_bundle() { + #[derive(Component, Default)] + #[require(B)] + struct A; + + #[derive(Component, Default)] + #[require(D(5))] + struct B; + + #[derive(Component, Default)] + #[require(D(10))] + struct C; + + #[derive(Component, Default, Debug, PartialEq)] + struct D(u8); + + let mut world = World::new(); + + world.register_bundle::(); + world.register_required_components::(); + + let entity = world.spawn(A); + assert_eq!(entity.get::(), Some(&D(10))); + } + + #[test] + fn register_required_components_with_higher_inheritance_depth_updates_bundle() { + #[derive(Component, Default)] + #[require(B)] + struct A; + + #[derive(Component, Default)] + #[require(C(5))] + struct B; + + #[derive(Component, Default, Debug, PartialEq)] + struct C(u8); + + let mut world = World::new(); + + world.register_bundle::(); + world.register_required_components_with::(|| C(10)); + + let entity = world.spawn(A); + assert_eq!(entity.get::(), Some(&C(10))); + } + + #[test] + fn register_required_components_with_further_required_with_equal_inheritance_depth_updates_bundle( + ) { + #[derive(Component, Default)] + #[require(B)] + struct A; + + #[derive(Component, Default)] + #[require(C)] + struct B; + + #[derive(Component, Default)] + #[require(F(5))] + struct C; + + #[derive(Component, Default)] + #[require(E)] + struct D; + + #[derive(Component, Default)] + #[require(F(10))] + struct E; + + #[derive(Component, Default, Debug, PartialEq)] + struct F(u8); + + let mut world = World::new(); + + world.register_bundle::(); + world.register_required_components::(); + + let entity = world.spawn(A); + assert_eq!(entity.get::(), Some(&F(10))); // todo: fix and add tests with lower and higher depth + } + #[test] fn required_components_inheritance_depth_bias() { #[derive(Component, PartialEq, Eq, Clone, Copy, Debug)] diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 5503c8dabc..cef7de4d72 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -548,7 +548,7 @@ impl World { }; self.bundles - .register_required_components(requiree, required); + .register_required_components(requiree, &required); Ok(()) } From 06da332055518249449342926d1ae0aea1644891 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Sun, 29 Jun 2025 13:32:07 +0200 Subject: [PATCH 03/20] inheritace_depth offset, temporal debug prints in tests to find issue --- crates/bevy_ecs/src/bundle.rs | 33 +++++++++++++++++++------ crates/bevy_ecs/src/lib.rs | 45 +++++++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index a8fb4e2de8..c3916c9f11 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -2117,19 +2117,36 @@ impl Bundles { ) { // take the vector listing the bundles with `requiree` so `components_in_bundles` can be mutated // while the list is iterated - let taken_bundles_with_requiree = self + let Some(taken_bundles_with_requiree) = self .components_in_bundles .get_mut(requiree.index()) .filter(|bundles| !bundles.is_empty()) - .map(core::mem::take); - - let Some(taken_bundles_with_requiree) = taken_bundles_with_requiree else { + .map(core::mem::take) else { return; }; // check each bundle if it needs to be updated for bundle_id in taken_bundles_with_requiree.iter() { let bundle_info = &mut self.bundle_infos[bundle_id.index()]; + std::println!("bundle {bundle_id:?}"); + + { + //debug + for (&id, required) in bundle_info.required_components().iter().zip(bundle_info.required_components.iter()) { + std::println!("existing_Required_id {id:?}, existing_inheritance_depth {}", required.inheritance_depth); + } + } + + // get inheritance_depth of requiree as that offsets the inheritance_depth of the required components for this bundle + let requiree_inheritance_depth = bundle_info + .required_components() + .iter() + .enumerate() + .find_map(|(index, &existing_requiree_id)| { + (existing_requiree_id == requiree).then_some(index) + }) + .map(|index| bundle_info.required_components[index].inheritance_depth) + .unwrap_or(0); // requiree is explicit component of bundle for (newly_required_id, newly_required_component) in required.iter() { let index = bundle_info @@ -2139,6 +2156,7 @@ impl Bundles { .find_map(|(index, &existing_required_id)| { (existing_required_id == *newly_required_id).then_some(index) }); + std::println!("newly_required_id: {newly_required_id:?}, newly_inheritance_depth: {} + {requiree_inheritance_depth}", newly_required_component.inheritance_depth); match index { Some(index) => { @@ -2146,13 +2164,14 @@ impl Bundles { // update `RequiredComponent` if the new one has a smaller or equal `existing_required_component` let existing_required_component = &mut bundle_info.required_components[index]; - if existing_required_component.inheritance_depth - >= newly_required_component.inheritance_depth - { + std::println!("index: {index}, existing_inheritance_depth: {}", existing_required_component.inheritance_depth); + let newly_inheritance_depth = newly_required_component.inheritance_depth + requiree_inheritance_depth; + if existing_required_component.inheritance_depth >= newly_inheritance_depth { *existing_required_component = newly_required_component.clone(); } } None => { + std::println!("new required component"); // required component was not yet known to this bundle // update both `BundleInfo` and `components_in_bundles` bundle_info.component_ids.push(*newly_required_id); diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 02f292c06c..588b14d76c 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -168,7 +168,7 @@ mod tests { marker::PhantomData, sync::atomic::{AtomicUsize, Ordering}, }; - use std::sync::Mutex; + use std::{println, sync::Mutex}; #[derive(Component, Resource, Debug, PartialEq, Eq, Hash, Clone, Copy)] struct A(usize); @@ -2569,6 +2569,9 @@ mod tests { let mut world = World::new(); + std::println!("A: {:?}", world.register_component::()); + std::println!("B: {:?}", world.register_component::()); + std::println!("C: {:?}", world.register_component::()); world.register_bundle::(); world.register_required_components::(); @@ -2595,6 +2598,10 @@ mod tests { let mut world = World::new(); + std::println!("A: {:?}", world.register_component::()); + std::println!("B: {:?}", world.register_component::()); + std::println!("C: {:?}", world.register_component::()); + std::println!("D: {:?}", world.register_component::()); world.register_bundle::(); world.register_required_components::(); @@ -2618,6 +2625,9 @@ mod tests { let mut world = World::new(); + std::println!("A: {:?}", world.register_component::()); + std::println!("B: {:?}", world.register_component::()); + std::println!("C: {:?}", world.register_component::()); world.register_bundle::(); world.register_required_components::(); @@ -2644,6 +2654,10 @@ mod tests { let mut world = World::new(); + std::println!("A: {:?}", world.register_component::()); + std::println!("B: {:?}", world.register_component::()); + std::println!("C: {:?}", world.register_component::()); + std::println!("D: {:?}", world.register_component::()); world.register_bundle::(); world.register_required_components::(); @@ -2666,6 +2680,9 @@ mod tests { let mut world = World::new(); + std::println!("A: {:?}", world.register_component::()); + std::println!("B: {:?}", world.register_component::()); + std::println!("C: {:?}", world.register_component::()); world.register_bundle::(); world.register_required_components_with::(|| C(10)); @@ -2676,6 +2693,10 @@ mod tests { #[test] fn register_required_components_with_further_required_with_equal_inheritance_depth_updates_bundle( ) { + /* + A -> B -> C -> F + '-> D -> E -> F + */ #[derive(Component, Default)] #[require(B)] struct A; @@ -2701,8 +2722,28 @@ mod tests { let mut world = World::new(); + let a_id = world.register_component::(); + std::println!("A: {:?}", a_id); + std::println!("B: {:?}", world.register_component::()); + std::println!("C: {:?}", world.register_component::()); + std::println!("D: {:?}", world.register_component::()); + std::println!("E: {:?}", world.register_component::()); + std::println!("F: {:?}", world.register_component::()); + + let required: Vec<_> = world + .components() + .get_info(a_id) + .unwrap() + .required_components() + .0 + .iter() + .map(|(id, required)| (id, required.inheritance_depth)) + .collect(); + + std::println!("required of A\n{required:#?}"); + world.register_bundle::(); - world.register_required_components::(); + world.register_required_components::(); let entity = world.spawn(A); assert_eq!(entity.get::(), Some(&F(10))); // todo: fix and add tests with lower and higher depth From 4e0743be1204db8e73c247d6962577189b8cdba3 Mon Sep 17 00:00:00 2001 From: urben1680 <55257931+urben1680@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:26:27 +0200 Subject: [PATCH 04/20] Update bundle.rs --- crates/bevy_ecs/src/bundle.rs | 269 ++++++++++++++++------------------ 1 file changed, 124 insertions(+), 145 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index c3916c9f11..b2442c0ef0 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -62,8 +62,8 @@ use crate::{ }, change_detection::MaybeLocation, component::{ - Component, ComponentId, Components, ComponentsRegistrator, RequiredComponent, - RequiredComponentConstructor, RequiredComponents, StorageType, Tick, + Component, ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor, + RequiredComponents, StorageType, Tick, }, entity::{Entities, Entity, EntityLocation}, lifecycle::{ADD, INSERT, REMOVE, REPLACE}, @@ -512,7 +512,7 @@ pub struct BundleInfo { /// and the range (0..`explicit_components_len`) must be in the same order as the source bundle /// type writes its components in. component_ids: Vec, - required_components: Vec, + required_components: Vec, explicit_components_len: usize, } @@ -527,7 +527,8 @@ impl BundleInfo { bundle_type_name: &'static str, storages: &mut Storages, components: &Components, - mut component_ids: Vec, + components_in_bundles: &mut Vec>, + component_ids: Vec, id: BundleId, ) -> BundleInfo { // check for duplicates @@ -555,41 +556,107 @@ impl BundleInfo { panic!("Bundle {bundle_type_name} has duplicate components: {names:?}"); } - // handle explicit components let explicit_components_len = component_ids.len(); - let mut required_components = RequiredComponents::default(); - for component_id in component_ids.iter().copied() { - // SAFETY: caller has verified that all ids are valid - let info = unsafe { components.get_info_unchecked(component_id) }; - required_components.merge(info.required_components()); - storages.prepare_component(info); - } - required_components.remove_explicit_components(&component_ids); - - // handle required components - let required_components = required_components - .0 - .into_iter() - .map(|(component_id, required_component)| { - // Safety: These ids came out of the passed `components`, so they must be valid. - let info = unsafe { components.get_info_unchecked(component_id) }; - storages.prepare_component(info); - // This adds required components to the component_ids list _after_ using that list to remove explicitly provided - // components. This ordering is important! - component_ids.push(component_id); - required_component - }) - .collect(); // SAFETY: The caller ensures that component_ids: // - is valid for the associated world // - has had its storage initialized // - is in the same order as the source bundle type - BundleInfo { + let mut info = BundleInfo { id, component_ids, - required_components, + required_components: Vec::new(), explicit_components_len, + }; + + // SAFETY: first call with the same `storages` and `components`. + unsafe { + info.set_required_components(storages, components, components_in_bundles, |_| true); + } + + info + } + + /// Update [`Self::component_ids`] to contain all required components, updates [`Self::required_components`] alongside. + /// + /// The filter `components_in_bundles_filter` determines for which components [`Self::id`] is added to `components_in_bundles`. + /// + /// # Safety + /// + /// `Self` must be constructed from [`Self::new`] with the same `storages` and `components`. + unsafe fn set_required_components( + &mut self, + storages: &mut Storages, + components: &Components, + components_in_bundles: &mut Vec>, + components_in_bundles_filter: impl Fn(ComponentId) -> bool, + ) { + // todo: wrong at second call, would add bundleid duplicates! + let mut components_in_bundles_push = |component: ComponentId| { + if !components_in_bundles_filter(component) { + return; + } + match components_in_bundles.get_mut(component.index()) { + Some(bundles) => bundles.push(self.id), + None => { + let missing_minus_one = component.index() - components_in_bundles.len(); + let iter = core::iter::repeat_n(Vec::new(), missing_minus_one) + .chain(core::iter::once(vec![self.id])); + components_in_bundles.reserve_exact(missing_minus_one + 1); + components_in_bundles.extend(iter); + } + } + }; + + // handle explicit components + let mut required_components = RequiredComponents::default(); + for component_id in self.component_ids.iter().copied() { + // SAFETY: caller of initial constructor has verified that all ids are valid + let info = unsafe { components.get_info_unchecked(component_id) }; + required_components.merge(info.required_components()); + storages.prepare_component(info); + components_in_bundles_push(component_id); + } + required_components.remove_explicit_components(&self.component_ids); + + // handle required components + self.required_components + .extend(required_components.0.into_iter().map(|(component_id, v)| { + // Safety: These ids came out of the passed `components`, so they must be valid. + let info = unsafe { components.get_info_unchecked(component_id) }; + storages.prepare_component(info); + // This adds required components to the component_ids list _after_ using that list to remove explicitly provided + // components. This ordering is important! + self.component_ids.push(component_id); + components_in_bundles_push(component_id); + v.constructor + })); + } + + /// Updates required components for new runtime-added components. + /// + /// # Safety + /// + /// `Self` must be constructed from [`Self::new`] with the same `storages` and `components`. + unsafe fn reset_required_components( + &mut self, + storages: &mut Storages, + components: &Components, + components_in_bundles: &mut Vec>, + ) { + let registered_components_in_bundles: HashSet<_> = + self.component_ids.iter().copied().collect(); + self.component_ids.truncate(self.explicit_components_len); + self.required_components.clear(); + + // SAFETY: caller ensured the same contract is fulfilled + unsafe { + self.set_required_components( + storages, + components, + components_in_bundles, + |component| !registered_components_in_bundles.contains(&component), + ); } } @@ -828,7 +895,7 @@ impl BundleInfo { for (index, component_id) in self.iter_required_components().enumerate() { if !current_archetype.contains(component_id) { - added_required_components.push(self.required_components[index].constructor.clone()); + added_required_components.push(self.required_components[index].clone()); added.push(component_id); // SAFETY: component_id exists let component_info = unsafe { components.get_info_unchecked(component_id) }; @@ -1804,10 +1871,7 @@ impl<'w> BundleSpawner<'w> { table, sparse_sets, &SpawnBundleStatus, - bundle_info - .required_components - .iter() - .map(|r| &r.constructor), + bundle_info.required_components.iter(), entity, table_row, self.change_tick, @@ -1959,7 +2023,6 @@ impl Bundles { storages: &mut Storages, ) -> BundleId { let bundle_infos = &mut self.bundle_infos; - *self.bundle_ids.entry(TypeId::of::()).or_insert_with(|| { let mut component_ids= Vec::new(); T::component_ids(components, &mut |id| component_ids.push(id)); @@ -1969,18 +2032,8 @@ impl Bundles { // - its info was created // - appropriate storage for it has been initialized. // - it was created in the same order as the components in T - unsafe { BundleInfo::new(core::any::type_name::(), storages, components, component_ids, id) }; - - for component in bundle_info.contributed_components() { - components_in_bundles_push( - &mut self.components_in_bundles, - *component, - id - ); - } - + unsafe { BundleInfo::new(core::any::type_name::(), storages, components, &mut self.components_in_bundles, component_ids, id) }; bundle_infos.push(bundle_info); - id }) } @@ -2051,7 +2104,6 @@ impl Bundles { component_ids: &[ComponentId], ) -> BundleId { let bundle_infos = &mut self.bundle_infos; - let components_in_bundles = &mut self.components_in_bundles; // Use `raw_entry_mut` to avoid cloning `component_ids` to access `Entry` let (_, bundle_id) = self @@ -2061,7 +2113,7 @@ impl Bundles { .or_insert_with(|| { let (id, storages) = initialize_dynamic_bundle( bundle_infos, - components_in_bundles, + &mut self.components_in_bundles, storages, components, Vec::from(component_ids), @@ -2088,14 +2140,13 @@ impl Bundles { component_id: ComponentId, ) -> BundleId { let bundle_infos = &mut self.bundle_infos; - let components_in_bundles = &mut self.components_in_bundles; let bundle_id = self .dynamic_component_bundle_ids .entry(component_id) .or_insert_with(|| { let (id, storage_type) = initialize_dynamic_bundle( bundle_infos, - components_in_bundles, + &mut self.components_in_bundles, storages, components, vec![component_id], @@ -2108,90 +2159,47 @@ impl Bundles { /// Updates the required components of bundles that contain `requiree`. /// - /// Bundles that already contain this required component only update their [`RequiredComponent`] - /// if its [`inheritance_depth`](RequiredComponent::inheritance_depth) is smaller or equal. - pub(crate) fn register_required_components( + /// # Safety + /// + /// The caller must pass the same `storages` and `components` to this method that were used + /// to create the stored bundles. + pub(crate) unsafe fn refresh_required_components( &mut self, + storages: &mut Storages, + components: &Components, requiree: ComponentId, - required: &[(ComponentId, RequiredComponent)], ) { - // take the vector listing the bundles with `requiree` so `components_in_bundles` can be mutated - // while the list is iterated let Some(taken_bundles_with_requiree) = self .components_in_bundles .get_mut(requiree.index()) .filter(|bundles| !bundles.is_empty()) - .map(core::mem::take) else { + .map(core::mem::take) + else { return; }; - // check each bundle if it needs to be updated for bundle_id in taken_bundles_with_requiree.iter() { - let bundle_info = &mut self.bundle_infos[bundle_id.index()]; - std::println!("bundle {bundle_id:?}"); + let bundle_info = self.bundle_infos.get_mut(bundle_id.index()); - { - //debug - for (&id, required) in bundle_info.required_components().iter().zip(bundle_info.required_components.iter()) { - std::println!("existing_Required_id {id:?}, existing_inheritance_depth {}", required.inheritance_depth); - } - } + // SAFETY: BundleIds in Self::components_in_bundles are valid ids of Self::bundle_infos + let bundle_info = unsafe { bundle_info.debug_checked_unwrap() }; - // get inheritance_depth of requiree as that offsets the inheritance_depth of the required components for this bundle - let requiree_inheritance_depth = bundle_info - .required_components() - .iter() - .enumerate() - .find_map(|(index, &existing_requiree_id)| { - (existing_requiree_id == requiree).then_some(index) - }) - .map(|index| bundle_info.required_components[index].inheritance_depth) - .unwrap_or(0); // requiree is explicit component of bundle - - for (newly_required_id, newly_required_component) in required.iter() { - let index = bundle_info - .required_components() - .iter() - .enumerate() - .find_map(|(index, &existing_required_id)| { - (existing_required_id == *newly_required_id).then_some(index) - }); - std::println!("newly_required_id: {newly_required_id:?}, newly_inheritance_depth: {} + {requiree_inheritance_depth}", newly_required_component.inheritance_depth); - - match index { - Some(index) => { - // required component is already in this bundle - // update `RequiredComponent` if the new one has a smaller or equal `existing_required_component` - let existing_required_component = - &mut bundle_info.required_components[index]; - std::println!("index: {index}, existing_inheritance_depth: {}", existing_required_component.inheritance_depth); - let newly_inheritance_depth = newly_required_component.inheritance_depth + requiree_inheritance_depth; - if existing_required_component.inheritance_depth >= newly_inheritance_depth { - *existing_required_component = newly_required_component.clone(); - } - } - None => { - std::println!("new required component"); - // required component was not yet known to this bundle - // update both `BundleInfo` and `components_in_bundles` - bundle_info.component_ids.push(*newly_required_id); - bundle_info - .required_components - .push(newly_required_component.clone()); - components_in_bundles_push( - &mut self.components_in_bundles, - *newly_required_id, - bundle_info.id, - ); - } - } + // SAFETY: Caller ensured `storages` and `components` match those used to create this bundle + unsafe { + bundle_info.reset_required_components( + storages, + components, + &mut self.components_in_bundles, + ); } } // put the list of bundles with `requiree` back let replaced = self.components_in_bundles.get_mut(requiree.index()); + // SAFETY: this list has to exist at this index to be taken in the first place let replaced = unsafe { replaced.debug_checked_unwrap() }; + *replaced = taken_bundles_with_requiree; } } @@ -2217,41 +2225,12 @@ fn initialize_dynamic_bundle( let id = BundleId(bundle_infos.len()); let bundle_info = // SAFETY: `component_ids` are valid as they were just checked - unsafe { BundleInfo::new("", storages, components, component_ids, id) }; - - for component in bundle_info.contributed_components() { - components_in_bundles_push(components_in_bundles, *component, id); - } - + unsafe { BundleInfo::new("", storages, components, components_in_bundles, component_ids, id) }; bundle_infos.push(bundle_info); (id, storage_types) } -/// Inserts `bundle` in `components_in_bundles` at [`component.index()`](ComponentId::index). -/// -/// Extends `components_in_bundles` with empty vectors if needed. -/// -/// This method does not check if the bundle is already known to contain this component, -/// so this function should only be called when this was asserted previously or the bundle -/// is new. -fn components_in_bundles_push( - components_in_bundles: &mut Vec>, - component: ComponentId, - bundle: BundleId, -) { - match components_in_bundles.get_mut(component.index()) { - Some(bundles) => bundles.push(bundle), - None => { - let missing_minus_one = component.index() - components_in_bundles.len(); - let iter = core::iter::repeat_n(Vec::new(), missing_minus_one) - .chain(core::iter::once(vec![bundle])); - components_in_bundles.reserve_exact(missing_minus_one + 1); - components_in_bundles.extend(iter); - } - } -} - fn sorted_remove(source: &mut Vec, remove: &[T]) { let mut remove_index = 0; source.retain(|value| { From 6c67a05ad160306ecce1fec0c8556a7091c3087d Mon Sep 17 00:00:00 2001 From: urben1680 <55257931+urben1680@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:26:46 +0200 Subject: [PATCH 05/20] Update component.rs --- crates/bevy_ecs/src/component.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 4779944141..615c5903f8 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1986,7 +1986,7 @@ impl Components { requiree: ComponentId, required: ComponentId, constructor: fn() -> R, - ) -> Result, RequiredComponentsError> { + ) -> Result<(), RequiredComponentsError> { // SAFETY: The caller ensures that the `requiree` is valid. let required_components = unsafe { self.get_required_components_mut(requiree) @@ -2006,9 +2006,7 @@ impl Components { // Register the required component for the requiree. // This is a direct requirement with a depth of `0`. - let required_component = required_components - .register_by_id(required, constructor, 0) - .clone(); + required_components.register_by_id(required, constructor, 0); // Add the requiree to the list of components that require the required component. // SAFETY: The component is in the list of required components, so it must exist already. @@ -2017,7 +2015,7 @@ impl Components { let mut required_components_tmp = RequiredComponents::default(); // SAFETY: The caller ensures that the `requiree` and `required` components are valid. - let mut inherited_requirements = unsafe { + let inherited_requirements = unsafe { self.register_inherited_required_components( requiree, required, @@ -2072,9 +2070,7 @@ impl Components { } } - inherited_requirements.push((required, required_component)); - - Ok(inherited_requirements) + Ok(()) } /// Registers the components inherited from `required` for the given `requiree`, @@ -2710,24 +2706,23 @@ impl RequiredComponents { component_id: ComponentId, inheritance_depth: u16, constructor: impl FnOnce() -> RequiredComponentConstructor, - ) -> &RequiredComponent { + ) { let entry = self.0.entry(component_id); match entry { - bevy_platform::collections::hash_map::Entry::Occupied(occupied) => { - let current = occupied.into_mut(); + bevy_platform::collections::hash_map::Entry::Occupied(mut occupied) => { + let current = occupied.get_mut(); if current.inheritance_depth > inheritance_depth { *current = RequiredComponent { constructor: constructor(), inheritance_depth, } } - current } bevy_platform::collections::hash_map::Entry::Vacant(vacant) => { vacant.insert(RequiredComponent { constructor: constructor(), inheritance_depth, - }) + }); } } } @@ -2755,7 +2750,7 @@ impl RequiredComponents { component_id: ComponentId, constructor: fn() -> C, inheritance_depth: u16, - ) -> &RequiredComponent { + ) { let erased = || { RequiredComponentConstructor({ // `portable-atomic-util` `Arc` is not able to coerce an unsized @@ -2815,7 +2810,7 @@ impl RequiredComponents { // `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_with(component_id, inheritance_depth, erased) } + unsafe { self.register_dynamic_with(component_id, inheritance_depth, erased) }; } /// Iterates the ids of all required components. This includes recursive required components. From d79b9a00b0cabf00814ade8d671ec057a9f73f1c Mon Sep 17 00:00:00 2001 From: urben1680 <55257931+urben1680@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:27:03 +0200 Subject: [PATCH 06/20] Update lib.rs --- crates/bevy_ecs/src/lib.rs | 209 ++++++------------------------------- 1 file changed, 33 insertions(+), 176 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 588b14d76c..183a6dffc6 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -145,7 +145,7 @@ pub struct HotPatched; #[cfg(test)] mod tests { use crate::{ - bundle::Bundle, + bundle::{Bundle, BundleId, BundleInfo}, change_detection::Ref, component::{Component, ComponentId, RequiredComponents, RequiredComponentsError}, entity::{Entity, EntityMapper}, @@ -168,7 +168,7 @@ mod tests { marker::PhantomData, sync::atomic::{AtomicUsize, Ordering}, }; - use std::{println, sync::Mutex}; + use std::sync::Mutex; #[derive(Component, Resource, Debug, PartialEq, Eq, Hash, Clone, Copy)] struct A(usize); @@ -2556,33 +2556,8 @@ mod tests { } #[test] - fn register_required_components_of_explicit_bundle_component_updates_bundle() { - #[derive(Component, Default)] - struct A; - - #[derive(Component, Default)] - #[require(C)] - struct B; - - #[derive(Component, Default)] - struct C; - - let mut world = World::new(); - - std::println!("A: {:?}", world.register_component::()); - std::println!("B: {:?}", world.register_component::()); - std::println!("C: {:?}", world.register_component::()); - world.register_bundle::(); - world.register_required_components::(); - - let entity = world.spawn(A); - assert!(entity.contains::()); - assert!(entity.contains::()); - } - - #[test] - fn register_required_components_of_required_bundle_component_updates_bundle() { - #[derive(Component, Default)] + fn update_required_components_in_bundle() { + #[derive(Component)] #[require(B)] struct A; @@ -2590,163 +2565,45 @@ mod tests { struct B; #[derive(Component, Default)] - #[require(D)] struct C; - #[derive(Component, Default)] - struct D; - - let mut world = World::new(); - - std::println!("A: {:?}", world.register_component::()); - std::println!("B: {:?}", world.register_component::()); - std::println!("C: {:?}", world.register_component::()); - std::println!("D: {:?}", world.register_component::()); - world.register_bundle::(); - world.register_required_components::(); - - let entity = world.spawn(A); - assert!(entity.contains::()); - assert!(entity.contains::()); - } - - #[test] - fn register_required_components_with_lower_inheritance_depth_keeps_bundle_unchanged() { - #[derive(Component, Default)] - #[require(C(5))] - struct A; - - #[derive(Component, Default)] - #[require(C(10))] - struct B; - - #[derive(Component, Default, Debug, PartialEq)] - struct C(u8); - - let mut world = World::new(); - - std::println!("A: {:?}", world.register_component::()); - std::println!("B: {:?}", world.register_component::()); - std::println!("C: {:?}", world.register_component::()); - world.register_bundle::(); - world.register_required_components::(); - - let entity = world.spawn(A); - assert_eq!(entity.get::(), Some(&C(5))); - } - - #[test] - fn register_required_components_with_equal_inheritance_depth_updates_bundle() { - #[derive(Component, Default)] - #[require(B)] - struct A; - - #[derive(Component, Default)] - #[require(D(5))] - struct B; - - #[derive(Component, Default)] - #[require(D(10))] - struct C; - - #[derive(Component, Default, Debug, PartialEq)] - struct D(u8); - - let mut world = World::new(); - - std::println!("A: {:?}", world.register_component::()); - std::println!("B: {:?}", world.register_component::()); - std::println!("C: {:?}", world.register_component::()); - std::println!("D: {:?}", world.register_component::()); - world.register_bundle::(); - world.register_required_components::(); - - let entity = world.spawn(A); - assert_eq!(entity.get::(), Some(&D(10))); - } - - #[test] - fn register_required_components_with_higher_inheritance_depth_updates_bundle() { - #[derive(Component, Default)] - #[require(B)] - struct A; - - #[derive(Component, Default)] - #[require(C(5))] - struct B; - - #[derive(Component, Default, Debug, PartialEq)] - struct C(u8); - - let mut world = World::new(); - - std::println!("A: {:?}", world.register_component::()); - std::println!("B: {:?}", world.register_component::()); - std::println!("C: {:?}", world.register_component::()); - world.register_bundle::(); - world.register_required_components_with::(|| C(10)); - - let entity = world.spawn(A); - assert_eq!(entity.get::(), Some(&C(10))); - } - - #[test] - fn register_required_components_with_further_required_with_equal_inheritance_depth_updates_bundle( - ) { - /* - A -> B -> C -> F - '-> D -> E -> F - */ - #[derive(Component, Default)] - #[require(B)] - struct A; - - #[derive(Component, Default)] - #[require(C)] - struct B; - - #[derive(Component, Default)] - #[require(F(5))] - struct C; - - #[derive(Component, Default)] - #[require(E)] - struct D; - - #[derive(Component, Default)] - #[require(F(10))] - struct E; - - #[derive(Component, Default, Debug, PartialEq)] - struct F(u8); + fn bundle_containing(world: &World, component: ComponentId) -> Option { + world + .bundles() + .iter_containing(component) + .next() + .map(BundleInfo::id) + } let mut world = World::new(); let a_id = world.register_component::(); - std::println!("A: {:?}", a_id); - std::println!("B: {:?}", world.register_component::()); - std::println!("C: {:?}", world.register_component::()); - std::println!("D: {:?}", world.register_component::()); - std::println!("E: {:?}", world.register_component::()); - std::println!("F: {:?}", world.register_component::()); + let b_id = world.register_component::(); + let c_id = world.register_component::(); - let required: Vec<_> = world - .components() - .get_info(a_id) - .unwrap() - .required_components() - .0 - .iter() - .map(|(id, required)| (id, required.inheritance_depth)) - .collect(); + let bundle = world.register_bundle::(); + let bundle_id = bundle.id(); + let contributed: HashSet<_> = bundle.contributed_components().iter().copied().collect(); - std::println!("required of A\n{required:#?}"); + assert_eq!(contributed.contains(&a_id), true); + assert_eq!(contributed.contains(&b_id), true); + assert_eq!(contributed.contains(&c_id), false); - world.register_bundle::(); - world.register_required_components::(); + assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, c_id), None); - let entity = world.spawn(A); - assert_eq!(entity.get::(), Some(&F(10))); // todo: fix and add tests with lower and higher depth + world.register_required_components::(); + let bundle = world.bundles().get(bundle_id).unwrap(); + let contributed: HashSet<_> = bundle.contributed_components().iter().copied().collect(); + + assert_eq!(contributed.contains(&a_id), true); + assert_eq!(contributed.contains(&b_id), true); + assert_eq!(contributed.contains(&c_id), true); + + assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); } #[test] From 224a22c97bfe46aa0f5bd7496c02bd48862f0c8b Mon Sep 17 00:00:00 2001 From: urben1680 <55257931+urben1680@users.noreply.github.com> Date: Tue, 1 Jul 2025 22:27:22 +0200 Subject: [PATCH 07/20] Update mod.rs --- crates/bevy_ecs/src/world/mod.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index cef7de4d72..15cc633657 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -542,13 +542,19 @@ impl World { let required = self.register_component::(); // SAFETY: We just created the `required` and `requiree` components. - let required = unsafe { + unsafe { self.components - .register_required_components::(requiree, required, constructor)? - }; + .register_required_components::(requiree, required, constructor)?; + } - self.bundles - .register_required_components(requiree, &required); + // SAFETY: all bundles are created with Self::storages and Self::components + unsafe { + self.bundles.refresh_required_components( + &mut self.storages, + &self.components, + requiree, + ); + } Ok(()) } From a9e392898871824d8fc7fd75dd38a506fa3c7674 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Tue, 1 Jul 2025 22:31:57 +0200 Subject: [PATCH 08/20] fmt --- crates/bevy_ecs/src/bundle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index b2442c0ef0..a60a956bc5 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -580,7 +580,7 @@ impl BundleInfo { /// Update [`Self::component_ids`] to contain all required components, updates [`Self::required_components`] alongside. /// /// The filter `components_in_bundles_filter` determines for which components [`Self::id`] is added to `components_in_bundles`. - /// + /// /// # Safety /// /// `Self` must be constructed from [`Self::new`] with the same `storages` and `components`. @@ -634,7 +634,7 @@ impl BundleInfo { } /// Updates required components for new runtime-added components. - /// + /// /// # Safety /// /// `Self` must be constructed from [`Self::new`] with the same `storages` and `components`. From e79134b628632337b3d6a711e5197120f9e9dd8d Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Tue, 1 Jul 2025 22:37:44 +0200 Subject: [PATCH 09/20] ci --- crates/bevy_ecs/src/bundle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index a60a956bc5..f5b9ce7a6a 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1993,9 +1993,9 @@ impl Bundles { .get(component.index()) .into_iter() .flatten() - .map(|id| unsafe { + .map(|id| { // SAFETY: components_in_bundles contains only valid ids - self.bundle_infos.get(id.index()).debug_checked_unwrap() + unsafe { self.bundle_infos.get(id.index()).debug_checked_unwrap() } }) } From 3d29fc29c966f39816c7a8f150361a883ffd7837 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Tue, 1 Jul 2025 22:51:42 +0200 Subject: [PATCH 10/20] ci --- crates/bevy_ecs/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 183a6dffc6..a518d73c47 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2585,9 +2585,9 @@ mod tests { let bundle_id = bundle.id(); let contributed: HashSet<_> = bundle.contributed_components().iter().copied().collect(); - assert_eq!(contributed.contains(&a_id), true); - assert_eq!(contributed.contains(&b_id), true); - assert_eq!(contributed.contains(&c_id), false); + assert!(contributed.contains(&a_id)); + assert!(contributed.contains(&b_id)); + assert!(!contributed.contains(&c_id)); assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); @@ -2597,9 +2597,9 @@ mod tests { let bundle = world.bundles().get(bundle_id).unwrap(); let contributed: HashSet<_> = bundle.contributed_components().iter().copied().collect(); - assert_eq!(contributed.contains(&a_id), true); - assert_eq!(contributed.contains(&b_id), true); - assert_eq!(contributed.contains(&c_id), true); + assert!(contributed.contains(&a_id)); + assert!(contributed.contains(&b_id)); + assert!(contributed.contains(&c_id)); assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); From c1221bee4fc53357f72508b1bc22178d57f41899 Mon Sep 17 00:00:00 2001 From: urben1680 <55257931+urben1680@users.noreply.github.com> Date: Tue, 1 Jul 2025 23:28:40 +0200 Subject: [PATCH 11/20] remove todo comment --- crates/bevy_ecs/src/bundle.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index f5b9ce7a6a..f38b281013 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -591,7 +591,6 @@ impl BundleInfo { components_in_bundles: &mut Vec>, components_in_bundles_filter: impl Fn(ComponentId) -> bool, ) { - // todo: wrong at second call, would add bundleid duplicates! let mut components_in_bundles_push = |component: ComponentId| { if !components_in_bundles_filter(component) { return; From 21020df71777c3cbae7af117bbe907a7f5545030 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Wed, 2 Jul 2025 22:40:35 +0200 Subject: [PATCH 12/20] error on adding to bundle used in remove_with_required --- crates/bevy_ecs/src/bundle.rs | 29 ++++++++++++++++++++++++----- crates/bevy_ecs/src/component.rs | 5 ++++- crates/bevy_ecs/src/lib.rs | 19 +++++++++++++++++++ crates/bevy_ecs/src/world/mod.rs | 11 +++-------- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index f38b281013..b14f59df85 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -63,7 +63,7 @@ use crate::{ change_detection::MaybeLocation, component::{ Component, ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor, - RequiredComponents, StorageType, Tick, + RequiredComponents, RequiredComponentsError, StorageType, Tick, }, entity::{Entities, Entity, EntityLocation}, lifecycle::{ADD, INSERT, REMOVE, REPLACE}, @@ -2167,16 +2167,33 @@ impl Bundles { storages: &mut Storages, components: &Components, requiree: ComponentId, - ) { - let Some(taken_bundles_with_requiree) = self + ) -> Result<(), RequiredComponentsError> { + let Some(bundles_with_requiree) = self .components_in_bundles .get_mut(requiree.index()) .filter(|bundles| !bundles.is_empty()) - .map(core::mem::take) else { - return; + // no bundle with `requiree` exists + return Ok(()); }; + // `EntityWorldMut::remove_with_requires` generate edges between archetypes where the required + // components matter for the the target archetype. These bundles cannot receive new required + // components as that would invalidate these edges. + // The mechanism is using `Self::contributed_bundle_ids` to track the bundles so we check here if it + // has any overlapping `BundleId`s with `bundles_with_requiree`. + // TODO: Remove this error and update archetype edges accordingly when required components are added + if bundles_with_requiree.iter().any(|bundles_with_requiree| { + self.contributed_bundle_ids + .values() + .any(|bundle: &BundleId| bundles_with_requiree == bundle) + }) { + return Err(RequiredComponentsError::RemovedFromArchetype(requiree)); + } + + // take it to update `Self::bundles_with_requiree` while iterating this vector + let taken_bundles_with_requiree = core::mem::take(bundles_with_requiree); + for bundle_id in taken_bundles_with_requiree.iter() { let bundle_info = self.bundle_infos.get_mut(bundle_id.index()); @@ -2200,6 +2217,8 @@ impl Bundles { let replaced = unsafe { replaced.debug_checked_unwrap() }; *replaced = taken_bundles_with_requiree; + + Ok(()) } } diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 615c5903f8..af75204f94 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -2622,9 +2622,12 @@ pub enum RequiredComponentsError { /// The component is already a directly required component for the requiree. #[error("Component {0:?} already directly requires component {1:?}")] DuplicateRegistration(ComponentId, ComponentId), - /// An archetype with the component that requires other components already exists + /// An archetype with the component that requires other components already exists. #[error("An archetype with the component {0:?} that requires other components already exists")] ArchetypeExists(ComponentId), + /// A bundle with the component that requires other components was removed via [`crate::world::EntityWorldMut::remove_with_requires`]. + #[error("A bundle with the component {0:?} that requires other components was removed including required components previously")] + RemovedFromArchetype(ComponentId), } /// A Required Component constructor. See [`Component`] for details. diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index a518d73c47..64b5f707fa 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2606,6 +2606,25 @@ mod tests { assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); } + #[test] + fn update_required_components_in_bundle_fails() { + #[derive(Component)] + struct A; + + #[derive(Component, Default)] + struct B; + + let mut world = World::new(); + + let a_id = world.register_component::(); + world.spawn(B).remove_with_requires::(); + + match world.try_register_required_components::() { + Err(RequiredComponentsError::RemovedFromArchetype(id)) => assert_eq!(id, a_id), + _ => panic!(), + } + } + #[test] fn required_components_inheritance_depth_bias() { #[derive(Component, PartialEq, Eq, Clone, Copy, Debug)] diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 15cc633657..0d74ded339 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -534,7 +534,7 @@ impl World { ) -> Result<(), RequiredComponentsError> { let requiree = self.register_component::(); - // TODO: Remove this panic and update archetype edges accordingly when required components are added + // TODO: Remove this error and update archetype edges accordingly when required components are added if self.archetypes().component_index().contains_key(&requiree) { return Err(RequiredComponentsError::ArchetypeExists(requiree)); } @@ -549,14 +549,9 @@ impl World { // SAFETY: all bundles are created with Self::storages and Self::components unsafe { - self.bundles.refresh_required_components( - &mut self.storages, - &self.components, - requiree, - ); + self.bundles + .refresh_required_components(&mut self.storages, &self.components, requiree) } - - Ok(()) } /// Retrieves the [required components](RequiredComponents) for the given component type, if it exists. From f32878f54b03e5b0b27b92175b9adb6f6ab392ef Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Wed, 2 Jul 2025 22:52:39 +0200 Subject: [PATCH 13/20] align failing test to other tests for that arror --- crates/bevy_ecs/src/lib.rs | 41 ++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 64b5f707fa..b8e126eff4 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2472,6 +2472,28 @@ mod tests { )); } + #[test] + fn runtime_required_components_of_remove_with_requires_bundle() { + #[derive(Component)] + struct A; + + #[derive(Component, Default)] + struct B; + + let mut world = World::new(); + + let _a_id = world.register_component::(); + world.spawn(B).remove_with_requires::(); + + // This should fail, removing A from an archetype with only B was previously noop + // and register_required_components cannot trivially update the edge to now cause + // a move to an archetype without A + assert!(matches!( + world.try_register_required_components::(), + Err(RequiredComponentsError::RemovedFromArchetype(_a_id)) + )); + } + #[test] fn required_components_inheritance_depth() { // Test that inheritance depths are computed correctly for requirements. @@ -2606,25 +2628,6 @@ mod tests { assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); } - #[test] - fn update_required_components_in_bundle_fails() { - #[derive(Component)] - struct A; - - #[derive(Component, Default)] - struct B; - - let mut world = World::new(); - - let a_id = world.register_component::(); - world.spawn(B).remove_with_requires::(); - - match world.try_register_required_components::() { - Err(RequiredComponentsError::RemovedFromArchetype(id)) => assert_eq!(id, a_id), - _ => panic!(), - } - } - #[test] fn required_components_inheritance_depth_bias() { #[derive(Component, PartialEq, Eq, Clone, Copy, Debug)] From e33c8fe568c341b3c5a7b88667d9bacaca443e09 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Wed, 2 Jul 2025 22:54:36 +0200 Subject: [PATCH 14/20] align failing test to other tests for that arror 2 --- crates/bevy_ecs/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index b8e126eff4..cfb55271c1 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2482,12 +2482,10 @@ mod tests { let mut world = World::new(); - let _a_id = world.register_component::(); - world.spawn(B).remove_with_requires::(); - // This should fail, removing A from an archetype with only B was previously noop // and register_required_components cannot trivially update the edge to now cause // a move to an archetype without A + world.spawn(B).remove_with_requires::(); assert!(matches!( world.try_register_required_components::(), Err(RequiredComponentsError::RemovedFromArchetype(_a_id)) From 9ea3707c5345b3705ec94f95f5bb6e11f6e40f07 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Wed, 2 Jul 2025 23:20:59 +0200 Subject: [PATCH 15/20] remove unused variable --- crates/bevy_ecs/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index cfb55271c1..717af94d08 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2488,7 +2488,7 @@ mod tests { world.spawn(B).remove_with_requires::(); assert!(matches!( world.try_register_required_components::(), - Err(RequiredComponentsError::RemovedFromArchetype(_a_id)) + Err(RequiredComponentsError::RemovedFromArchetype(_)) )); } From 0d20581527ae40642192f637fe388dfc20991d98 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Mon, 14 Jul 2025 21:25:55 +0200 Subject: [PATCH 16/20] fmt --- crates/bevy_ecs/src/bundle/info.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index 563e990c1d..6805cba2b8 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -9,7 +9,8 @@ use crate::{ bundle::{Bundle, DynamicBundle}, change_detection::MaybeLocation, component::{ - ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor, RequiredComponents, RequiredComponentsError, StorageType, Tick + ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor, + RequiredComponents, RequiredComponentsError, StorageType, Tick, }, entity::Entity, query::DebugCheckedUnwrap as _, @@ -451,7 +452,7 @@ impl Bundles { pub fn iter(&self) -> impl Iterator { self.bundle_infos.iter() } - + /// Iterate over [`BundleInfo`] containing `component`, either explicitly or as required. pub fn iter_containing(&self, component: ComponentId) -> impl Iterator { self.components_in_bundles @@ -623,7 +624,7 @@ impl Bundles { }); *bundle_id } - + /// Updates the required components of bundles that contain `requiree`. /// /// # Safety From cafc95013857a2fbbbdbb3df20b5e6e4ba77e040 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Mon, 14 Jul 2025 21:45:34 +0200 Subject: [PATCH 17/20] assert no errors before doing mutations --- crates/bevy_ecs/src/bundle/info.rs | 48 ++++++++++++++++++++---------- crates/bevy_ecs/src/world/mod.rs | 19 +++++++++--- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index 6805cba2b8..44b8688536 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -625,25 +625,24 @@ impl Bundles { *bundle_id } - /// Updates the required components of bundles that contain `requiree`. + /// Checks if the bundles containing `requiree` can have their required components be updated, + /// in which case this returns `Ok(true)`. /// - /// # Safety + /// Returns `Ok(false)` if there are no known bundles with this component. /// - /// The caller must pass the same `storages` and `components` to this method that were used - /// to create the stored bundles. - pub(crate) unsafe fn refresh_required_components( - &mut self, - storages: &mut Storages, - components: &Components, + /// Returns the [`RequiredComponentsError::RemovedFromArchetype`] error if bundles cannot be updated + /// in which case the registration of any new required component must be refused. + pub(crate) fn verify_to_refresh_required_components( + &self, requiree: ComponentId, - ) -> Result<(), RequiredComponentsError> { + ) -> Result { let Some(bundles_with_requiree) = self .components_in_bundles - .get_mut(requiree.index()) + .get(requiree.index()) .filter(|bundles| !bundles.is_empty()) else { // no bundle with `requiree` exists - return Ok(()); + return Ok(false); }; // `EntityWorldMut::remove_with_requires` generate edges between archetypes where the required @@ -660,8 +659,29 @@ impl Bundles { return Err(RequiredComponentsError::RemovedFromArchetype(requiree)); } - // take it to update `Self::bundles_with_requiree` while iterating this vector - let taken_bundles_with_requiree = core::mem::take(bundles_with_requiree); + Ok(true) + } + + /// Updates the required components of bundles that contain `requiree`. + /// + /// # Safety + /// + /// The caller must have confirmed [`Self::verify_to_refresh_required_components`] returned `Ok(true)` + /// for this `requiree` and must pass the same `storages` and `components` to this method that were used + /// to create the stored bundles. + pub(crate) unsafe fn refresh_required_components( + &mut self, + storages: &mut Storages, + components: &Components, + requiree: ComponentId, + ) { + // take list of bundles to update `Self::bundles_with_requiree` while iterating this + let taken_bundles_with_requiree = self + .components_in_bundles + .get_mut(requiree.index()) + .filter(|bundles| !bundles.is_empty()) + .map(core::mem::take) + .expect("verify_to_refresh_required_components confirmed existing bundles to refresh"); for bundle_id in taken_bundles_with_requiree.iter() { let bundle_info = self.bundle_infos.get_mut(bundle_id.index()); @@ -686,8 +706,6 @@ impl Bundles { let replaced = unsafe { replaced.debug_checked_unwrap() }; *replaced = taken_bundles_with_requiree; - - Ok(()) } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index cc3e21485f..94acafe29b 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -542,17 +542,28 @@ impl World { let required = self.register_component::(); + let update_bundles = self + .bundles + .verify_to_refresh_required_components(requiree)?; + // SAFETY: We just created the `required` and `requiree` components. unsafe { self.components .register_required_components::(requiree, required, constructor)?; } - // SAFETY: all bundles are created with Self::storages and Self::components - unsafe { - self.bundles - .refresh_required_components(&mut self.storages, &self.components, requiree) + if update_bundles { + // SAFETY: all bundles are created with Self::storages and Self::components + unsafe { + self.bundles.refresh_required_components( + &mut self.storages, + &self.components, + requiree, + ); + } } + + Ok(()) } /// Retrieves the [required components](RequiredComponents) for the given component type, if it exists. From efbfac9f2dcea5a379fa88d577f68bfa5127cf0f Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Mon, 14 Jul 2025 21:49:11 +0200 Subject: [PATCH 18/20] undo unintentional doc changes --- crates/bevy_ecs/src/bundle/info.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index 44b8688536..ca0a68e230 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -275,12 +275,12 @@ impl BundleInfo { /// For example, if the original archetype already has `ComponentA` and `T` also has `ComponentA`, the status /// should be `Existing`. If the original archetype does not have `ComponentA`, the status should be `Added`. /// - /// When "inserting" a bundle into an existing entity, [`ArchetypeAfterBundleInsert`] + /// When "inserting" a bundle into an existing entity, [`ArchetypeAfterBundleInsert`](crate::archetype::SpawnBundleStatus) /// should be used, which will report `Added` vs `Existing` status based on the current archetype's structure. /// - /// When spawning a bundle, [`SpawnBundleStatus`] can be used instead, which removes the need - /// to look up the [`ArchetypeAfterBundleInsert`] in the archetype graph, which requires - /// ownership of the entity's current archetype. + /// When spawning a bundle, [`SpawnBundleStatus`](crate::archetype::SpawnBundleStatus) can be used instead, + /// which removes the need to look up the [`ArchetypeAfterBundleInsert`](crate::archetype::ArchetypeAfterBundleInsert) + /// in the archetype graph, which requires ownership of the entity's current archetype. /// /// `table` must be the "new" table for `entity`. `table_row` must have space allocated for the /// `entity`, `bundle` must match this [`BundleInfo`]'s type From 177ecbe3f226958728ceb9538ca07b703e36b614 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Tue, 15 Jul 2025 21:31:36 +0200 Subject: [PATCH 19/20] documentation, test adjustment, minor implementation changes --- crates/bevy_ecs/src/bundle/info.rs | 113 ++++++++++++++-------- crates/bevy_ecs/src/component/required.rs | 3 +- crates/bevy_ecs/src/lib.rs | 35 +++++++ crates/bevy_ecs/src/world/entity_ref.rs | 2 + 4 files changed, 113 insertions(+), 40 deletions(-) diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index ca0a68e230..8cf9131a93 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -75,6 +75,9 @@ pub struct BundleInfo { impl BundleInfo { /// Create a new [`BundleInfo`]. /// + /// The `component_to_containing_bundles` parameter comes from [`Bundles::component_to_containing_bundles`], + /// see its documentation for more information. + /// /// # Safety /// /// Every ID in `component_ids` must be valid within the World that owns the `BundleInfo` @@ -83,7 +86,7 @@ impl BundleInfo { bundle_type_name: &'static str, storages: &mut Storages, components: &Components, - components_in_bundles: &mut Vec>, + component_to_containing_bundles: &mut Vec>, component_ids: Vec, id: BundleId, ) -> BundleInfo { @@ -127,15 +130,24 @@ impl BundleInfo { // SAFETY: first call with the same `storages` and `components`. unsafe { - info.set_required_components(storages, components, components_in_bundles, |_| true); + info.set_required_components( + storages, + components, + component_to_containing_bundles, + |_| true, + ); } info } - /// Update [`Self::component_ids`] to contain all required components, updates [`Self::required_components`] alongside. + /// Update [`Self::component_ids`] from containing only the explicit components to additionally contain all required + /// components as well and update [`Self::required_components`] alongside. /// - /// The filter `components_in_bundles_filter` determines for which components [`Self::id`] is added to `components_in_bundles`. + /// The `component_to_containing_bundles` parameter comes from [`Bundles::component_to_containing_bundles`], + /// see its documentation for more information. + /// + /// The filter `component_to_containing_bundles_filter` determines for which components [`Self::id`] is added to `component_to_containing_bundles`. /// /// # Safety /// @@ -144,21 +156,19 @@ impl BundleInfo { &mut self, storages: &mut Storages, components: &Components, - components_in_bundles: &mut Vec>, - components_in_bundles_filter: impl Fn(ComponentId) -> bool, + component_to_containing_bundles: &mut Vec>, + component_to_containing_bundles_filter: impl Fn(ComponentId) -> bool, ) { - let mut components_in_bundles_push = |component: ComponentId| { - if !components_in_bundles_filter(component) { + let mut component_to_containing_bundles_push = |component: ComponentId| { + if !component_to_containing_bundles_filter(component) { return; } - match components_in_bundles.get_mut(component.index()) { + match component_to_containing_bundles.get_mut(component.index()) { Some(bundles) => bundles.push(self.id), None => { - let missing_minus_one = component.index() - components_in_bundles.len(); - let iter = core::iter::repeat_n(Vec::new(), missing_minus_one) - .chain(core::iter::once(vec![self.id])); - components_in_bundles.reserve_exact(missing_minus_one + 1); - components_in_bundles.extend(iter); + component_to_containing_bundles + .resize_with(component.index() + 1, || Vec::new()); + *component_to_containing_bundles.last_mut().unwrap() = vec![self.id]; } } }; @@ -170,11 +180,12 @@ impl BundleInfo { let info = unsafe { components.get_info_unchecked(component_id) }; required_components.merge(info.required_components()); storages.prepare_component(info); - components_in_bundles_push(component_id); + component_to_containing_bundles_push(component_id); } required_components.remove_explicit_components(&self.component_ids); - // handle required components + // handle required components, clear first in case this gets called again for this bundle + self.required_components.clear(); self.required_components .extend(required_components.0.into_iter().map(|(component_id, v)| { // Safety: These ids came out of the passed `components`, so they must be valid. @@ -183,13 +194,16 @@ impl BundleInfo { // This adds required components to the component_ids list _after_ using that list to remove explicitly provided // components. This ordering is important! self.component_ids.push(component_id); - components_in_bundles_push(component_id); + component_to_containing_bundles_push(component_id); v.constructor })); } /// Updates required components for new runtime-added components. /// + /// The `component_to_containing_bundles` parameter comes from [`Bundles::component_to_containing_bundles`], + /// see its documentation for more information. + /// /// # Safety /// /// `Self` must be constructed from [`Self::new`] with the same `storages` and `components`. @@ -197,20 +211,19 @@ impl BundleInfo { &mut self, storages: &mut Storages, components: &Components, - components_in_bundles: &mut Vec>, + component_to_containing_bundles: &mut Vec>, ) { - let registered_components_in_bundles: HashSet<_> = + let registered_component_to_containing_bundles: HashSet<_> = self.component_ids.iter().copied().collect(); self.component_ids.truncate(self.explicit_components_len); - self.required_components.clear(); // SAFETY: caller ensured the same contract is fulfilled unsafe { self.set_required_components( storages, components, - components_in_bundles, - |component| !registered_components_in_bundles.contains(&component), + component_to_containing_bundles, + |component| !registered_component_to_containing_bundles.contains(&component), ); } } @@ -434,7 +447,9 @@ pub struct Bundles { /// Cache optimized dynamic [`BundleId`] with single component dynamic_component_bundle_ids: HashMap, dynamic_component_storages: HashMap, - components_in_bundles: Vec>, + /// Cache [`BundleId`]s that contain a certain [`ComponentId`], which uses + /// [`ComponentId::index`] as the index of the outer vector into the inner + component_to_containing_bundles: Vec>, } impl Bundles { @@ -454,13 +469,16 @@ impl Bundles { } /// Iterate over [`BundleInfo`] containing `component`, either explicitly or as required. - pub fn iter_containing(&self, component: ComponentId) -> impl Iterator { - self.components_in_bundles + pub(crate) fn iter_containing( + &self, + component: ComponentId, + ) -> impl Iterator { + self.component_to_containing_bundles .get(component.index()) .into_iter() .flatten() .map(|id| { - // SAFETY: components_in_bundles contains only valid ids + // SAFETY: component_to_containing_bundles contains only valid ids unsafe { self.bundle_infos.get(id.index()).debug_checked_unwrap() } }) } @@ -498,7 +516,7 @@ impl Bundles { // - its info was created // - appropriate storage for it has been initialized. // - it was created in the same order as the components in T - unsafe { BundleInfo::new(core::any::type_name::(), storages, components, &mut self.components_in_bundles, component_ids, id) }; + unsafe { BundleInfo::new(core::any::type_name::(), storages, components, &mut self.component_to_containing_bundles, component_ids, id) }; bundle_infos.push(bundle_info); id }) @@ -507,6 +525,8 @@ impl Bundles { /// Registers a new [`BundleInfo`], which contains both explicit and required components for a statically known type. /// /// Also registers all the components in the bundle. + /// + /// Using this forbids components in this bundle to add more required components via [`Self::refresh_required_components`]. pub(crate) fn register_contributed_bundle_info( &mut self, components: &mut ComponentsRegistrator, @@ -581,7 +601,7 @@ impl Bundles { .or_insert_with(|| { let (id, storages) = initialize_dynamic_bundle( bundle_infos, - &mut self.components_in_bundles, + &mut self.component_to_containing_bundles, storages, components, Vec::from(component_ids), @@ -614,7 +634,7 @@ impl Bundles { .or_insert_with(|| { let (id, storage_type) = initialize_dynamic_bundle( bundle_infos, - &mut self.components_in_bundles, + &mut self.component_to_containing_bundles, storages, components, vec![component_id], @@ -632,12 +652,15 @@ impl Bundles { /// /// Returns the [`RequiredComponentsError::RemovedFromArchetype`] error if bundles cannot be updated /// in which case the registration of any new required component must be refused. + /// This does not check for [`RequiredComponentsError::ArchetypeExists`] as that is done by + /// [`Components::register_required_components`] that needs to be called before + /// [`Self::refresh_required_components`] anyway. pub(crate) fn verify_to_refresh_required_components( &self, requiree: ComponentId, ) -> Result { let Some(bundles_with_requiree) = self - .components_in_bundles + .component_to_containing_bundles .get(requiree.index()) .filter(|bundles| !bundles.is_empty()) else { @@ -650,6 +673,9 @@ impl Bundles { // components as that would invalidate these edges. // The mechanism is using `Self::contributed_bundle_ids` to track the bundles so we check here if it // has any overlapping `BundleId`s with `bundles_with_requiree`. + // It would also be an error if the bundle is part of any archetype (edge) involving inserts, + // but that error is checked on by `Components::register_required_components` so doing that here + // would be redundant. // TODO: Remove this error and update archetype edges accordingly when required components are added if bundles_with_requiree.iter().any(|bundles_with_requiree| { self.contributed_bundle_ids @@ -664,11 +690,14 @@ impl Bundles { /// Updates the required components of bundles that contain `requiree`. /// + /// This assumes that `requiree` cannot have been added itself as a new required component. + /// /// # Safety /// - /// The caller must have confirmed [`Self::verify_to_refresh_required_components`] returned `Ok(true)` - /// for this `requiree` and must pass the same `storages` and `components` to this method that were used - /// to create the stored bundles. + /// The caller assures that immediately prior calling this, ... + /// - [`Self::verify_to_refresh_required_components`] returned `Ok(true)` for `requiree` + /// - [`Components::register_required_components`] did not return `Err` for `requiree` + /// - and `storages` and `components` must be the same that were used to create the stored bundles pub(crate) unsafe fn refresh_required_components( &mut self, storages: &mut Storages, @@ -677,7 +706,7 @@ impl Bundles { ) { // take list of bundles to update `Self::bundles_with_requiree` while iterating this let taken_bundles_with_requiree = self - .components_in_bundles + .component_to_containing_bundles .get_mut(requiree.index()) .filter(|bundles| !bundles.is_empty()) .map(core::mem::take) @@ -686,7 +715,7 @@ impl Bundles { for bundle_id in taken_bundles_with_requiree.iter() { let bundle_info = self.bundle_infos.get_mut(bundle_id.index()); - // SAFETY: BundleIds in Self::components_in_bundles are valid ids of Self::bundle_infos + // SAFETY: BundleIds in Self::component_to_containing_bundles are valid ids of Self::bundle_infos let bundle_info = unsafe { bundle_info.debug_checked_unwrap() }; // SAFETY: Caller ensured `storages` and `components` match those used to create this bundle @@ -694,17 +723,23 @@ impl Bundles { bundle_info.reset_required_components( storages, components, - &mut self.components_in_bundles, + &mut self.component_to_containing_bundles, ); } } // put the list of bundles with `requiree` back - let replaced = self.components_in_bundles.get_mut(requiree.index()); + let replaced = self + .component_to_containing_bundles + .get_mut(requiree.index()); // SAFETY: this list has to exist at this index to be taken in the first place let replaced = unsafe { replaced.debug_checked_unwrap() }; + // put the bundles back, the placeholder that was put in its place earlier cannot have been updated + // because only the lists of the newly required components get new bundles added. + // if that happened for this list, that would mean `requiree` would be its own new required component, + // which is invalid. *replaced = taken_bundles_with_requiree; } } @@ -713,7 +748,7 @@ impl Bundles { /// and initializes a [`BundleInfo`]. fn initialize_dynamic_bundle( bundle_infos: &mut Vec, - components_in_bundles: &mut Vec>, + component_to_containing_bundles: &mut Vec>, storages: &mut Storages, components: &Components, component_ids: Vec, @@ -730,7 +765,7 @@ fn initialize_dynamic_bundle( let id = BundleId(bundle_infos.len()); let bundle_info = // SAFETY: `component_ids` are valid as they were just checked - unsafe { BundleInfo::new("", storages, components, components_in_bundles, component_ids, id) }; + unsafe { BundleInfo::new("", storages, components, component_to_containing_bundles, component_ids, id) }; bundle_infos.push(bundle_info); (id, storage_types) diff --git a/crates/bevy_ecs/src/component/required.rs b/crates/bevy_ecs/src/component/required.rs index e4d8055168..2e0a9bb068 100644 --- a/crates/bevy_ecs/src/component/required.rs +++ b/crates/bevy_ecs/src/component/required.rs @@ -24,7 +24,8 @@ impl Components { /// /// # Safety /// - /// The given component IDs `required` and `requiree` must be valid. + /// - The given component IDs `required` and `requiree` must be valid + /// - [`crate::bundle::Bundles::verify_to_refresh_required_components`] returned `Ok` for `requiree` /// /// # Errors /// diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 0dc70dcb00..1f7470e6f3 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2586,8 +2586,15 @@ mod tests { struct B; #[derive(Component, Default)] + #[require(D)] struct C; + #[derive(Component, Default)] + struct D; + + #[derive(Component, Default)] + struct E; + fn bundle_containing(world: &World, component: ComponentId) -> Option { world .bundles() @@ -2601,6 +2608,8 @@ mod tests { let a_id = world.register_component::(); let b_id = world.register_component::(); let c_id = world.register_component::(); + let d_id = world.register_component::(); + let e_id = world.register_component::(); let bundle = world.register_bundle::(); let bundle_id = bundle.id(); @@ -2609,11 +2618,16 @@ mod tests { assert!(contributed.contains(&a_id)); assert!(contributed.contains(&b_id)); assert!(!contributed.contains(&c_id)); + assert!(!contributed.contains(&d_id)); + assert!(!contributed.contains(&e_id)); assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); assert_eq!(bundle_containing(&world, c_id), None); + assert_eq!(bundle_containing(&world, d_id), None); + assert_eq!(bundle_containing(&world, e_id), None); + // check if registration succeeds world.register_required_components::(); let bundle = world.bundles().get(bundle_id).unwrap(); let contributed: HashSet<_> = bundle.contributed_components().iter().copied().collect(); @@ -2621,10 +2635,31 @@ mod tests { assert!(contributed.contains(&a_id)); assert!(contributed.contains(&b_id)); assert!(contributed.contains(&c_id)); + assert!(contributed.contains(&d_id)); + assert!(!contributed.contains(&e_id)); assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, d_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, e_id), None); + + // check if another registration can be associated to the bundle using the previously registered component + world.register_required_components::(); + let bundle = world.bundles().get(bundle_id).unwrap(); + let contributed: HashSet<_> = bundle.contributed_components().iter().copied().collect(); + + assert!(contributed.contains(&a_id)); + assert!(contributed.contains(&b_id)); + assert!(contributed.contains(&c_id)); + assert!(contributed.contains(&d_id)); + assert!(contributed.contains(&e_id)); + + assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, d_id), Some(bundle_id)); + assert_eq!(bundle_containing(&world, e_id), Some(bundle_id)); } #[test] diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 90438b8d6e..6ecdf6dbae 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -2099,6 +2099,8 @@ impl<'w> EntityWorldMut<'w> { /// # Panics /// /// If the entity has been despawned while this `EntityWorldMut` is still alive. + // Implementation note: Using this forbids components in this bundle to add more required components + // See `Bundles::verify_to_refresh_required_components` #[track_caller] pub fn remove_with_requires(&mut self) -> &mut Self { self.remove_with_requires_with_caller::(MaybeLocation::caller()) From 0e4643c2b95d141916174f215ad07de06fa6e698 Mon Sep 17 00:00:00 2001 From: Urben1680 Date: Tue, 15 Jul 2025 21:36:41 +0200 Subject: [PATCH 20/20] ci --- crates/bevy_ecs/src/bundle/info.rs | 20 ++------------------ crates/bevy_ecs/src/lib.rs | 28 +--------------------------- 2 files changed, 3 insertions(+), 45 deletions(-) diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index 8cf9131a93..56f18313f0 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -166,8 +166,7 @@ impl BundleInfo { match component_to_containing_bundles.get_mut(component.index()) { Some(bundles) => bundles.push(self.id), None => { - component_to_containing_bundles - .resize_with(component.index() + 1, || Vec::new()); + component_to_containing_bundles.resize_with(component.index() + 1, Vec::new); *component_to_containing_bundles.last_mut().unwrap() = vec![self.id]; } } @@ -468,21 +467,6 @@ impl Bundles { self.bundle_infos.iter() } - /// Iterate over [`BundleInfo`] containing `component`, either explicitly or as required. - pub(crate) fn iter_containing( - &self, - component: ComponentId, - ) -> impl Iterator { - self.component_to_containing_bundles - .get(component.index()) - .into_iter() - .flatten() - .map(|id| { - // SAFETY: component_to_containing_bundles contains only valid ids - unsafe { self.bundle_infos.get(id.index()).debug_checked_unwrap() } - }) - } - /// Gets the metadata associated with a specific type of bundle. /// Returns `None` if the bundle is not registered with the world. #[inline] @@ -535,7 +519,7 @@ impl Bundles { if let Some(id) = self.contributed_bundle_ids.get(&TypeId::of::()).cloned() { id } else { - let explicit_bundle_id = self.register_info::(components, storages); + let explicit_bundle_id: BundleId = self.register_info::(components, storages); // SAFETY: reading from `explicit_bundle_id` and creating new bundle in same time. Its valid because bundle hashmap allow this let id = unsafe { let (ptr, len) = { diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 1f7470e6f3..9890de837d 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -146,7 +146,7 @@ pub struct HotPatched; #[cfg(test)] mod tests { use crate::{ - bundle::{Bundle, BundleId, BundleInfo}, + bundle::Bundle, change_detection::Ref, component::{Component, ComponentId, RequiredComponents, RequiredComponentsError}, entity::{Entity, EntityMapper}, @@ -2595,14 +2595,6 @@ mod tests { #[derive(Component, Default)] struct E; - fn bundle_containing(world: &World, component: ComponentId) -> Option { - world - .bundles() - .iter_containing(component) - .next() - .map(BundleInfo::id) - } - let mut world = World::new(); let a_id = world.register_component::(); @@ -2621,12 +2613,6 @@ mod tests { assert!(!contributed.contains(&d_id)); assert!(!contributed.contains(&e_id)); - assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, c_id), None); - assert_eq!(bundle_containing(&world, d_id), None); - assert_eq!(bundle_containing(&world, e_id), None); - // check if registration succeeds world.register_required_components::(); let bundle = world.bundles().get(bundle_id).unwrap(); @@ -2638,12 +2624,6 @@ mod tests { assert!(contributed.contains(&d_id)); assert!(!contributed.contains(&e_id)); - assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, d_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, e_id), None); - // check if another registration can be associated to the bundle using the previously registered component world.register_required_components::(); let bundle = world.bundles().get(bundle_id).unwrap(); @@ -2654,12 +2634,6 @@ mod tests { assert!(contributed.contains(&c_id)); assert!(contributed.contains(&d_id)); assert!(contributed.contains(&e_id)); - - assert_eq!(bundle_containing(&world, a_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, b_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, c_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, d_id), Some(bundle_id)); - assert_eq!(bundle_containing(&world, e_id), Some(bundle_id)); } #[test]