documentation, test adjustment, minor implementation changes

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<Vec<BundleId>>,
+        component_to_containing_bundles: &mut Vec<Vec<BundleId>>,
         component_ids: Vec<ComponentId>,
         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<Vec<BundleId>>,
+        component_to_containing_bundles: &mut Vec<Vec<BundleId>>,
-        components_in_bundles_filter: impl Fn(ComponentId) -> bool,
+        component_to_containing_bundles_filter: impl Fn(ComponentId) -> bool,
     ) {
-        let mut components_in_bundles_push = |component: ComponentId| {
+        let mut component_to_containing_bundles_push = |component: ComponentId| {
-            if !components_in_bundles_filter(component) {
+            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();
+                    component_to_containing_bundles
-                    let iter = core::iter::repeat_n(Vec::new(), missing_minus_one)
+                        .resize_with(component.index() + 1, || Vec::new());
-                        .chain(core::iter::once(vec![self.id]));
+                    *component_to_containing_bundles.last_mut().unwrap() = vec![self.id];
-                    components_in_bundles.reserve_exact(missing_minus_one + 1);
-                    components_in_bundles.extend(iter);
                 }
             }
         };
@@ -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<Vec<BundleId>>,
+        component_to_containing_bundles: &mut Vec<Vec<BundleId>>,
     ) {
-        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_to_containing_bundles,
-                |component| !registered_components_in_bundles.contains(&component),
+                |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<ComponentId, BundleId>,
     dynamic_component_storages: HashMap<BundleId, StorageType>,
-    components_in_bundles: Vec<Vec<BundleId>>,
+    /// 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<Vec<BundleId>>,
 }
 
 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<Item = &BundleInfo> {
+    pub(crate) fn iter_containing(
-        self.components_in_bundles
+        &self,
+        component: ComponentId,
+    ) -> impl Iterator<Item = &BundleInfo> {
+        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::<T>(), storages, components, &mut self.components_in_bundles, component_ids, id) };
+                unsafe { BundleInfo::new(core::any::type_name::<T>(), 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<T: Bundle>(
         &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<bool, RequiredComponentsError> {
         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)`
+    /// The caller assures that immediately prior calling this, ...
-    /// for this `requiree` and must pass the same `storages` and `components` to this method that were used
+    /// - [`Self::verify_to_refresh_required_components`] returned `Ok(true)` for `requiree`
-    /// to create the stored bundles.
+    /// - [`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<BundleInfo>,
-    components_in_bundles: &mut Vec<Vec<BundleId>>,
+    component_to_containing_bundles: &mut Vec<Vec<BundleId>>,
     storages: &mut Storages,
     components: &Components,
     component_ids: Vec<ComponentId>,
@@ -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("<dynamic bundle>", storages, components, components_in_bundles, component_ids, id) };
+        unsafe { BundleInfo::new("<dynamic bundle>", storages, components, component_to_containing_bundles, component_ids, id) };
     bundle_infos.push(bundle_info);
 
     (id, storage_types)

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
     ///

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<BundleId> {
             world
                 .bundles()
@@ -2601,6 +2608,8 @@ mod tests {
         let a_id = world.register_component::<A>();
         let b_id = world.register_component::<B>();
         let c_id = world.register_component::<C>();
+        let d_id = world.register_component::<D>();
+        let e_id = world.register_component::<E>();
 
         let bundle = world.register_bundle::<A>();
         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::<B, C>();
         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::<D, E>();
+        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]

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<T: Bundle>(&mut self) -> &mut Self {
         self.remove_with_requires_with_caller::<T>(MaybeLocation::caller())