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

triggering state transition from player input locks the game #1700

Closed
mockersf opened this issue Mar 19, 2021 · 18 comments
Closed

triggering state transition from player input locks the game #1700

mockersf opened this issue Mar 19, 2021 · 18 comments
Labels
A-ECS Entities, components, systems, and events A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior

Comments

@mockersf
Copy link
Member

mockersf commented Mar 19, 2021

Bevy version

c78b76b

What you did

In my game, I have several screens which I can go through by clicking the mouse (with just_pressed on Input<MouseButton>). Each screen has its own state.

Very dumb example that reproduce my issue:

use bevy::prelude::*;

fn main() {
    App::build()
        .add_plugins(DefaultPlugins)
        .add_state(AppState::State1)
        .add_system_set(SystemSet::on_enter(AppState::State1).with_system(enter_state.system()))
        .add_system_set(SystemSet::on_update(AppState::State1).with_system(next_state.system()))
        .add_system_set(SystemSet::on_enter(AppState::State2).with_system(enter_state.system()))
        .add_system_set(SystemSet::on_update(AppState::State2).with_system(next_state.system()))
        .run();
}

#[derive(Clone, Eq, PartialEq, Debug)]
enum AppState {
    State1,
    State2,
}

fn enter_state(state: Res<State<AppState>>) {
    eprintln!("entering {:?}", state.current());
}

fn next_state(mut state: ResMut<State<AppState>>, mouse_button_input: Res<Input<MouseButton>>) {
    if mouse_button_input.just_pressed(MouseButton::Left) {
        match state.current() {
            AppState::State1 => state.set_next(AppState::State2),
            AppState::State2 => state.set_next(AppState::State1),
        }
        .unwrap();
    }
}

What you expected to happen

One click to go from State1 to State2, then one click to go from State2 to State1.

What actually happened

On first click, it changes state in a loop and lock the game.

Additional information

Now that system set can rerun in same frame, input isn't reset between two passes so just_pressed is always true

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Mar 19, 2021
@alice-i-cecile alice-i-cecile added this to the Bevy 0.5 milestone Mar 19, 2021
@Ratysz
Copy link
Contributor

Ratysz commented Mar 20, 2021

Looks like it's working as intended: state chanes are the inner loop, compared to input processing. You'll have to break that loop yourself.

@mockersf
Copy link
Member Author

Yup I know, but I think that's very user-unfriendly, I would hope we have a better solution for that

@Ixentus
Copy link
Contributor

Ixentus commented Mar 20, 2021

From my actual game (but not compiled like this):

use bevy::{app::AppExit, prelude::*};

fn main() {
    App::build()
    .add_state(AppState::Menu)
    .add_system_set(SystemSet::on_update(AppState::Menu).with_system(menu_input.system())
    .add_system_set(SystemSet::on_update(AppState::Menu).with_system(game_input.system())
    .run();

pub enum AppState {
    Menu,
    InGame,
}

pub fn menu_input(
    keyboard_input: Res<Input<KeyCode>>,
    mut app_state: ResMut<State<AppState>>,
    mut app_exit_events: EventWriter<AppExit>,
) {
    if keyboard_input.just_pressed(KeyCode::Space) {
        app_state.set_next(AppState::InGame).unwrap();
    } else if keyboard_input.just_pressed(KeyCode::Escape) {
        app_exit_events.send(AppExit);
    }
}

pub fn input(
    mut app_state: ResMut<State<AppState>>,
    keyboard_input: Res<Input<KeyCode>>,
    mut pause_events: EventWriter<PauseEvent>,
) {
    if keyboard_input.just_pressed(KeyCode::Escape) {
        app_state.set_next(AppState::Menu).unwrap();
    }
    if keyboard_input.just_pressed(KeyCode::Space) {
        //pause logic omited
        pause_events.send(PauseEvent);
    }
}

On 0.4 you could start the game by pressing space and the game would start unpaused, on current main it starts paused.
On 0.4 you could exit the game to the menu by pressing Esc, now it exits the whole game.
Unless I'm misunderstanding something, there is no loop to break here and just_pressed() is just broken on state transition.

@mockersf
Copy link
Member Author

a workaround is to add a on_enter system for your states that just does:

fn reset_input(mut keyboard_input: ResMut<Input<KeyCode>>,) {
      keyboard_input.update();
}

I hope we can find a better fix 😄

@Ratysz
Copy link
Contributor

Ratysz commented Mar 21, 2021

There is no loop, but the gist of the problem is the same: state changes are processed faster than input, so if you're consecutively switching several states based on an input edge you're gonna experience this issue. I still don't think this is something that needs fixing on Bevy side.

My solution would be a layer of indirection that converts inputs into app-specific events (the same layer would also handle keybindings). So, when a key or a button is down (or just pressed), instead of changing state an event is emitted, and the state change system consumes that event.

@Ixentus
Copy link
Contributor

Ixentus commented Mar 21, 2021

a workaround is to add a on_enter system for your states

No need for a whole new system. I ended up taking your suggestion, but simply updating right after reading just_pressed:

if keyboard_input.just_pressed(KeyCode::Space) {
        app_state.set_next(AppState::InGame).unwrap();
        keyboard_input.update();
}

Still feels against the spirit of just_ to me, but simple enough.

@cart
Copy link
Member

cart commented Mar 25, 2021

My solution would be a layer of indirection that converts inputs into app-specific events (the same layer would also handle keybindings). So, when a key or a button is down (or just pressed), instead of changing state an event is emitted, and the state change system consumes that event.

In this case I think it probably makes sense to just use a shared ManualEventReader<KeyboardInput> resource to ensure a given set of systems only consume the event once.

This is a bit of a foot-gun though. Worth playing around with ways to make it fool-proof (edit: I'm not implying that people that hit it are fools 😄)

@cart cart removed this from the Bevy 0.5 milestone Mar 25, 2021
@cart
Copy link
Member

cart commented Mar 25, 2021

I've removed this from the 0.5 milestone because I don't think theres much we can do in the short term about this and we do have workarounds.

@cart
Copy link
Member

cart commented Mar 25, 2021

If we find a better solution before release (and its not a major change) I'll consider including it.

@mockersf
Copy link
Member Author

not a fix, but I added documentation and helpers around this issue in #1781

@CooCooCaCha
Copy link

CooCooCaCha commented Apr 8, 2021

I just ran into this when porting to 0.5 and I have to say this is surprising behavior. My mental intuition was that states own the entire frame. So if a state transition happens, it doesn't apply until the next frame.

bors bot pushed a commit that referenced this issue Apr 13, 2021
related to #1700 

This PR:
* documents all methods on `Input<T>`
* adds documentation on the struct about how to use it, and how to implement it for a new input type
* renames method `update` to a easier to understand `clear`
* adds two methods to check for state and clear it after, allowing easier use in the case of #1700 

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@alice-i-cecile alice-i-cecile added the A-Input Player input via keyboard, mouse, gamepad, and more label Apr 15, 2021
jihiggins pushed a commit to jihiggins/bevy that referenced this issue Apr 18, 2021
related to bevyengine#1700 

This PR:
* documents all methods on `Input<T>`
* adds documentation on the struct about how to use it, and how to implement it for a new input type
* renames method `update` to a easier to understand `clear`
* adds two methods to check for state and clear it after, allowing easier use in the case of bevyengine#1700 

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@ndarilek
Copy link
Contributor

This has bitten me hard twice today.

First I hit this exact issue, though further complicated by my use of bevy_input_actionmap which was an extra layer of abstraction between me and the keycodes which made clearing more difficult. I added a very aggressive clear function that seems to nuke everything from orbit.

Now I'm hitting this trying to make egui accessible. My use case is that I'm trying to set an initial keyboard focus. When I transition to a new menu, I transition to a new state and set its keyboard focus. But because state changes are instantaneous, I suspect the old menu isn't clearing. So the click event from the first menu is still around because it's in the same frame, which propagates back to the first menu. I've been shuffling things around, and everything basically looks like I'm pushing state transitions for menus, but the actual buttons getting clicked are on menus from previous states because the immediate mode GUI isn't getting an opportunity to clear its state and rerender.

In short, things might be a lot easier if state changes took hold the next frame, letting any retained state reset.

@vildapavlicek
Copy link

Emiting event doesn't help to solve this issue / workaround. See #2205
Adding key_input.update(); right after done with processing input solved the issues. But for that I have to use ResMut<Input<KeyCode>> instead of just Res<Input<KeyCode>>

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 26, 2021

The Bevy Cheatbook lists the following workaround.

"If you want to use Input to trigger state transitions using a button/key press, you need to clear the input manually by calling .reset:"

fn esc_to_menu(
    mut keys: ResMut<Input<KeyCode>>,
    mut app_state: ResMut<State<AppState>>,
) {
    if keys.just_pressed(KeyCode::Escape) {
        app_state.set(AppState::MainMenu).unwrap();
        keys.reset(KeyCode::Escape);
    }
}

ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue Jul 27, 2021
related to bevyengine#1700 

This PR:
* documents all methods on `Input<T>`
* adds documentation on the struct about how to use it, and how to implement it for a new input type
* renames method `update` to a easier to understand `clear`
* adds two methods to check for state and clear it after, allowing easier use in the case of bevyengine#1700 

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@DJMcNab
Copy link
Member

DJMcNab commented Sep 20, 2021

I think a better potential workaround might be to check whether the resource 'is_changed'. I.e. whether the input has been updated since the last time this system ran.

This would be a slightly unwieldy pattern, so it might make sense to add a special SystemParam wrapper around Res<Input> for this

Alternatively we could add an extension trait for Res<Input> and rename just_pressed to something more unwieldy, using that name for this extension.

@forbjok
Copy link
Contributor

forbjok commented Feb 16, 2022

This sounds like it could be the same issue I've run into numerous times when switching between different screens in my game. The way I've solved it so far is by having an IgnoreInput resource that gets checked by each input system and tells them to ignore input for a specified number of frames (usually 1), which gets incremented whenever changing screens. If it's possible, resetting the inputs might be a better solution.

@mockersf
Copy link
Member Author

@alice-i-cecile
Copy link
Member

Fixed by #7267.

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 A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

10 participants