Fix window spawning triggering ButtonInput<KeyCode>::just_pressed/just_released

[SpecificProtagonist]

Objective

Fix #12273

Solution

– Only emit KeyboardFocusLost when the keyboard focus is lost
– ignore synthetic key releases too, not just key presses (as they're already covered by KeyboardFocusLost)

---

Changelog

Fixed

• Don't trigger ButtonInput<KeyCode>::just_pressed/just_released when spawning a window/focus moving between Bevy windows

[alice-i-cecile]

Does this fix obsolete #12367 ?

[SpecificProtagonist]

Does this fix obsolete #12367 ?

No, but it means I can leave out the "Currently this happens even if the focus switches from one Bevy window to another (for example because a new window was just spawned)" sentence.

[alice-i-cecile]

On board with this behavior, but I'd like to avoid accumulating tech debt here. Can you please factor out the new filtering code into a function and add some tests for it? :)

The other question I have is: "why should this be in bevy_input, rather than filtering events at the very top-level in bevy_winit? That will save us work, and more importantly, ensure our data doesn't desynchronize.

[SpecificProtagonist]

Can you please factor out the new filtering code into a function and add some tests for it? :)

Will do!

The other question I have is: "why should this be in bevy_input, rather than filtering events at the very top-level in bevy_winit? That will save us work, and more importantly, ensure our data doesn't desynchronize.

These events could be useful when reading KeyboardInput events since they carry window information. I'm not confident enough to assert that this isn't useful.

[alice-i-cecile]

These events could be useful when reading KeyboardInput events since they carry window information. I'm not confident enough to assert that this isn't useful.

@UkoeHB @aevyrie @mockersf any strong opinions here? I'm fine to leave this as a more scoped fix for now, especially with the logic extracted and tested.

[UkoeHB]

WindowEvent::KeyboardInput from winit includes an is_synthetic field. This field says Currently, this is only functional on X11 and Windows, so it might be best to filter them out in bevy_winit for cross-platform consistency.

[SpecificProtagonist]

so it might be best to filter them out in bevy_winit for cross-platform consistency.

Sounds like that would be best.

This field says Currently, this is only functional on X11 and Windows

Does anyone know how Bevy behaves on other platforms – when a user holds a key, Bevy loses focus, and then the user releases the key, at what point does is_pressed return false?

[shanecelis]

Hi, I'm on macOS 14.3.1 x86 exercising the multiple_windows example with this system:

fn keyboard_input_system(keyboard_input: Res<ButtonInput<KeyCode>>) {

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

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

The output is always pairs of press and release. I haven't been able to provoke two presses in a row (for the same key) or two releases in a row. However, if I press Shift in window a, switch focus to window b then release, there is no release message. Here's an annotated log.

// Press Shift.
2024-03-08T04:37:18.330416Z  INFO multiple_windows: 'Shift' just pressed
// Switch focus to window b. Release Shift. No log of a release.

// Press Shift. No log.
// Release Shift while focused on window b then it will finally emit a release message.
2024-03-08T04:37:26.599310Z  INFO multiple_windows: 'Shift' just released

Also if Shift is held down from outside the app, then focus is changed to the app, Shift is released, no release message is emitted.

If one presses Shift, changes focus to window b, changes focus back to window a, and releases, then the press and release messages are emitted as expected.

Exercising some of the same scenarios with the A key does not have some of this funny behavior. In fact you can press A switch to another app and when you release A in another app, and the release message is emitted by our app.

I hope this helps explain some of the behavior on macOS. Let me know if there are specific scenarios you'd like to see exercised.

[SpecificProtagonist]

Thanks! Troublesome.

Could you try the reverse (pressing A while not focused, then focusing the window)? And could you try this with also spawning a new window?

…
info!("'A' just pressed");
commands.spawn(Window::default());
…

Looks like it's a bit more work to be consistent across platforms. What behavior do we both want and support? For normal keys, maybe we need to deliberately interpret window focus loss as releasing all keys (and gaining focus as pressing the relevant ones). No idea what to do about modifier keys.

[SpecificProtagonist]

Winit issue: rust-windowing/winit#3575

[shanecelis]

Sure thing. Pressing A while not focused, switching to the app, then releasing A emits no logs. Pressing and releasing A then works as expected.

Spawning another window works fine for the A key, but behaves the same way for the Shift key as described in the annotated log. That is a release and press event for Shift are omitted.

[SpecificProtagonist]

#13696 resolved the question of how we should handle synthetic events and #13678 changed what we do on window focus loss. I've reworked this to take these into account. This also means the logic that needs a unit test is gone; an integration test with actual input would still be helpful, but we don't have the setup for that.

[richchurcher]

@SpecificProtagonist sorry to be a pain, any chance you could update your branch to make it a tad easier to review? Be nice to get this one into 0.15 🙂

[richchurcher]

I note that this and #14379 are doing related things, though I'm not quite sure of the subtleties around whether one makes the other redundant 🤔

[SpecificProtagonist]

I note that this and #14379 are doing related things, though I'm not quite sure of the subtleties around whether one makes the other redundant 🤔

Well spotted – both remove handling of synthetic key releases. However that PR still sets just_pressed & just_released when switching between Bevy windows. I think we ultimately want both PRs.

[Azorlogh]

I'm still receiving KeyboadFocusLost intermittently when switching between bevy windows 😕 I think this happens because sometimes the app processes events after one window lost focus, but just before the other window gained focus.
This sounds like a really tricky case to handle properly 😬

1728136435.mp4