Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use register_dynamic for merging #18028

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ElliottjPierce
Copy link
Contributor

@ElliottjPierce ElliottjPierce commented Feb 25, 2025

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 RequriedComponentss 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.

    #[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.

Does this need a Migration Guide?

I don't believe this needs a migration guide since it's a bug fix, but it could break user's complex requirement tree if they depended on the bug. If I should add this, let me know.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Feb 25, 2025
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants