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

flush ButtonInput<KeyCode> cache when Bevy window loses focus #12878

Closed
wants to merge 1 commit into from

Conversation

gavlig
Copy link
Contributor

@gavlig gavlig commented Apr 4, 2024

Objective

This helps avoiding stuck key presses after switching from and back to Bevy window. Key press event gets stuck because window loses focus before receiving a key release event thus we end up with false positive in ButtonInput.

Solution

I saw two ways to fix this:

  1. add bevy_window as dependency and read WindowFocus events
  2. add a KeyboardFocusLost event specifically for this.

I chose the latter because adding another dependency felt wrong, but if that is more preferable changing this pr won't be a problem. Also if someone sees another way please let me know.


To test the bug use this small modification over examples/keyboard_input.rs:
(it will work only if you have Alt-Tab combination for switching between windows in your OS, otherwise change AltLeft accordingly)

//! Demonstrates handling a key press/release.

use bevy::{prelude::*, input::keyboard::KeyboardInput};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Update, keyboard_input_system)
        .run();
}

/// This system prints 'Alt' key state
fn keyboard_input_system(keyboard_input: Res<ButtonInput<KeyCode>>, mut keyboard_input_events: EventReader<KeyboardInput>) {
    for event in keyboard_input_events.read() {
        info!("{:?}", event);
    }

    if keyboard_input.pressed(KeyCode::AltLeft) {
        info!("'Alt' currently pressed");
    }

    if keyboard_input.just_pressed(KeyCode::AltLeft) {
        info!("'Alt' just pressed");
    }
    if keyboard_input.just_released(KeyCode::AltLeft) {
        info!("'Alt' just released");
    }
}

Here i made a quick video with demo of the fix: https://youtu.be/qTvUCk4IHvo
In first part i press Alt and Alt+Tab to switch back and forth from example app, logs will indicate that too. In second part I applied fix and you'll see that Alt will no longer be pressed when window gets unfocused

Migration Guide

WinitEvent has a new enum variant: WinitEvent::KeyboardFocusLost.

Copy link
Contributor

github-actions bot commented Apr 4, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@gavlig gavlig changed the title flush key_input cache when Bevy loses focus flush ButtonInput<KeyCode> cache when Bevy window loses focus Apr 4, 2024
@gavlig gavlig force-pushed the stuck_keypress_fix branch 2 times, most recently from f619d02 to 6d360c1 Compare April 4, 2024 20:34
This helps avoiding stuck key presses after switching from and then back to Bevy
@SolarLiner SolarLiner added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 5, 2024
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 8, 2024
Copy link
Contributor

github-actions bot commented Apr 8, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run into this bug before and found it quite frustrating. I think this is a good fix that should result in consistent cross-platform behavior.

@dimvoly
Copy link

dimvoly commented May 26, 2024

Not sure if this is related, but when I run debug builds, I seem to get random keys starting in the pressed state (e.g. KeyCode::At, or KeyCode::Key6). I haven't pressed these keys when I launch my app, to resolve I have to insert a breakpoint to find out the stuck key, then press it to get it unpressed. Happens sporadically so kind of hard to untangle the reasons for it happening. Am on 0.10.1 on Ubuntu 20.04.

A general purpose flush like this PR I think would be welcome as a catch-all for .pressed() desyncing from the keyboard.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but should be merged after the 0.30 pr to avoid conflicting with that pr.

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 29, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 29, 2024
@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jun 3, 2024

Unblocked

@hymm hymm removed the S-Blocked This cannot move forward until something else changes label Jun 3, 2024
@alice-i-cecile alice-i-cecile requested a review from hymm June 3, 2024 18:02
@mockersf
Copy link
Member

mockersf commented Jun 3, 2024

@gavlig could you resolve the conflicts or do you need help for that?

Worst case if they're too hard, you can open a new PR with the same changes

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jun 4, 2024
@alice-i-cecile
Copy link
Member

I'll adopt this tomorrow and resolve the merge conflicts.

@alice-i-cecile
Copy link
Member

This has been merged via #13678 :) Thanks a ton for your contribution @gavlig: see you in the 0.14 contributors list!

github-merge-queue bot pushed a commit that referenced this pull request Jun 5, 2024
This was adopted from #12878. I rebased the changes resolved the
following merge conflicts:

- moved over the changes originally done in bevy_winit/src/lib.rs's
`handle_winit_event` into bevy_winit/src/state.rs's `window_event`
function
- moved WinitEvent::KeyboardFocusLost event forwarding originally done
in bevy_winit/src/winit_event.rs to the equivalent in
bevy_winit/src/state.rs

Tested this by following the modified keyboard_input example from the
original PR.

First, I verified I could reproduce the issue without the changes. Then,
after applying the changes, I verified that when I Alt+Tabbed away from
the running example that the log showed I released Alt and when I tabbed
back it didn't behave like Alt was stuck.

 
 The following is from the original pull request by @gavlig 
 
 # Objective
 
This helps avoiding stuck key presses after switching from and back to
Bevy window. Key press event gets stuck because window loses focus
before receiving a key release event thus we end up with false positive
in ButtonInput.
 ## Solution
 
 I saw two ways to fix this:
 
     1. add bevy_window as dependency and read WindowFocus events
 
     2. add a KeyboardFocusLost event specifically for this.
 
 
I chose the latter because adding another dependency felt wrong, but if
that is more preferable changing this pr won't be a problem. Also if
someone sees another way please let me know.
 
To test the bug use this small modification over
examples/keyboard_input.rs: (it will work only if you have Alt-Tab
combination for switching between windows in your OS, otherwise change
AltLeft accordingly)
 
 ```
 //! Demonstrates handling a key press/release.
 
 use bevy::{prelude::*, input::keyboard::KeyboardInput};
 
 fn main() {
     App::new()
         .add_plugins(DefaultPlugins)
         .add_systems(Update, keyboard_input_system)
         .run();
 }
 
 /// This system prints 'Alt' key state
fn keyboard_input_system(keyboard_input: Res<ButtonInput<KeyCode>>, mut
keyboard_input_events: EventReader<KeyboardInput>) {
     for event in keyboard_input_events.read() {
         info!("{:?}", event);
     }
 
     if keyboard_input.pressed(KeyCode::AltLeft) {
         info!("'Alt' currently pressed");
     }
 
     if keyboard_input.just_pressed(KeyCode::AltLeft) {
         info!("'Alt' just pressed");
     }
     if keyboard_input.just_released(KeyCode::AltLeft) {
         info!("'Alt' just released");
     }
 }
 ```
 
Here i made a quick video with demo of the fix:
https://youtu.be/qTvUCk4IHvo In first part i press Alt and Alt+Tab to
switch back and forth from example app, logs will indicate that too. In
second part I applied fix and you'll see that Alt will no longer be
pressed when window gets unfocused
 ## Migration Guide
 
 `WinitEvent` has a new enum variant: `WinitEvent::KeyboardFocusLost`.

Co-authored-by: gavlig <[email protected]>
@gavlig
Copy link
Contributor Author

gavlig commented Jun 5, 2024

Thank you for handling this and for all your work on Bevy! Should I close this pr or will it be closed automatically?

@mockersf mockersf closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants