bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.05k stars 3.56k forks source link

Shouldn't Identity state transition trigger OnExit/OnEnter ? #14447

Open Inspirateur opened 3 months ago

Inspirateur commented 3 months ago

I don't know about other bevy users but personally my state switching logic always looks something like this:

if I'm in state A and condition is met => transition to state B

There are however many cases where we want state A to lead to state A (refreshing UI, going to the next level when we're already in a level, etc…), so in these cases I naturally try to write:

if I'm in state A and condition is met => transition to state A

wishing to trigger OnExit(state A) and OnEnter(state A). But it doesn't work, as we know. Instead, we have to go through hoops and write quite a bit more code to fulfill this simple need.

Now if there are any technical reasons for which it is not desirable to make identity state transition trigger OnExit/OnEnter, I would understand.

If however, if this wasn't implemented for UX reasons, then I want to argue this decision, because:

  1. this behavior is desirable in many legit use cases
  2. I always know which state I'm in when I execute a transition, it's part of the natural state machine design;
    so there is no way I would "accidentally" transition from state A to state A and be surprised that OnExit/OnEnter triggers, it's not a footgun
  3. if for some reason I'm blindly transitioning to state A without checking the state I'm in and also didn't want OnExit/OnEnter, the simple solution would be to just… well check the current state ? Problem solved.

So I'm raising this issue to bring up these points because I think it's a common need (from what I could gather on discord anyway) and if it's intentional I think it's interesting to list here the arguments for and against for future google searches :)

benfrankel commented 3 months ago

I completely agree. In pyri_state you can mutate the next state, and then if current != next, change detection will set a flag to trigger a state transition. However, you can also set that flag yourself to "force" a transition, even if the state hasn't changed.

And then to schedule a system to handle only state change transitions, or both state change and refresh transitions, etc., is up to the user via run conditions so they have maximum flexibility there. And the basic run conditions are provided.

MiniaczQ commented 3 months ago

There is some unwanted behavior when using substates/computed states. If parent state transitioned (identity transitions included), the child will always emit a transition (identity transitions included). This can be very difficult to debug in the case of identity transitions that are desired, but not caused by parent states.

benfrankel commented 3 months ago

IMO that's an example of computed / sub states being the wrong abstraction compared to dependent states, which don't have issues like that. But that requires a larger change ofc.

shanecelis commented 3 months ago

I always know which state I'm in when I execute a transition, it's part of the natural state machine design; so there is no way I would "accidentally" transition from state A to state A and be surprised that OnExit/OnEnter triggers, it's not a footgun.

The API design is such that reading states and writing states are distinct. I do have systems that set states without reading them, so for me this kind of feature would interfere with those systems. And I'd expect a state transition to mean a change so I feel like this violates the principle of least surprise.

Rather than overloading OnEnter/OnExit, I wonder if instead there's a new verb like OnReset that might not suffer from those problems.

Inspirateur commented 3 months ago

I do have systems that set states without reading them, so for me this kind of feature would interfere with those systems.

And in those systems you really need identity transition to not trigger OnExit/OnEnter ? Would it not be acceptable to check the state here as a workaroud ?

I'd be really curious to have a concrete example of your usecase if possible

shanecelis commented 3 months ago

Sure thing.

/// Check components to determine MinibufferState's state.
pub(crate) fn set_minibuffer_state(
    query: Query<&AskyPromptState>,
    key_chords: Query<&GetKeyChord>,
    mut next_minibuffer_state: ResMut<NextState<MinibufferState>>,
) {
    let is_active = query.iter().any(|x| matches!(x, AskyPromptState::Waiting))
        || key_chords.iter().next().is_some();

    next_minibuffer_state.set(if is_active {
        MinibufferState::Active
    } else {
        MinibufferState::Inactive
    });
}

Any system that has NextState without State is an instance of this pattern.

shanecelis commented 3 months ago

Here's an idea for an alternative:

        .add_systems(OnExit(AppState::Menu), cleanup_menu)
        .add_systems(OnEnter(AppState::InGame), setup_game)
        .add_systems(OnExit(AppState::InGame), cleanup_game)
        .add_systems(Update, (cleanup_game, setup_game).chain().run_if(on_reset(AppState::InGame)))
//...

#[derive(Event)]
pub struct ResetState<S>(pub S);

pub fn on_reset<T: States>(state: T) -> impl FnMut(EventReader<ResetState<T>>) -> bool + Clone {
    move |mut reader: EventReader<ResetState<T>>| reader.read().any(|e| e.0 == state)
}
MiniaczQ commented 3 months ago

To give context as to where this stands right now:

The reason why the "Re" schedules aren't in core is because it adds overhead even when not used. A more resilient approach would be to decide which schedules to enable when adding state:

app.add_state(MyState::Startup)
  .no_default_transitions()
  .reenter()
  .reexit();

but that was way out of scope for 0.14. We can definitely have this in 0.15 though.

benfrankel commented 3 months ago

Rather than overloading OnEnter/OnExit, I wonder if instead there's a new verb like OnReset that might not suffer from those problems.

Users can have any verb they want (without adding overhead for users that don't want them) if we use run conditions + system sets instead of schedules for "on enter" / "on exit" etc.

MiniaczQ commented 1 month ago

@Inspirateur does the https://github.com/MiniaczQ/bevy/blob/main/examples/state/custom_transitions.rs resolve this issue?

Inspirateur commented 1 month ago

@Inspirateur does the https://github.com/MiniaczQ/bevy/blob/main/examples/state/custom_transitions.rs resolve this issue?

Been a while but I think this is the "going through hoops and write quite a bit more code" I was referring to. I would like it to be simpler :/

MiniaczQ commented 1 month ago

It's a copy-paste solution, best we can do is ship it in Bevy, doesn't get much better than that. There are too many downsides to using it as the default behavior, so if that's what you're requesting, I'd count this issue as won't fix. :/

benfrankel commented 1 month ago

No, there's a proof of concept in pyri_state that works just fine. This is not an unsolveable problem.

There's also this PR with a minimal implementation of same-state transitions on top of bevy_state that would have been totally fine, but I closed it because of purity-related pushback (which I still disagree with) and because I'd just published pyri_state.

benfrankel commented 1 month ago

I've created an issue with a proposal to upstream the approach taken in the custom_transitions example: https://github.com/bevyengine/bevy/issues/15478. I don't have time currently to work on implementing this myself.