Use register_dynamic for merging

[ElliottjPierce]

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.

Migration Guide

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

Old:

required_components.register_dynamic(
      component_id,
      component_constructor.clone(),
      requirement_inheritance_depth,
);

New:

required_components.register_dynamic_with(
      component_id,
      requirement_inheritance_depth,
      || component_constructor.clone(),
);

This can prevent unnecessary cloning.

[alice-i-cecile]

No migration guide for bug fixes in general :)

[alice-i-cecile]

Good test: that clearly demonstrated the problem to me.

[ElliottjPierce]

@alice-i-cecile The additional cloning during merging was annoying us. I was going to have to fix it anyway for #17871, but since we aren't going in that direction anymore, I went ahead and fixed it here. I added register_dynamic_with and changed to using that. That saved I think 4 cloning paths, which we now only do if the registration actually goes through.

I kept register_dynamic even though it's not used anymore. It's public, and I didn't want this to be a breaking change. Happy to remove it though, if you say the word.

[alice-i-cecile]

Let's axe register_dynamic (just have a little migration guide showing the snippet you currently have). It really doesn't seem to add much value here. Once that's done, this LGTM.

[ElliottjPierce]

@alice-i-cecile

Let's axe register_dynamic

Done! Also needs M-Needs-Migration-Guide if I understand correctly. Thanks!

[cart]

This makes sense to me. Nice catch!

[Jondolf]

Changes make sense, looks good!