Skip to content

Commit ded5ce2

Browse files
authored
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.
1 parent d3e9ecb commit ded5ce2

File tree

2 files changed

+51
-8
lines changed

2 files changed

+51
-8
lines changed

crates/bevy_ecs/src/component.rs

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1159,15 +1159,14 @@ impl Components {
11591159
// NOTE: This should maybe be private, but it is currently public so that `bevy_ecs_macros` can use it.
11601160
// We can't directly move this there either, because this uses `Components::get_required_by_mut`,
11611161
// which is private, and could be equally risky to expose to users.
1162-
/// Registers the given component `R` as a [required component] for `T`,
1163-
/// and adds `T` to the list of requirees for `R`.
1162+
/// Registers the given component `R` and [required components] inherited from it as required by `T`,
1163+
/// and adds `T` to their lists of requirees.
11641164
///
11651165
/// The given `inheritance_depth` determines how many levels of inheritance deep the requirement is.
11661166
/// A direct requirement has a depth of `0`, and each level of inheritance increases the depth by `1`.
11671167
/// Lower depths are more specific requirements, and can override existing less specific registrations.
11681168
///
1169-
/// This method does *not* recursively register required components for components required by `R`,
1170-
/// nor does it register them for components that require `T`.
1169+
/// This method does *not* register any components as required by components that require `T`.
11711170
///
11721171
/// Only use this method if you know what you are doing. In most cases, you should instead use [`World::register_required_components`],
11731172
/// or the equivalent method in `bevy_app::App`.
@@ -1196,15 +1195,14 @@ impl Components {
11961195
}
11971196
}
11981197

1199-
/// Registers the given component `R` as a [required component] for `T`,
1200-
/// and adds `T` to the list of requirees for `R`.
1198+
/// Registers the given component `R` and [required components] inherited from it as required by `T`,
1199+
/// and adds `T` to their lists of requirees.
12011200
///
12021201
/// The given `inheritance_depth` determines how many levels of inheritance deep the requirement is.
12031202
/// A direct requirement has a depth of `0`, and each level of inheritance increases the depth by `1`.
12041203
/// Lower depths are more specific requirements, and can override existing less specific registrations.
12051204
///
1206-
/// This method does *not* recursively register required components for components required by `R`,
1207-
/// nor does it register them for components that require `T`.
1205+
/// This method does *not* register any components as required by components that require `T`.
12081206
///
12091207
/// [required component]: Component#required-components
12101208
///
@@ -1232,6 +1230,27 @@ impl Components {
12321230
// Assuming it is valid, the component is in the list of required components, so it must exist already.
12331231
let required_by = unsafe { self.get_required_by_mut(required).debug_checked_unwrap() };
12341232
required_by.insert(requiree);
1233+
1234+
// Register the inherited required components for the requiree.
1235+
let required: Vec<(ComponentId, RequiredComponent)> = self
1236+
.get_info(required)
1237+
.unwrap()
1238+
.required_components()
1239+
.0
1240+
.iter()
1241+
.map(|(id, component)| (*id, component.clone()))
1242+
.collect();
1243+
1244+
for (id, component) in required {
1245+
// Register the inherited required components for the requiree.
1246+
// The inheritance depth is increased by `1` since this is a component required by the original required component.
1247+
required_components.register_dynamic(
1248+
id,
1249+
component.constructor.clone(),
1250+
component.inheritance_depth + 1,
1251+
);
1252+
self.get_required_by_mut(id).unwrap().insert(requiree);
1253+
}
12351254
}
12361255

12371256
#[inline]

crates/bevy_ecs/src/lib.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2321,6 +2321,30 @@ mod tests {
23212321
);
23222322
}
23232323

2324+
#[test]
2325+
fn runtime_required_components_propagate_up() {
2326+
// `A` requires `B` directly.
2327+
#[derive(Component)]
2328+
#[require(B)]
2329+
struct A;
2330+
2331+
#[derive(Component, Default)]
2332+
struct B;
2333+
2334+
#[derive(Component, Default)]
2335+
struct C;
2336+
2337+
let mut world = World::new();
2338+
2339+
// `B` requires `C` with a runtime registration.
2340+
// `A` should also require `C` because it requires `B`.
2341+
world.register_required_components::<B, C>();
2342+
2343+
let id = world.spawn(A).id();
2344+
2345+
assert!(world.entity(id).get::<C>().is_some());
2346+
}
2347+
23242348
#[test]
23252349
fn runtime_required_components_existing_archetype() {
23262350
#[derive(Component)]

0 commit comments

Comments
 (0)