Summary/Description

This PR extends states to allow support for a wider variety of state types and patterns, by providing 3 distinct types of state:

Standard [States] can only be changed by manually setting the [NextState<S>] resource. These states are the baseline on which the other state types are built, and can be used on their own for many simple patterns. See the state example for a simple use case - these are the states that existed so far in Bevy.
[SubStates] are children of other states - they can be changed manually using [NextState<S>], but are removed from the [World] if the source states aren't in the right state. See the sub_states example for a simple use case based on the derive macro, or read the trait docs for more complex scenarios.
[ComputedStates] are fully derived from other states - they provide a compute method that takes in the source states and returns their derived value. They are particularly useful for situations where a simplified view of the source states is necessary - such as having an InAMenu computed state derived from a source state that defines multiple distinct menus. See the computed state example to see a sampling of uses for these states.

Objective

This PR is another attempt at allowing Bevy to better handle complex state objects in a manner that doesn't rely on strict equality. While my previous attempts (https://github.com/bevyengine/bevy/pull/10088 and https://github.com/bevyengine/bevy/pull/9957) relied on complex matching capacities at the point of adding a system to application, this one instead relies on deterministically deriving simple states from more complex ones.

As a result, it does not require any special macros, nor does it change any other interactions with the state system once you define and add your derived state. It also maintains a degree of distinction between State and just normal application state - your derivations have to end up being discreet pre-determined values, meaning there is less of a risk/temptation to place a significant amount of logic and data within a given state.

Addition - Sub States

closes #9942 After some conversation with Maintainers & SMEs, a significant concern was that people might attempt to use this feature as if it were sub-states, and find themselves unable to use it appropriately. Since ComputedState is mainly a state matching feature, while SubStates are more of a state mutation related feature - but one that is easy to add with the help of the machinery introduced by ComputedState, it was added here as well. The relevant discussion is here: https://discord.com/channels/691052431525675048/1200556329803186316

Solution

closes #11358

The solution is to create a new type of state - one implementing ComputedStates - which is deterministically tied to one or more other states. Implementors write a function to transform the source states into the computed state, and it gets triggered whenever one of the source states changes.

In addition, we added the FreelyMutableState trait , which is implemented as part of the derive macro for States. This allows us to limit use of NextState<S> to states that are actually mutable, preventing mis-use of ComputedStates.

Changelog

Added ComputedStates trait
Added FreelyMutableState trait
Converted NextState resource to an Enum, with Unchanged and Pending
Added App::add_computed_state::<S: ComputedStates>(), to allow for easily adding derived states to an App.
Moved the StateTransition schedule label from bevy_app to bevy_ecs - but maintained the export in bevy_app for continuity.
Modified the process for updating states. Instead of just having an apply_state_transition system that can be added anywhere, we now have a multi-stage process that has to run within the StateTransition label. First, all the state changes are calculated - manual transitions rely on apply_state_transition, while computed transitions run their computation process before both call internal_apply_state_transition to apply the transition, send out the transition event, trigger dependent states, and record which exit/transition/enter schedules need to occur. Once all the states have been updated, the transition schedules are called - first the exit schedules, then transition schedules and finally enter schedules.
Added SubStates trait
Adjusted apply_state_transition to be a no-op if the State<S> resource doesn't exist

Migration Guide

If the user accessed the NextState resource's value directly or created them from scratch they will need to adjust to use the new enum variants:

if they created a NextState(Some(S)) - they should now use NextState::Pending(S)
if they created a NextState(None) -they should now use NextState::Unchanged
if they matched on the NextState value, they would need to make the adjustments above

If the user manually utilized apply_state_transition, they should instead use systems that trigger the StateTransition schedule.

Future Work

There is still some future potential work in the area, but I wanted to keep these potential features and changes separate to keep the scope here contained, and keep the core of it easy to understand and use. However, I do want to note some of these things, both as inspiration to others and an illustration of what this PR could unlock.

NextState::Remove - Now that the State related mechanisms all utilize options (#11417), it's fairly easy to add support for explicit state removal. And while ComputedStates can add and remove themselves, right now FreelyMutableStates can't be removed from within the state system. While it existed originally in this PR, it is a different question with a separate scope and usability concerns - so having it as it's own future PR seems like the best approach. This feature currently lives in a separate branch in my fork, and the differences between it and this PR can be seen here: https://github.com/lee-orr/bevy/pull/5
NextState::ReEnter - this would allow you to trigger exit & entry systems for the current state type. We can potentially also add a NextState::ReEnterRecirsive to also re-trigger any states that depend on the current one.
More mechanisms for State updates - This PR would finally make states that aren't a set of exclusive Enums useful, and with that comes the question of setting state more effectively. Right now, to update a state you either need to fully create the new state, or include the Res<Option<State<S>>> resource in your system, clone the state, mutate it, and then use NextState.set(my_mutated_state) to make it the pending next state. There are a few other potential methods that could be implemented in future PRs:
- Inverse Compute States - these would essentially be compute states that have an additional (manually defined) function that can be used to nudge the source states so that they result in the computed states having a given value. For example, you could use set the IsPaused state, and it would attempt to pause or unpause the game by modifying the AppState as needed.
- Closure-based state modification - this would involve adding a NextState.modify(f: impl Fn(Option<S> -> Option<S>) method, and then you can pass in closures or function pointers to adjust the state as needed.
- Message-based state modification - this would involve either creating states that can respond to specific messages, similar to Elm or Redux. These could either use the NextState mechanism or the Event mechanism.
~SubStates - which are essentially a hybrid of computed and manual states. In the simplest (and most likely) version, they would work by having a computed element that determines whether the state should exist, and if it should has the capacity to add a new version in, but then any changes to it's content would be freely mutated.~ this feature is now part of this PR. See above.
Lastly, since states are getting more complex there might be value in moving them out of bevy_ecs and into their own crate, or at least out of the schedule module into a states module. #11087

As mentioned, all these future work elements are TBD and are explicitly not part of this PR - I just wanted to provide them as potential explorations for the future.

Jan 19 '24 21:01 lee-orr

@alice-i-cecile & @MiniaczQ - just wanted to ping you for this new PR, as promised a few weeks ago here: https://github.com/bevyengine/bevy/pull/10088#issuecomment-1871655690

Jan 19 '24 21:01 lee-orr

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Jan 19 '24 21:01 github-actions[bot]

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Jan 20 '24 03:01 github-actions[bot]

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Jan 20 '24 04:01 github-actions[bot]

Overall I like this solution, it's very flexible when it comes to reading states.

I originally thought the computed state is not stored in the world, and instead computed directly on the spot. But storing it certainly blends in better with the existing states API.

One thing that has to be addressed before this gets merged is the NextState, we cannot have it exist for computed states.

Speaking of which, a sort of mirrored API of this for mutating part of the AppState would be an ideal complement to make a full solution. This one would use NextState, would not be saved in the world, and it would need to be ordered correctly against the state it's modifying. That way you could apply both solutions to a single type and effectively have an alternative state representation which you can read and mutate.

Jan 20 '24 15:01 MiniaczQ

Overall I like this solution, it's very flexible when it comes to reading states.

I originally thought the computed state is not stored in the world, and instead computed directly on the spot. But storing it certainly blends in better with the existing states API.

One thing that has to be addressed before this gets merged is the NextState, we cannot have it exist for computed states.

Speaking of which, a sort of mirrored API of this for mutating part of the AppState would be an ideal complement to make a full solution. This one would use NextState, would not be saved in the world, and it would need to be ordered correctly against the state it's modifying. That way you could apply both solutions to a single type and effectively have an alternative state representation which you can read and mutate.

I would love to provide that mirrored API and avoid relying on NextState - my main concern around it was that I didn't want to bloat up this PR too much - especially where it might add complexity. Changing things so that ComputedStates can't use NextState, as well as adding an optional, more "message-driven", state mutation API would require more changes to the base implementation of States, which I was trying to avoid here.

@MiniaczQ @alice-i-cecile - I am happy to add that into this PR, or to create a separate PR with a proposal around that, but would really want some feedback on whether that complexity is worth risking this not going through.

The other element I think is important is state removal - I explained why I think it's needed above (https://github.com/bevyengine/bevy/pull/11426#discussion_r1460483356), but would like to hear your feedback with that in mind.

Jan 21 '24 00:01 lee-orr

What would happen if you had 3 states: Foo Bar and FooBar (computed from previous two) and both Foo and Bar changed in the same tick? Would it trigger one FooBar transition, then the other? What would the order be?

Jan 23 '24 00:01 MiniaczQ

What would happen if you had 3 states: Foo Bar and FooBar (computed from previous two) and both Foo and Bar changed in the same tick? Would it trigger one FooBar transition, then the other? What would the order be?

The new value would be computed twice, but the transition should only run once. That's the reason for separating the transitions being triggered from the systems that modify the state values - it let's us deduplicate this kind of thing, and maintain a consistent order of entering/exiting states.

However, I just had a thought on a potential issue here, so I will validate this and let you know.

Jan 23 '24 02:01 lee-orr

My biggest problem with the implementation of ComputedStates is as follows. Keep in mind this is a user-facing perspective and I don't have the full context on the actual implementation of state, so take everything I say with a grain of salt.

I'm unsold on using 'None' to represent an active variant of state. The PR which was just merged and added support for optional state seemed to be mostly focused on preventing panics, but, in this example, 'None' is now used to represent an actual logical result when computing the state.

This seems like we are adding unnecessary complexity. Personally, I'd advise on moving away from using marker types for state where None is seen as a logical variant. To me, None seems like to should only be used to represent uninitialized or removed state.

The user facing API I'd like to see in the examples would be, for example, fn compute(sources: AppState) -> Self {. Then, internally, Bevy could compute any derived state where any source state is None as None.

My argument for this is that, as is, the .set(...) API only allows setting the state to some variant of State, and does not allow setting the state to None. Not including None for ComputedStates would make the ComputedStates trait API a closer match to the API for the FreelyMutableState (i.e. NextState). More specifically, and I could be mistaken, but I'm not aware that a .remove(...) API that exists for NextState. I.e.: the only way to set FreelyMutableState to none is by manually removing the state resource.

Edit: Other than that, this looks good! I think if we agree on the suggested changes above, ComputedStates becomes quite simple. I think it could be a useful tool for a couple of scenarios in my game, and I'd be happy to give this community approval.

Jan 27 '24 17:01 marcelchampagne

This seems like we are adding unnecessary complexity. Personally, I'd advise on moving away from using marker types for state where None is seen as a logical variant. To me, None seems like to should only be used to represent uninitialized or removed state.

If we can get away with it, this would be my preference too. Reducing complexity is key here, both for implementation and learning.

Jan 27 '24 18:01 alice-i-cecile

My biggest problem with the implementation of ComputedStates is as follows. Keep in my this is a user-facing perspective and I don't have the full context on the actual implementation of state, so take everything I say with a grain of salt.

I'm unsold on using 'None' to represent an active variant of state. The PR which was just merged and added support for optional state seemed to be mostly focused on preventing panics, but, in this example, 'None' is now used to represent an actual logical result when computing the state.

This seems like we are adding unnecessary complexity. Personally, I'd advise on moving away from using marker types for state where None is seen as a logical variant. To me, None seems like to should only be used to represent uninitialized or removed state.

The user facing API I'd like to see in the examples would be, for example, fn compute(sources: AppState) -> Self {. Then, internally, Bevy could compute any derived state where any source state is None as None.

My argument for this is that, as is, the .set(...) API only allows setting the state to some variant of State, and does not allow setting the state to None. Not including None for ComputedStates would make the ComputedStates trait API a closer match to the API for the FreelyMutableState (i.e. NextState). More specifically, and I could be mistaken, but I'm not aware that a .remove(...) API that exists for NextState. I.e.: the only way to set FreelyMutableState to none is by manually removing the state resource.

Edit: Other than that, this looks good! I think if we agree on the suggested changes above, ComputedStates becomes quite simple. I think it could be a useful tool for a couple of scenarios in my game, and I'd be happy to give this community approval.

@marcelchampagne - This is actually something I disagree with for a few reasons:

First, there are situations where a state's existence doesn't really make sense, for example: "Paused/UnPaused" is meaningless when you are in the main menu. Not utilizing None means that end users will need to embed a None variant into their states, for example having Paused/UnPaused/CantBePaused variants, which is both not idiomatic to Rust and prevents the use of many of the features Option provides without added implementation overhead, like the ? operator and .map/.and_then/etc. Or, you force the users to implement their compute states on Option<IsPaused> rather than IsPaused, and if they query the State resource directly they'll have multiple unwraps.
Second, for situations that are a simple binary, you now have to create an Enum or a struct with a Boolean rather than following the Marker pattern that ECS's encourage. Because people will already be used to using Marker components, having Marker states seems, at least to me, to be an easy and simple extension.
Thirdly, people might, for whatever reason, remove the State<S> resource manually. Because that is possible, it's probably a good idea for these systems to be robust to that kind of behavior. And for ComputedStates - it might cause weird situations. For example, if the user manually removed a source state that can be used, but isn't necessary, for a computed state to resolve (for example, removing a source state that deals with in-game data when you exit the game, but is used by the computed state to determine which tutorial to display at a given time. It doesn't need the in-game state to display the menu tutorials, but it does depend on them to compute which tutorials to show). Or, if the user manually removed the State<S> for the computed state itself, but it showed up the next frame because it was re-computed with a state change.

There are also 2 future features I would love to add in separate PRs that will depend on this:

SubStates (i.e. states that only exist when certain conditions are met, but which can be manually modified)
NextState.remove - which was removed from this PR due to being out of scope, but is also likely to be useful. Especially since it could discourage people from trying to manually remove State resources as a workaround when the system doesn't expose it nicely or handle it in a robust manner.

Jan 27 '24 18:01 lee-orr

There are some "middle ground" options I can think of (though haven't tried, so they might not work), but I fear they all add more complexity overall than they reduce the user-facing learning curve. I will leave it to y'all whether it's worth attempting though:

adjusting the "StateSet" impl so you need to manually specify which types can be optional and which can't. If one of the non-optional types doesn't exist, the computed state is removed. Otherwise, it calls the computation.
adding a trait that is auto-implemented for both the ComputedState and Option<ComputedState>, and having the compute function return something that implements that trait. That way if you don't need the state to be removable, you just return the computed state directly.
creating a OptionalComputedState trait with the current parameters, and having ComputedState not support any optional arguments or returns - it would just be removed if any of it's inputs don't exist.

Jan 27 '24 18:01 lee-orr

@marcelchampagne @alice-i-cecile - please let me know your thoughts on this

Jan 27 '24 18:01 lee-orr

First, there are situations where a state's existence doesn't really make sense, for example: "Paused/UnPaused" is meaningless when you are in the main menu.

This is the strongest argument to me.

Second, for situations that are a simple binary, you now have to create an Enum or a struct with a Boolean rather than following the Marker pattern that ECS's encourage. Because people will already be used to using Marker components, having Marker states seems, at least to me, to be an easy and simple extension.

This is nice, but generally speaking, users have an easier time reasoning about more verbose, less implicit APIs (when they get started).

Thirdly, people might, for whatever reason, remove the State<S> resource manually. Because that is possible, it's probably a good idea for these systems to be robust to that kind of behavior.

Agreed, I think it's good to be robust to this regardless of our final API :)

All of the proposed middle-ground solutions feel worse than the two extremes. Overall I'm content to leave this as is: I think it's reasonably straightforward to teach and maintain, and I do appreciate that it correctly represents meaningless questions.

We don't have to get the exact API perfect on the first go: I'd much rather move forward and ship this, then adapt based on user feedback and experimentation.

Jan 27 '24 18:01 alice-i-cecile

This is actually something I disagree with for a few reasons:

First, there are situations where a state's existence doesn't really make sense, for example: "Paused/UnPaused" is meaningless when you are in the main menu. Not utilizing None means that end users will need to embed a None variant into their states, for example having Paused/UnPaused/CantBePaused variants, which is both not idiomatic to Rust and prevents the use of many of the features Option provides without added implementation overhead, like the ? operator and .map/.and_then/etc. Or, you force the users to implement their compute states on Option<IsPaused> rather than IsPaused, and if they query the State resource directly they'll have multiple unwraps.

Hmm, this is a good point. And makes me slightly more in favor, but I'm still slightly worried we are over-optimizing for complex use cases here. Adding a None variant to an enum might not be strictly idiomatic Rust but in my opinion it feels okay as a fallback if in 95% of cases users don't need to reach for that.

Second, for situations that are a simple binary, you now have to create an Enum or a struct with a Boolean rather than following the Marker pattern that ECS's encourage. Because people will already be used to using Marker components, having Marker states seems, at least to me, to be an easy and simple extension.

State seems more akin to resources and marker resources aren't a common pattern AFAIK. Reading the examples in this PR, it would have been much easier for me to parse what was going on from a strict readability perspective if the example had used a struct with a boolean or an enum rather than Marker states. The reason is that in order to understand the Marker struct usage here you need to understand a bit about the internals of Bevy state. Off the top of my head, I wouldn't know what happens when you use NextState<T> and the state is None, for example.

Thirdly, people might, for whatever reason, remove the State<S> resource manually. Because that is possible, it's probably a good idea for these systems to be robust to that kind of behavior. And for ComputedStates - it might cause weird situations. For example, if the user manually removed a source state that can be used, but isn't necessary, for a computed state to resolve (for example, removing a source state that deals with in-game data when you exit the game, but is used by the computed state to determine which tutorial to display at a given time. It doesn't need the in-game state to display the menu tutorials, but it does depend on them to compute which tutorials to show). Or, if the user manually removed the State<S> for the computed state itself, but it showed up the next frame because it was re-computed with a state change.

This seems like something that is an advanced use-case and should be discouraged. Other than supporting Marker states, what is the advantage of removing a state manually? It just seems like it means there is more (excuse the pun) state to reason about.

There are also 2 future features I would love to add in separate PRs that will depend on this:

SubStates (i.e. states that only exist when certain conditions are met, but which can be manually modified)

This seems like a compelling reason! I don't personally have enough context to understand why compute needs to take an optional state to support this so I can't comment.

NextState.remove - which was removed from this PR due to being out of scope, but is also likely to be useful. Especially since it could discourage people from trying to manually remove State resources as a workaround when the system doesn't expose it nicely or handle it in a robust manner.

Same concern as above that this just introduces more complexity. Happy to hear the argument for it though! I'd like to see some concrete use cases that give some significant ergonomic improvements over the alternative.

Jan 27 '24 18:01 marcelchampagne

We don't have to get the exact API perfect on the first go: I'd much rather move forward and ship this, then adapt based on user feedback and experimentation.

I also agree with this. This implementation isn't forcing users into using Marker states if they choose not to and the downside of having to use Option<State> in the ComputedStates compute method isn't big enough for me to think we should block shipping this. Either way, I think you've done a good job on this and have a fairly usable and useful solution for users!

Jan 27 '24 19:01 marcelchampagne

One more thought. If you want optional computed state it seems reasonable for users to: a) Implement their computed states on Option<SubState> OR b) Explicitly define computed state with an Option

#[derive(Debug, Clone, Copy, Default, Eq, PartialEq, Hash, States, Deref)]
pub struct MyState(Option<MyEnum>);

Is there a major downside to a) other than if they query the State resource directly they'll have multiple unwraps? That seems like a pretty minor and rarely encounter downside, if so.

Jan 27 '24 19:01 marcelchampagne

@marcelchampagne - I think there's a bit of a weird mental gap with States, at least based on my experience. When I first started trying to make games with Bevy, I ran into a lot of issues that came from the fact that states couldn't be removed, and couldn't be effectively synchronized. I had a lot of strange bugs with UI and stuff in my early game jams due to this stuff. But once I understood how States work, it became unnecessary in most situations... So now it's a "tool for advanced use", but it was also the intuitive thing when I knew nothing.

I do think that making it as clear as possible for people is important - do you feel non-marker computed states need more presence in the example? (right now, IsPaused & Tutorial are both enum based).

Jan 27 '24 19:01 lee-orr

As for reasons to query state directly, the biggest ones I can think of are:

if you have an element that should indicate the state change, but isn't worth it's own separate system to manage. For example, the "Turbo Mode" in the example is queried manually, and it's existence is used to determine the movement speed.
if you need to make a partial modification of the State. This would only involve querying non-computed states ATM, but since we don't have a built in way to partially update a state right now you need to query it, mutate a clone, and then use NextState.set to actually change it. This isn't something you would have run into often so far, since all states had to be mutually exclusive, but it'll be more necessary now. That being said, I have used it in the past to - one of my games had multiple tools you could toggle between, and I had to query the current tool to know which the next one would be.

Jan 27 '24 19:01 lee-orr

@lee-orr Ahh, by queried directly you just mean a system which uses Res<State<MyComputedState>>? Yes, that quite common in my experience and I use that pattern throughout my game as well.

I'm still slightly confused as to why you say multiple unwraps are required though. If you add Res<State<MyComputedState>> as the query, you'd have the same number, right?

Having Bevy internally default to using Option<Res<State<S>>> like in https://github.com/bevyengine/bevy/pull/11417 makes sense. Having that be the default thing we except users to use, however, doesn't seem desirable. I think that your argument is that removing resources was the intuitive thing for you, is that correct? If so, I'm not sure I understand why. Is it because you were used to using ZSTs, and expected state to behave in the same way?

Jan 27 '24 19:01 marcelchampagne

Actually no - I didn't get used to ZSTs until much later.

The easiest example I can give is loading into a level from a menu screen: the resource defining the current level is created when I load in, and removed when I return to the main menu. This way it is always re-set when I enter a level, and doesn't need to be worried about outside there (I can have systems that run if the resource exists, or when it's added, and can have it reference specific entities that I will remove later)

In the game I'm thinking of ATM, I would have multiple tools you could toggle between, but not all of them were available every level. But, the state resource for the current tool was. This meant that I was getting issues where sometimes that resource would persist between different levels, and my solution was to destroy it when leaving a level and re-create it when entering - just like my level definition resource.

Jan 27 '24 19:01 lee-orr

You're right, it a user could just use Res rather than Option<Res>. That would only require one unwrap there, I just find that approach less intuitive in this case. I think that might be because we already have the capacity for optional resources, so it feels like an unnecessary added layer to me.

Jan 27 '24 19:01 lee-orr

@lee-orr That all makes sense! I guess the trade-off here is do we optimize the APIs (compute trait method, Res<State<MyState>>, etc.) for optional state or non-optional state? My intuition was that most users wouldn't reach for optional state except in advanced use-cases and thus that we would want to optimize the ComputedState trait around that. I do see your argument that this is the more coherent API for working with optional state, and if we think that is a common enough pattern that we to encourage users to utilize that it makes sense to stick with it.

That said, I think I still would prefer fn compute(sources: RootState) -> Self { as I think it would pretty significantly reduce the complexly of the example and primary use cases of this feature but the final call is definitely yours and I'd be support merging this either way!

As far as:

I do think that making it as clear as possible for people is important - do you feel non-marker computed states need more presence in the example? (right now, IsPaused & Tutorial are both enum based).

I'm fine with the example as it stands if we decide to stick with optional state as the default API.

One more thought / EDIT:

the state resource for the current tool was. This meant that I was getting issues where sometimes that resource would persist between different levels, and my solution was to destroy it when leaving a level and re-create it when entering - just like my level definition resource.

Curious if you think updating your game to use an enum with a None variant for your state and not using optional state would actually make your game any less ergonomic? If Bevy pushes you in that direction through the examples and docs, maybe that's the best final API and we could eliminate any of the confusion you ran into early on.

Jan 27 '24 22:01 marcelchampagne

Note from me, because it seems like 2 topics are being confused here. Marker states and none-able computed states are separate. I too don't see myself using ZSTs for computed states, but I don't see why we should prevent anyone from doing so. None-able computed states are a necessity, even if you fully forbid ZSTs. Here's a very simple example:

enum AppState {
  Menu,
  Game(GameState)
}

// Note: this is both
// - part of app-state
// - computed state that acts as a sub-set of app state
enum GameState {
  Running,
  Paused
}

app.init_state::<AppState>();
app.compute_state::<GameState>();

In this example there are no ZSTs, but GameState computed state makes absolutely no sense during AppState::Menu and it's unreasonable to expect every state to introduce a "none" variant.

Jan 27 '24 23:01 MiniaczQ

@MiniaczQ

In this example there are no ZSTs, but GameState computed state makes absolutely no sense during AppState::Menu and it's unreasonable to expect every state to introduce a "none" variant.

I guess that's where my opinion differs. Personally, I think it feels pretty reasonable and actually less opaque to write:

enum GameState {
  Uninitialized,
  Running,
  Paused
}

If you are in a menu, it makes sense to me to say that the GameState is Uninitialized. In fact, I'd argue from a functional standpoint it is actually more useful because you can schedule systems for that variant: i.e. OnEnter(GameState::Uninitialized, system) or OnExit(GameState::Uninitialized, system).

It also means implementing the trait becomes very concise:

impl ComputedStates for GameState {
    type SourceStates = AppState;

    fn compute(sources: AppState) -> Self {
        match sources {
            AppState::InGame(state) => state,
            _ => GameState::Uninitialized,
        }
    }
}

I'm curious what makes that feel unreasonable and/or not make sense to you?

Jan 27 '24 23:01 marcelchampagne

It's unreasonable to scale that up, it looks fine with one enum, but you will have to do that to every enum. The same 'none' variant to every state. On the other hand Option is the type for uninitialized things.

Another argument can be made that besides ZSTs and enums there are structs, and holding an 'uninitialized: bool' field in a struct makes no sense.

Also something specific for my example is that you cannot reuse the same enum for sub-state and computed state, because computed state would require additional variant. While it's only an example, I think it has a high potential of becoming one of the go-to patterns because it exposes nested substates to transition schedules with little boilerplate.

Jan 27 '24 23:01 MiniaczQ

It's unreasonable to scale that up, it looks fine with one enum, but you will have to do that to every enum. The same 'none' variant to every state. On the other hand Option is the type for uninitialized things.

That seems like a somewhat unfounded claim. If it works well for one enum, can be defined concisely, adds very little boilerplate, and is more ergonomic / less verbose for new users -- why exactly doesn't it scale? I have a 40K LOC project with a handful of states and I don't see any reason why having an uninitialized variant for computed enum states that you want to support being uninitialized is an issue at all.

Maybe what you mean is that you feel it isn't idiomatic Rust, but that's not an argument that wins me over by default. Bevy does many things that aren't considered ergonomic Rust (e.g. encouraging deriving Deref on types that aren't smart pointers, moving a fair amount of safety to runtime panics, etc.) in order to get a better and more ergonomic APIs for end users.

If we went with the API I am suggesting optional computed state is absolutely still supported by doing impl ComputedStates for Option<GameState>. It just means that if you use it, you'd have a slightly less intuitive / ergonomic API. @lee-orr is that a correct assessment?

Another argument can be made that besides ZSTs and enums there are structs, and holding an 'uninitialized: bool' field in a struct makes no sense.

As above, you can still absolutely use optional state and I agree that in the case of a struct it would absolutely be the right choice. To me, it's a matter of trade-offs and if optional state is somewhat uncommon, it's better to make the API much cleaner at the cost of making a more niche API use-case slightly more convoluted.

Also I don't want to add too much noise here or derail this PR! I wish there was a feature for starting a top-level comment thread so we could discuss this in more detail.

Jan 28 '24 00:01 marcelchampagne

I too think spamming in here is not helpful, the PR is pretty complete as is and the solution can be modified later if necessary. If you want to continue the discussion, we do have a discord thread related to this PR.

Jan 28 '24 00:01 MiniaczQ

This seems well thought out, but I'm trying to see the utility. When would this API be preferable to adding a regular resource or state whose value is computed based on another using a regular system? It's possible to write .add_systems(StateTransition, my_system.after(apply_state_transition::<S>), which pretty much gives you complete freedom to define your own state transitions.

@JoJoJet - There are a few elements here:

Bevy already supports complex state objects (i.e. states that aren't just flat Enums), but the mechanisms around it (like OnEnter/OnExit and in_state) are all designed to only work with discreet, pre-defined values. What that means is that at this point in time you can't actually utilize many of the mechanisms around States for many valid states. This provides a means of transforming those states in a way that allows you to use them well.
Manually making these changes becomes complicated very quickly, if I have a bunch of unrelated states that should actually just be part of a single state machine I suddenly need to manually handle a lot of coordination. And even in the best case of using the method above, that is actually not enough - you would need to, for every state you use as a source, add the line above, then add a apply_state_transition::<MyComputedState> to run after that. And it's worth noting that this method is not obvious - it requires knowing how states work internally, which really shouldn't be something we expect of users. As a result - a much more likely situation is spreading out state mutations in a whole bunch of places in an attempt to keep things in sync, with a high risk of missing places where a mutation should happen, or cases where it shouldn't. The fact that sub-states and state matching have come up as requests or elements in discussion as much as they have shows a need or desire for something more intuitive and built in, rather than a workaround.
Another element is clarity - having a clear distinction between "computed" states and normal states makes it easy for someone to look at the codebase and understand what is happening. I have an example from one of my game jam games, where I wanted to add some features a few months after the fact and gave up because it was difficult to find and follow the way I had to adjust states and trigger systems with them to get things working. It's not that it was impossible, and I definitely could have made the codebase clearer given more time at the start, but the complexity of trying to deal with states that are tied to one another was a significant hurdle.
Lastly, the approach you suggested doesn't really handle circular dependencies or multiple dependencies changing in a single frame very well. If I have a state that depends on two source states, it will trigger all the transition elements twice - with one of them being a transient state that might be invalid. And if I have a circular dependency, I'd expect a run-time crash (hopefully while setting things up). In this approach dependencies are clear, known at compile time, can't be circular, and only trigger a transition after all states it depend on have changed (and doesn't require computation at all if none of the source states have changed)

Jan 28 '24 16:01 lee-orr

The fact that this solves all ordering issues is also very helpful, but it's a byproduct of the graph structure

Jan 28 '24 16:01 MiniaczQ

Computed State & Sub States

Summary/Description

Objective

Addition - Sub States

Solution

Changelog

Migration Guide

Future Work