Rewrite required components

SkiFire13 · SkiFire13 · commit d0a8c6c0b869 · 2025-07-13T15:28:37.000+02:00
diff --git a/crates/bevy_ecs/macros/src/component.rs b/crates/bevy_ecs/macros/src/component.rs
@@ -237,38 +237,17 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
 
     let requires = &attrs.requires;
     let mut register_required = Vec::with_capacity(attrs.requires.iter().len());
-    let mut register_recursive_requires = Vec::with_capacity(attrs.requires.iter().len());
     if let Some(requires) = requires {
         for require in requires {
             let ident = &require.path;
-            register_recursive_requires.push(quote! {
-                <#ident as #bevy_ecs_path::component::Component>::register_required_components(
-                    requiree,
-                    components,
-                    required_components,
-                    inheritance_depth + 1
-                );
+            let constructor = match &require.func {
+                Some(func) => quote! { || { let x: #ident = (#func)().into(); x } },
+                None => quote! { <#ident as Default>::default },
+            };
+            register_required.push(quote! {
+                // SAFETY: we registered all components with the same instance of components.
+                unsafe { required_components.register::<#ident>(components, #constructor) };
             });
-            match &require.func {
-                Some(func) => {
-                    register_required.push(quote! {
-                        components.register_required_components_manual::<Self, #ident>(
-                            required_components,
-                            || { let x: #ident = (#func)().into(); x },
-                            inheritance_depth
-                        );
-                    });
-                }
-                None => {
-                    register_required.push(quote! {
-                        components.register_required_components_manual::<Self, #ident>(
-                            required_components,
-                            <#ident as Default>::default,
-                            inheritance_depth
-                        );
-                    });
-                }
-            }
         }
     }
     let struct_name = &ast.ident;
@@ -308,14 +287,12 @@ pub fn derive_component(input: TokenStream) -> TokenStream {
         impl #impl_generics #bevy_ecs_path::component::Component for #struct_name #type_generics #where_clause {
             const STORAGE_TYPE: #bevy_ecs_path::component::StorageType = #storage;
             type Mutability = #mutable_type;
-            fn register_required_components(
+            unsafe fn register_required_components(
                 _requiree: #bevy_ecs_path::component::ComponentId,
                 components: &mut #bevy_ecs_path::component::ComponentsRegistrator,
                 required_components: &mut #bevy_ecs_path::component::RequiredComponents,
-                inheritance_depth: u16,
             ) {
                 #(#register_required)*
-                #(#register_recursive_requires)*
             }
 
             #on_add
diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs
@@ -3,14 +3,15 @@ use bevy_platform::collections::{HashMap, HashSet};
 use bevy_ptr::OwningPtr;
 use bevy_utils::TypeIdMap;
 use core::{any::TypeId, ptr::NonNull};
+use indexmap::{IndexMap, IndexSet};
 
 use crate::{
     archetype::{Archetype, BundleComponentStatus, ComponentStatus},
     bundle::{Bundle, DynamicBundle},
     change_detection::MaybeLocation,
     component::{
-        ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor,
-        RequiredComponents, StorageType, Tick,
+        ComponentId, Components, ComponentsRegistrator, RequiredComponentConstructor, StorageType,
+        Tick,
     },
     entity::Entity,
     query::DebugCheckedUnwrap as _,
@@ -59,6 +60,7 @@ pub enum InsertMode {
 /// [`World`]: crate::world::World
 pub struct BundleInfo {
     pub(super) id: BundleId,
+
     /// The list of all components contributed by the bundle (including Required Components). This is in
     /// the order `[EXPLICIT_COMPONENTS][REQUIRED_COMPONENTS]`
     ///
@@ -67,9 +69,10 @@ pub struct BundleInfo {
     /// must have its storage initialized (i.e. columns created in tables, sparse set created),
     /// and the range (0..`explicit_components_len`) must be in the same order as the source bundle
     /// type writes its components in.
-    pub(super) component_ids: Vec<ComponentId>,
-    pub(super) required_components: Vec<RequiredComponentConstructor>,
-    pub(super) explicit_components_len: usize,
+    pub(super) contributed_components: Vec<ComponentId>,
+
+    /// The list of constructors for all required components indirectly contributed by this bundle.
+    pub(super) required_component_constructors: Vec<RequiredComponentConstructor>,
 }
 
 impl BundleInfo {
@@ -86,11 +89,10 @@ impl BundleInfo {
         mut component_ids: Vec<ComponentId>,
         id: BundleId,
     ) -> BundleInfo {
+        let explicit_component_ids = component_ids.iter().copied().collect::<IndexSet<_>>();
+
         // check for duplicates
-        let mut deduped = component_ids.clone();
-        deduped.sort_unstable();
-        deduped.dedup();
-        if deduped.len() != component_ids.len() {
+        if explicit_component_ids.len() != component_ids.len() {
             // TODO: Replace with `Vec::partition_dedup` once https://github.com/rust-lang/rust/issues/54279 is stabilized
             let mut seen = <HashSet<_>>::default();
             let mut dups = Vec::new();
@@ -111,41 +113,39 @@ 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() {
+        let mut depth_first_components = IndexMap::new();
+        for &component_id in &component_ids {
             // SAFETY: caller has verified that all ids are valid
             let info = unsafe { components.get_info_unchecked(component_id) };
-            required_components.merge(info.required_components());
+
+            for (&required_id, required_component) in &info.required_components().all {
+                depth_first_components
+                    .entry(required_id)
+                    .or_insert_with(|| required_component.clone());
+            }
+
             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, 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!
-                component_ids.push(component_id);
-                v.constructor
+
+        let required_components = depth_first_components
+            .iter()
+            .filter(|&(required_id, _)| !explicit_component_ids.contains(required_id))
+            .inspect(|&(&required_id, _)| {
+                // SAFETY: These ids came out of the passed `components`, so they must be valid.
+                storages.prepare_component(unsafe { components.get_info_unchecked(required_id) });
+                component_ids.push(required_id);
             })
-            .collect();
+            .map(|(_, required_component)| required_component.constructor.clone())
+            .collect::<Vec<_>>();
 
         // 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 {
             id,
-            component_ids,
-            required_components,
-            explicit_components_len,
+            contributed_components: component_ids,
+            required_component_constructors: required_components,
         }
     }
 
@@ -155,27 +155,32 @@ impl BundleInfo {
         self.id
     }
 
+    /// Returns the length of the explicit components part of the [contributed_components](Self::contributed_components) list.
+    pub(super) fn explicit_components_len(&self) -> usize {
+        self.contributed_components.len() - self.required_component_constructors.len()
+    }
+
     /// Returns the [ID](ComponentId) of each component explicitly defined in this bundle (ex: Required Components are excluded).
     ///
     /// For all components contributed by this bundle (including Required Components), see [`BundleInfo::contributed_components`]
     #[inline]
     pub fn explicit_components(&self) -> &[ComponentId] {
-        &self.component_ids[0..self.explicit_components_len]
+        &self.contributed_components[0..self.explicit_components_len()]
     }
 
     /// Returns the [ID](ComponentId) of each Required Component needed by this bundle. This _does not include_ Required Components that are
     /// explicitly provided by the bundle.
     #[inline]
     pub fn required_components(&self) -> &[ComponentId] {
-        &self.component_ids[self.explicit_components_len..]
+        &self.contributed_components[self.explicit_components_len()..]
     }
 
     /// Returns the [ID](ComponentId) of each component contributed by this bundle. This includes Required Components.
     ///
     /// For only components explicitly defined in this bundle, see [`BundleInfo::explicit_components`]
     #[inline]
     pub fn contributed_components(&self) -> &[ComponentId] {
-        &self.component_ids
+        &self.contributed_components
     }
 
     /// Returns an iterator over the [ID](ComponentId) of each component explicitly defined in this bundle (ex: this excludes Required Components).
@@ -190,7 +195,7 @@ impl BundleInfo {
     /// To iterate only components explicitly defined in this bundle, see [`BundleInfo::iter_explicit_components`]
     #[inline]
     pub fn iter_contributed_components(&self) -> impl Iterator<Item = ComponentId> + Clone + '_ {
-        self.component_ids.iter().copied()
+        self.contributed_components().iter().copied()
     }
 
     /// Returns an iterator over the [ID](ComponentId) of each Required Component needed by this bundle. This _does not include_ Required Components that are
@@ -236,7 +241,7 @@ impl BundleInfo {
         // bundle_info.component_ids are also in "bundle order"
         let mut bundle_component = 0;
         let after_effect = bundle.get_components(&mut |storage_type, component_ptr| {
-            let component_id = *self.component_ids.get_unchecked(bundle_component);
+            let component_id = *self.contributed_components.get_unchecked(bundle_component);
             // SAFETY: bundle_component is a valid index for this bundle
             let status = unsafe { bundle_component_status.get_status(bundle_component) };
             match storage_type {
diff --git a/crates/bevy_ecs/src/bundle/insert.rs b/crates/bevy_ecs/src/bundle/insert.rs
@@ -433,7 +433,7 @@ impl BundleInfo {
         }
         let mut new_table_components = Vec::new();
         let mut new_sparse_set_components = Vec::new();
-        let mut bundle_status = Vec::with_capacity(self.explicit_components_len);
+        let mut bundle_status = Vec::with_capacity(self.explicit_components_len());
         let mut added_required_components = Vec::new();
         let mut added = Vec::new();
         let mut existing = Vec::new();
@@ -457,7 +457,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_component_constructors[index].clone());
                 added.push(component_id);
                 // SAFETY: component_id exists
                 let component_info = unsafe { components.get_info_unchecked(component_id) };
diff --git a/crates/bevy_ecs/src/bundle/spawner.rs b/crates/bevy_ecs/src/bundle/spawner.rs
@@ -108,7 +108,7 @@ impl<'w> BundleSpawner<'w> {
                 table,
                 sparse_sets,
                 &SpawnBundleStatus,
-                bundle_info.required_components.iter(),
+                bundle_info.required_component_constructors.iter(),
                 entity,
                 table_row,
                 self.change_tick,
diff --git a/crates/bevy_ecs/src/component/info.rs b/crates/bevy_ecs/src/component/info.rs
@@ -1,5 +1,5 @@
 use alloc::{borrow::Cow, vec::Vec};
-use bevy_platform::{collections::HashSet, sync::PoisonError};
+use bevy_platform::sync::PoisonError;
 use bevy_ptr::OwningPtr;
 #[cfg(feature = "bevy_reflect")]
 use bevy_reflect::Reflect;
@@ -10,6 +10,7 @@ use core::{
     fmt::Debug,
     mem::needs_drop,
 };
+use indexmap::IndexSet;
 
 use crate::{
     archetype::ArchetypeFlags,
@@ -30,7 +31,10 @@ pub struct ComponentInfo {
     pub(super) descriptor: ComponentDescriptor,
     pub(super) hooks: ComponentHooks,
     pub(super) required_components: RequiredComponents,
-    pub(super) required_by: HashSet<ComponentId>,
+    /// The set of components that require this components.
+    /// Invariant: this is stored in a depth-first order, that is components are stored after the components
+    /// that they depend on.
+    pub(super) required_by: IndexSet<ComponentId>,
 }
 
 impl ComponentInfo {
@@ -505,6 +509,13 @@ impl Components {
             .and_then(|info| info.as_mut().map(|info| &mut info.hooks))
     }
 
+    #[inline]
+    pub(crate) fn get_required_components(&self, id: ComponentId) -> Option<&RequiredComponents> {
+        self.components
+            .get(id.0)
+            .and_then(|info| info.as_ref().map(|info| &info.required_components))
+    }
+
     #[inline]
     pub(crate) fn get_required_components_mut(
         &mut self,
@@ -516,7 +527,7 @@ impl Components {
     }
 
     #[inline]
-    pub(crate) fn get_required_by(&self, id: ComponentId) -> Option<&HashSet<ComponentId>> {
+    pub(crate) fn get_required_by(&self, id: ComponentId) -> Option<&IndexSet<ComponentId>> {
         self.components
             .get(id.0)
             .and_then(|info| info.as_ref().map(|info| &info.required_by))
@@ -526,7 +537,7 @@ impl Components {
     pub(crate) fn get_required_by_mut(
         &mut self,
         id: ComponentId,
-    ) -> Option<&mut HashSet<ComponentId>> {
+    ) -> Option<&mut IndexSet<ComponentId>> {
         self.components
             .get_mut(id.0)
             .and_then(|info| info.as_mut().map(|info| &mut info.required_by))
diff --git a/crates/bevy_ecs/src/component/mod.rs b/crates/bevy_ecs/src/component/mod.rs
@@ -522,11 +522,14 @@ pub trait Component: Send + Sync + 'static {
     }
 
     /// Registers required components.
-    fn register_required_components(
+    ///
+    /// # Safety
+    ///
+    /// - `_required_components` must only contain components valid in `_components`.
+    unsafe fn register_required_components(
         _component_id: ComponentId,
         _components: &mut ComponentsRegistrator,
         _required_components: &mut RequiredComponents,
-        _inheritance_depth: u16,
     ) {
     }
 
diff --git a/crates/bevy_ecs/src/component/register.rs b/crates/bevy_ecs/src/component/register.rs
@@ -5,6 +5,7 @@ use core::any::Any;
 use core::ops::DerefMut;
 use core::{any::TypeId, fmt::Debug, ops::Deref};
 
+use crate::component::enforce_no_required_components_recursion;
 use crate::query::DebugCheckedUnwrap as _;
 use crate::{
     component::{
@@ -189,8 +190,9 @@ impl<'w> ComponentsRegistrator<'w> {
     #[inline]
     pub(super) fn register_component_checked<T: Component>(&mut self) -> ComponentId {
         let type_id = TypeId::of::<T>();
-        if let Some(id) = self.indices.get(&type_id) {
-            return *id;
+        if let Some(&id) = self.indices.get(&type_id) {
+            enforce_no_required_components_recursion(self, &self.recursion_check_stack, id);
+            return id;
         }
 
         if let Some(registrator) = self
@@ -229,8 +231,15 @@ impl<'w> ComponentsRegistrator<'w> {
 
         self.recursion_check_stack.push(id);
         let mut required_components = RequiredComponents::default();
-        T::register_required_components(id, self, &mut required_components, 0);
+        // SAFETY: `required_components` is empty
+        unsafe { T::register_required_components(id, self, &mut required_components) };
+        // SAFETY:
+        // - `id` was just registered in `self`
+        // - `register_required_components` have been given `self` to register components in
+        //   (TODO: this is not really true... but the alternative would be making `Component` `unsafe`...)
+        unsafe { self.register_required_by(id, &required_components) };
         self.recursion_check_stack.pop();
+
         // SAFETY: we just inserted it in `register_component_inner`
         let info = unsafe {
             &mut self
diff --git a/crates/bevy_ecs/src/component/required.rs b/crates/bevy_ecs/src/component/required.rs
