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

Implemented support for creating a render surface from a canvas or offscreencanvas #6147

Closed
wants to merge 6 commits into from

Conversation

anlumo
Copy link

@anlumo anlumo commented Oct 2, 2022

Objective

  • bevy uses raw-window-handle for creating a rendering context. That crate does not really work for the web target (issue).

Solution


Changelog

  • Added two new variants to the enum called bevy_window::window::AbstractWindowHandle that can take either an HtmlCanvasElement or OffscreenCanvas.
  • Added three new functions to bevy_window::window::Window:
    • new_canvas for creating a Window based on a canvas element directly.
    • new_canvas_selector for creating a Window based on a CSS-style selector.
    • new_offscreen_canvas for creating a Window with an offscreen canvas.
  • Added WindowDescriptor::set_canvas_from_selector and WindowDescriptor::set_canvas to allow winit to use a canvas element directly instead of creating its own (which is still supported).
  • All of this only applies to the wasm target! winit only supports a regular HtmlCanvasElement, but not an OffscreenCanvas. This is a limitation of winit.

Migration Guide

  • The canvas field in Window does no longer exists for non-wasm targets.
  • If you want to specify a canvas for winit, you now have to use WindowDescriptor::set_canvas_from_selector or WindowDescriptor::set_canvas instead.

@rparrett rparrett added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in O-Web Specific to web (WASM) builds labels Oct 2, 2022
@mockersf
Copy link
Member

mockersf commented Oct 2, 2022

How does this relate to the existing canvas field?

It's using with_canvas on the window builder from winit to use that canvas

@anlumo
Copy link
Author

anlumo commented Oct 2, 2022

How does this relate to the existing canvas field?

The existing canvas field is a String, which is not helpful for referencing a JsValue. I thought about just replacing that field, but that would potentially break existing code.

It's using with_canvas on the window builder from winit to use that canvas

The goal of this PR is to allow OffscreenCanvas usage in bevy, mostly for moving bevy into a Web Worker and thus avoiding blocking the UI thread with rendering operations. This is not something winit supports (rust-windowing/winit#1518).

btw, I just confirmed that this is working with bevy running in a web worker in a test application using this PR.

@anlumo
Copy link
Author

anlumo commented Oct 2, 2022

Here is a minimal example for using bevy in a web worker using this PR: https://github.com/anlumo/bevy-webworker-test

@alice-i-cecile
Copy link
Member

I thought about just replacing that field, but that would potentially break existing code.

This PR is out of my area of expertise, but if you think this is a better solution, just replace that field. We're deliberately open to breaking changes to improve the API at this stage.

@mockersf
Copy link
Member

mockersf commented Oct 2, 2022

I agree with Alice, breaking is not an issue, but having the api offers two ways around the same thing isn't nice.

This api seems a little harder to use for the simple case of just passing the canvas id though.

Here is a minimal example for using bevy in a web worker using this PR: https://github.com/anlumo/bevy-webworker-test

Thanks ! That's quite interesting

@anlumo
Copy link
Author

anlumo commented Oct 3, 2022

This api seems a little harder to use for the simple case of just passing the canvas id though.

True, I've added a WindowDescriptor::set_canvas_from_selector function to simplify the transition. It's simply a wrapper around web_sys::Window::query_selector that sets the same member of the struct.

I've also removed the old selector-based entry!

@rparrett
Copy link
Contributor

rparrett commented Oct 3, 2022

The decision to include canvas and fit_canvas_to_parent in non-web targets was intentional.

See #4726 and #4683

@anlumo
Copy link
Author

anlumo commented Oct 3, 2022

The decision to include canvas and fit_canvas_to_parent in non-web targets was intentional.

That's unfortunately no longer possible once it's using a web-sys type for canvas, and since it's already a special case there's no point to include the other flag on other platforms.

An alternative would be to have a fake flag there for non-wasm32, like a PhantomData<()>, an Option<()> or bool. Would that be an acceptable solution?

@rparrett
Copy link
Contributor

rparrett commented Oct 3, 2022

I doubt it's a big deal if it's unavoidable, just wanted to surface that history.

@anlumo
Copy link
Author

anlumo commented Oct 14, 2022

Needs some testing from my side before I can remove the draft flag.

@anlumo anlumo marked this pull request as ready for review October 16, 2022 21:23
@anlumo anlumo force-pushed the offscreen-canvas branch 3 times, most recently from eb401a5 to 4190609 Compare October 16, 2022 21:33
@anlumo anlumo marked this pull request as draft October 16, 2022 22:05
Virtual windows will by default not have a surface texture associated to
them, but implementors can set the texture in `ExtractedWindow`
manually.

This is intended to be used when embedding games into other appications
like editors or for running games headless.
…creencanvas elements on the wasm target. This only works when not using winit.
…ent. However, the bevy API doesn't allow for that yet.
@anlumo
Copy link
Author

anlumo commented Oct 18, 2022

Rebased to new version of #6256 and also added winit support (without OffscreenCanvas, because winit doesn't support that at all right now).

@anlumo
Copy link
Author

anlumo commented Oct 20, 2022

Holding off on this until #6256 is merged, since it changes around all the time at the moment, causing merge conflicts.

@richchurcher
Copy link
Contributor

Backlog cleanup: closing due to inactivity, and seeming lack of consensus. Lots of reorganisation and refactoring happening in the render space at the moment, likely to have significant bit rot.

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-Bug An unexpected or incorrect behavior O-Web Specific to web (WASM) builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants