Run all OnExit before all OnEnter

[stepancheg]

StateTransition schedule executes apply_state_transition system for each state in order. apply_state_transition executes OnExit, OnTransition, OnEnter schedules in order.

https://github.com/bevyengine/bevy/blob/4eafd60ce9dd3dd579c07798ceb43f88a13d7094/crates/bevy_ecs/src/schedule/state.rs#L154-L161

This is somewhat inconvenient.

For example, application state can be controller by two state types, StateA and StateB (for example, state and substate).

Each state may want to for example, set cursor icon on enter and reset it to default on exit (clean up). This is not possible now, because cleanup of second system (OnExit) can be executed after init of first system (OnEnter).

Propose to execute systems in order:

OnExit<A>
OnExit<B>
OnTransition<A>
OnTransition<B>
OnEnter<A>
OnEnter<B>

[alice-i-cecile]

Hmm 🤔 I'm not sure we can easily do this: the apply_state_transitions system is generic over a single state.

And without "trait queries for resources" or something, I don't have a good sense of how we'd do this in general.

[stepancheg]

I'm not sure we can easily do this

Split apply_split_transition into three systems, add them separately, add common (non-generic) dependency between them: add two SystemSet labels (AfterOnExit, BeforeOnEnter) and schedule like this:

.add_systems(
  StateTransition,
  (
    apply_on_enter::<S>.before(AfterOnExit),
    apply_on_transition::<S>.after(AfterOnExit).before(BeforeOnEnter),
    apply_on_enter::<S>.after(BeforeOnEnter)
  )
)

[alice-i-cecile]

Oh clever: that's a good architecture. I need to think a bit more about whether this is a good idea to do still.

I can see your point in the original issue, but by breaking it apart, you lose atomicity of the state transitions.

Currently it's Very Hard to see / interact with the world when a state is in an in-between state. Which is obviously helpful for correctness and modularity.

[JoJoJet]

I think this schedule execution order would be more consistent than what we have now. The instances of apply_state_transition in the StateTransition schedule have no fixed run order, which means if you have a system that runs OnEnter<A> and accesses Res<State<B>>, there's no guarantee if the system will run before or after B's state transition, and it might even change order from frame to frame.

That said, wanting atomicity is a good point. If you put a call to apply_state_transition<A> inside of an OnEnter<B> schedule, it would break A, which may not be obvious. Then again, nesting calls to apply_state_transition might be asking to have your code broken.

[robojeb]

I think reordering the scheduling this way is a huge win. It should be much easier to document the expected behavior and thus reason about what is happening.

I feel like having Atomic transitions has less value than a clearly defined order for interleaving different state transitions.

Ideally if you have two different states they should be as decoupled as possible. If it is really critical for StateA, to be able to reason about what StateB is doing, then I think the user should add explicit orderings between the transition schedules (like enforcing OnExit(Substate) before OnExit(SuperState)).

[ryo33]

How about introducing StateSet that works atomically for different state sets like as is but batches the state application within the same state sets. I encountered this in my current project. In my case, it's a problem that, after changing two different state A and B in the same time, a system OnExit(A::Something) gets the old value of B. So I've created a custom state system tailered for the states A and B, that works in a way like the one suggested in here. But I noticed it can be generalized to any states. I know the atomicity is somewhat convenient in some cases, but a sync point for different states is a critical requirement in my case. StateSet gives us an opt-in option to have a sync point for specific states.

[benfrankel]

This is fixed in pyri_state by allowing you to specify dependencies between states:

#[derive(State, Clone, PartialEq, Eq)]
#[state(before(B))]
struct A;

#[derive(State, Clone, PartialEq, Eq)]
// You can specify the dependency here instead: #[state(after(A))]
struct B;

Then their system sets will run in the StateFlush schedule in this order:

StateFlushSet::<A>::Resolve
  StateFlushSet::<A>::Trigger
  StateFlushSet::<A>::Flush
    StateFlushSet::<A>::Exit
    StateFlushSet::<A>::Transition
    StateFlushSet::<A>::Enter

...

StateFlushSet::<B>::Resolve
  StateFlushSet::<B>::Trigger
  StateFlushSet::<B>::Flush
    StateFlushSet::<B>::Exit
    StateFlushSet::<B>::Transition
    StateFlushSet::<B>::Enter

Or you can manually specify any ordering requirements you want by configuring the system sets yourself.