Repository navigation
Making Required Components force existence as a follow-up of #18514 #18566
Description
What problem does this solve or what need does it fill?
The #18514 proposes a way to enforce the invariant of existence of required components.
While the following opinion can be controversial, I believe that this invariant should be held by default - as even the wording of Required Components means that the specified component is required for correct behaviour of the component it is specified for.
As for justification of having the API to enforce such a variant - see #18514, it is described there wonderfully that this both improves DX and can improve performance by removing unnecessary checks.
I propose that the behaviour of
#[removal(stops(B)]
struct A;(states that when A is present, B can not be removed) should be the default behaviour of required components, and that
#[removal(when(B)]
struct A;(states that when B is removed, A is also removed) and the current behaviour of allowing to remove the required component afterwards should be options on the required component.
What solution would you like?
#[require(B)]andworld.register_required_componentsnow also apply the logic of#[removal(stops(B)]by default.- Add a parameter to
world.register_required_componentsfor specifying the behaviour of removal tracking. - If you want the current behaviour of allowing removals of required component on
#[require(B)], you should write#[require(B, allow_removal)] struct A;
- If you want the behaviour of
#[removal(when(B)]on#[require(B)], you should write#[require(B, removed_alongside)] struct A;
What alternative(s) have you considered?
Do not change the behaviour, so in most cases the developer would write either (most of the time)
#[required(B)]
#[removal(stops(B)]
struct A;or like now (second place of commonness) to allow removal
#[required(B)]
struct A;or (in I think very rare cases)
#[required(B)]
#[removal(when(B)]
struct A;I personally think this is a worse DX than changing the behaviour of required components.
Additional context
Unity has the notion of required components in its EC architecture, and it prevents the removal of the required component both in Editor and in Playmode both from Editor UI and from scripts with the error Can't remove *B* because *A* depends on it.
Activity
alice-i-cecile commentedon Mar 28, 2025
@cart is on the fence about this, but I think that this makes sense as one of a few related variants on the concept.
ElliottjPierce commentedon Mar 28, 2025
Coming from #18514, I actually like this solution generally better from a DX perspective. But here's a few things to think about.
First, I'd like to see inverses. Not just
stopsbutstopped_by, too. That will make this more reusable besides required components. This would be trivial to layer on top of this, though I agree that it's not needed ATM.Second, I'm not sure about baking this into the syntax of
#[require()]There's lots of patterns you can use in there that might be incompatible. But I do like thatrequireadds this invariance by default. Maybe we add#[removal(allow)]to overwrite the default?Third, I don't think a silent fail with
stopsis enough. What if I need the component to be removed? I think replacing the component with a default or something (maybe reusing the required component constructor) is a better default. What if the component has aHandlewe're trying to unload? What if it has some kind of concurrent structure we need to drop its end of? I likestopsas a default, but I think it could be dangerous. I think we need aremoval(replace)or something. It's situational, but I thinkremoveshould (by default) always do the removal (even if a brand new component of the same type just replaces it).Finally, I think we should implement this very generally. Just like spawning has affects (and more planned), I think we should make this similarly extendable (even though it's not planned yet). And I think we can implement some of this at the
BundleRemoverlevel (#18521) because that will be faster than removing it with hooks, etc.inact1v1ty commentedon Mar 28, 2025
I think it can be a good idea not to bake removal into required syntax and add
#[removal(allow)]. This can result in a better DX.In this (I think a pretty rare) case, for example, unloading a
Handle, my opinion is that you can manually just do an.insertwith some value you like (Default, custom constructor or whatever), as it will overwrite the component. Stopping removal won't stop this.I personally think that doing this by default is a strange and misleading behaviour, when something tries to remove a component and it suddenly has somewhat corrupted data.
Additionally, maybe trying to remove a required component should produce a warning or error.
I think I agree with this.
ElliottjPierce commentedon Mar 28, 2025
I agree that it's rare. But having the user do
insertthemselves won't work everywhere. What the user wants is not "I want to replace this" but "I want/need to destroy this".insertdoesn't do that. We also can't let the user have separate queries for "remove these cuz they're normal" and "insert on these because I can't remove them". That's complex and, the user can't know ahead of time what will require the component in advance. Unless we makeremovereturn some kind of result, I still think (for these admittedly few situations) we need something more thanstopsorinsert.I agree here too, but all of this is "strange and misleading". It is misleading that
removedoesn't always remove. It would be misleading thatremoveis actuallyinsertbut only sometimes. And it's misleading that, depending on what downstream crates register, removingAcould also removeBas a result, even though as far as the crate with the removal knows,AandBare not related by requirement.100% agree here. This should tap into the new error handling somehow.
I think making
removereturn an error is the simplest solution, but it's worth discussion.inact1v1ty commentedon Mar 28, 2025
I agree with your points, with them considered my opinion is strongly on making
removereturn an error if it is trying to remove a required component.Also, you brought up an interesting thing:
This is a source of misleading behaviour and even bugs, maybe. It should be thought through carefully.
It is somewhat like Rust's orphans rule prevents, by the way.
ElliottjPierce commentedon Mar 28, 2025
Yeah, I agree. We're on the same page; it's just tricky lol.
alice-i-cecile commentedon Mar 28, 2025
Before we return an error here, I want to solve #17273.
ElliottjPierce commentedon Mar 30, 2025
I'm thinking about this more, and I think this is really just a special case of #1481 (archetype invariants). It's just that we want to register these invariants when we register required components. I think we should probably tackle (incrementally) #1481 and then come back to make required components trigger this behavior.
I do still think
remove,take,insert,spawn(maybe others) should return results, where it errors when the request would produce an invalid archetype.