winit icon indicating copy to clipboard operation
winit copied to clipboard

Use `ResizeObserver` to control the size of the canvas

Open Liamolucko opened this issue 2 years ago • 11 comments

  • [X] Tested on all platforms changed

Specifically on Firefox Developer Edition and Safari.

  • [X] Compilation warnings were addressed
  • [X] cargo fmt has been run on this branch
  • [X] cargo doc builds successfully
  • [X] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • [X] Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • [ ] Created or updated an example program if it would help users understand this functionality
  • [ ] ~~Updated feature matrix, if new features were added or implemented~~

Resolves #1661

I've changed the web backend to set the window size entirely based on the canvas' CSS size using ResizeObserver. It uses device-pixel-content-box where available, but falls back on ResizeObserverEntry.contentRect if it's not supported.

One issue is how to handle the window's initial size. ResizeObserver does fire an event for the initial size when created, but it won't run until the task finishes, which means it isn't available for any init code before the event loop starts. I've worked around this by just getting the element's computed size and subtracting padding, and using that to estimate its size for the initial inner_size value, but this doesn't work at all if the canvas is created by winit and then added to the DOM. If a canvas isn't in the DOM, its size is reported as 0, so if the canvas is created this way its initial size will always be 0.

At the moment this has to pull in a git version of web-sys and enable cfg(web_sys_unstable_apis), so it can't really be shipped. The API does seem to be supported in pretty much all modern browsers, but it's marked unstable since it's a draft.

To-do

  • [x] Wait for web-sys to release a new version
  • [X] Either wait for ResizeObserver to be stabilised or feature gate this somehow

Unresolved questions

  • [ ] Should this be hidden behind a mode, instead of always enabled? Applications which already resize manually using set_inner_size should work functionally identically anyway; this should only really affect things which don't touch the size at all.
  • [ ] Should setting min/max size set the CSS min/max-width/height properties?
  • [ ] Should outer size refer to the canvas' border box? Now that CSS can be freely applied to the canvas, it's possible for it to have padding or borders, in which case the element's outer size might not be the same as its content size.

Liamolucko avatar Nov 27 '21 00:11 Liamolucko

Looks very cool! Let me know if you need help testing.

maxammann avatar Dec 11 '21 21:12 maxammann

I tried this branch on my app and my app blows up in wgpu because I'm giving it a window with size of 0x0. Maybe we should we be respecting https://docs.rs/winit/latest/winit/window/struct.Window.html#method.set_min_inner_size or at least hardcode a minimum size of 1x1 ?

Would also be good to rebase, to make using the PR via [patch.crates-io] easier.

You can reproduce the problem by cloning https://github.com/rukai/wgpu/tree/resize_observer and then running ./run-wasm-example.sh hello-triangle webgl

rukai avatar Dec 23 '21 02:12 rukai

I tried this branch on my app and my app blows up in wgpu because I'm giving it a window with size of 0x0. Maybe we should we be respecting https://docs.rs/winit/latest/winit/window/struct.Window.html#method.set_min_inner_size or at least hardcode a minimum size of 1x1 ?

That issue was occuring because the window's initial size was being measured in Window::new, before the canvas was added to the DOM. At that point, the canvas's size is reported as 0x0. To work around that, I've changed it to measure the canvas's initial size on the first call to Window::inner_size instead, if it isn't in the DOM during Window::new.

Liamolucko avatar Jan 07 '22 01:01 Liamolucko

Maybe we should we be respecting https://docs.rs/winit/latest/winit/window/struct.Window.html#method.set_min_inner_size or at least hardcode a minimum size of 1x1?

Getting resize events that tell you the window is 0x0 is currently expected behaviour, so user code generally has to check for it before passing the size on to wgpu anyway. This is, however, a common stumbling block, so maybe there's room for changing the API to guide users toward something that works by default.

maroider avatar Jan 09 '22 15:01 maroider

Any status update on this PR? The bug it solves is likely to hit pretty much everyone using Rust with GPU on the web (through Bevy or something else), so even an imperfect implementation that relies on some unstable-but-widely-available feature is certainly much better than the status quo.

stephanemagnenat avatar Feb 15 '22 16:02 stephanemagnenat

I tested again and it now works perfectly for my application!

Thanks so much for implementing this, I would love to see it merged under a feature flag before ResizeObserver is stabilized.

For anyone else wanting to make use of this, here is the html, css and rust I used to get value out of this. The result is a winit app that fills all the available horizontal space at a 4/3 aspect ratio.

<div id="render">
    <canvas gets inserted here by rust code>
</div>
#render {
    width: 100%;
    aspect-ratio: 4 / 3;
}
        let window = Window::new(&event_loop).unwrap();

        let canvas = window.canvas();
        canvas
            .style()
            .set_css_text("display: block; width: 100%; height: 100%");

        document.get_element_by_id("render").unwrap()
            .append_child(&web_sys::Element::from(canvas))
            .unwrap();

rukai avatar Feb 20 '22 04:02 rukai

Given that the spec for the API in question is still a "working draft", I'm a bit hesitant to enable it by default, despite its "wide availablity".

maroider avatar Feb 24 '22 08:02 maroider

I've put the ResizeObserver functionality behind a css-size feature, so that this can be used without everyone having to enable cfg(web_sys_unstable_apis).

I've noticed a bit of an issue, though - if the scale factor is changed before the event loop starts, it'll panic, since it tries to handle the event synchronously when the event loop isn't running. It used to just not check for them at all before the event loop started, but that's not great either. How do other platforms handle this?

Liamolucko avatar Feb 25 '22 07:02 Liamolucko

From looking at the table here: https://developer.mozilla.org/en-US/docs/Web/API/Resize_Observer_API#browser_compatibility It looks like all the unstable API's we need have been stabilized? Can wasm-bindgen be updated accordingly and this PR progressed?

Maybe wasm-bindgen has stronger requirements for it to be considered stable though...?

rukai avatar Jul 10 '22 08:07 rukai

It was marked unstable because the spec is a 'Working Draft', which isn't technically a real spec yet. web-sys already has plenty of Working Drafts not marked unstable, though (e.g. the File API), so I think it'd be fine to stabilize it.

Liamolucko avatar Jul 10 '22 09:07 Liamolucko

https://github.com/rust-windowing/winit/pull/2209 has merged, so it would be good to rebase/merge to make it easy to test on the new example.

rukai avatar Sep 04 '22 11:09 rukai

This is indispensable for any canvas application compiled to wasm, any progress?

marcofugaro avatar Nov 16 '22 15:11 marcofugaro

Given that the spec for the API in question is still a "working draft", I'm a bit hesitant to enable it by default, despite its "wide availablity".

Pointed here from a conversation on Mastodon. I know effectively nothing about Rust but I am the editor of one or two web standards, so I figured I could help clarify this point at least.

A spec being a "Working Draft" doesn't mean much in terms of how stable, widespread, or standardized the specification has become. Ideally the spec authors want their specification to be granted "Candidate Recommendation" (CR) status by the W3C, but that typically isn't even considered until at least two independent browsers have been shipping compatible implementations of the feature for at least a little while. And even the spec moving from Draft to CR status is a formality that has no real bearing on how browsers, developers, or users approach the feature.

What this means for developers is that CanIUse.com is a far more practical reference than the spec documents for determining what is actually shipped and stable. In this specific case ResizeObserver has been ~universally available since Jan 2020, and is definitely not going to be removed from the web platform or see any breaking changes in the foreseeable future. Chrome usage stats show that over 20% of web pages that users load make use of ResizeObserver, and nobody is eager to break 20% of the web, regardless of whether the spec says "Draft" at the top or not.

Hope that helps clear up some of the (admittedly confusing) verbiage around web specs!

toji avatar Apr 29 '23 02:04 toji

It appears that this code has been working for over a year and the ResizeObserver "draft" API has been unchanged for over three years. Caniuse has ResizeObserver working on everything except IE11.

I was hoping to release some wgpu sample code before Chrome's initial release of webgpu next week (?). --cfg=web_sys_unstable_apis is not a problem for this application because the whole of webgpu is unstable. This PR is somewhere between useful and essential for practical webgpu use. I set the version of winit in my sample code's Cargo.toml ( https://github.com/mcclure/rs-hello 2b3303b / https://data.runhello.com/j/wgpu/2/) to directly pull this branch from github and as-is the PR works great (although it's too bad because I think this means I'm basing my code on a version of winit older than 0.28.3?). I can't speak to your package's stability policy but is there a way some version of this could get into shipping winit versions? The feature is a very good one.

Thanks!

mcclure avatar Apr 29 '23 03:04 mcclure

Thanks for the clarification, @toji. If anyone is willing to pick up work on this again (merge conflicts and whatnot), then I'd be happy to see us use ResizeObserver by default.

I can't speak to your package's stability policy but is there a way some version of this could get into shipping winit versions?

If I'm not mistaken, some fixes do get back-ported, but I don't recall the criteria for it.

maroider avatar May 17 '23 06:05 maroider

I'm happy to get this PR sorted out and rebased after https://github.com/rust-windowing/winit/pull/2789.

daxpedda avatar May 17 '23 09:05 daxpedda

Awesome!

If I'm not mistaken, some fixes do get back-ported, but I don't recall the criteria for it.

Sorry, I meant "can it get in a future shipping version", not that I needed it compatible with any specific shipping version.

mcclure avatar May 17 '23 14:05 mcclure

I think the big problem right now is how to deal with --cfg=web_sys_unstable_apis, it's a pretty terrible user experience, though I wouldn't mind still going through with this as long as it's gated behind that instead of introducing a crate feature that requires it.

I made https://github.com/rustwasm/wasm-bindgen/pull/3459 to avoid this whole issue though. So we should probably wait for that.

daxpedda avatar Jun 05 '23 12:06 daxpedda

Replaced by #2859.

daxpedda avatar Jun 07 '23 20:06 daxpedda