Need the OnEnter() and OnExit() to be run even if when transitioning to and from the same state.

[king0952]

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

When using bevy 0.10.1, I can trigger a button click event and enter StateA, and query some resources and spawn some bundles when I enter this StateA with add_system(setup.in_schedule(OnEnter(StateA))), every time I click the button (without leave the StateA), I can despawn bundles by using add_system(exit.in_schedule(OnExit(StateA))); and spawn new bundles according to the resources again. But after I update to bevy 0.11, this cannot be done because of #8359.

What solution would you like?

I hope OnEnter and OnExit can be run again when the system enters the same state, or at least, can make it optional.

What alternative(s) have you considered?

No effective methods were found.

[Luminoth]

Re-entering states is definitely a useful thing to be able to do, having this functionality back in some form would be really great to see.

[vacuus]

Add OnEachEnter and OnEachExit schedules?

[Selene-Amanita]

Add OnEachEnter and OnEachExit schedules?

I think some people might not expect their state to be reentered andit would create some bugs if simply re-added to OnEnter. Having an alternative solution like this is better.

[rlidwka]

OnEnter should be triggered when re-entering the same state.

If a user doesn't want that to happen, she needs a way to check the previous state (not sure if previous state is kept track of now).

[vacuus]

There are use cases for both re-triggering and not re-triggering. See #8191. I don't know what the overhead of a schedule is in Bevy, but if it's not significant then both use cases should be easily supported. The docs should be clear on the distinction. I think it's not really cognitive overhead; the state transition logic maps quite nicely to the schedules.

[Luminoth]

I think also some more care should be given to changing things as fundamental as this without soliciting at least some feedback on how it's being used. I know everything is still in pre-1.0 unstable land but it really upends a lot to have to completely rework something like state transitions because one person got surprised by it.

[nightactive-git]

I would also highly appreciate if the state transition is triggered, also if the next state is the same as the current state. State transitions are a very powerful and robust mechanism to execute a bunch of actions/systems in a specific order (or in parallel).
Workarounds are painful and cause a lot of clumsy code, which (in my option) doesn't harmonize with the spirit of an ECS engine.
I would really appreciate a hot fix for that.

[urben1680]

An associated constant bool on the States trait (and it's derive) could be the simple fix here that determines what should happen if the StateNext<S> resource is updated with a new state that is equal to the inner value of the resource State<S>.

A hacky workaround until a solution for Bevy has been deployed can be to manually implement PartialEq for your state type and make eq always return false.

EDIT: The current docs for the apply_state_transition system should mention inequality matters here.

[nightactive-git]

Thank you @urben1680 for this advice.
When I override the function eq so that it always returns false, the state transition doesn't seem to work anymore. Is there anything else, I have to regard?

impl PartialEq for ToolbarState { fn eq(&self, &other: &Self) -> bool { return false; } }

[urben1680]

@nightactive-git Oh too bad, I did not test it as it seemed obvious to me that it would work. Sorry.
This might work better, I only tested it if it compiles though:
Use this wrapper for all states you want to also trigger transitions even if it is equal to the previous state.

#[derive(Clone, PartialEq, Eq, Hash, Debug, Default)]
pub struct UnequalState<Inner>{
    state: Inner,
    flip: bool
}

// States derive only works with fieldless enums
type ArrayIter<T> = std::array::IntoIter<UnequalState<T>, 2>;
impl<T> States for UnequalState<T> where T: States{
    type Iter = FlatMap<<T as States>::Iter, ArrayIter<T>, fn(T) -> ArrayIter<T>>;
    fn variants() -> Self::Iter {
        T::variants().flat_map(|state| [
            Self { state: state.clone(), flip: false },
            Self { state, flip: true },
        ].into_iter())
    }
}

impl<T> UnequalState<T> {
    /// use this to construct a state using the current state
    /// &self should be taken from `Res<State<>>`, not `NextState<>`!
    fn unequal_new(&self, state: T) -> Self {
        Self {
            state,
            flip: !self.flip
        }
    }
}

[alice-i-cecile]

I'm in favor of supporting this as an opt-in behavior: it's clearly useful, but still quite surprising. Reverting is my preference over keeping the current behavior though.

[benfrankel]

Made a PR re-adding the behavior as opt-in via NextState::set_forced.

[MiniaczQ]

I don't think there is a need to complicate NextState with an opt-in API, I'd much better prefer a SystemParam wrapper around (State, NextState) pair that performs the check for me. It's less invasive.

Another argument for this would be this.
If user has multiple NextState setters, they will overwrite each others force flags and the user won't be aware because it gets resolved in some alien (to them) system.
But with a "soft set" wrapper, whether to overwrite gets resolved immediately so you don't need to concern yourself as much with potential race conditions and accidental misusage.

[benfrankel]

I don't think there is a need to complicate NextState with an opt-in API, I'd much better prefer a SystemParam wrapper around (State, NextState) pair that performs the check for me. It's less invasive.

That would imply that using NextState directly would have the reverted behavior, and you'd have to use the SystemParam instead for the current behavior. (not an argument for/against just clarifying)

Also, if you e.g. call param.set(Foo) followed by param.set(Bar) in the same system, while the current state is Bar, this would queue a transition to Foo, which could be confusing.

[MiniaczQ]

The wrapper can check for values inside of NextState as well, the fun part is that it can be as complicated as you want because it doesn't mess up the simpler state logic

[benfrankel]

Okay I see what you're saying. You could give param.set(current_state) the current behavior by setting next_state.0 = None instead of Some(current_state), and param.set_forced can have the old behavior by setting Some(current_state). And then no check in apply_state_transitions.

[MiniaczQ]

Few more thoughts:

The NextState force flag idea won't work with cases like:

enum FooBar {
  #[default]
  Foo,
  Bar,
}

app.add_systems(Update, (sys_a, sys_b).chain());

fn sys_a(next: ResMut<NextState<FooBar>>) {
  // Should set Foo to Bar
  nest.set_force(FooBar::Bar);
}

fn sys_b(next: ResMut<NextState<FooBar>>) {
  // Replaces Bar, no state change occurs
  nest.set_force(FooBar::Foo);
}

which could be the intended way for this, but definitely not the one I'd expect.

[MiniaczQ]

Another problem with the entire idea of forced updates is it straight up doesn't make sense when you nest states.
Which state is meant to be 'forced'? You can't decide that during NextState::set like in the suggestions, you can only set it during compilation by replacing Eq implementation, which may, and most likely will mess up any derived implementations.

I think we shouldn't support forced updates, at least not like this.

[benfrankel]

Few more thoughts:

The NextState force flag idea won't work with cases like:

...

which could be the intended way for this, but definitely not the one I'd expect.

It is the intended behavior. If it's unexpected, that's probably because of the name of the method. It's supposed to convey that the state transition will happen (if it's still queued when apply_state_transitions runs) regardless of the from state -- but you could also read it as conveying that the state transition will happen regardless of other calls to next_state.set.

[MiniaczQ]

If the current state is Foo, then I'd expect a set to be ignored if I try to use Foo, but proceed with an update if it's anything else, like Bar.
The problem here is that the state change is deferred, so your decision that would succeed is overwritten by a decision that will fail. Hence resolving it immediately seems more reasonable to me.

[pablo-lua]

The Idea of removing the verification and let the user verifying themselves if they are trying to change something to the same state would probably be the simplest and a good way to do this?

[benfrankel]

I don't think setting the state to Foo should be fully ignored if the current state is Foo. There was a logical intention for the next state to be Foo, whether that means nothing should happen because the state is already Foo, or there should be an OnEnter(Foo) / OnExit(Foo), either way, you would expect the state to be Foo in the next frame.

[benfrankel]

The Idea of removing the verification and let the user verifying themselves if they are trying to change something to the same state would probably be the simplest and a good way to do this?

Yeah that effectively lines up with @MiniaczQ's proposal which I agree with. You'd choose between ChangeState::set and ChangeState::set_forced where ChangeState { current: Res<State<S>>, next: ResMut<NextState<S>> }, so the verification happens in the user system instead of apply_state_transitions.

[MiniaczQ]

There was a logical intention for the next state to be Foo

What if this happened due to system order ambiguity, then it's an additional thing to worry about

[benfrankel]

What if this happened due to system order ambiguity, then it's an additional thing to worry about

If you have a race condition / ambiguity between systems that set the next state, that's just a logic error. I don't think there's a consistently sane way for bevy to handle that.

[MiniaczQ]

Just resolving whether an state update should occur during set, instead of the transition system would fix this

Edit:
No it wouldn't 🤡 I'm silly
I'll recheck this topic some other day

[benfrankel]

Made a PR for the system param approach, which I prefer. I'm leaving the NextState::set_forced PR open for now.

Names are all bikesheddable, I didn't think too hard about them.

[benfrankel]

Until this is fixed, the easiest workaround is to add a RestartFoo state and a system upon entering RestartFoo to set the next state to Foo. This introduces a 1 frame delay and a little bit of boilerplate, but it should be easy to migrate to actually re-entering Foo when that becomes possible.

[benfrankel]

This is fixed in my 3rd-party crate pyri_state. I was unable to get any fix upstreamed unfortunately after a lot of discussion, because there's more than one way to implement this and it's not unanimous.

In pyri_state you can trigger a same-state transition explicitly with refresh, or you can just set the next state to whatever you want (potentially the same state) and mark the state to flush unconditionally with set_flush(true).