Direct users to commands.set_state instead of NextState

[alice-i-cecile]

Second thoughts, I'd like this to be the standard way of updating states.
Using commands:

• implies the deferred nature of this operation
• reduces clutter associated with NextState, especially when many are used
• does not make problems with system ambiguities worse
• allows for falliable operations with a warning, which we will need when the traits get unified and not everything will use NextState
• with a different command you can force the transition to happen immediately without writing an exclusive system

I don't think this change has to happen now, but it looks like a better API overall to me

Originally posted by @MiniaczQ in #15083 (comment)

[alice-i-cecile]

@Shatur @MiniaczQ, pulling the discussion into an issue here.

I'm personally on the fence about it. I like exposing users to the "states are resources" model, but I think that the way that this implies the deferred nature of state transitions is really nice.

IMO swapping state transitions to use observers and swapping purely to a command or event-based model while removing NextState and the dedicated state transition schedule is a good direction.

[MiniaczQ]

I like being transparent with users too, I think merging NextState into State may be a good direction.
On the offtopic, I'd like a few more fields:

struct State {
  current: S
  /// Never exposed for mutability, only commands
  next: Option<S>,
  /// Previous (different!) S value, does not update on re-enters
  previous: Option<S>,
  /// How many times did this state re-enter into itself
  repeats: usize,
}

I'll discuss this further when I start working on it

[alice-i-cecile]

I think merging NextState into State may be a good direction

FYI, the reason we didn't do this initially is because it mucks with change detection. Being able to ask is the state has changed is moderately useful, and if next_state is a field on State, this has surprising false positives.

[MiniaczQ]

FYI, the reason we didn't do this initially is because it mucks with change detection. Being able to ask is the state has changed is moderately useful, and if next_state is a field on State, this has surprising false positives.

Fair, but state transitions are the main way of reacting to state changes

[MiniaczQ]

Back onto topic, what's the intended scope of this issue?
Updating examples and docs?

[alice-i-cecile]

Personally, I don't think we should update examples and docs unless we decide to fully remove NextState. I think doing that + moving to observers for state transitions is the right choice, but my confidence level there is still quite low and I'm annoyed to be changing the states API yet again.

[MiniaczQ]

Yeah, if we want to break states API again it should be a larger chunk, refactored until completion and in the form of a swap-in crate.

[benfrankel]

I'm personally opposed to this because it's incompatible with the "direct mutation" feature, e.g. level.0 += 1 or color_mode.red = false, which allows multiple systems to mutate the next state for the same state type on the same frame without fully overwriting each other. bevy_state doesn't currently support this feature, but it could in the future (proof of concept in pyri_state).

Direct mutation and commands.set_state can co-exist, but if you remove NextState completely, it makes the former impossible to implement.

Also, trait unification while still using NextState is possible via "dependent states", which also has a POC in pyri_state.

[MiniaczQ]

I'm still of the opinion that partial mutation is not something we want.
In case of the color example it's an architecture issue, each part should be it's own state.
In case of level, there is nothing wrong with next_state.set(current_state.0 + 1) in fact making next state additive is more of a footgun if someone accidentally increments it twice.

[MiniaczQ]

And yes, trait unification can be done at any point in time, but it's a breaking change.
We want to add them all together.

[benfrankel]

If each part of the color mode is its own state, how would you e.g. schedule a system to run on a transition from yellow to black? It would not make sense to have each field as its own state, because they cannot be interpreted separately in this example.

If two systems both try to increment the level on the same frame, surely you would actually want a +2? If not, direct mutation enables += 1 but it doesn't prevent = current + 1, so you can still do that.

I can come up with more examples where direct mutation is desirable, if the examples I've provided are not convincing.

[MiniaczQ]

Also reminder that states are state machines, which are a function of current state (Dependencies, we don't have self available yet) and input signal (NextState).
Adding complexity to NextState can happen through an abstraction, it doesn't have to complicate the core architecture.

[Shatur]

@benfrankel You can add your own NextState, increment it and issue commands if you want.

[MiniaczQ]

If each part of the color mode is its own state, how would you e.g. schedule a system to run on a transition from yellow to black? It would not make sense to have each field as its own state, because they cannot be interpreted separately in this example.

Use 2 RGB/enum fields instead of hardcoded color flags.
Still an architectural issue.

I can come up with more examples where direct mutation is desirable, if the examples I've provided are not convincing.

No and yes.
Now that we noticed this can be implemented on top of NextState any potential use case doesn't justify this.
Note that having this as a top-level feature allows it to work with more backends, hence it's more flexible and less obscure than just hardcoding it into the existing solution.

[benfrankel]

Use 2 RGB/enum fields instead of hardcoded color flags. Still an architectural issue.

I'm not sure what you mean by this, but it's definitely not an architectural issue.

Now that we noticed this can be implemented on top of NextState any potential use case doesn't justify this.

FWIW same-state transitions can be implemented on top of bevy_state 0.14 but users are clearly not eager to do so, and all of bevy_state can be implemented on top of bevy_ecs etc., so this doesn't necessarily follow. But sure, removing NextState from bevy_state is not strictly incompatible because a user could reimplement it for their own state types.

[Shatur]

If we keep NextState, we will have two ways of changing the state out of the box. And I think that command-based way is much better.
Since it's trivial to implement NextState on your own if some niche use case is needed, I think that removal is worth it.

[MiniaczQ]

Use 2 RGB/enum fields instead of hardcoded color flags. Still an architectural issue.

I'm not sure what you mean by this, but it's definitely not an architectural issue.

It's an issue caused by improperly structuring data.

FWIW same-state transitions can be implemented on top of bevy_state 0.14 but users are clearly not eager to do so, and all of bevy_state can be implemented on top of bevy_ecs etc., so this doesn't necessarily follow. But sure, removing NextState from bevy_state is not strictly incompatible because a user could reimplement it for their own state types.

Same state transitions cannot be implemented without major overhead, it'd need to re-simulate all the state transitions.
That's why it was implemented at the core, where we'd rather ignore them if unnecessary than re-simulate everything.

bevy_state literally builds on bevy_ecs what's the point here.
It's meant to be on top of ECS, not a part of it, exactly what we want.

NextState is not being removed, it's being obfuscated by commands which are more ergonomic for the typical use case.
What you want is a fold-like NextState which combines various requests.
This is a niche use case and be built on top of the existing NextState by introducing a new abstraction that feeds into NextState when it collected all the data.

[Shatur]

NextState is not being removed, it's being obfuscated by commands which are more ergonomic for the typical use case.
What you want is a fold-like NextState which combines various requests.
This is a niche use case and be built on top of the existing NextState by introducing a new abstraction that feeds into NextState when it collected all the data.

Could you elaborate? We currently have commands.set_state and NextState. Why keep the latter?
If someone needs to combine multiple requests, it's possible for user to introduce a custom resource that accumulates changes and issues a command at the end of the schedule with the desired state.

[MiniaczQ]

Could you elaborate? We currently have commands.set_state and NextState. Why keep the latter?

We still need to buffer NextState until the StateTransition schedule.
The command will set the value of NextState in a cleaner way than requesting another resource in the system.
Flushing state transitions immediately may cause invalid entities to form, so as a default they should happen at known intervals.
You can still flush them by hand if you have the confidence to do it right

If someone needs to combine multiple requests, it's possible for user to introduce a custom resource that accumulates changes and issues a command in the end with the desired state.

Yeah

[benfrankel]

If we keep NextState, we will have two ways of changing the state out of the box.

There are three ways of sending an event (not counting observer events): commands.send_event, EventWriter, and ResMut<Events>.

And I think that command-based way is much better. Since it's trivial to implement NextState on your own some esoteric use case is needed, I would removed it.

I'll agree to disagree.

It's an issue caused by improperly structuring data.

Nothing improper about ColorMode { red: bool, green: bool, blue: bool } with systems mutating each individual field and state transitions looking at the value holistically.

Same state transitions cannot be implemented without major overhead, it'd need to re-simulate all the state transitions.

Well they can be, since I've done it in pyri_state. I assume you mean it can't be done without major overhead in bevy_state's current design? Even then, it can be, and I've made PRs with approaches that can natively support same-state transitions without major overhead.

bevy_state literally builds on bevy_ecs what's the point here.
It's meant to be on top of ECS, not a part of it, exactly what we want.

Yes of course. The point is that just because it's built on top of something that's already provided by bevy, doesn't mean it shouldn't also be provided by bevy. I brought up same-state transitions as another example of this.

[MiniaczQ]

It's an issue caused by improperly structuring data.

Nothing improper about ColorMode { red: bool, green: bool, blue: bool } with systems mutating each individual field and state transitions looking at the value holistically.

If you need to modify the existing solution to make it work then clearly something isn't done right.
I'm not saying this is incorrect in other scenarios, but you could find a solution that works, but you'd much rather advocate for complicating the existing solution.

Same state transitions cannot be implemented without major overhead, it'd need to re-simulate all the state transitions.

Well they can be, since I've done it in pyri_state. I assume you mean it can't be done without major overhead in bevy_state's current design? Even then, it can be, and I've made PRs with approaches that can natively support same-state transitions without major overhead.

It's the difference between being able to do it as purely additive feature (NextState and derivatives) and a feature that only works well if integrated into the core (identity transitions).

If you had a bevy_state without identity transitions and wanted to add them, you'd need to calculate the entire state graph again, not very good for performance or maintenance.
What we did was add the identity transitions internally with negligible overhead and allowed users to opt-in.
Most people don't even know it's a feature.

If you have bevy_state without NextState, but there exists a way to "set the next state" you can still very easily add NextState or anything similar.
But if we remade NextState to fold many values provided by the user, we're adding complexity which we really don't need at that low of a level and users can't opt out of it.

Those are on the opposite type of spectrum.
One add a new feature which is hard to add additively, but is non-default, so it doesn't bother everyone.
The other can very easily be additive, but you'd rather make life harder for everyone by forcing it into existing NextState.

[MiniaczQ]

Exchanging walls of text isn't very productive, we can switch over to discord if you want.
I feel like we're missing something, because I'm pretty sure you'd come to the same conclusion maybe under different circumstances.

[benfrankel]

Okay, I discussed with @MiniaczQ on Discord for a bit. Here is my conclusion from the discussion:

• This issue would be better titled "Direct users to commands.set_state instead of NextState".
• This issue is not incompatible with direct mutation. That feature requires changing the structure of NextState, which is not being removed.
• NextState must be kept around because commands.set_state only defers until the next sync point, but the next state flush point (aka the StateTransitions schedule) generally runs all the way in the next frame.
• commands.set_state may allude to the fact that state transitions are deferred until later, whereas for commands.send_event that would be undesirable because events can be read immediately after they're written (although there'll probably still be a sync point in-between so that the system writing the event is not ambiguous with the system reading it, since it's a write followed by a read of the same resource).
• commands.set_state has the ergonomic benefit of not requiring a new system param for each state type.
• commands.set_state + system ordering will add a sync point if the system was not already using Commands, which has performance implications.

@MiniaczQ was not 100% sold on direct mutation as a feature, but said that the implementation details don't look too bad. However, that discussion is now off-topic for this issue as it's a separate feature and still compatible.

My only remaining concern is in the discrepancy between commands.set_state and commands.send_event. The latter is not suggested as the de facto way to send events, while the former would be for setting states with this issue. I'm not convinced that the difference is significant enough. IMO the docs should definitely mention commands.set_state as an option, and just make it clear that state transitions happen later.

[alice-i-cecile]

commands.send_event is much less generally useful than the state equivalent IMO: there's likely to be far more events, which means the performance overhead matters more, and events are not inherently deferred like state transitions. I'm strongly opposed to encouraging its general use.