Winit 0.29 follow up

[ThierryBerger]

A few shortcuts were taken in https://github.com/bevyengine/bevy/pull/10702 in order to merge it in a reasonable amount of time.

This issue serves to :

• coordinate addressing those follow-ups
• offer a place to discuss the winit update
• mentoring opportunities

Feel free to create or ask a dedicated issue for any listed task, if you feel like it's worth it.

Known regressions (important follow ups?)

• ⚠️ Fix big problem in event loop https://github.com/bevyengine/bevy/issues/11052#issuecomment-1872498051
  • [x] Preformance fix in https://github.com/bevyengine/bevy/pull/11227
  • [ ] more eventloop cleanups : https://github.com/bevyengine/bevy/pull/11227/files?diff=unified&w=1#diff-b1447f30819280931099e7498b2ced388a98de5cda1b631b493107bb96193cc0R806
• [x] ⚠️ UpdateMode::Reactive is broken
  • https://github.com/bevyengine/bevy/issues/11235
  • https://github.com/bevyengine/bevy/issues/11320
    • Fixed in https://github.com/bevyengine/bevy/pull/11325
• [ ] 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 retrieve exact layout.
• [ ] We don't receive characters through alt+numpad (e.g. alt + 151 = "ù") anymore ; reproduced on winit example "ime". maybe related to https://github.com/rust-windowing/winit/issues/2945
• [ ] (windows) Window content doesn't refresh at all when resizing. By reading https://github.com/rust-windowing/winit/issues/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.
• [x] (windows) unreleased winit fix for using set_control_flow in AboutToWait https://github.com/rust-windowing/winit/issues/3215
  • Fixed in winit 0.29.4

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:

• [ ] Cleanup CanvasParentResizePlugin (https://github.com/bevyengine/bevy/pull/10702#discussion_r1417068856)
  • [x] Remove CanvasParentResizePlugin https://github.com/bevyengine/bevy/pull/11057
  • [ ] Allow to set canvas properties https://github.com/bevyengine/bevy/issues/11052#issuecomment-1867411523 ; https://github.com/bevyengine/bevy/pull/11057#issuecomment-1877869943
    • 🚧 WIP: https://github.com/bevyengine/bevy/pull/11278
• [ ] avoid mentionning explicitly winit in docs from bevy_window ?
• [ ] NamedKey integration on bevy_input: https://github.com/rust-windowing/winit/pull/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 https://github.com/bevyengine/bevy/pull/10702#pullrequestreview-1748336313
  • [x] Done: https://github.com/bevyengine/bevy/pull/11400
  • [ ] Update examples to use that feature (context: https://github.com/bevyengine/bevy/pull/11400#issuecomment-1898768973)
• [ ] https://github.com/bevyengine/bevy/pull/9905 added a workaround on a bug allegedly fixed by winit 0.29. We should check if it's still necessary.
• [x] update to raw_window_handle 0.6
  • Done: https://github.com/bevyengine/bevy/pull/11280
• ~Rename KeyCode to PhysicalKeyCode https://github.com/bevyengine/bevy/pull/10702#discussion_r1404595015~
  • ~dropped: https://github.com/bevyengine/bevy/pull/11692~
• [x] remove instant dependency, replaced by web_time), we'd need to update to fastrand >= 2.0.
  • With the asset rework we don't depend on fastrand anymore.
• [ ] Verify license, see discussion
  • 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.
• [x] investigate this unwrap (winit_window.canvas().unwrap();)
  • Removed with https://github.com/bevyengine/bevy/pull/11065
• [ ] Call pre_present_notify https://github.com/bevyengine/bevy/issues/11052#issuecomment-1900024414
• Use more good things about winit's update
  • [ ] https://github.com/bevyengine/bevy/pull/10689#issuecomment-1823560428
  • [ ] https://github.com/bevyengine/bevy/issues/11229

[daxpedda]

• (windows) unreleased winit fix for using set_control_flow in AboutToWait Windows: set_control_flow has no effect from AboutToWait event rust-windowing/winit#3215 ; ⚠️ I'm not sure what the implications are, but that feels bad 🤔

This has been addressed in Winit v0.29.4.

• investigate this unwrap (winit_window.canvas().unwrap();)

See https://github.com/bevyengine/bevy/pull/10702#discussion_r1403453383.

EDIT: This part of the code will go away in #11065.

---

One thing I believe was overlooked is the missing documentation on setting a CSS size on the canvas. This will become a major footgun for any deployments targeting any platform with a scale factor that's not 1.0.

FWIW: Window::fit_canvas_to_parent was still useful to apply the CSS width: 100%; height: 100%, something similar should be introduced again (without all the resizing stuff, it should literally only apply these CSS values).

[rniii]

Could that property be bought back for setting the CSS? Right now, the canvas seems to still get an inline style with width/height (and max-/min-) set, so you also need to use !important on your stylesheet.

Another thing that seems to have went unnoticed is that canvas elements default to display: inline, which causes them to not exactly fit in the parent element and overflow, so this option could also add display: block (or it could be always there).

Either way, I do agree it should be at least documented if not added.

[daxpedda]

I didn't review #10702, but looking at it now there is at least one big problem: AboutToWait is used to update and draw. This can't work unfortunately, because AboutToWait has no relationship with time or drawing or anything really, it's wholly dependent on user input and other various events, it could fire once per second or a million times.

EDIT: I looked into it a bit further, and as far as I can see Bevy has some system in place to update based on RedrawRequested, but still do the redrawing in AboutToWait. This could be fine, but unfortunately I couldn't find any reference to WindowEvent::RedrawRequested, so I assume that this was missed during the update.

The only meaning ascribed to it should be that there are currently no further events. It is recommended to draw either based on your own timings (maybe in combination with ControlFlow::WaitUntil), or simply use Window::request_redraw(), which has to be called every frame to queue the next one, and then draw on RedrawRequested.

If Bevy wants to also update exactly as fast as it draws, then it can do the update in RedrawRequested as well.

My assumption is that this is the culprit behind https://github.com/bevyengine/bevy/issues/11122 and https://github.com/rust-windowing/winit/issues/1418 (which is linked by a bunch of Bevy issues).

Please feel free to tag me at any Winit related issues/PRs, I'm happy to chime in or review them.

[miketwenty1]

@daxpedda @rniii Not having fit_canvas_to_parent available has made things very difficult/weird for me. I'm creating my js/wasm files with wasm-bindgen and it's a bit of black box. I don't have access to directly manipulate the canvas element, so I made a sort of hacky js work around. However, even after resizing my canvas to full width/height it's creating issues for mobile I think alluded to by:

This will become a major footgun for any deployments targeting any platform with a scale factor that's not 1.0.

Not sure if this is alluding to scale factor / aspect ratio for mobile resolution vs viewport?

Are there any issues with reverting back this change until something better is possible? Fwiw fit_canvas_to_parent was working very well for me on mobile and desktop.

[daxpedda]

web-sys = { version = "0.3", features = [
	"CssStyleDeclaration",
	"Document",
	"HtmlCanvasElement",
	"Window",
] }

fn fit_canvas_to_parent(windows: Query<&Window, With<PrimaryWindow>>) {
    let window = windows.single();
    let canvas: HtmlCanvasElement = web_sys::window()
        .unwrap()
        .document()
        .unwrap()
        .query_selector(window.canvas().unwrap())
        .unwrap()
        .unwrap();
    let style = canvas.style();
    style.set_property("width", "100%").unwrap();
    style.set_property("height", "100%").unwrap();
}

This should do the trick, please let me know if this doesn't work out for you.

[miketwenty1]

@daxpedda I'm getting this error with copy-pasta

275 |       let canvas: HtmlCanvasElement = web_sys::window()
    |  _________________-----------------___^
    | |                 |
    | |                 expected due to this
276 | |         .unwrap()
277 | |         .document()
278 | |         .unwrap()
279 | |         .query_selector(window.canvas().unwrap())
280 | |         .unwrap()
281 | |         .unwrap();
    | |_________________^ expected `HtmlCanvasElement`, found `Element`

Changing HtmlCanvasElement to Element causes this error: no method named canvasfound for reference&bevy::prelude::Window in the current scope field, not a method

This is my web-sys dep:

[dependencies.web-sys]
version = "0.3"
features = [
    "CssStyleDeclaration",
    "Document",
    "HtmlCanvasElement",
    "Clipboard",
    "Window",
    "Navigator",
    "Permissions",
]

[daxpedda]

So I actually tried out my suggestion and fixed it up in https://github.com/bevyengine/bevy/issues/11052#issuecomment-1879322238. Unfortunately I can see now that Bevy doesn't actually populate Window::canvas, so this should probably be removed now as well.

So this is what I tried and I can confirm it works:

use web_sys::HtmlCanvasElement;
use wasm_bindgen::JsCast;

fn fit_canvas_to_parent() {
    let canvas: HtmlCanvasElement = web_sys::window()
        .unwrap()
        .document()
        .unwrap()
        .query_selector("canvas")
        .unwrap()
        .unwrap()
        .unchecked_into();
    let style = canvas.style();
    style.set_property("width", "100%").unwrap();
    style.set_property("height", "100%").unwrap();
}

also add the following to your App:

        .add_systems(Startup, fit_canvas_to_parent)

[miketwenty1]

@daxpedda Things seem to be much better doing this. On mobile though the cursor/touch position and where the mouse/touch is.. is off while running off main.. and gets worse the further from top left of the screen i go. Any ideas here? Possibly unrelated but thought i should check.

view port in my metadata is: <meta name="viewport" content="width=device-width, initial-scale=1.0">

I'm guessing this is a viewport issue scale issue not resizing for mobile now for some reason. I'm assuming the fit_canvas_to_parent was doing something here to address this? only me speculating.

[daxpedda]

What browser and OS are you using? Could you also tell me the output of window.devicePixelRatio by entering that in the JS console?

This might be a Winit bug, but we supposedly fixed that a long while ago.

I'm assuming the fit_canvas_to_parent was doing something here to address this?

I don't think so, but maybe Bevy was adjusting these positions somewhere to fix the old bug in Winit?

[miketwenty1]

tested on a bunch devicePixelRatio on old build with bevy 12.1 and new build with main. On old build pretty much any devicePixelRatio will be good.

On new build given the following config I've setup with you, only window.devicePixelRatio resulting in 2 will work.

[miketwenty1]

I think this is for sure a bug. @daxpedda so I opened an issue. Still not 100% sure if it's related to this or something else. https://github.com/bevyengine/bevy/issues/11231

[alice-i-cecile]

Another near duplicate: https://github.com/bevyengine/bevy/issues/11320.

[daxpedda]

Following up on https://github.com/bevyengine/bevy/pull/11227#discussion_r1443818934: https://github.com/bevyengine/bevy/blob/83d6600267f54faa9ead083516c9f42c16491ed5/crates/bevy_render/src/renderer/mod.rs#L74-L77 https://github.com/gfx-rs/wgpu/pull/5093 seems to now clarify whats going on here, so Window::pre_present_notify() should be called here.

[tripokey]

PSA: Removing the instant dependency seems to trigger the following error (in the browsers web console) if you happen to have instant somewhere else in your dependency tree:

Uncaught TypeError: Failed to resolve module specifier "env". Relative references must start with either "/", "./", or "../".

This happens because bevy previously enabled the wasm-bindgen feature for the instant crate which hides the error.

A workaround for this issue is to add the instant crate to your application with the wasm-bindgen feature enabled.

[tripokey]

So I actually tried out my suggestion and fixed it up in #11052 (comment). Unfortunately I can see now that Bevy doesn't actually populate Window::canvas, so this should probably be removed now as well.

So this is what I tried and I can confirm it works:

use web_sys::HtmlCanvasElement;
use wasm_bindgen::JsCast;

fn fit_canvas_to_parent() {
    let canvas: HtmlCanvasElement = web_sys::window()
        .unwrap()
        .document()
        .unwrap()
        .query_selector("canvas")
        .unwrap()
        .unwrap()
        .unchecked_into();
    let style = canvas.style();
    style.set_property("width", "100%").unwrap();
    style.set_property("height", "100%").unwrap();
}

also add the following to your App:

        .add_systems(Startup, fit_canvas_to_parent)

@daxpedda thanks for this, it saved me since the information is not present in the migration guide: https://bevyengine.org/learn/migration-guides/0-12-to-0-13/#remove-canvasparentresizeplugin

[ThierryBerger]

@tripokey thanks for commenting your experience, sorry about the migration, I think https://github.com/bevyengine/bevy/pull/11278 would have helped, can you weigh in ?

[tripokey]

@Vrixyz I merged your MR on top of the 0.13 release and replace the manual fit_canvas_to_parent system with the fit_canvas_to_parent Window settings. And it seems to work.

[nuzzles]

PSA: Removing the instant dependency seems to trigger the following error (in the browsers web console) if you happen to have instant somewhere else in your dependency tree:

Uncaught TypeError: Failed to resolve module specifier "env". Relative references must start with either "/", "./", or "../".

This happens because bevy previously enabled the wasm-bindgen feature for the instant crate which hides the error.

A workaround for this issue is to add the instant crate to your application with the wasm-bindgen feature enabled.

I'm really interested to know how you discovered this. I found your comment and thank gosh I did! Is this being tracked anywhere?

[tripokey]

PSA: Removing the instant dependency seems to trigger the following error (in the browsers web console) if you happen to have instant somewhere else in your dependency tree:

Uncaught TypeError: Failed to resolve module specifier "env". Relative references must start with either "/", "./", or "../".

This happens because bevy previously enabled the wasm-bindgen feature for the instant crate which hides the error. A workaround for this issue is to add the instant crate to your application with the wasm-bindgen feature enabled.

I'm really interested to know how you discovered this. I found your comment and thank gosh I did! Is this being tracked anywhere?

I just got lucky googling the error and found: https://github.com/rustwasm/wasm-bindgen/discussions/3500#discussioncomment-6346681

Then double checked my Cargo.lock file and Bevy's dependencies 😅