Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
king0952 opened this issue Jul 12, 2023 · 29 comments
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature

Comments

@king0952
Copy link

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.

@king0952 king0952 added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Jul 12, 2023
@Luminoth
Copy link

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
Copy link

vacuus commented Jul 12, 2023

Add OnEachEnter and OnEachExit schedules?

@Selene-Amanita Selene-Amanita added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Jul 15, 2023
@Selene-Amanita
Copy link
Member

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
Copy link
Contributor

rlidwka commented Jul 15, 2023

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
Copy link

vacuus commented Jul 16, 2023

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
Copy link

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
Copy link

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
Copy link
Contributor

urben1680 commented Jul 31, 2023

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
Copy link

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
Copy link
Contributor

urben1680 commented Aug 1, 2023

@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
Copy link
Member

alice-i-cecile commented Dec 31, 2023

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
Copy link
Contributor

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

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Dec 31, 2023

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
Copy link
Contributor

benfrankel commented Dec 31, 2023

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
Copy link
Contributor

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
Copy link
Contributor

benfrankel commented Dec 31, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

benfrankel commented Dec 31, 2023

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
Copy link
Contributor

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
Copy link
Contributor

benfrankel commented Dec 31, 2023

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
Copy link
Contributor

MiniaczQ commented Dec 31, 2023

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
Copy link
Contributor

benfrankel commented Dec 31, 2023

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
Copy link
Contributor

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
Copy link
Contributor

benfrankel commented May 23, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature
Projects
None yet