forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Use register_dynamic for merging (#18028)

Browse Source 
# Objective

I found a bug while working on #17871. When required components are
registered, ones that are more specific (smaller inheritance depth) are
preferred to others. So, if a ComponentA is already required, but it is
registered as required again, it will be updated if and only if the new
requirement has a smaller inheritance depth (is more specific). However,
this logic was not reflected in merging `RequriedComponents`s together.
Hence, for complicated requirement trees, the wrong initializer could be
used.

## Solution

Re-write merging to work by extending the collection via
`require_dynamic` instead of blindly combining the inner storage.

## Testing

I created this test to ensure this bug doesn't creep back in. This test
fails on main, but passes on this branch.

```rs
    #[test]
    fn required_components_inheritance_depth_bias() {
        #[derive(Component, PartialEq, Eq, Clone, Copy, Debug)]
        struct MyRequired(bool);

        #[derive(Component, Default)]
        #[require(MyRequired(|| MyRequired(false)))]
        struct MiddleMan;

        #[derive(Component, Default)]
        #[require(MiddleMan)]
        struct ConflictingRequire;

        #[derive(Component, Default)]
        #[require(MyRequired(|| MyRequired(true)))]
        struct MyComponent;

        let mut world = World::new();
        let order_a = world
            .spawn((ConflictingRequire, MyComponent))
            .get::<MyRequired>()
            .cloned();
        let order_b = world
            .spawn((MyComponent, ConflictingRequire))
            .get::<MyRequired>()
            .cloned();

        assert_eq!(order_a, Some(MyRequired(true)));
        assert_eq!(order_b, Some(MyRequired(true)));
    }
```
Note that when the inheritance depth is 0 (Like if there were no middle
man above), the order of the components in the bundle still matters.

## Migration Guide

`RequiredComponents::register_dynamic` has been changed to
`RequiredComponents::register_dynamic_with`.

Old:
```rust
required_components.register_dynamic(
      component_id,
      component_constructor.clone(),
      requirement_inheritance_depth,
);
```

New:
```rust
required_components.register_dynamic_with(
      component_id,
      requirement_inheritance_depth,
      || component_constructor.clone(),
);
```

This can prevent unnecessary cloning.

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
This commit is contained in:
Eagster 2025-03-10 17:35:55 -04:00 committed by GitHub
parent 913eb46324
commit 79e7f8ae0c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 124 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

164
crates/bevy_ecs/src/component.rs
View File

@ -1412,10 +1412,10 @@ impl Components {
// SAFETY: Component ID and constructor match the ones on the original requiree.
// The original requiree is responsible for making sure the registration is safe.
unsafe {
required_components.register_dynamic(
required_components.register_dynamic_with(
*component_id,
component.constructor.clone(),
component.inheritance_depth + depth + 1,
|| component.constructor.clone(),
);
};
}
@ -1458,21 +1458,21 @@ impl Components {
.collect();
// Register the new required components.
for (component_id, component) in inherited_requirements.iter().cloned() {
for (component_id, component) in inherited_requirements.iter() {
// Register the required component for the requiree.
// SAFETY: Component ID and constructor match the ones on the original requiree.
unsafe {
required_components.register_dynamic(
component_id,
component.constructor,
required_components.register_dynamic_with(
*component_id,
component.inheritance_depth,
|| component.constructor.clone(),
);
};
// Add the requiree to the list of components that require the required component.
// SAFETY: The caller ensures that the required components are valid.
let required_by = unsafe {
self.get_required_by_mut(component_id)
self.get_required_by_mut(*component_id)
.debug_checked_unwrap()
};
required_by.insert(requiree);
@ -2028,25 +2028,30 @@ impl RequiredComponents {
/// `constructor` _must_ initialize a component for `component_id` in such a way that
/// matches the storage type of the component. It must only use the given `table_row` or `Entity` to
/// initialize the storage for `component_id` corresponding to the given entity.
pub unsafe fn register_dynamic(
pub unsafe fn register_dynamic_with(
&mut self,
component_id: ComponentId,
constructor: RequiredComponentConstructor,
inheritance_depth: u16,
constructor: impl FnOnce() -> RequiredComponentConstructor,
) {
self.0
.entry(component_id)
.and_modify(|component| {
if component.inheritance_depth > inheritance_depth {
// New registration is more specific than existing requirement
component.constructor = constructor.clone();
component.inheritance_depth = inheritance_depth;
let entry = self.0.entry(component_id);
match entry {
bevy_platform_support::collections::hash_map::Entry::Occupied(mut occupied) => {
let current = occupied.get_mut();
if current.inheritance_depth > inheritance_depth {
*current = RequiredComponent {
constructor: constructor(),
inheritance_depth,
}
}
})
.or_insert(RequiredComponent {
constructor,
inheritance_depth,
});
}
bevy_platform_support::collections::hash_map::Entry::Vacant(vacant) => {
vacant.insert(RequiredComponent {
constructor: constructor(),
inheritance_depth,
});
}
}
}
/// Registers a required component.
@ -2073,64 +2078,66 @@ impl RequiredComponents {
constructor: fn() -> C,
inheritance_depth: u16,
) {
let erased: RequiredComponentConstructor = RequiredComponentConstructor({
// `portable-atomic-util` `Arc` is not able to coerce an unsized
// type like `std::sync::Arc` can. Creating a `Box` first does the
// coercion.
//
// This would be resolved by https://github.com/rust-lang/rust/issues/123430
let erased = || {
RequiredComponentConstructor({
// `portable-atomic-util` `Arc` is not able to coerce an unsized
// type like `std::sync::Arc` can. Creating a `Box` first does the
// coercion.
//
// This would be resolved by https://github.com/rust-lang/rust/issues/123430
#[cfg(not(target_has_atomic = "ptr"))]
use alloc::boxed::Box;
#[cfg(not(target_has_atomic = "ptr"))]
use alloc::boxed::Box;
type Constructor = dyn for<'a, 'b> Fn(
&'a mut Table,
&'b mut SparseSets,
Tick,
TableRow,
Entity,
MaybeLocation,
);
type Constructor = dyn for<'a, 'b> Fn(
&'a mut Table,
&'b mut SparseSets,
Tick,
TableRow,
Entity,
MaybeLocation,
);
#[cfg(not(target_has_atomic = "ptr"))]
type Intermediate<T> = Box<T>;
#[cfg(not(target_has_atomic = "ptr"))]
type Intermediate<T> = Box<T>;
#[cfg(target_has_atomic = "ptr")]
type Intermediate<T> = Arc<T>;
#[cfg(target_has_atomic = "ptr")]
type Intermediate<T> = Arc<T>;
let boxed: Intermediate<Constructor> = Intermediate::new(
move |table, sparse_sets, change_tick, table_row, entity, caller| {
OwningPtr::make(constructor(), |ptr| {
// SAFETY: This will only be called in the context of `BundleInfo::write_components`, which will
// pass in a valid table_row and entity requiring a C constructor
// C::STORAGE_TYPE is the storage type associated with `component_id` / `C`
// `ptr` points to valid `C` data, which matches the type associated with `component_id`
unsafe {
BundleInfo::initialize_required_component(
table,
sparse_sets,
change_tick,
table_row,
entity,
component_id,
C::STORAGE_TYPE,
ptr,
caller,
);
}
});
},
);
let boxed: Intermediate<Constructor> = Intermediate::new(
move |table, sparse_sets, change_tick, table_row, entity, caller| {
OwningPtr::make(constructor(), |ptr| {
// SAFETY: This will only be called in the context of `BundleInfo::write_components`, which will
// pass in a valid table_row and entity requiring a C constructor
// C::STORAGE_TYPE is the storage type associated with `component_id` / `C`
// `ptr` points to valid `C` data, which matches the type associated with `component_id`
unsafe {
BundleInfo::initialize_required_component(
table,
sparse_sets,
change_tick,
table_row,
entity,
component_id,
C::STORAGE_TYPE,
ptr,
caller,
);
}
});
},
);
Arc::from(boxed)
});
Arc::from(boxed)
})
};
// SAFETY:
// `component_id` matches the type initialized by the `erased` constructor above.
// `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(component_id, erased, inheritance_depth) };
unsafe { self.register_dynamic_with(component_id, inheritance_depth, erased) };
}
/// Iterates the ids of all required components. This includes recursive required components.
@ -2148,11 +2155,26 @@ impl RequiredComponents {
}
}
// Merges `required_components` into this collection. This only inserts a required component
// if it _did not already exist_.
/// Merges `required_components` into this collection. This only inserts a required component
/// if it _did not already exist_ *or* if the required component is more specific than the existing one
/// (in other words, if the inheritance depth is smaller).
///
/// See [`register_dynamic_with`](Self::register_dynamic_with) for details.
pub(crate) fn merge(&mut self, required_components: &RequiredComponents) {
for (id, constructor) in &required_components.0 {
self.0.entry(*id).or_insert_with(|| constructor.clone());
for (
component_id,
RequiredComponent {
constructor,
inheritance_depth,
},
) in required_components.0.iter()
{
// SAFETY: This exact registration must have been done on `required_components`, so safety is ensured by that caller.
unsafe {
self.register_dynamic_with(*component_id, *inheritance_depth, || {
constructor.clone()
});
}
}
}
}

31
crates/bevy_ecs/src/lib.rs
View File

@ -2643,6 +2643,37 @@ mod tests {
assert_eq!(to_vec(required_z), vec![(b, 0), (c, 1)]);
}
#[test]
fn required_components_inheritance_depth_bias() {
#[derive(Component, PartialEq, Eq, Clone, Copy, Debug)]
struct MyRequired(bool);
#[derive(Component, Default)]
#[require(MyRequired(|| MyRequired(false)))]
struct MiddleMan;
#[derive(Component, Default)]
#[require(MiddleMan)]
struct ConflictingRequire;
#[derive(Component, Default)]
#[require(MyRequired(|| MyRequired(true)))]
struct MyComponent;
let mut world = World::new();
let order_a = world
.spawn((ConflictingRequire, MyComponent))
.get::<MyRequired>()
.cloned();
let order_b = world
.spawn((MyComponent, ConflictingRequire))
.get::<MyRequired>()
.cloned();
assert_eq!(order_a, Some(MyRequired(true)));
assert_eq!(order_b, Some(MyRequired(true)));
}
#[test]
#[should_panic = "Recursive required components detected: A → B → C → B\nhelp: If this is intentional, consider merging the components."]
fn required_components_recursion_errors() {