More forms of related components: suggested and included components #16267
Conversation
alice-i-cecile
added
C-Feature
alice-i-cecile
changed the title
There was a problem hiding this comment.
This is my first Bevy PR, so not approving, just leaving comments. I thought I'd try and help out with this one since I've been so chatty on this subject on Discord!
However, I'm not familiar enough with Bevy internals to be too confident.
I've mostly identified things that escaped the find & replace (or got accidentally caught in the find & replace blast radius!) in comments. I might have commented on too many places (it's hard to tell!) and I've almost certainly missed some, too! Your mind starts to swim looking for all the "required", "related", "relations", "relatedness", etc! 😁
There are only a couple of places I've identified actual variables or code changes that might want to be made.
As I've mentioned in some of the comments, it's hard to discuss this refactor without wanting to use the word "relation" and "relationship", which is a bit of an unfortunate terminology overlap with the ongoing work on Bevy relationships. Not really sure what the winning move is here.
We could also do with figuring out the terms that more generally replace "requiree" and "required". Tricky!
Also: I am assuming that this PR is "the bare minimum to introduce the user facing concepts and make it vaguely work", because currently there are some things that are either missing or I'm too unfamiliar with Bevy's internals to understand:
- No tests for the new functionality, just porting the existing tests to the new API.
- The included and suggested components might not be exposed publicly, or at least not to the same degree that there are getters for required components.
- There doesn't seem to actually be any API yet for opting out of (or opting in to) included components.
I'm not familiar enough with Bevy's internals to understand exactly how this change ensures that required & included are added, but suggested aren't, and (as I say above) how included can be toggled. So someone else will have to verify that aspect of the PR!
Hope I've helped and not just added noise :)
| @@ -913,8 +923,8 @@ impl Components { | |||
| }) | |||
| }; | |||
| if registered { | |||
| let mut required_components = RequiredComponents::default(); | |||
| T::register_required_components(id, self, storages, &mut required_components, 0); | |||
| let mut required_components = RelatedComponents::default(); | |||
There was a problem hiding this comment.
Should this variable be related_components?
| &mut self, | ||
| id: ComponentId, | ||
| ) -> Option<&mut RequiredComponents> { | ||
| ) -> Option<&mut RelatedComponents> { | ||
| self.components | ||
| .get_mut(id.0) | ||
| .map(|info| &mut info.required_components) |
There was a problem hiding this comment.
Should this info.required_components be referring to related components?
| /// | ||
| /// In most cases, required components should be registered using the `require` attribute as shown above. | ||
| /// In most cases, required components should be registered using attributes like `require` as shown above. | ||
| /// However, in some cases, it may be useful to register required components at runtime. |
There was a problem hiding this comment.
| /// However, in some cases, it may be useful to register required components at runtime. | |
| /// However, in some cases, it may be useful to register related components at runtime. |
Maybe?
| @@ -792,8 +792,8 @@ impl App { | |||
| /// # let mut app = App::new(); | |||
| /// # app.add_plugins(MinimalPlugins).add_systems(Startup, setup); | |||
| /// // Register B as required by A and C as required by B. | |||
There was a problem hiding this comment.
| /// // Register B as required by A and C as required by B. | |
| /// // Register B as related to A and C as related to B. |
?
| /// | ||
| /// For the non-panicking version, see [`App::try_register_required_components`]. | ||
| /// For the non-panicking version, see [`App::try_register_related_components`]. | ||
| /// | ||
| /// Note that requirements must currently be registered before `T` is inserted into the world |
There was a problem hiding this comment.
| /// Note that requirements must currently be registered before `T` is inserted into the world | |
| /// Note that related components must currently be registered before `T` is inserted into the world |
?
| /// Note that requirements must currently be registered before `T` is inserted into the world | |
| /// Note that relations must currently be registered before `T` is inserted into the world |
?
| /// Note that requirements must currently be registered before `T` is inserted into the world | ||
| /// for the first time. This limitation may be fixed in the future. |
There was a problem hiding this comment.
| /// Note that requirements must currently be registered before `T` is inserted into the world | |
| /// for the first time. This limitation may be fixed in the future. | |
| /// Note that relations must currently be registered before `T` is inserted into the world | |
| /// for the first time. This limitation may be fixed in the future. |
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns a [`RequiredComponentsError`] if `R` is already a directly required component for `T`, or if `T` has ever been added | ||
| /// Returns a [`RelatedComponentsError`] if `R` is already a directly required component for `T`, or if `T` has ever been added |
There was a problem hiding this comment.
| /// Returns a [`RelatedComponentsError`] if `R` is already a directly required component for `T`, or if `T` has ever been added | |
| /// Returns a [`RelatedComponentsError`] if `R` is already a directly related component for `T`, or if `T` has ever been added |
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns a [`RequiredComponentsError`] if `R` is already a directly required component for `T`, or if `T` has ever been added | ||
| /// Returns a [`RelatedComponentsError`] if `R` is already a directly required component for `T`, or if `T` has ever been added | ||
| /// on an entity before the registration. | ||
| /// | ||
| /// Indirect requirements through other components are allowed. In those cases, any existing requirements |
There was a problem hiding this comment.
| /// Indirect requirements through other components are allowed. In those cases, any existing requirements | |
| /// Indirect relations through other components are allowed. In those cases, any existing relations |
| let requiree = self.register_component::<T>(); | ||
|
|
||
| // TODO: Remove this panic and update archetype edges accordingly when required components are added | ||
| if self.archetypes().component_index().contains_key(&requiree) { | ||
| return Err(RequiredComponentsError::ArchetypeExists(requiree)); | ||
| return Err(RelatedComponentsError::ArchetypeExists(requiree)); | ||
| } | ||
|
|
||
| let required = self.register_component::<R>(); | ||
|
|
||
| // SAFETY: We just created the `required` and `requiree` components. |
There was a problem hiding this comment.
As previous: would be nice to come up with nouns for each end of the relatedness relationship.
| /// Retrieves the [required components](RequiredComponents) for the given component type, if it exists. | ||
| pub fn get_required_components<C: Component>(&self) -> Option<&RequiredComponents> { | ||
| /// Retrieves the [related components](RelatedComponents) for the given component type, if it exists. | ||
| pub fn get_required_components<C: Component>(&self) -> Option<&RelatedComponents> { |
There was a problem hiding this comment.
Do there need to be new getters here to expose included and suggested components?
alice-i-cecile
added
S-Waiting-on-Author
|
Non-blocking, but it may be nice to add / update examples on how to do this. I'm curious how to use this myself, but don't know enough about ECS internals to read the actual source! |
| &mut self, | ||
| constructor: fn() -> R, | ||
| ) -> Result<(), RequiredComponentsError> { | ||
| relatedness: Relatedness, |
There was a problem hiding this comment.
Working very hard not to say relationship I see :)
There was a problem hiding this comment.
Yeah :p "Related" needs a rename, as I brought up on Discord.
| } | ||
| } | ||
|
|
||
| /// Retrieves the [required components](RequiredComponents) for the given component type, if it exists. | ||
| pub fn get_required_components<C: Component>(&self) -> Option<&RequiredComponents> { | ||
| /// Retrieves the [related components](RelatedComponents) for the given component type, if it exists. |
There was a problem hiding this comment.
This function should probably be called get_related_components now
| /// Retrieves the [required components](RequiredComponents) for the component of the given [`ComponentId`], if it exists. | ||
| pub fn get_required_components_by_id(&self, id: ComponentId) -> Option<&RequiredComponents> { | ||
| /// Retrieves the [related components](RelatedComponents) for the component of the given [`ComponentId`], if it exists. | ||
| pub fn get_required_components_by_id(&self, id: ComponentId) -> Option<&RelatedComponents> { |
There was a problem hiding this comment.
This function should probably be called get_related_components_by_id now
| for component_id in component_ids.iter().copied() { | ||
| // SAFETY: caller has verified that all ids are valid | ||
| let info = unsafe { components.get_info_unchecked(component_id) }; | ||
| required_components.merge(info.required_components()); | ||
| related_components.merge(info.related_components()); |
There was a problem hiding this comment.
This merge is happening per component per bundle type. Including suggested components in this computation when they are essentially "debug info" is prohibitively expensive.
This is a symptom of a larger issue. Suggested components are fundamentally different in terms of purpose when compared to required and included components. Treating them as the "the same" here has produced multiple "bad" outcomes already:
- We're (accidentally) doing a bunch of unnecessary runtime work because we decided to treat them as if they are "the same".
- Suggested components shouldn't have constructors (because they aren't used at runtime), but we parse and store them as if they do.
I think they should probably have a separate code path to prevent these sorts of things.
There was a problem hiding this comment.
Yeah, @Jondolf is of the same opinion WRT splitting out suggested components, and I think I'm largely convinced at this point.
| let required_components = required_components | ||
| .0 | ||
| related_components.remove_explicit_components(&component_ids); | ||
| let required_components = related_components |
There was a problem hiding this comment.
This doesn't actually wire up included components. Given that this is computing final "runtime" results, I believe the "merge" that happens in this function should produce a single unified hashmap across both required and included components. Using the RequiredComponents (now RelatedComponents) collection is no longer correct in this new context.
| .0 | ||
| let inherited_requirements: Vec<(ComponentId, RelatedComponent)> = required_component_info | ||
| .related_components() | ||
| .required_components |
There was a problem hiding this comment.
This should register all related components, not just requires.
|
|
||
| // Because the macro crate cannot access the `bevy_ecs` crate, we need to create our own equivalent. | ||
| // This should be kept in sync with the actual `bevy_ecs` crate! | ||
| enum Relatedness { |
There was a problem hiding this comment.
This PR needs enough work, and is "involved" enough that I'm worried about rushing it in this late in the game. This feels like the kind of thing that deserves time to be settled and tested. Additionally, this framing has been on our mind for 3 days. I think it might be the right path forward, but the whole thing is still settling in my head. It feels like we're rushing into this.
Given how the required components conversation is going (in the release post PR), I think we can swing require-only for 0.15, especially if we add the "break-glass "#[remove_require_risky(Transform)]". Given that this whole "other related components" effort is motivated by implementing the removal behavior for include to nail down the difference between the relationships, and given that this PR does not yet implement that behavior, I think our limited resources / time / risk tolerance should be spent on implementing the removal functionality (remove_require_risky). That will already be reasonably invasive.
That then gives us a whole cycle to decide what the global picture should look like, ensure the implementation is sound, and actually make use of new tools like include ( I don't think we have the time in 0.15 to go through the engine and decide what deserves require vs include).
In the 0.15 release post, we could communicate that we plan on adding additional relationship tools for more flexibility.
There was a problem hiding this comment.
Yep, I'm on board with that decision. This is involved enough and I have enough to do that I'm not keen on pushing it in last minute.
|
This is more involved than I anticipated; bumping to 0.16 to avoid shipping bugs (and also to prioritize migration guide + release notes). |
BenjaminBrienen
added
the
D-Modest
alice-i-cecile
added
S-Adopt-Me
|
did not read all of it, but does it include an idea of |
|
It does not. That's a form of #1481, which this PR covers some of, but not that element. I agree that that would be nice though! |
Objective
As discussed in #16246, required components are a great first step, but it makes sense to use this machinery for less stringent relatonships.
Similarly, #15580 complains that required components are too restrictive for power users with unusual needs, and should be able to be disabled at runtime.
Solution
As discussed on Discord, I've added two new variants of required components (collectively, related components):
See the
Relatednessdocs included with this PR for the exact behavior.Fixes #16246; fixes #15580.
Testing
So far, it compiles...
TODO