From ded5ce27ae8d6805c1b2a41c2a862b2cfc60a9d0 Mon Sep 17 00:00:00 2001 From: Joona Aalto Date: Sun, 17 Nov 2024 15:51:39 +0200 Subject: [PATCH] Fix bubbling of runtime requirements for `#[require(...)]` attribute (#16410) # Objective Fixes #16406. Currently, the `#[require(...)]` attribute internally registers component requirements using `register_required_components_manual`. This is done recursively in a way where every requirement in the "inheritance tree" is added into a flat `RequiredComponents` hash map with component constructors and inheritance depths stored. However, this does not consider runtime requirements: if a plugins has already registered `C` as required by `B`, and a component `A` requires `B` through the macro attribute, spawning an entity with `A` won't add `C`. The `required_by` hash set for `C` doesn't have `A`, and the `RequiredComponents` of `A` don't have `C`. Intuitively, I would've thought that the macro attribute's requirements were always added *before* runtime requirements, and in that case I believe this shouldn't have been an issue. But the macro requirements are based on `Component::register_required_components`, which in a lot of cases (I think) is only called *after* the first time a bundle with the component is inserted. So if a runtime requirement is defined *before* this (as is often the case, during `Plugin::build`), the macro may not take it into account. ## Solution Register requirements inherited from the `required` component in `register_required_components_manual_unchecked`. ## Testing I added a test, essentially the same as in #16406, and it now passes. I also ran some of the tests in #16409, and they seem to work as expected. All the existing tests for required components pass. --- crates/bevy_ecs/src/component.rs | 35 ++++++++++++++++++++++++-------- crates/bevy_ecs/src/lib.rs | 24 ++++++++++++++++++++++ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 022b9def6b..7c7df1b90a 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1159,15 +1159,14 @@ impl Components { // NOTE: This should maybe be private, but it is currently public so that `bevy_ecs_macros` can use it. // We can't directly move this there either, because this uses `Components::get_required_by_mut`, // which is private, and could be equally risky to expose to users. - /// Registers the given component `R` as a [required component] for `T`, - /// and adds `T` to the list of requirees for `R`. + /// Registers the given component `R` and [required components] inherited from it as required by `T`, + /// and adds `T` to their lists of requirees. /// /// The given `inheritance_depth` determines how many levels of inheritance deep the requirement is. /// A direct requirement has a depth of `0`, and each level of inheritance increases the depth by `1`. /// Lower depths are more specific requirements, and can override existing less specific registrations. /// - /// This method does *not* recursively register required components for components required by `R`, - /// nor does it register them for components that require `T`. + /// This method does *not* register any components as required by components that require `T`. /// /// Only use this method if you know what you are doing. In most cases, you should instead use [`World::register_required_components`], /// or the equivalent method in `bevy_app::App`. @@ -1196,15 +1195,14 @@ impl Components { } } - /// Registers the given component `R` as a [required component] for `T`, - /// and adds `T` to the list of requirees for `R`. + /// Registers the given component `R` and [required components] inherited from it as required by `T`, + /// and adds `T` to their lists of requirees. /// /// The given `inheritance_depth` determines how many levels of inheritance deep the requirement is. /// A direct requirement has a depth of `0`, and each level of inheritance increases the depth by `1`. /// Lower depths are more specific requirements, and can override existing less specific registrations. /// - /// This method does *not* recursively register required components for components required by `R`, - /// nor does it register them for components that require `T`. + /// This method does *not* register any components as required by components that require `T`. /// /// [required component]: Component#required-components /// @@ -1232,6 +1230,27 @@ impl Components { // Assuming it is valid, the component is in the list of required components, so it must exist already. let required_by = unsafe { self.get_required_by_mut(required).debug_checked_unwrap() }; required_by.insert(requiree); + + // Register the inherited required components for the requiree. + let required: Vec<(ComponentId, RequiredComponent)> = self + .get_info(required) + .unwrap() + .required_components() + .0 + .iter() + .map(|(id, component)| (*id, component.clone())) + .collect(); + + for (id, component) in required { + // Register the inherited required components for the requiree. + // The inheritance depth is increased by `1` since this is a component required by the original required component. + required_components.register_dynamic( + id, + component.constructor.clone(), + component.inheritance_depth + 1, + ); + self.get_required_by_mut(id).unwrap().insert(requiree); + } } #[inline] diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 67254d6298..f18bde71f7 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -2321,6 +2321,30 @@ mod tests { ); } + #[test] + fn runtime_required_components_propagate_up() { + // `A` requires `B` directly. + #[derive(Component)] + #[require(B)] + struct A; + + #[derive(Component, Default)] + struct B; + + #[derive(Component, Default)] + struct C; + + let mut world = World::new(); + + // `B` requires `C` with a runtime registration. + // `A` should also require `C` because it requires `B`. + world.register_required_components::(); + + let id = world.spawn(A).id(); + + assert!(world.entity(id).get::().is_some()); + } + #[test] fn runtime_required_components_existing_archetype() { #[derive(Component)]