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

Use ResizeObserver to control the size of the canvas #2074

Closed
wants to merge 10 commits into from

Conversation

Liamolucko
Copy link
Contributor

@Liamolucko Liamolucko commented Nov 27, 2021

  • Tested on all platforms changed

Specifically on Firefox Developer Edition and Safari.

  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • 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

  • Wait for web-sys to release a new version
  • 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 Liamolucko changed the title Resize observer2 Use ResizeObserver to control the size of the canvas Nov 27, 2021
@maxammann
Copy link

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

@rukai
Copy link
Contributor

rukai commented Dec 23, 2021

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

@Liamolucko
Copy link
Contributor Author

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.

@maroider
Copy link
Member

maroider commented Jan 9, 2022

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.

@stephanemagnenat
Copy link

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.

@rukai
Copy link
Contributor

rukai commented Feb 20, 2022

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();

@maroider
Copy link
Member

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".

@Liamolucko Liamolucko marked this pull request as ready for review February 25, 2022 07:15
@Liamolucko
Copy link
Contributor Author

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?

@maroider maroider added the C - waiting on maintainer A maintainer must review this code label Feb 25, 2022
@maroider maroider self-requested a review February 25, 2022 07:23
@rukai rukai mentioned this pull request Feb 26, 2022
5 tasks
bors bot pushed a commit to bevyengine/bevy that referenced this pull request May 20, 2022
Currently Bevy's web canvases are "fixed size". They are manually set to specific dimensions. This might be fine for some games and website layouts, but for sites with flexible layouts, or games that want to "fill" the browser window, Bevy doesn't provide the tools needed to make this easy out of the box.

There are third party plugins like [bevy-web-resizer](https://github.com/frewsxcv/bevy-web-resizer/) that listen for window resizes, take the new dimensions, and resize the winit window accordingly. However this only covers a subset of cases and this is common enough functionality that it should be baked into Bevy.

A significant motivating use case here is the [Bevy WASM Examples page](https://bevyengine.org/examples/). This scales the canvas to fit smaller windows (such as mobile). But this approach both breaks winit's mouse events and removes pixel-perfect rendering (which means we might be rendering too many or too few pixels).  bevyengine/bevy-website#371

In an ideal world, winit would support this behavior out of the box. But unfortunately that seems blocked for now: rust-windowing/winit#2074. And it builds on the ResizeObserver api, which isn't supported in all browsers yet (and is only supported in very new versions of the popular browsers).

While we wait for a complete winit solution, I've added a `fit_canvas_to_parent` option to WindowDescriptor / Window, which when enabled will listen for window resizes and resize the Bevy canvas/window to fit its parent element. This enables users to scale bevy canvases using arbitrary CSS, by "inheriting" their parents' size. Note that the wrapper element _is_ required because winit overrides the canvas sizing with absolute values on each resize.

There is one limitation worth calling out here: while the majority of  canvas resizes will be triggered by window resizes, modifying element layout at runtime (css animations, javascript-driven element changes, dev-tool-injected changes, etc) will not be detected here. I'm not aware of a good / efficient event-driven way to do this outside of the ResizeObserver api. In practice, window-resize-driven canvas resizing should cover the majority of use cases. Users that want to actively poll for element resizes can just do that (or we can build another feature and let people choose based on their specific needs).

I also took the chance to make a couple of minor tweaks:
* Made the `canvas` window setting available on all platforms. Users shouldn't need to deal with cargo feature selection to support web scenarios. We can just ignore the value on non-web platforms. I added documentation that explains this.
*  Removed the redundant "initial create windows" handler. With the addition of the code in this pr, the code duplication was untenable.

This enables a number of patterns:

## Easy "fullscreen window" mode for the default canvas

The "parent element" defaults to the `<body>` element.

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    ..default()
  })
``` 
And CSS:
```css
html, body {
    margin: 0;
    height: 100%;
}
```

## Fit custom canvas to "wrapper" parent element

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    canvas: Some("#bevy".to_string()),
    ..default()
  })
``` 
And the HTML:
```html
<div style="width: 50%; height: 100%">
  <canvas id="bevy"></canvas>
</div>
```
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
…gine#4726)

Currently Bevy's web canvases are "fixed size". They are manually set to specific dimensions. This might be fine for some games and website layouts, but for sites with flexible layouts, or games that want to "fill" the browser window, Bevy doesn't provide the tools needed to make this easy out of the box.

There are third party plugins like [bevy-web-resizer](https://github.com/frewsxcv/bevy-web-resizer/) that listen for window resizes, take the new dimensions, and resize the winit window accordingly. However this only covers a subset of cases and this is common enough functionality that it should be baked into Bevy.

A significant motivating use case here is the [Bevy WASM Examples page](https://bevyengine.org/examples/). This scales the canvas to fit smaller windows (such as mobile). But this approach both breaks winit's mouse events and removes pixel-perfect rendering (which means we might be rendering too many or too few pixels).  bevyengine/bevy-website#371

In an ideal world, winit would support this behavior out of the box. But unfortunately that seems blocked for now: rust-windowing/winit#2074. And it builds on the ResizeObserver api, which isn't supported in all browsers yet (and is only supported in very new versions of the popular browsers).

While we wait for a complete winit solution, I've added a `fit_canvas_to_parent` option to WindowDescriptor / Window, which when enabled will listen for window resizes and resize the Bevy canvas/window to fit its parent element. This enables users to scale bevy canvases using arbitrary CSS, by "inheriting" their parents' size. Note that the wrapper element _is_ required because winit overrides the canvas sizing with absolute values on each resize.

There is one limitation worth calling out here: while the majority of  canvas resizes will be triggered by window resizes, modifying element layout at runtime (css animations, javascript-driven element changes, dev-tool-injected changes, etc) will not be detected here. I'm not aware of a good / efficient event-driven way to do this outside of the ResizeObserver api. In practice, window-resize-driven canvas resizing should cover the majority of use cases. Users that want to actively poll for element resizes can just do that (or we can build another feature and let people choose based on their specific needs).

I also took the chance to make a couple of minor tweaks:
* Made the `canvas` window setting available on all platforms. Users shouldn't need to deal with cargo feature selection to support web scenarios. We can just ignore the value on non-web platforms. I added documentation that explains this.
*  Removed the redundant "initial create windows" handler. With the addition of the code in this pr, the code duplication was untenable.

This enables a number of patterns:

## Easy "fullscreen window" mode for the default canvas

The "parent element" defaults to the `<body>` element.

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    ..default()
  })
``` 
And CSS:
```css
html, body {
    margin: 0;
    height: 100%;
}
```

## Fit custom canvas to "wrapper" parent element

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    canvas: Some("#bevy".to_string()),
    ..default()
  })
``` 
And the HTML:
```html
<div style="width: 50%; height: 100%">
  <canvas id="bevy"></canvas>
</div>
```
@rukai
Copy link
Contributor

rukai commented Jul 10, 2022

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...?

@Liamolucko
Copy link
Contributor Author

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.

@rukai
Copy link
Contributor

rukai commented Sep 4, 2022

#2209 has merged, so it would be good to rebase/merge to make it easy to test on the new example.

@marcofugaro
Copy link

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

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I gave parts of it a cursory look, but this is a huge change and I'm not well-versed enough in web technologies to really tell if things are correct or not.

Sadly, we currently don't have a maintainer for the web platform, so the best way forwards @marcofugaro and other interested parties, would be to review the changeset with your detail-oriented eyes on (adding a simple "approving" review via. GitHub is also data), and test it in edge-cases (e.g. also when the css-size feature is not enabled, or when changing monitors and the like).

That would give me more confidence that the change is correct, and I would be much more willing to merge it then.

Comment on lines +143 to +147
let prop = |name| -> f64 {
let value = style.get_property_value(name).unwrap();
// Cut off the last two characters to remove the `px` from the end.
value[..value.len() - 2].parse().unwrap()
};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Deduplicate this helper closure into a function (since it's the same as above).

let window = web_sys::window().unwrap();
let style = window
.get_computed_style(raw)
// This can't fail according to the spec; I don't know why web-sys marks it as throwing and having an optional result.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should open an issue in web-sys about it, then?

Comment on lines +253 to +254
// I don't think it's possible for an element to become fullscreen whilst not being in the DOM.
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Use expect with such a message instead

if self.0.all_canvases.borrow().is_empty() {
/// Handle a change in scale factor, without any other information.
///
/// `initial` is whether this is coming from the initial `ScaleChangeDetector`, rather than the `ResizeObserver`.
Copy link
Member

Choose a reason for hiding this comment

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

Flags like this is IMHO usually bad practice, perhaps things can be refactored slightly to avoid it? (concretely: I have trouble immediately parsing the !(initial && observer_will_run) conditional).

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…gine#4726)

Currently Bevy's web canvases are "fixed size". They are manually set to specific dimensions. This might be fine for some games and website layouts, but for sites with flexible layouts, or games that want to "fill" the browser window, Bevy doesn't provide the tools needed to make this easy out of the box.

There are third party plugins like [bevy-web-resizer](https://github.com/frewsxcv/bevy-web-resizer/) that listen for window resizes, take the new dimensions, and resize the winit window accordingly. However this only covers a subset of cases and this is common enough functionality that it should be baked into Bevy.

A significant motivating use case here is the [Bevy WASM Examples page](https://bevyengine.org/examples/). This scales the canvas to fit smaller windows (such as mobile). But this approach both breaks winit's mouse events and removes pixel-perfect rendering (which means we might be rendering too many or too few pixels).  bevyengine/bevy-website#371

In an ideal world, winit would support this behavior out of the box. But unfortunately that seems blocked for now: rust-windowing/winit#2074. And it builds on the ResizeObserver api, which isn't supported in all browsers yet (and is only supported in very new versions of the popular browsers).

While we wait for a complete winit solution, I've added a `fit_canvas_to_parent` option to WindowDescriptor / Window, which when enabled will listen for window resizes and resize the Bevy canvas/window to fit its parent element. This enables users to scale bevy canvases using arbitrary CSS, by "inheriting" their parents' size. Note that the wrapper element _is_ required because winit overrides the canvas sizing with absolute values on each resize.

There is one limitation worth calling out here: while the majority of  canvas resizes will be triggered by window resizes, modifying element layout at runtime (css animations, javascript-driven element changes, dev-tool-injected changes, etc) will not be detected here. I'm not aware of a good / efficient event-driven way to do this outside of the ResizeObserver api. In practice, window-resize-driven canvas resizing should cover the majority of use cases. Users that want to actively poll for element resizes can just do that (or we can build another feature and let people choose based on their specific needs).

I also took the chance to make a couple of minor tweaks:
* Made the `canvas` window setting available on all platforms. Users shouldn't need to deal with cargo feature selection to support web scenarios. We can just ignore the value on non-web platforms. I added documentation that explains this.
*  Removed the redundant "initial create windows" handler. With the addition of the code in this pr, the code duplication was untenable.

This enables a number of patterns:

## Easy "fullscreen window" mode for the default canvas

The "parent element" defaults to the `<body>` element.

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    ..default()
  })
``` 
And CSS:
```css
html, body {
    margin: 0;
    height: 100%;
}
```

## Fit custom canvas to "wrapper" parent element

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    canvas: Some("#bevy".to_string()),
    ..default()
  })
``` 
And the HTML:
```html
<div style="width: 50%; height: 100%">
  <canvas id="bevy"></canvas>
</div>
```
@toji
Copy link

toji commented Apr 29, 2023

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!

@mcclure
Copy link

mcclure commented Apr 29, 2023

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!

@maroider
Copy link
Member

maroider commented May 17, 2023

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.

@daxpedda
Copy link
Member

I'm happy to get this PR sorted out and rebased after #2789.

@mcclure
Copy link

mcclure commented May 17, 2023

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.

@daxpedda
Copy link
Member

daxpedda commented Jun 5, 2023

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 rustwasm/wasm-bindgen#3459 to avoid this whole issue though. So we should probably wait for that.

@daxpedda
Copy link
Member

daxpedda commented Jun 7, 2023

Replaced by #2859.

@daxpedda daxpedda closed this Jun 7, 2023
Subserial pushed a commit to Subserial/bevy_winit_hook that referenced this pull request Jan 24, 2024
Currently Bevy's web canvases are "fixed size". They are manually set to specific dimensions. This might be fine for some games and website layouts, but for sites with flexible layouts, or games that want to "fill" the browser window, Bevy doesn't provide the tools needed to make this easy out of the box.

There are third party plugins like [bevy-web-resizer](https://github.com/frewsxcv/bevy-web-resizer/) that listen for window resizes, take the new dimensions, and resize the winit window accordingly. However this only covers a subset of cases and this is common enough functionality that it should be baked into Bevy.

A significant motivating use case here is the [Bevy WASM Examples page](https://bevyengine.org/examples/). This scales the canvas to fit smaller windows (such as mobile). But this approach both breaks winit's mouse events and removes pixel-perfect rendering (which means we might be rendering too many or too few pixels).  bevyengine/bevy-website#371

In an ideal world, winit would support this behavior out of the box. But unfortunately that seems blocked for now: rust-windowing/winit#2074. And it builds on the ResizeObserver api, which isn't supported in all browsers yet (and is only supported in very new versions of the popular browsers).

While we wait for a complete winit solution, I've added a `fit_canvas_to_parent` option to WindowDescriptor / Window, which when enabled will listen for window resizes and resize the Bevy canvas/window to fit its parent element. This enables users to scale bevy canvases using arbitrary CSS, by "inheriting" their parents' size. Note that the wrapper element _is_ required because winit overrides the canvas sizing with absolute values on each resize.

There is one limitation worth calling out here: while the majority of  canvas resizes will be triggered by window resizes, modifying element layout at runtime (css animations, javascript-driven element changes, dev-tool-injected changes, etc) will not be detected here. I'm not aware of a good / efficient event-driven way to do this outside of the ResizeObserver api. In practice, window-resize-driven canvas resizing should cover the majority of use cases. Users that want to actively poll for element resizes can just do that (or we can build another feature and let people choose based on their specific needs).

I also took the chance to make a couple of minor tweaks:
* Made the `canvas` window setting available on all platforms. Users shouldn't need to deal with cargo feature selection to support web scenarios. We can just ignore the value on non-web platforms. I added documentation that explains this.
*  Removed the redundant "initial create windows" handler. With the addition of the code in this pr, the code duplication was untenable.

This enables a number of patterns:

## Easy "fullscreen window" mode for the default canvas

The "parent element" defaults to the `<body>` element.

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    ..default()
  })
``` 
And CSS:
```css
html, body {
    margin: 0;
    height: 100%;
}
```

## Fit custom canvas to "wrapper" parent element

```rust
app
  .insert_resource(WindowDescriptor {
    fit_canvas_to_parent: true,
    canvas: Some("#bevy".to_string()),
    ..default()
  })
``` 
And the HTML:
```html
<div style="width: 50%; height: 100%">
  <canvas id="bevy"></canvas>
</div>
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - web
Development

Successfully merging this pull request may close these issues.

Allow canvas size to be controlled by page layout on web target