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

Replace winit's synthetic events by our own key tracking #14379

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Azorlogh
Copy link
Contributor

@Azorlogh Azorlogh commented Jul 18, 2024

Objective

Defocusing a window while a key is held (such as when Alt+tabbing), will currently send a key release on X11 and Windows. This is likely the behavior that most people expect.
However it's synthetic events from winit are unimplemented for WASM and some other platforms. (See rust-windowing/winit#1272).
While we can implement it upstream, there is also some doubt about the synthetic events API as a whole (rust-windowing/winit#3543), so I propose to do it in bevy directly for now.

Solution

This PR implements key tracking in bevy directly so we can synthesize our own key release events across all platforms.

Note regarding X11 specifically:

  • On main, pressing a keyboard shortcut to unfocus the window (Ctrl + Super + ArrowRight in my case) will yield the following events:
Pressed Control
Pressed Super
Released Control
Released ArrowRight
Released Super
  • On this branch, it will yield the following sequence:
Pressed Control
Pressed Super
Released Control
Released Super

To me the behavior of this branch is more expected than main, because main produces an ArrowRight release without producing a press first.

Testing

Tested in WASM and X11 with

App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Update, |mut keys: EventReader<KeyboardInput>| {
            for ev in keys.read() {
                error!("received {ev:?}");
            }
        })
        .run();

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in labels Jul 18, 2024
Copy link
Contributor

@RobWalt RobWalt left a comment

Choose a reason for hiding this comment

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

Code looks good to me!

crates/bevy_winit/src/system.rs Outdated Show resolved Hide resolved
@Azorlogh Azorlogh marked this pull request as ready for review July 19, 2024 10:10
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jul 19, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 19, 2024
@NEON725
Copy link

NEON725 commented Jul 22, 2024

Will this fix the issue of the Alt key "sticking" when alt-tabbing out of the browser running a WASM build? Logic seems like it should so long as WASM builds correctly track window focus.

@Azorlogh
Copy link
Contributor Author

Will this fix the issue of the Alt key "sticking" when alt-tabbing out of the browser running a WASM build? Logic seems like it should so long as WASM builds correctly track window focus.

Yep, this should fix it.

@alice-i-cecile
Copy link
Member

@richchurcher @SpecificProtagonist can I have your review over here please?

@richchurcher
Copy link
Contributor

@Azorlogh I think because #12372 got merged and touched some of the same areas, this is gonna have a few conflicts. Would you mind taking another look and updating the branch?

@Azorlogh
Copy link
Contributor Author

Azorlogh commented Oct 5, 2024

I updated it for #12372, though I have a couple concerns on reliability:

@alice-i-cecile
Copy link
Member

Extra release events should generally be safe enough.

I think this is broadly fine.

  • Also, check_keyboard_focus_lost doesn't emit events in Events<bevy_window::WindowEvent>, so if the user listens to those events they will miss them. (I guess we can just emit them in both streams as a quick fix though)

Emitting it in both would be good.

@alice-i-cecile alice-i-cecile removed this from the 0.15 milestone Oct 8, 2024
@Azorlogh
Copy link
Contributor Author

Azorlogh commented Oct 9, 2024

Extra release events should generally be safe enough

Fair. They are now emitted in both streams 👍

@BenjaminBrienen BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 31, 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 A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants