winit 0.29: Ergonomics, examples + keymap cache

[Vrixyz]

Context for this PR: bevyengine#8745 .

Dynamic key mapping to rely on physical key rather than only logical key

Problem description

winit 0.29 logical key with modifiers (shift) is hard to use cache with, because winit can send pressed('A'), and then released('a').

So Res::<Key>::is_pressed('A') returns true when pressing 'A' then releasing 'a'. That's not correct!

Details

A branch without the Dynamic key mapping can be tried there: https://github.com/Vrixyz/bevy/tree/update-winit-without-dynamic-key-mapping

Detailed scenario

Bevy adds a helper function (is_pressed) to know if a key is currently pressed or not, we react to winit::event::KeyboardInput and cache its value when changed.

Now, I adapted the code to the more customizable "SmolStr" ; and I'm having issues when caching this value with modifiers, because the logical key changes to a "released" without being pressed

We receive: (azerty keyboard)
1: press shift right

KeyEvent {
    physical_key: ShiftRight,
    logical_key: Shift,
    text: None,
    location: Right,
    state: Pressed,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: None,
        key_without_modifiers: Shift,
    },
}

2: press "a" (shift right still pressed)

KeyEvent {
    physical_key: KeyQ,
    logical_key: Character(
        "A", // <--- We store cache["A"] = pressed
    ),
    text: Some(
        "A",
    ),
    location: Standard,
    state: Pressed,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: Some(
            "A",
        ),
        key_without_modifiers: Character(
            "a",
        ),
    },
}

3: release Shift Right, "a" still pressed

KeyEvent {
    physical_key: ShiftRight,
    logical_key: Shift,
    text: None,
    location: Right,
    state: Released,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: None,
        key_without_modifiers: Shift,
    },
}

4: release "a"

KeyEvent {
    physical_key: KeyQ,
    logical_key: Character(
        "a", // <--- We store cache["a"] = released;
    ),
    text: None,
    location: Standard,
    state: Released,
    repeat: false,
    platform_specific: KeyEventExtra {
        text_with_all_modifers: None,
        key_without_modifiers: Character(
            "a",
        ),
    },
}

Detailed solution

Problem recap

With shift modifier:

user sends keypress(logical = "A" ; KeyCode::KeyQ)

user might release key: (logical = "a" ; KeyCode::KeyQ)

So is_pressed("A") cannot rely on is_released("A")

My solution

When a bevy user asks is_pressed/released("A") -> bevy should check on the more reliable KeyCode::KeyQ.

To map both together, I store the physical key (KeyCode), alongside a map between logical key and physical key.

• press("A", KeyCode::Q)) -> adds a KeyCode::Q, + a mapping logic("A") -> KeyCode::Q
• is_pressed("A") -> check if we have a mapping with logic("A"), we have, so we check our stored KeyCode::Q
• release("a") -> we use the reliable KeyCode::Q from winit to release logical key.

New aggregated resource Res<Input<KeyLogic>>

This is less necessary, but I noticed a lot of duplicated code between Res<Input<KeyCode>> and Res<Input<Key>>, this is an attempt to simplify the API and improve maintainability.

Resources impacts

Breaking: removed Res<Input<KeyCode>> and Res<Input<ScanCode>>, in favor of Res<Input<KeyLogic>>

• ScanCode information can be inferred within KeyLogic::PhysicalKey.
• a new concept of LogicalKey (depending on OS software layout) can be accessed through KeyLogic::Key.

API impacts

These key snippets assume a keys: Res<Input<KeyLogic>>

You can now query for physical or logical key codes with:

• Physical: keys.pressed(KeyLogic::Physical(KeyCode::KeyA))
• Logical: keys.pressed(KeyLogic::Logic(Key::Character(SmollStr::new("a")))

Through From trait impls, ergonomics are still good:

• Physical: keys.pressed(KeyCode::KeyA)
• Logical: keys.pressed("a")
  • or keys.pressed(KeyLogic::Logic("a"))
  • or keys.pressed(Key::Character("a"))

[Vrixyz]

To allow FromReflect on Input<T>, T needs to be FromReflect too because of the HashMap, it's a bit unfortunate because it adds a constraint on T.

[alice-i-cecile]

This is a confusing comment.

[alice-i-cecile]

This name isn't good: it's hard to intuit what it might mean and it's quite unnatural in English.

[alice-i-cecile]

This is a very interesting change. I'm not sure I agree with the design: why would the physical and logical keys be the same type?

[Vrixyz]

This change is led by the inability from winit to be sure a key_pressed(Character("a")) leads to a key_released(Character("a")), so bevyengine#8745 believes character("a") is still pressed.

To understand the issue, I invite you to try the example keyboard_input.

I have to keep track on original keycode to update its cached keypress status.

Now I could have made that change only for Input (logical key), but as the data model is shared, that would have led to code duplication, which I thought was not worth it.

Another option would be to open an issue on winit, to evaluate if we can have a character released without the modifiers, or an event for key_change: from a modifier (lowercase) to another (uppercase) 🤔

[alice-i-cecile]

Unifying these into a single type seems incorrect: it seems likely to increase confusion and boilerplate. See comment about about the HashMap.

[alice-i-cecile]

Very interesting set of changes, but I really think we should leave this out of the 0.29 PR. That one is hard to understand and review as is, and this set of changes deserves a full round of review in its own right.

[Vrixyz]

@alice-i-cecile I'd love to isolate this PR out, but then how do I fix #3 (comment) ?

Another option might be to not handle Character(SmolStr) in Input: only support it in the ReceivedCharacters events 🤔

[Vrixyz]

that's abandoned in favor of exposing raw KeyEvent.