StateScoped should support OnRemove observers

[UkoeHB]

What problem does this solve or what need does it fill?

A StateScoped(SomeState) entity might have a component that triggers OnRemove observers. However, any side effects of those observers potentially race with cleanup systems that run in OnExit(SomeState), since StateScoped cleanup happens in StateTransitionSteps::ExitSchedules.

Additionally, one potentially common side effect of OnRemove observers is emitting events that are handled in SomeState. It's very likely that most such events should be isolated to that state, and hence should be cleaned up in scope. Adding ergonomics here would be a win.

What solution would you like?

• Move StateScoped cleanup to a new system set that runs before StateTransitionSteps::ExitSchedules.
• Add .add_state_scoped_event<E>(s: impl States) extension to App. This should register the event for queue cleanup in a system set that runs after StateScoped entity cleanup and before StateTransitionSteps::ExitSchedules. Or on the other side (or both), it should run immediately before StateTransitionSteps::EnterSchedules when entering the target state.

What alternative(s) have you considered?

None

Additional context

• You can't easily spawn entities within an observer. This turns out to be advantageous for OnRemove handlers that run when StateScoped cleans stuff up because it means you can't easily spawn new StateScoped entities that will leak outside the state (Even though you may want to do so as part of OnRemove handling. For example I have a handler in my last jam game that runs when a CollectableDrop component is removed from any entity in order to spawn a new collectable where the dying entity was. I use an event to 'extract' the spawn command out of the observer.).
• There is some nuance to state-scoped events. If you have an event that is sent OnRemove for a StateScoped(S) entity, then the event will be naturally cleaned up without leaking back into the next S if the next S is not set for the tick immediately after the tick with the state that S exited into (since events only live for 2 ticks).

[benfrankel]

Can you provide a more concrete example? What is the OnRemove observer doing and what is it racing with? Not being able to despawn entities in OnExit(..) seems problematic, if I'm reading this correctly.

[UkoeHB]

Can you provide a more concrete example?

I hesitate to get too deep into an example, which often ends up with someone saying 'but don't do it that way'. OnRemove observers can have a lot of side effects: sending events, mutating entities, mutating resources. The example I have in mind is something thrown together for a jam game - attackable entities can drop stuff on the ground when they die. I despawn entities when they die and use an OnRemove observer to extract the info about what should be dropped. Since observers can't spawn anything, I use a normal bevy event to send that info into another system that spawns drops. Is it possible to redesign this to not use observers? Yes. But it's an intuitive pattern that many users will discover and implement in the future.

The 'recommended' pattern for games is to use states to transition between menu and gameplay (back and forth repeatedly). This means it's important for nothing to leak between sessions, including all OnRemove side effects from StateScoped entity cleanup (which I think unavoidably has the to be 'the way' to cleanup entities on state transition - no manual despawning in OnExit unless you can guarantee no side effects).

It's certainly possible (and maybe even generally better) to perform cleanup in OnEnter when re-entering a state as part of state initialization, and only use OnExit to collect information that should be passed out of the state. However, the library can't enforce that and users may have good reasons to do cleanup in OnExit.

[benfrankel]

I will say "don't do it that way" for this case because despawning != dying, and you ought to clear any lingering "spawn drops" events on exit as well (unless you can guarantee that the events will always be handled in the same frame after they're sent, which would be better practice anyways). This sounds to me like a logic issue similar to system ordering bugs that are difficult to track down but still the user's responsibility to fix.

That's not to say that there isn't a usability footgun here or maybe an actual scheduling issue, that a different example could elucidate, or again maybe I'm missing something.

It's certainly possible (and maybe even generally better) to perform cleanup in OnEnter when re-entering a state as part of state initialization, and only use OnExit to collect information that should be passed out of the state.

All cleanup should go in OnExit, so I wouldn't suggest otherwise. The primary purpose of the separate schedules is to guarantee that tearing down the previous state occurs before setting up the next state. As for whether e.g. setting a resource to its default value counts as "setup" or "teardown", that may not always be immediately obvious I guess.

[benfrankel]

My overall take on this is that trying to control all possible points where your entity could be despawned is not reasonable. If you're setting an OnRemove observer, you need to be prepared for the consequences that it could be triggered at any point in the frame. And better docs are needed to make sure this is communicated.

[UkoeHB]

My overall take on this is that trying to control all possible points where your entity could be despawned is not feasible. If you're setting an OnRemove observer, you need to be prepared for the consequences that it could be triggered at any point in the frame.

If you add StateScoped to an entity and then use OnExit to perform cleanup, you are explicitly opting into a States-oriented data management model. The fact you can't clean up the side effects of StateScoped within that framework is the fundamental problem.

And better docs are needed to make sure this is communicated.

Docs are great, but it doesn't solve the underlying ambiguity. This ambiguity makes it A) harder to use the States API correctly, B) harder to compensate for problems caused by writing less-than-perfect code (such as OnRemove side effects causing bugs).

[UkoeHB]

I added a prototype of state scoped events here.

[MiniaczQ]

We didn't design states with observers in mind, they were still in development back then. Something we can consider is moving the entire state architecture to observers, but this is quite a big change that I don't think is worth it until we unify all 3 state traits.

[MiniaczQ]

Another thing, we can add an OnAdd hook to StateScoped that will check the state and remove the entity if it's incorrect. This compliments the existing on-exit behavior by doing on-spawn validation.

[UkoeHB]

Something we can consider is moving the entire state architecture to observers, but this is quite a big change that I don't think is worth it until we unify all 3 state traits.

This seems pretty risky from a conceptual model standpoint and it's pretty unclear how it would even work. And also unclear what it would specifically do to help the StateScoped -> OnRemove issue.

[benfrankel]

Another thing, we can add an OnAdd hook to StateScoped that will check the state and remove the entity if it's incorrect. This compliments the existing on-exit behavior by doing on-spawn validation.

This is off-topic for the issue, but I don't believe that's the correct complementary behavior. The current behavior is "despawn on exit", so its complement would be "spawn on enter", which would be genuinely useful but is blocked on the better scenes work. As another example, "show on enter" + "hide on exit" is already possible.