Sub State Support

[lee-orr]

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

At this point in time, all states are entirely distinct from one another, and exclusivity between them is handled on a type-by-type basis. This is great in situations where I might want two independent state types - such as having a Walking | Running | Jumping state and a Casual | Tense | Danger state. However, sometimes I might have a set of states that can only occur when I'm within another state - for example Playing | Paused only makes sense if I'm InGame not if I'm in MainMenu.

Right now, the general approach (as outlined here: https://discord.com/channels/691052431525675048/1156638003029082254) is:

1. create a new State type with a "None" variant.
2. upon entering the parent state, you would set the state to the actual "Default" you want.
3. upon exiting the parent state, you would set the state to "None".

However, this does create some issues... For example, I could accidentally set the internal state type to something other than None outside of the valid parent state, which could lead to systems relying on that state running when they shouldn't.

Another potential issue is that I can forget to "reset" the state properly - which might lead to situations where I start on the wrong sub-state.

The other potential solution is to manually flatten the hierarchy into a single state with many variants. (So MainMenu | InGamePlaying | InGamePaused), but that will mean that systems that should run every time we're in game will have to be defined to work in both InGamePlaying and InGamePaused, but enter/exit triggers should not adjust things between those two states... all of which is harder to define the logic for when adding systems to the app.

What solution would you like?

Adding a capacity to optionally add Parent States to a state when you define it, Potentially something like:

#[derive(States, Clone, PartialEq, Eq, Hash, Debug, Default)]
pub enum RootState {
  #[default]
  Loading,
  Menu,
  InGame,
  InMiniGame
}

#[derive(States, Clone, PartialEq, Eq, Hash, Debug, Default)]
#[parent_states(RootState::Menu)]
pub enum Menus {
  #[default]
Main,
Options
}

#[derive(States, Clone, PartialEq, Eq, Hash, Debug, Default)]
#[parent_states(RootState::InGame, RootState::InMiniGame)]
pub enum Pause {
#[default] 
  Playing,
  Paused
}

Internally, the State resource would either have an optional state set to none or would fully not exist when a valid parent state isn't active.

What alternative(s) have you considered?

• Adding an .add_sub_state<State, Parent>(Parent::Variant) function to app for defining sub states, and using that to define a different set of systems related to changing state, triggering schedules, and/or "in_state". The main undesirable point here is that it forces a fully separate but compatible API for sub state's compared to normal states.

• Adjusting the "State" macro to define sub-states as values in a variant, and then set up all the change state/schedules/etc to rely on matching rather than value comparison. So for example:

enum MyState {
  Loading,
  Menu(MenuType),
  InGame(Movement, Pause),
  InMiniGame(Pause)
}

enum MenuType {
  MainMenu,
  Options,
}

enum Movement {
  Walking,
  Running
}

enum Pause {
  Playing,
  Paused
}

This causes a few issues. First - you can have an issue of combinatorial explosion on variants if you allow multiple sub states per state. Second - you need to bring all sub states into the definition of their parent state. And third, each sub-state can only have a single parent state, since a match against InGame(_,Pause::Paused) won't match `InMiniGame(Pause::Paused). So in actuality, it is treated as though it were two separate states.

Additional context

The previous model of a stack based state system would have supported some of these use cases, but had the issue of many invalid states still being possible. This approach would actively prevent invalid state combinations from occurring in game, and would be able to flag if there was a situation where an attempt was made to set an invalid state, without sacrificing the flexibility of the current state system.

Also of note - I would like to work on this, I just wanted to raise this proposal for discussion before I actually start to make sure there's a chance for people to comment on it and avoid me wasting time attempting to implement this if there's reason not to.

[lee-orr]

@mockersf raised a concern with the parent_states attribute on discord that lead to me taking some more time thinking about the "add_sub_state" api, and I actually think I might have a solution to my original concern with it.

Given I think I have a good solution there - I will see if I can implement a mock version with that API, which should provide a couple of benefits over an attribute on the type:

• You would be able to add states to arbitrary parents, including if the state is provided by an external crate. So a pattern that might become valid is plugins exporting a State type and allowing you to add it either without a parent, or with a parent that would limit the workings of the plugin to the relevant states.

• This could also open up the possibility of a "conditionally existing state" - where you provide a conditional system, and if it is true the state exists (initialized to the default, but can be changed later), and when the condition becomes false so the state will exit and get removed. possibly with an API like add_conditional_state<S>(Condition).

[lee-orr]

There was some discussion in the ecs-dev channel on discord, leading to the realization that the "variants" field in the States trait was unused - and opening up to a wider variety of structures to represent state - as seen in this PR: #9945

As a result, I'd like to fully adjust the suggestion here to work with that, providing a much more customizable solution.

The basic approach is that we'd have a States trait for the object representing the state, and then a StateMatcher trait that will be used to validate whether we are in a matching state or not (with a default implementation for States + Eq).

Things like OnEnter, OnExit and in_state would accept StateMatcher's rather than States as their parameter, though in cases that match the 0.11 version of the api nothing will have changed, since all those States implement Eq and thus are also StateMatchers.

The benefit is that you could then create much more complex states and state matchers by de-coupling these elements. For example, if I had a state that looks like this:

// Thanks to Eq, this is also a valid StateMatcher
#[derive(States, Clone, PartialEq, Eq, Hash)]
pub enum AppState {
  Loading,
  Menu,
  InGame { paused: bool }
}

pub enum GameState {
   Any,
   Playing,
   Paused
}

impl StateMatcher<AppState> for GameState {
  fn is_in_state(&self, state: &AppState) -> bool {
    match state {
      AppState::InGame { paused } => match self {
         GameState::Playing => !paused ,
         GameState::Paused => paused,
         GameState::Any => true,
      },
      _ => false
    }
  }
}

...
// In my plugin build function or the app main
app.add_systems(OnEnter(GameState::Paused), is_paused_system).add_systems(Update, in_game_regardless.run_if(in_state(GameState::Any)))

[lee-orr]

Another element worth adding here, I think, is the NextState resource. Updating state could become much more complex if these kind of nested state objects become common. A potential solution to that is allowing NextState to accept None, Some(States) or Some(Fn(S) -> S) via some trait magic. That way a user could approach it in any of the following ways easily:

NextState(Some(AppState::InGame { paused: false } ))

or

NextState(Some(|value| match value {
  AppState::InGame { _ } => AppState::InGame { paused: true },
   _ => value
}))

or

fn toggle_pause(value: AppState) -> AppState {
match value {
  AppState::InGame { paused } => AppState::InGame { paused: !paused },
   _ => value
}
}

NextState(Some(toggle_pause))

This would retain the current approach of only the most recent NextState to be set is applicable, but allow some more variation in how it occurs.

[IronGremlin]

My gut feels pretty strongly that allowing someone to write NextState(None) and have that type check, ever, is a bad idea.

I understand the need/desire for some kind of void state in some typed state representations, but each individual set of states has it's own meaningfully distinct 'None', because it's expressly scoped to that type, and not all typed state representations have anything that could be None - and in fact, many will choose to avoid that rather deliberately.

[lee-orr]

I think "None" in this case actually means "don't move to a new state". However, I agree that that is confusing, and it might be better to have something like:

enum NextState<S: States> {
  MaintainCurrent,
  StateValue(S),
  StateSetter(dyn Box<Fn(S) -> S>)
}

[lee-orr]

A big part of why I didn't initially propose that is wanting to avoid breaking current uses, but if I'm not worrying about that this is what I'd go for.

[IronGremlin]

Sorry, my understanding was that you were attempting to offer tools such that a user of State would be able to express which states were invalid via the type system.

It seems like this is not the case, and what you want to do is leverage the type system to allow a user to centrally express which states/pairings of states are or are not valid at runtime, such that a user could effectively define guardrails to prevent callers from entering invalid application states.

In which case, I would suggest that option or things that work like option aren't really what you'd want, you'd want result or something that works like result, because the caller is going to want the ability to handle failures to set desired states, and if they're doing that, information about why the call failed is important.

Also, we probably don't want to force users to care about relationships between states. EG, if OnEnter or OnExit require a StateMatcher, then any program that uses State needs at least 2 distinct types of State, which is a little silly.

Perhaps we could leverage something that expects a tuple of State types here instead?

[jnhyatt]

This causes a few issues. First - you can have an issue of combinatorial explosion on variants if you allow multiple sub states per state. Second - you need to bring all sub states into the definition of their parent state. And third, each sub-state can only have a single parent state, since a match against InGame(_,Pause::Paused) won't match `InMiniGame(Pause::Paused). So in actuality, it is treated as though it were two separate states.

These are very valid points. I like the new impl for the reason that it actually allows both. I can nest sub-states where it makes sense, and elsewhere I can add states when I enter a specific state or set of states, then remove them again whenever we want. I think it's important to point out that both of these are very valid patterns (which is called out in the example).

Still working through the PR and slowly getting on board. Under the hood it's looking really good. I'm not crazy about some of the exposed APIs, though. In particular:

• Why do we need custom matching? Can we dial that back to simple pattern matching? States are traditionally POD after all
• I don't think on_enter vs on_enter_strict is an intuitive distinction -- I'm not sure what the difference is even after reading through the impl

Most of the stuff I didn't love came from skimming through the modified state example -- there's a lot of new stuff in there that might be unintuitive/unapproachable from a beginner. The state_matcher! macro in particular, but also the custom StateMatcher impl were notably more confusing than anything in the original example. I'm not crazy about the StateMatcher API but if we absolutely need to keep it, I think it needs a rename to be clearer to end users -- although as I've mentioned, I think we should nix it. I think the ideal API would add the following:

• in_state!($pat) macro -- takes a pattern and generates a run condition matching against $pat
• OnEnter/OnExit!($pat) macro -- takes a pattern and generates a ConditionalScheduleLabel matching against `$pat$
• Some way to add a sub-state that's automatically added/removed when the parent state matches a pattern

I wouldn't add much beyond this for the sake of keeping state management simple. Let me know what you think!

[lee-orr]

Thanks for the feedback! I'm responding with some of my thoughts & questions, please feel free to let me know what you think!

* Why do we need custom matching? Can we dial that back to simple pattern matching? States are traditionally POD after all

I'm unsure what you mean by custom matching - it is just normal pattern matching

* I don't think `on_enter` vs `on_enter_strict` is an intuitive distinction -- I'm not sure what the difference is even after reading through the impl

I went back and forth on the naming here a few times, and am still uncertain. This comes in because with pattern matching, we can have a situation where we're exiting a state that matches, and entering another that also matches. The "strict" version will always execute, while the regular version will only execute if the previous (for on_enter) or next (for on_exit) state do not match. Any ideas on other names?

Most of the stuff I didn't love came from skimming through the modified state example -- there's a lot of new stuff in there that might be unintuitive/unapproachable from a beginner. The state_matcher! macro in particular, but also the custom StateMatcher impl were notably more confusing than anything in the original example. I'm not crazy about the StateMatcher API but if we absolutely need to keep it, I think it needs a rename to be clearer to end users -- although as I've mentioned, I think we should nix it.

These are necessary, and I think they can also be very useful - but having them separated out to a distinct example will probably help clarify these. I would likely still keep the state_matcher! macro in the original example, but with a clearer explanation of why it exists - which is for re-usable matching.

I think the ideal API would add the following:

* `in_state!($pat)` macro -- takes a pattern and generates a run condition matching against `$pat`

* `OnEnter/OnExit!($pat)` macro -- takes a pattern and generates a `ConditionalScheduleLabel` matching against `$pat$

* Some way to add a sub-state that's automatically added/removed when the parent state matches a pattern

A version of each of these does exist, though with a small caveat: the macros require 2 parameters in_state!($type, $pat) because Rust can't infer the type from the pattern.

[jnhyatt]

A version of each of these does exist, though with a small caveat: the macros require 2 parameters in_state!($type, $pat) because Rust can't infer the type from the pattern.

Ouch. That stinks. Although I see why it would be necessary: in_state!(_) // state type???

I went back and forth on the naming here a few times, and am still uncertain. This comes in because with pattern matching, we can have a situation where we're exiting a state that matches, and entering another that also matches. The "strict" version will always execute, while the regular version will only execute if the previous (for on_enter) or next (for on_exit) state do not match. Any ideas on other names?

I understand the problem now. In that case, strict isn't such a bad way to explain, but it still comes off confusing when you first encounter it... what about on_match and on_enter? And then on_unmatch and on_exit? on_enter/on_exit would fire any time you enter or exit a matching state, where on_match and on_unmatch would fire whenever the current state starts or stops matching? What about on_start_match and on_stop_match for those two instead?

[lee-orr]

While I wasn't able to find a way to make Rust infer the macro types, I was able to find a more ergonomic solution - allowing you to avoid the type in the pattern match.

For example, if we have the following state:

enum AppState {
  MainMenu,
  InGame { paused: bool }
}

and wanted to schedule a system whenever AppState is either MainMenu or the game is paused, the previous design would be:

on_enter!(AppState, AppState::MainMenu | AppState::InGame { paused: true } )

while now it would be:

on_enter!(AppState, MainMenu | InGame { paused: true})

As for the strict stuff, I wound up keeping the existing naming but documenting the macros better. The reasons are:

1. all of these are matching based, so on_match and on_enter would be confusing, since on_enter is also relying on matches rather than something else
2. the way we think about state is leaving one state and entering another, so on_enter and on_exit match expectations better
3. the actual distinction is just every time we enter vs. entering after we've been away. Maybe strict isn't the best word choice, but I feel it's the best one we've found so far...