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

KeyCode::key_char #10689

Closed
wants to merge 1 commit into from
Closed

Conversation

stepancheg
Copy link
Contributor

Objective

To implement basic (e.g. self-debugging) text input using bevy without extensions, some code is needed to convert keys to text.

Solution

KeyCode::key_char does naive conversion of keys to chars, for example for KeyCode::Key1 without shift it returns '1' without shift and '!' with shift.

Changelog

Added

  • KeyCode::key_char function to map from keycode to Rust char

@alice-i-cecile alice-i-cecile added A-Input Player input via keyboard, mouse, gamepad, and more C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 22, 2023
@alice-i-cecile
Copy link
Member

@Vrixyz how compatible is this with the upcoming winit changes?

@Vrixyz
Copy link
Member

Vrixyz commented Nov 22, 2023

I'd say the concept is compatible with the winit update, it's a helper function over our KeyCode, owned by bevy_input and not bevy_winit.

If this PR is accepted, winit PR will have to rewrite most of its KeyCode mappings due to their changes, but it would be a minor conversion.

@Vrixyz
Copy link
Member

Vrixyz commented Nov 22, 2023

That being said, such conversions are highly "debugging level", I'm curious why provide mapping over shift modifier, and not alt/ctrl ?

I'm curious of a practical use case which wouldn't be covered by Debug (or a simpler key_char mapping without shift, where the user has to combine with shift modifier afterwards

@rlidwka
Copy link
Contributor

rlidwka commented Nov 22, 2023

KeyCode::key_char does naive conversion of keys to chars

Is there a non-naive conversion? What would it look like? Do we want to have it?

Just want to make sure we aren't introducing a bad practice here, which would break code for users with different keyboard layouts.

edit: (see below)
maybe name it key_char_in_us_keyboard_layout at the very least to highlight the fact
and clearly describe in docs where users can get actual typed character from (which is usually stored alongside KeyCode I guess)

Copy link
Contributor

@rlidwka rlidwka left a comment

Choose a reason for hiding this comment

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

  1. I suggest to rename KeyCode::key_char() to something that reflects the fact that it works in specific keyboard layout (for example, KeyCode::in_us_layout()).

  2. Please add documentation that clarifies, for all the people who have Res<Input<KeyCode>> in their code and want to get actual (layout-dependent) symbol typed by the user, how to change their code to do so. Otherwise, they are going to use this method and do the wrong thing.

@stepancheg
Copy link
Contributor Author

KeyCode::in_us_layout()

This wouldn't be 100% correct, but OK.

Please add documentation that clarifies, for all the people who have Res<Input> in their code and want to get actual (layout-dependent) symbol typed by the user, how to change their code to do so

I can do small improvement like this PR, or I can abandon this PR if you think this is not improvement. I cannot afford to spend hours researching this topic.

Otherwise, they are going to use this method and do the wrong thing.

In my subjective opinion, being able to do somewhat incorrect thing is better than not being able to do it at all.

@stepancheg
Copy link
Contributor Author

Renamed.

@mockersf
Copy link
Member

mockersf commented Nov 22, 2023

keyboard event from winit 0.29 is https://docs.rs/winit/0.29.3/winit/event/struct.KeyEvent.html
This can be updated to work on the next version of the key code, but it will be duplicated and less correct than what is provided by winit, which will give the actual text according to the keyboard layout and given by the OS

github-merge-queue bot pushed a commit that referenced this pull request Dec 21, 2023
# Objective

- Update winit dependency to 0.29

## Changelog

### KeyCode changes

- Removed `ScanCode`, as it was [replaced by
KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292).
- `ReceivedCharacter.char` is now a `SmolStr`, [relevant
doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text).
- Changed most `KeyCode` values, and added more.

KeyCode has changed meaning. With this PR, it refers to physical
position on keyboard rather than the printed letter on keyboard keys.

In practice this means:
- On QWERTY keyboard layouts, nothing changes
- On any other keyboard layout, `KeyCode` no longer reflects the label
on key.
- This is "good". In bevy 0.12, when you used WASD for movement, users
with non-QWERTY keyboards couldn't play your game! This was especially
bad for non-latin keyboards. Now, WASD represents the physical keys. A
French player will press the ZQSD keys, which are near each other,
Kyrgyz players will use "Цфыв".
- This is "bad" as well. You can't know in advance what the label of the
key for input is. Your UI says "press WASD to move", even if in reality,
they should be pressing "ZQSD" or "Цфыв". You also no longer can use
`KeyCode` for text inputs. In any case, it was a pretty bad API for text
input. You should use `ReceivedCharacter` now instead.

### Other changes
- Use `web-time` rather than `instant` crate.
(rust-windowing/winit#2836)
- winit did split `run_return` in `run_onDemand` and `pump_events`, I
did the same change in bevy_winit and used `pump_events`.
- Removed `return_from_run` from `WinitSettings` as `winit::run` now
returns on supported platforms.
- I left the example "return_after_run" as I think it's still useful.
- This winit change is done partly to allow to create a new window after
quitting all windows: emilk/egui#1918 ; this
PR doesn't address.
- added `width` and `height` properties in the `canvas` from wasm
example
(#10702 (comment))

## Known regressions (important follow ups?)
- Provide an API for reacting when a specific key from current layout
was released.
- possible solutions: use winit::Key from winit::KeyEvent ; mapping
between KeyCode and Key ; or .
- We don't receive characters through alt+numpad (e.g. alt + 151 = "ù")
anymore ; reproduced on winit example "ime". maybe related to
rust-windowing/winit#2945
- (windows) Window content doesn't refresh at all when resizing. By
reading rust-windowing/winit#2900 ; I suspect
we should just fire a `window.request_redraw();` from `AboutToWait`, and
handle actual redrawing within `RedrawRequested`. I'm not sure how to
move all that code so I'd appreciate it to be a follow up.
- (windows) unreleased winit fix for using set_control_flow in
AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm
not sure what the implications are, but that feels bad 🤔

## Follow up 

I'd like to avoid bloating this PR, here are a few follow up tasks
worthy of a separate PR, or new issue to track them once this PR is
closed, as they would either complicate reviews, or at risk of being
controversial:
- remove CanvasParentResizePlugin
(#10702 (comment))
- avoid mentionning explicitly winit in docs from bevy_window ?
- NamedKey integration on bevy_input:
rust-windowing/winit#3143 introduced a new
NamedKey variant. I implemented it only on the converters but we'd
benefit making the same changes to bevy_input.
- Add more info in KeyboardInput
#10702 (review)
- #9905 added a workaround on a
bug allegedly fixed by winit 0.29. We should check if it's still
necessary.
- update to raw_window_handle 0.6
  - blocked by wgpu
- Rename `KeyCode` to `PhysicalKeyCode`
#10702 (comment)
- remove `instant` dependency, [replaced
by](rust-windowing/winit#2836) `web_time`), we'd
need to update to :
  - fastrand >= 2.0
- [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7
    - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0
- Verify license, see
[discussion](#8745 (comment))
  - we might be missing a short notice or description of changes made
- Consider using https://github.com/rust-windowing/cursor-icon directly
rather than vendoring it in bevy.
- investigate [this
unwrap](#8745 (comment))
(`winit_window.canvas().unwrap();`)
- Use more good things about winit's update
- #10689 (comment)
## Migration Guide

This PR should have one.
@Vrixyz Vrixyz mentioned this pull request Dec 21, 2023
23 tasks
Subserial pushed a commit to Subserial/bevy_winit_hook that referenced this pull request Feb 20, 2024
# Objective

- Update winit dependency to 0.29

## Changelog

### KeyCode changes

- Removed `ScanCode`, as it was [replaced by
KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292).
- `ReceivedCharacter.char` is now a `SmolStr`, [relevant
doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text).
- Changed most `KeyCode` values, and added more.

KeyCode has changed meaning. With this PR, it refers to physical
position on keyboard rather than the printed letter on keyboard keys.

In practice this means:
- On QWERTY keyboard layouts, nothing changes
- On any other keyboard layout, `KeyCode` no longer reflects the label
on key.
- This is "good". In bevy 0.12, when you used WASD for movement, users
with non-QWERTY keyboards couldn't play your game! This was especially
bad for non-latin keyboards. Now, WASD represents the physical keys. A
French player will press the ZQSD keys, which are near each other,
Kyrgyz players will use "Цфыв".
- This is "bad" as well. You can't know in advance what the label of the
key for input is. Your UI says "press WASD to move", even if in reality,
they should be pressing "ZQSD" or "Цфыв". You also no longer can use
`KeyCode` for text inputs. In any case, it was a pretty bad API for text
input. You should use `ReceivedCharacter` now instead.

### Other changes
- Use `web-time` rather than `instant` crate.
(rust-windowing/winit#2836)
- winit did split `run_return` in `run_onDemand` and `pump_events`, I
did the same change in bevy_winit and used `pump_events`.
- Removed `return_from_run` from `WinitSettings` as `winit::run` now
returns on supported platforms.
- I left the example "return_after_run" as I think it's still useful.
- This winit change is done partly to allow to create a new window after
quitting all windows: emilk/egui#1918 ; this
PR doesn't address.
- added `width` and `height` properties in the `canvas` from wasm
example
(bevyengine/bevy#10702 (comment))

## Known regressions (important follow ups?)
- Provide an API for reacting when a specific key from current layout
was released.
- possible solutions: use winit::Key from winit::KeyEvent ; mapping
between KeyCode and Key ; or .
- We don't receive characters through alt+numpad (e.g. alt + 151 = "ù")
anymore ; reproduced on winit example "ime". maybe related to
rust-windowing/winit#2945
- (windows) Window content doesn't refresh at all when resizing. By
reading rust-windowing/winit#2900 ; I suspect
we should just fire a `window.request_redraw();` from `AboutToWait`, and
handle actual redrawing within `RedrawRequested`. I'm not sure how to
move all that code so I'd appreciate it to be a follow up.
- (windows) unreleased winit fix for using set_control_flow in
AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm
not sure what the implications are, but that feels bad 🤔

## Follow up 

I'd like to avoid bloating this PR, here are a few follow up tasks
worthy of a separate PR, or new issue to track them once this PR is
closed, as they would either complicate reviews, or at risk of being
controversial:
- remove CanvasParentResizePlugin
(bevyengine/bevy#10702 (comment))
- avoid mentionning explicitly winit in docs from bevy_window ?
- NamedKey integration on bevy_input:
rust-windowing/winit#3143 introduced a new
NamedKey variant. I implemented it only on the converters but we'd
benefit making the same changes to bevy_input.
- Add more info in KeyboardInput
bevyengine/bevy#10702 (review)
- bevyengine/bevy#9905 added a workaround on a
bug allegedly fixed by winit 0.29. We should check if it's still
necessary.
- update to raw_window_handle 0.6
  - blocked by wgpu
- Rename `KeyCode` to `PhysicalKeyCode`
bevyengine/bevy#10702 (comment)
- remove `instant` dependency, [replaced
by](rust-windowing/winit#2836) `web_time`), we'd
need to update to :
  - fastrand >= 2.0
- [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7
    - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0
- Verify license, see
[discussion](bevyengine/bevy#8745 (comment))
  - we might be missing a short notice or description of changes made
- Consider using https://github.com/rust-windowing/cursor-icon directly
rather than vendoring it in bevy.
- investigate [this
unwrap](bevyengine/bevy#8745 (comment))
(`winit_window.canvas().unwrap();`)
- Use more good things about winit's update
- bevyengine/bevy#10689 (comment)
## Migration Guide

This PR should have one.
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants