Skip to content

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

Merged
alice-i-cecile merged 7 commits intobevyengine:mainfrom
Azorlogh:replace-winit-synthetic-events
Mar 10, 2025
Merged

Replace winit's synthetic events by our own key tracking#14379
alice-i-cecile merged 7 commits intobevyengine:mainfrom
Azorlogh:replace-winit-synthetic-events

Conversation

@Azorlogh
Copy link
Copy Markdown
Contributor

@Azorlogh Azorlogh commented Jul 18, 2024 •
edited
Loading

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
JasmineLowen
JasmineLowen reviewed Jul 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@JasmineLowen JasmineLowen 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!

@JasmineLowen JasmineLowen mentioned this pull request Jul 19, 2024
Closed
@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
Copy Markdown

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
Copy Markdown
Contributor Author

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

Yep, this should fix it.

@bas-ie bas-ie mentioned this pull request Sep 30, 2024
Merged
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Sep 30, 2024

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

@bas-ie
Copy link
Copy Markdown
Contributor

bas-ie commented Oct 4, 2024

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

alice-i-cecile commented Oct 8, 2024

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
Copy Markdown
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
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
@Azorlogh
Copy link
Copy Markdown
Contributor Author

Azorlogh commented Mar 6, 2025

@BenjaminBrienen I just updated the PR to main 👍 From my side I think it's ready

@BenjaminBrienen BenjaminBrienen removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Mar 6, 2025
@BenjaminBrienen BenjaminBrienen added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Mar 6, 2025
alice-i-cecile
alice-i-cecile approved these changes Mar 6, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Good comments and I much prefer having some cross-platform consistency here. Not thrilled to be doing this rather than winit but bugs are bugs.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 6, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 10, 2025
Merged via the queue into bevyengine:main with commit b054d11 Mar 10, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+2 more reviewers

@JasmineLowen JasmineLowen JasmineLowen left review comments

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Azorlogh @NEON725 @alice-i-cecile @bas-ie @JasmineLowen @BenjaminBrienen