Improve usability of StateStage and cut down on "magic"

[cart]

The Problem

The "right" way to use StateStage was a bit unclear and there was too much "implicit" behavior. Additionally it was cumbersome for the "common case", which is "appending systems to a state's lifecycle event stage". This pr aims to improve the situation:

This PR

remove IntoStage trait

Builder functions that need Stages now just take stages directly instead of taking IntoStage. IntoStage made it easy to use either a Stage or a System when stages are needed, but it caused more confusion than it was worth.

StateStages now have default SystemStage handlers for their lifecycle events

This makes the internal implementation simpler / reduces branching. It also cuts down on user facing boilerplate.

on_state_X functions now take Systems

on_state_X take systems instead of Stages. These methods assume that the lifecycle stage is a SystemStage (which is true by default).

removed app.add_state()

This was too "magical". It created a new stage after UPDATE with an arbitrary name. This has been replaced with the less ergonomic, but much clearer:

app
    .add_resource(State::new(AppState::Menu))
    .add_stage_after(stage::UPDATE, STAGE, StateStage::<AppState>::default())

Usage

// generally this is the only pattern people will need
app
    .add_resource(State::new(AppState::Menu))
    .add_stage_after(stage::UPDATE, STAGE, StateStage::<AppState>::default())
    .on_state_enter(STAGE, AppState::Menu, setup_menu)
    .on_state_update(STAGE, AppState::Menu, menu)
    .on_state_exit(STAGE, AppState::Menu, cleanup_menu)
    .on_state_enter(STAGE, AppState::InGame, setup_game)
    .on_state_update(STAGE, AppState::InGame, movement)
    .on_state_update(STAGE, AppState::InGame, change_color)


// for more complicated scenarios you can do things like this
app
    .add_stage_after(stage::UPDATE, STAGE, StateStage::<AppState>::default())
    .stage(STAGE, |stage: &mut StateStage<AppState>|
        stage
            .on_state_enter(AppState::Menu, some_system)
            .set_enter_stage(AppState::InGame, MyCustomStage::new())
            .exit_stage(AppState::InGame, |stage: &mut SystemStage|
                stage.add_system(some_other_system)
            )
    )

Related: #1053 #1021

[cart]

Pinging @Ratysz @alice-i-cecile @sapir @alec-deason for feedback

[alec-deason]

Seems like StateStage should be in prelude since this requires access to it in the normal case.

[alice-i-cecile]

So, to reiterate my understanding of the way this works.

1. States are used for storing useful game state, like whether an option is turned on, a menu is opened, or the game is paused.
2. States are constructed as enums, displaying one of a finite number of possible states, possibly with data attached.
3. Internally, these are stored as resources, and can be accessed by ordinary systems.
4. When we want to handle transitions in and out of these states, we create a new stage, exclusively for handling the state transition cleanly.
5. on_state_enter is run when the State is changed to the specified value in our enum. on_state_exit is the same, but for changing away from it. on_state_update occurs every frame if the state is set to that specific value.
6. We can modify existing stages with .stage, calling a lambda that alters the stage specified in a fashion similar to the pattern with commands.

Questions

1. Can we have multiple StateStages for the same state?
2. How do we handle State transitions where we care about the intersection of two or more States? I suppose you could chain it with a .with_run_criteria or make a compound enum? (I'm struggling to come up with examples as to why you might want to do this however).
3. Is there a clearer name for on_state_update? It's use isn't apparent at first glance, unlike the other two, due to ambiguity between "each update" and "when this state's value is updated".
4. Is on_state_update redundant to .with_run_criteria that checks whether a state has the specified value? Particularly once system sets are live, I'd prefer to write a system set with a run criteria that run in the stage I prefer, rather than sticking all of my logic in this unique stage.

[alec-deason]

1. Can we have multiple StateStages for the same state?

I think this has to be true because it may not be possible to run all a given state's systems in the same stage. Some may need to be before UPDATE and some after, etc.

2. How do we handle State transitions where we care about the intersection of two or more States? I suppose you could chain it with a `.with_run_criteria` or make a compound enum? (I'm struggling to come up with examples as to why you might want to do this however).

I have an example from my game. The player may be in a level (which is a state) and also have the ship editor active (which is a different state). If they exit the level at that point they must transition out of both the level state and the editor state. I've been handling that by having a central block of functions which handle complex transitions by flipping lower level states on or off. So the end_level function would change the level state and also the editor state if it was active. I haven't yet explored how I'll handle the issue with this new state API but it will likely be similar.

3. Is there a clearer name for `on_state_update`? It's use isn't apparent at first glance, unlike the other two, due to ambiguity between "each update" and "when this state's value is updated".

Hmm. That ambiguity didn't bother to me at all, or even occur to me. I just assumed it meant every tick while the state is active.

[alec-deason]

I know that this isn't at all how the system works and I think it's already been discussed elsewhere but this is what I really want the state system to do: I want to be able to run a system in any arbitrary stage, but only when a state is active. So my states would mostly be mixed all together in UPDATE but they would run conditionally. I don't think any of the new state or conditional stage stuff actually handles that and maybe it just isn't practical.

I think I can get something close to that by having multiple stages associated with a given state but it's not quite the same. Maybe that's the problem the SystemSets are meant to solve. I still haven't caught up with the discussion.

[cart]

@alice-i-cecile

Can we have multiple StateStages for the same state?

You can, with the caveat that State "transitions" are resolved within the "currently executing StateStage". If you have a STATE_UPDATE StateStage<MyState> and a STATE_POST_UPDATE StateStage<MyState>, and you change from MyState::A to MyState::B in the STATE_UPDATE stage, then the STATE_POST_UPDATE stage will just run its MyState::B update stage.

How do we handle State transitions where we care about the intersection of two or more States? I suppose you could chain it with a .with_run_criteria or make a compound enum? (I'm struggling to come up with examples as to why you might want to do this however).

This is the pattern that makes the most sense to me for "state stage nesting"

enum PauseState {
    Paused,
    Running,
}

enum AppState {
    Menu,
    InGame,
}

// This app will stop running the AppState stage whenever PauseState is "Paused"
app
    .add_stage("pause", StateStage::<PauseState>::default())
    .stage("pause", |pause_stage: &mut StateStage<PauseState>|
        pause_stage
            .set_update_stage(PauseState::Running, StateStage::<AppState>::default())
            .update_stage(PauseState::Running, |app_stage: &mut StateStage<AppState>|
                app_stage
                    .on_state_update(AppStat::Menu, some_menu_system)
                    .on_state_update(AppStat::InGame, some_game_system)
            )

    )

Is there a clearer name for on_state_update? It's use isn't apparent at first glance, unlike the other two, due to ambiguity between "each update" and "when this state's value is updated".

I can't think of anything I like more. Maybe on_state_run? Given how arbitrary these concepts are I think its hard to be fully self-documenting.

Is on_state_update redundant to .with_run_criteria that checks whether a state has the specified value? Particularly once system sets are live, I'd prefer to write a system set with a run criteria that run in the stage I prefer, rather than sticking all of my logic in this unique stage.

Currently it definitely isn't the same as slapping with_run_criteria on a SystemStage because state transitions wouldn't be handled correctly. We may or may not retire StateStage when SystemSets land if it looks like it doesn't provide any additional value. But we'll cross that bridge when we come to it 😄

@alec-deason it definitely sounds like you want SystemSets. Thats exactly what they're for 😄

[cart]

@alice-i-cecile if you really want "state intersection" logic (rather than the nesting logic), then with_run_criteria should work fine for that.

[alec-deason]

@alec-deason it definitely sounds like you want SystemSets. Thats exactly what they're for smile

Cool. I look forward to that feature then. But I have been able to express all my state logic with this version so far and I don't foresee any major problems as I move the rest over. Seems good.

[Ratysz]

To be honest, I dislike cross-hierarchy methods like these. I understand they're there to make the simple cases easy to express, but I can't help but feel that there's a better way to facilitate this than duplicating the API of a lower-level construct on a higher-level one and forwarding the calls to some present-by-default instance.

Auto-wrapping low-level bricks into appropriate high-level ones (when the user doesn't need to interact with said high level of the hierarchy directly) doesn't have the code duplication problem, but is indeed "magical" and occasionally ambiguous. Maybe making this wrapping explicit would be a good middle ground?

Sort of related, I'm not quite sure how exactly system sets are going to fit together with states.

[cart]

To be honest, I dislike cross-hierarchy methods like these.

Thats a fair take. I agree (to an extent). Ideally we have one api that works for everything. But for me this is a case of "Wanting to have my cake and eat it too". I think the "dynamicness" of the underlying api forces a certain amount of boilerplate on us for the "low level" api. Removing the "high level" api would mean pushing boilerplate on users for the common case (ex: the app.stage() api isn't as nice to use due to the explicit type requirement + closure). And I don't consider removal of the "low-level" api (downcasting + lambda) an option.

Maybe making this wrapping explicit would be a good middle ground?

I'm not quite sure what you are suggesting here. Could you provide an example?

Sort of related, I'm not quite sure how exactly system sets are going to fit together with states.

I don't have all of the details sorted out, but the general idea is to add a new SystemSet for each lifecycle handler (Ex: enter + MyState::A, exit + MyState::B, etc). Then have each SystemSet criteria look something like this:

// For the "MyState::A + exit" SystemSet

if state.just_exited() == Some(MyState::A)  {
   ShouldRun::YesAndLoop
} else if self.finished_evaluating(&state) {
   ShouldRun::No
} else {
   ShouldRun::NoAndLoop
}

That doesn't solve the "how do we actually transition states" problem though. There could be a new system that drives the transitions, but we'd need a way to force it to run last.

[Ratysz]

Maybe making this wrapping explicit would be a good middle ground?

I'm not quite sure what you are suggesting here. Could you provide an example?

Oh, nothing advanced: it's like when we had .system() for systems, but for things like stages, system sets, etc. You don't have to spell out the specific types with nested constructors, because the compiler infers everything, but it also isn't magical since you do have to add .into_stage() or .into_set() or a chain of those at the end of the builder.

That doesn't solve the "how do we actually transition states" problem though. There could be a new system that drives the transitions, but we'd need a way to force it to run last.

If all else fails the transitions can be handled by the executor explicitly, but I'm reasonably sure the implementation I'm working on will allow expressing "I want this to run at the very very end and it needs exclusive access to everything" as a system. Too early to go into specifics, though.

[cart]

Oh, nothing advanced: it's like when we had .system() for systems, but for things like stages, system sets, etc. You don't have to spell out the specific types with nested constructors, because the compiler infers everything, but it also isn't magical since you do have to add .into_stage() or .into_set() or a chain of those at the end of the builder.

I do like the "into" pattern you mentioned, but I still don't see how that would notably solve the "add system to StateStage on_enter lifecycle stage, assuming it is a SystemStage" problem, other than punting those semantics to the add_system() function (and a SystemDescriptor of some kind). I don't consider that to be a fundamental improvement in api design. Worth considering, but we'd still be adding a second "high level" api.

I'm always down to consider other apis, but the one in this pr strikes a decent balance between flexibility and usability (the best balance I've come up with yet). I think I'm cool with merging this. We can always change it later if a better design comes along. At this stage in the project, we don't need to be perfect, just "better than we were before".

If all else fails the transitions can be handled by the executor explicitly, but I'm reasonably sure the implementation I'm working on will allow expressing "I want this to run at the very very end and it needs exclusive access to everything" as a system. Too early to go into specifics, though.

Yup thats where my head is at too. We can always hard-code logic if we can't build a suitable generic abstraction.