Add logical key data to KeyboardInput #11400

Vrixyz · 2024-01-18T08:30:03Z

Add logical key data to KeyboardInput

Addresses an item of #11052

rparrett · 2024-01-18T15:56:19Z

It seems like this would close #11297 for me. At least the behavior between web/native on MacOS for KeyInput::logical_key(Key::Character(s)) seems predictable enough to work with. It would be great if someone could test with a non-US keyboard.

I am still sort of baffled by the myriad of ways of getting a character from a winit key event, so I hope I'm doing this correctly.

I'm testing in this branch of bevy_simple_text_input: https://github.com/rparrett/bevy_simple_text_input/blob/bevy_main/src/lib.rs#L75

alice-i-cecile · 2024-01-18T16:00:51Z

crates/bevy_input/src/keyboard.rs

+///
+/// ## Technical
+///
+/// Its values map 1 to 1 to winit's Key.


Doc link here would be useful.

We can't really "doc link", not to the exact type, because we could rely on another backend. This note is more for contributors to understand where this is coming from. I think a link to winit would be useful but maybe difficult to maintain ?

Oh hmm, true. I suppose this is the best we can readily do.

alice-i-cecile · 2024-01-18T16:01:22Z

crates/bevy_input/src/keyboard.rs

+/// different values on different platforms, which is one of the reasons this is a per-platform
+/// enum.
+///
+/// This enum is primarily used to store raw keysym when Winit doesn't map a given native logical


I believe it's short for key symbol.

Those descriptions are coming mostly from winit, I'm not sure referring to them as keysym helps much the user indeed. A google search on that term doesn't respond with a clear description, so I assume it's a vague term for referring to key codes, or logical key code names.

I'm not opposed to remove the term. Or maybe bikeshed that in another issue

@alice-i-cecile another question on that line is the reference to winit which we should probably avoid within bevy_input in favor of "window backend" or equivalent.

Yeah, "windowing backend" is slightly better.

at the same time, I'm curious how many users don't use winit ; blurring the line of the dependency to winit comes at a price of increased complexity: the user has to understand what is the "windowing backend".

It can be argued it's a "small complexity", or an "implementation detail", but I hate when I have to jump through a lot of "soft references", I'll consider this "extreme abstraction" shadowing winit controversial.

It's listed in the todo on #11052 so this discussion will come again :)

alice-i-cecile

A couple of doc suggestions but nothing blocking.

alice-i-cecile · 2024-01-18T16:03:22Z

I am still sort of baffled by the myriad of ways of getting a character from a winit key event, so I hope I'm doing this correctly.

I would love to see an example for this, but it probably shouldn't be this PR.

rparrett · 2024-01-18T16:46:18Z

I would love to see an example for this, but it probably shouldn't be this PR.

Agreed. Maybe the text_input example should be modified. It's a bit awkward because

using ReceivedCharacter for whitespace doesn't work consistently cross-platform (winit bug)
combining ReceivedCharacter and KeyboardInput is buggy because there's no way to know which event came first

Which makes ReceivedCharacter useless, practically. Personally, I'm not sure why it even needs to exist. Definitely a discussion for another GitHub issue / PR though.

Did not intend in any way to derail. This functionality is desperately needed.

crates/bevy_input/src/keyboard.rs

alice-i-cecile · 2024-01-18T17:22:06Z

@Vrixyz lemme know when you're ready and then I'll merge. Otherwise, I'll merge Monday.

Co-authored-by: Mateusz Wachowiak <mateusz_wachowiak@outlook.com>

Vrixyz · 2024-01-18T21:51:32Z

crates/bevy_input/src/keyboard.rs

+/// It is used as the generic `T` value of an [`Input`] to create a `Res<Input<Key>>`.
+/// The resource values are mapped to the current layout of the keyboard and correlate to a [`KeyCode`].


This is wrong, it's only used in KeyboardInput.

I wanted to support that use case but decided to remove it there: 9c7a452#diff-175a1b1cfe0a099d4b8927aff78925ab491b7f48c14a0ad99ad1f34c688e3e51.

More insights here: Vrixyz#3

TL;DR: adding support for reliable is_released(a key) after is_pressed(same key) has fired is controversial at best.

Vrixyz · 2024-01-18T22:10:26Z

crates/bevy_input/src/keyboard.rs

    pub key_code: KeyCode,
+    /// The logical key of the input
+    pub logical_key: Key,


maybe key_logical ? so nomenclature is similar for similar properties.

And rename key_codeto key_physical ? Probably as a follow up, close to an existing one in #11052 ("Rename KeyCode to PhysicalKeyCode")

or I guess we'll probably rename key_code to physical_key.

I just like the similar starting keystroks for similar properties, because it's autocompletion friendly, but I won't insist on it. (especially since winit uses physical_key and logical_key, so LGTM for now.

Vrixyz · 2024-01-18T22:15:13Z

crates/bevy_input/src/lib.rs

@@ -114,7 +114,10 @@ impl Plugin for InputPlugin {

        // Register keyboard types
        app.register_type::<KeyboardInput>()
-            .register_type::<KeyCode>();
+            .register_type::<KeyCode>()
+            .register_type::<NativeKeyCode>()


That's a tiny bit out of scope, but I guess it's alright.

Vrixyz · 2024-01-18T22:16:12Z

@Vrixyz lemme know when you're ready and then I'll merge. Otherwise, I'll merge Monday.

Thanks @alice-i-cecile ! I think I'm done with it now, I left a few self-reviews to guide more discussion if need be.

add logical key data to KeyboardInput

52b7407

Vrixyz added C-Feature A new feature, making something new possible A-Input Player input via keyboard, mouse, gamepad, and more labels Jan 18, 2024

alice-i-cecile reviewed Jan 18, 2024

View reviewed changes

alice-i-cecile approved these changes Jan 18, 2024

View reviewed changes

matiqo15 approved these changes Jan 18, 2024

View reviewed changes

crates/bevy_input/src/keyboard.rs Outdated Show resolved Hide resolved

crates/bevy_input/src/keyboard.rs Outdated Show resolved Hide resolved

crates/bevy_input/src/keyboard.rs Outdated Show resolved Hide resolved

matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 18, 2024

alice-i-cecile mentioned this pull request Jan 18, 2024

Add example demonstrating how to correctly decode a stream of characters from winit #11408

Open

Vrixyz and others added 3 commits January 18, 2024 20:54

Update keyboard.rs

4b8e445

Co-authored-by: Mateusz Wachowiak <mateusz_wachowiak@outlook.com>

Update keyboard.rs

15024cf

Co-authored-by: Mateusz Wachowiak <mateusz_wachowiak@outlook.com>

Update crates/bevy_input/src/keyboard.rs

ab10641

Co-authored-by: Mateusz Wachowiak <mateusz_wachowiak@outlook.com>

Vrixyz commented Jan 18, 2024

View reviewed changes

Vrixyz added 2 commits January 18, 2024 22:58

removed wrong doc comment

4096723

fix reflect for Key, NativeKey and NativeKeyCode

07775ad

Vrixyz commented Jan 18, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Jan 22, 2024

Merged via the queue into bevyengine:main with commit 2165793 Jan 22, 2024
26 checks passed

This was referenced Jan 22, 2024

WASM Input (needs to be better) vladbat00/bevy_egui#246

Open

Winit 0.29 follow up #11052

Closed

nicopap mentioned this pull request Jan 29, 2024

0.13 announcement post bevyengine/bevy-website#891

Merged

Vrixyz mentioned this pull request Feb 3, 2024

Update to egui 0.25.0 vladbat00/bevy_egui#248

Closed

rparrett mentioned this pull request Feb 4, 2024

Provide a unified stream of key press events with character equivalents #11297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logical key data to KeyboardInput #11400

Add logical key data to KeyboardInput #11400

Vrixyz commented Jan 18, 2024

rparrett commented Jan 18, 2024

alice-i-cecile Jan 18, 2024

Vrixyz Jan 18, 2024

alice-i-cecile Jan 18, 2024

alice-i-cecile Jan 18, 2024

matiqo15 Jan 18, 2024

Vrixyz Jan 18, 2024

Vrixyz Jan 18, 2024

alice-i-cecile Jan 18, 2024

Vrixyz Jan 18, 2024

alice-i-cecile left a comment

alice-i-cecile commented Jan 18, 2024

rparrett commented Jan 18, 2024 •

edited

Loading

alice-i-cecile commented Jan 18, 2024

Vrixyz Jan 18, 2024

Vrixyz Jan 18, 2024

Vrixyz Jan 18, 2024

Vrixyz Jan 18, 2024

Vrixyz commented Jan 18, 2024 •

edited

Loading

		/// It is used as the generic `T` value of an [`Input`] to create a `Res<Input<Key>>`.
		/// The resource values are mapped to the current layout of the keyboard and correlate to a [`KeyCode`].

Add logical key data to KeyboardInput #11400

Add logical key data to KeyboardInput #11400

Conversation

Vrixyz commented Jan 18, 2024

rparrett commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jan 18, 2024

rparrett commented Jan 18, 2024 • edited Loading

alice-i-cecile commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vrixyz commented Jan 18, 2024 • edited Loading

rparrett commented Jan 18, 2024 •

edited

Loading

Vrixyz commented Jan 18, 2024 •

edited

Loading