More state driven entity disabling components#24351
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨ |
ncthbrt
left a comment
There was a problem hiding this comment.
The recursive removal of Disable on children makes me uneasy. I'm not super read up on how the Disabled state is normally propagated to children but it feels like a child should be able to remain disabled if it was already disabled prior to entering or leaving the state (depending on what was specified).
| if transition.entered.as_ref() == Some(&enabled_in.0) { | ||
| commands | ||
| .entity(entity) | ||
| .remove_recursive::<Children, Disabled>(); |
There was a problem hiding this comment.
Removing Disabled from Children feels a little iffy. Is it not possible to have an enabled parent but a disabled child?
There was a problem hiding this comment.
is it alright to mark this as resolved?
|
Perhaps define the logic such that a child with its own DisabledIf / DisabledIn kind of component is skipped? Might get unintuitive though. We might have to add a ManuallyDisabed component or something, to cover all cases. |
I have added a new |
|
Hmmmm... I'm wondering if the design needs a rethink. The behaviour isn't quite what I'd expect. I wanted children to be disabled when their parent was disabled, as this still makes a lot of sense but crucially, when the parent is re-enabled, it shouldn't automatically remove disabled from entities that were disabled through other means. One potential design, though might be a bit contentious, would be a component that tracks how many times children have been disabled through the mechanism of their parents being disabled. An alternative design would perhaps be a DisabledSelf component that would shield itself and its children from being re-enabled when the parent is reactivated. This is very similar to your current implementation, however it would still allow the Disabled state to correctly propagate to all children. |
I went with DisabledSelf here. Would love a re-review :) |
|
If you're not going to rename it to disabled self, I'd suggest that you call it OwnsEnabled as that is more accurate to the behaviour of what the component does. Otherwise if you choose to rename it to DisabledSelf or similar, you will probably need to add an observer that adds the disabled state when the DisabledSelf component is added |
|
Are there similarities to |
the functionality we have is kinda different - since there is a case where we want only to propagate the insertion of pub enum DisabledControl {
#[default]
Independent,
InheritDisable,
}Also this should be better naming for what we have right? @ncthbrt I'd love any more feedback in this regard as well I had tried exploring to use |
Objective
EnabledIn,EnabledIf,DisabledIn,DisabledIfComponents for State driven Entity Disabling.DisabledControlcomponent that defines how Disabled propagates to its children/descendants.Solution
For each new state driven disabling component
StateTransitionEvent<S>Message and update the Entity on state transitions.On<Insert, Component>to sync when component is inserted. So user does not have to manually supply theDisabledcomponent when the Entity spawns Disabled.DisabledControlDisablecomponent removal stops at any child carryingDisabledControland its addition only stops atDisabledControl::Independent.DisabledControl::Independentso their children are independent.The children entities that only want Disabling to propagate can use
DisabledControl::InheritDisableTesting
Did you test these changes? If so, how? - unit tests with cargo test
Are there any parts that need more testing? Combining more than one of these components on a single entity.
How can other people (reviewers) test your changes? Is there anything specific they need to know?
Insert any of the new components to an entity, the
DisabledComponent can be added or removed based on the current state at insertion and then any further state changes(specifically on state enters) - which can be enabled for querying by usingAllow<Disabled>Also can try spawning children entities to test propagation of Disabling
If relevant, what platforms did you test these changes on, and are there any important ones you can't test? - Windows11