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.
This commit is contained in:
		
							parent
							
								
									d3e9ecbb8c
								
							
						
					
					
						commit
						ded5ce27ae
					
				@ -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.
 | 
					    // 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`,
 | 
					    //       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.
 | 
					    //       which is private, and could be equally risky to expose to users.
 | 
				
			||||||
    /// Registers the given component `R` as a [required component] for `T`,
 | 
					    /// Registers the given component `R` and [required components] inherited from it as required by `T`,
 | 
				
			||||||
    /// and adds `T` to the list of requirees for `R`.
 | 
					    /// and adds `T` to their lists of requirees.
 | 
				
			||||||
    ///
 | 
					    ///
 | 
				
			||||||
    /// The given `inheritance_depth` determines how many levels of inheritance deep the requirement is.
 | 
					    /// 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`.
 | 
					    /// 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.
 | 
					    /// 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`,
 | 
					    /// This method does *not* register any components as required by components that require `T`.
 | 
				
			||||||
    /// nor does it register them for 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`],
 | 
					    /// 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`.
 | 
					    /// 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`,
 | 
					    /// Registers the given component `R` and [required components] inherited from it as required by `T`,
 | 
				
			||||||
    /// and adds `T` to the list of requirees for `R`.
 | 
					    /// and adds `T` to their lists of requirees.
 | 
				
			||||||
    ///
 | 
					    ///
 | 
				
			||||||
    /// The given `inheritance_depth` determines how many levels of inheritance deep the requirement is.
 | 
					    /// 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`.
 | 
					    /// 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.
 | 
					    /// 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`,
 | 
					    /// This method does *not* register any components as required by components that require `T`.
 | 
				
			||||||
    /// nor does it register them for components that require `T`.
 | 
					 | 
				
			||||||
    ///
 | 
					    ///
 | 
				
			||||||
    /// [required component]: Component#required-components
 | 
					    /// [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.
 | 
					        //         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() };
 | 
					        let required_by = unsafe { self.get_required_by_mut(required).debug_checked_unwrap() };
 | 
				
			||||||
        required_by.insert(requiree);
 | 
					        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]
 | 
					    #[inline]
 | 
				
			||||||
 | 
				
			|||||||
@ -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::<B, C>();
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					        let id = world.spawn(A).id();
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					        assert!(world.entity(id).get::<C>().is_some());
 | 
				
			||||||
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    #[test]
 | 
					    #[test]
 | 
				
			||||||
    fn runtime_required_components_existing_archetype() {
 | 
					    fn runtime_required_components_existing_archetype() {
 | 
				
			||||||
        #[derive(Component)]
 | 
					        #[derive(Component)]
 | 
				
			||||||
 | 
				
			|||||||
		Loading…
	
		Reference in New Issue
	
	Block a user