-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 #8745
Conversation
IMO we should probably just remove the |
The Web backend in Winit had quite a bit of churn lately, let me know if you uncover any bugs or issues. |
with 54483f9, I'm exploring:
|
Unfortunately this option proves to be difficult, bevy's input system involves |
Removing the |
Prompted by winit's update bevyengine#8745.
# Objective To upgrade winit's dependency, it's useful to reuse SmolStr, which replaces/improves the too restrictive Key letter enums. As Input<Key> is a resource it should implement Reflect through all its fields. ## Solution Add smol_str to bevy_reflect supported types, behind a feature flag. This PR blocks winit's upgrade PR: #8745. # Current state - I'm discovering bevy_reflect, I appreciate all feedbacks, and send me your nitpicks! - Lacking more tests --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Gino Valente <[email protected]>
@alice-i-cecile I think this PR can be removed from 0.11 milestone, because winit doesn't plan to release 0.29 until august. |
Example |
1 similar comment
Example |
Example |
Example |
No longer blocked following the winit 0.29 release 🎉 |
🙏 @Vrixyz for those who may stuble upon this, how is this issue/pr related to fixing the bevy_egui issue where non-qwerty keyboards can't type symbols or special characters on wasm32? |
few format tweaks, initially spotted working on #8745
To answer "in short": I tracked it to the example char_input_events on bevy wasm, and noticed such special characters were not printed. winit refactored the keyboard input and I believe that's a good step towards fixing that issue. For a longer context: vladbat00/bevy_egui#178 (comment) -> this doesn't explicitly mentions special characters but it's a bit all related (other recap of the context here: https://thierryberger.com/drafts/contribution-story-winit/) |
few format tweaks, initially spotted working on bevyengine#8745
For exactness, we were still blocked by accesskit, but it's not longer the case following https://github.com/AccessKit/accesskit/releases/tag/accesskit_winit-v0.16.0 🎉 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed! This is a great step forward for input handling c:
converters::convert_keyboard_input(event, window_entity); | ||
if let bevy_input::keyboard::Key::Character(c) = &keyboard_event.logical_key | ||
{ | ||
if let Some(first_char) = c.chars().next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sending only the first character correct here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.. guess not, but sending the whole SmolStr within ReceivedCharacter seemed a big change when I was on it.
I can see multiple solutions:
- sending SmolStr.
- sending several ReceivedCharacter(char)
- splitting ReceivedCharacter and ReceivedMultipleCharacters ?
I thought this implementation was the closest to current main behaviour, so that might be OK as a first approach.
I'm writing that in "Follow Up" for the time being.
crates/bevy_winit/src/lib.rs
Outdated
if inner_size_writer.request_inner_size(new_inner_size).is_ok() { | ||
new_inner_size = maybe_new_inner_size; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it makes sense to log the failure case here.
crates/bevy_winit/src/lib.rs
Outdated
} else { | ||
run(event_loop, event_handler); | ||
} | ||
let _ = run(event_loop, event_handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should log the error here.
crates/bevy_winit/src/system.rs
Outdated
@@ -174,7 +174,7 @@ pub(crate) fn changed_windows( | |||
window.resolution.physical_width(), | |||
window.resolution.physical_height(), | |||
); | |||
winit_window.set_inner_size(physical_size); | |||
let _ = winit_window.request_inner_size(physical_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we should handle this returning Some
. From the docs:
On platforms where the size is entirely controlled by the user the applied size will be returned immediately, resize event in such case may not be generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(link to docs)
" may not be generated " 🙁 ; I don't like uncertainty...
this seems to only affect wayland? ; but the exact path to the maybe not generated event is quite hard to follow 🤔...
I'll follow the trail to winit and might open an issue because I'd like to handle differently:
- Some(newValue): generate an event resize like we do when reacting from events
⚠️ the "may" from the docs may cause the event to be sent twice. - Some(oldValue): do not generate an event resize
⚠️ I cannot differentiate it from a new value, so I'd send "useless" resize. Well I guess we're not supposed to send a resize event in those platforms anyway but still. - None: event will be sent afterward, or not ✅
maybe the docs is wrong and should read "will not be generated", so I can handle it with certainty 🤞.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -194,7 +194,7 @@ impl WinitWindows { | |||
use winit::platform::web::WindowExtWebSys; | |||
|
|||
if window.canvas.is_none() { | |||
let canvas = winit_window.canvas(); | |||
let canvas = winit_window.canvas().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for this state
Only returns the canvas if called from inside the window.
I'm not sure what that means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's alright to call unwrap on that, I believe it's instantiated at the same time of a window, so it might fail if you're doing convoluted things like modifying the canvas from javascript.
But that's not a rabbit hole I'm ready to follow yet. I'll make a note about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only returns the canvas if called from inside the window.
Apparently this didn't turn out very clear. This will only return None
if called from a Web worker, it will always succeed if called from the "main thread" (the "window" context).
//! - <https://docs.rs/bevy/latest/bevy/winit/struct.WinitSettings.html#structfield.return_from_run> | ||
//! not recommended because: | ||
//! - `App::run()` will never return on iOS and Web. | ||
//! - It is not possible to recreate a window afer the event loop has been terminated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now a Bevy limitation, not winit's, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
examples/audio/audio_control.rs
Outdated
if keyboard_input.just_pressed(KeyCode::Plus) { | ||
if keyboard_input.just_pressed(KeyCode::NumpadAdd) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up with Plus
? Did it become Equal
? but Equals
already existed, hmmm
Not everyone has a numpad though, so should we use something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure; I'll use equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were swapped in winit
by mistake IIRC :(
// TOCLEAN: Thierry: this is originally SmolStr, but difficult to translate to FromReflect. | ||
Web(SmolStr), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment up-to-date?
/// Program busy indicator. | ||
Wait, | ||
/// Help indicator (often rendered as a "?") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about all these empty lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to be as similar as the source, which was updated this way: https://github.com/rust-windowing/cursor-icon/blob/main/src/lib.rs
as cursor-icon was given its own dependency, maybe we could rely on that directly rather than copying it?
Also, licensing is probably not respected (and maybe other places too (see PR description "Call for help")), so I'm happy to discuss a better way of handling that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying the message in winit's 'keyboard.rs' verbatim to our 'keyboard.rs' file(Where Key
and KeyCode
are defined) will satisfy the W3C license :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from https://www.w3.org/copyright/software-license-2002/ ; I think we'd need to also include the changes made. they are pretty straightforward:
- add Reflect capabilities
- use another feature name for serialization
- maybe not using non_exhaustive for enums?
But I lack knowledge on how to write that. I'll copy the licenses as I believe it's better than nothing :)
I'm closing this PR in favor of #10702 ; which builds on this one but it's time to get a shorter comment thread |
# 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.
few format tweaks, initially spotted working on bevyengine#8745
# 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.
#10702 only builds on this PR but without obsolete comments and discussions.
Objective
Changelog
KeyLogic::Logic(Key)
, with "regular" characters behind a SmolStr.web-time
rather thaninstant
crate. (Replaceinstant
withweb-time
rust-windowing/winit#2836)run_return
inrun_onDemand
andpump_events
, I did the same change in bevy_winit and usedpump_events
.return_from_run
fromWinitSettings
aswinit::run
now returns on supported platforms.Call for help
Answered interrogations
API complexity
I'm not sure about this new API, it seems cumbersome to do #8745 (comment)
key.is_pressed("q")
Copy incompatible with SmolStr
We should add a logical
Key
based on winit's Key, but bevy'sKeyboardInput
is Copy, but SmolStr is not, so I'm not sure how to proceed ?add Copy upstream to SmolStr (I think that won't pass, if at all possible)use another type ? plain String...use our curated list of supported characters/string as enum, but that doesn't sound great (because chinese characters, copy/paste/IME probably wouldn't be supported this way.Blockers
winit
0.29accesskit_winit
: to update to winit 0.29 ; okFollow 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 one this PR is closed:
ReceivedCharacter
: This PR only sends the first character, commentinstant
dependency, replaced byweb_time
)async-executor
futures-lite
Migration Guide
This PR should have one.