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

Update winit dependency to 0.29 #10702

Merged
merged 66 commits into from
Dec 21, 2023

Conversation

Vrixyz
Copy link
Member

@Vrixyz Vrixyz commented Nov 22, 2023

Objective

  • Update winit dependency to 0.29

Changelog

KeyCode changes

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

Known regressions (important follow ups?)

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:

Migration Guide

This PR should have one.

Vrixyz added 30 commits June 1, 2023 18:33
@Vrixyz Vrixyz mentioned this pull request Dec 21, 2023
23 tasks
github-merge-queue bot pushed a commit that referenced this pull request Dec 21, 2023
# Objective

#10702 has overridden the changes
that #10980 did.

## Solution

Re-add `SystemTime` to `bevy_utils`, along with a few other types.

---

## Changelog

- Rexported `SystemTime`, `SystemTimeError`, and `TryFromFloatSecsError`
from `bevy_utils`.
github-merge-queue bot pushed a commit that referenced this pull request Dec 21, 2023
# Objective

- After #10702, it seems `libxkbcommon-x11-0` is now a default
dependency
```
2023-12-21T14:13:14.876926Z  INFO log: Failed loading `libxkbcommon-x11.so.0`. Error: CantOpen(DlOpen { desc: "libxkbcommon-x11.so.0: cannot open shared object file: No such file or directory" })   
```

## Solution

- Add the new dependency on linux
github-merge-queue bot pushed a commit that referenced this pull request Dec 22, 2023
# Objective

- #10702 introduced some changes that broke patches for the example
showcase

## Solution

- Update those patches
github-merge-queue bot pushed a commit that referenced this pull request Jan 6, 2024
# Objective

- Since #10702, the way bevy updates the window leads to major slowdowns
as seen in
    - #11122 
    - #11220
- Slow is bad, furthermore, _very_ slow is _very_ bad. We should fix
this issue.

## Solution

- Move the app update code into the `Event::WindowEvent { event:
WindowEvent::RedrawRequested }` branch of the event loop.
- Run `window.request_redraw()` When `runner_state.redraw_requested`
- Instead of swapping `ControlFlow` between `Poll` and `Wait`, we always
keep it at `Wait`, and use `window.request_redraw()` to schedule an
immediate call to the event loop.
- `runner_state.redraw_requested` is set to `true` when
`UpdateMode::Continuous` and when a `RequestRedraw` event is received.
- Extract the redraw code into a separate function, because otherwise
I'd go crazy with the indentation level.
- Fix #11122.

## Testing

I tested the WASM builds as follow:

```sh
cargo run -p build-wasm-example -- --api webgl2 bevymark
python -m http.server --directory examples/wasm/ 8080
# Open browser at http://localhost:8080
```

On main, even spawning a couple sprites is super choppy. Even if it says
"300 FPS". While on this branch, it is smooth as butter.

I also found that it fixes all choppiness on window resize (tested on
Linux/X11). This was another issue from #10702 IIRC.

So here is what I tested:

- On `wasm`: `many_foxes` and `bevymark`, with `argh::from_env()`
commented out, otherwise we get a cryptic error.
- Both with `PresentMode::AutoVsync` and `PresentMode::AutoNoVsync`
  - On main, it is consistently choppy.
- With this PR, the visible frame rate is consistent with the diagnostic
numbers
- On native (linux/x11) I ran similar tests, making sure that
`AutoVsync` limits to monitor framerate, and `AutoNoVsync` doesn't.

## Future work

Code could be improved, I wanted a quick solution easy to review, but we
really need to make the code more accessible.

- #9768
- ~~**`WinitSettings::desktop_app()` is completely borked.**~~ actually
broken on main as well

### Review guide

Consider enable the non-whitespace diff to see the _real_ change set.
@Shute052
Copy link

Shute052 commented Feb 3, 2024

Is there any ways can translate a KeyCode into its logical Key or ReceivedCharacter?

@Vrixyz
Copy link
Member Author

Vrixyz commented Feb 3, 2024

No, I'm guessing you're asking in the context of vladbat00/bevy_egui#236 ; what we'll probably want is to hook on KeyboardInput to retrieve the logical key.

@Shute052
Copy link

Shute052 commented Feb 3, 2024

No, I'm guessing you're asking in the context of mvlabat/bevy_egui#236 ; what we'll probably want is to hook on KeyboardInput to retrieve the logical key.

The problem is listed in the leafwing-input-manager PR for updates with Bevy 0.13

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-Windowing Platform-agnostic interface layer to run your app in C-Dependencies A change to the crates that Bevy depends on D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-Regression Functionality that used to work but no longer does. Add a test for this! 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
Development

Successfully merging this pull request may close these issues.