Enable creating "virtual" windows without corresponding OS window #6256

MDeiml · 2022-10-14T12:44:30Z

Objective

Enable creating windows without an associated OS window
- This will be useful for embedding games into other applications like editors
- It will also enable games to be run headless without much change for the specific game

Solution

Make the raw_window_handle attribute an Option
Ignore windows with a raw_window_handle of None in prepare_windows
- Instead, the creator of a virtual window is responsible for creating a render target texture for the window and injecting it into the ExtractedWindow

Changelog

Added a new_virtual method to Window, which does not take a RawWindowHandleWrapper
Changed Window::raw_window_handle to return None if it was created using new_virtual
Changed prepare_windows to ignore windows that were created using new_virtual

alice-i-cecile · 2022-10-14T13:04:38Z

Related to #6114, which makes this field an Option.

#6147 seems to be tackling similar problems as well. Ping @anlumo and @targrub; I'd like to talk through how all of these PRs inter-relate and how we should move forward.

MDeiml · 2022-10-14T14:13:39Z

Oh, I missed #6114, which seems to make almost the same changes with mainly different intention / language around them. It also adds a test example and changes the RenderApp to only run if there is a primary window present (which I'm not sure is the right behavior).

I'd suggest rebasing @targrub's changes on my implementation. My pull request seems to be more minimal and be more open to different use cases. #6114 for example says

/// During normal use, this can be safely unwrapped; the value should only be [`None`] when synthetically constructed for tests.

which rules out the use cases I am suggesting. Of course we should probably mention the use of "virtual windows" for testing and @targrub's test example is also a good addition.

I'm not sure how #6147 fits in. The windows created there do seem to always have a RawWindowHandleWrapper, but it looks like this handle is not used. Maybe it makes sense to change the handle into an enum

enum AbstractWindowHandle {
    Raw(RawWindowHandleWrapper),
    Virtual,
    Canvas(Canvas),
}

instead.

alice-i-cecile · 2022-10-14T14:33:28Z

Sounds good; I'm on board with that course of action.

MDeiml · 2022-10-14T15:02:30Z

I rewrote my code to use an enum instead of Option. I did not include a Canvas variant, but it should be easy for @anlumo to adapt their PR to use the same enum.

anlumo · 2022-10-14T16:02:43Z

I did not include a Canvas variant, but it should be easy for @anlumo to adapt their PR to use the same enum.

I've updated my PR to use your enum instead.

targrub · 2022-10-15T12:33:56Z

Oh, I missed #6114, which seems to make almost the same changes with mainly different intention / language around them. It also adds a test example and changes the RenderApp to only run if there is a primary window present (which I'm not sure is the right behavior).

I've fixed that behavior in a subsequent commit.

I'd suggest rebasing @targrub's changes on my implementation. My pull request seems to be more minimal and be more open to different use cases. #6114 for example says

I'm not familiar with that phrasing; what does that mean, "rebasing @targrub's changes on my implementation"?

/// During normal use, this can be safely unwrapped; the value should only be [None] when synthetically constructed for tests.

That comment was written by @alice-i-cecile . :)

MDeiml · 2022-10-15T12:56:04Z

I'm not familiar with that phrasing; what does that mean, "rebasing @targrub's changes on my implementation"?

It just means that my PR gets merged first. Your PR would then show a message say something about "conflicts" because we are editing the same lines in the same file. You would then merge the (then) current main branch from bevy into your branch solving these conflicts (or alternatively, run git rebase main on it, that's where the name comes from).

All in all the commit history should then look like

...older commits -> my commits -> your commits

You can also do that work before my PR gets merged, but that requires some git magic.

alice-i-cecile · 2022-10-15T14:19:31Z

I'm going to merge targrub's PR first to spare them the git pain :) I also think it's less complex / controversial than this change.

targrub · 2022-10-16T03:01:38Z

I'm going to merge targrub's PR first to spare them the git pain :) I also think it's less complex / controversial than this change.

Gee, thanks!

anlumo · 2022-10-16T22:03:09Z

It's causing a lot of trouble for my side to not have the AbstractWindowHandle being part of WindowDescriptor, instead generating it by dedicated functions. I don't know why the RawWindowHandle wasn't part of the struct beforehand, and this PR just copies that design choice.

Wouldn't it be a good idea to include AbstractWindowHandle in that struct? The way I understand it is that the WindowDescriptor is supposed to provide the information to generate the Window, but this piece of information is added with the actual function call of creating the Window for some reason instead.

MDeiml · 2022-10-17T09:24:43Z

It's causing a lot of trouble for my side to not have the AbstractWindowHandle being part of WindowDescriptor, instead generating it by dedicated functions. I don't know why the RawWindowHandle wasn't part of the struct beforehand, and this PR just copies that design choice.

Wouldn't it be a good idea to include AbstractWindowHandle in that struct? The way I understand it is that the WindowDescriptor is supposed to provide the information to generate the Window, but this piece of information is added with the actual function call of creating the Window for some reason instead.

Unfortunately I don't think this is possible. As I understand the main purpose of WindowDescriptor is the CreateWindow event. Obviously the window handle cannot exist yet when this event is created. Also this is "user facing" API and should probably not contain implementation details.

For your PR it would make sense though to add the canvas as a field of WindowDescriptor, but maybe someone more qualified can answer that. 😅

anlumo · 2022-10-17T14:27:26Z

There are many types called “Window” in bevy. The Window created by CreateWindow is the one used by winit to link the actual OS window to the engine. It's possible that it already exists when this event is called, I think the OS window is just created lazily when it doesn't exist yet.

However, I didn't write that code and it's entirely undocumented, so I'm mostly guessing here based on the snippets I've read through in the course of creating my PR.

Anyways, the problem is that there's no way get the canvas to the winit initializer at the moment without having a web-only API, which I don't want to force.

MDeiml · 2022-10-17T14:34:15Z

Couldn't there just be a canvas attribute in WindowDescriptor that's hidden behind a feature flag or that is only active for the web platform?

anlumo · 2022-10-17T14:36:00Z

That was my previous solution, but that complicates things in combination with this PR, making a few things redundant.

I'll have to take a look if I can get that working again in the new architecture.

alice-i-cecile

Very clean, and the use cases are good. Can you add a full example to examples/window demonstrating how to use this in practice? The "render a texture to it" makes a lot of sense, but will be hard for users to piece together.

MDeiml · 2022-10-17T15:48:03Z

Sure, good idea. I'm gonna try to come up with something simple.

alice-i-cecile · 2022-10-18T23:48:35Z

@anlumo, can I get a review from you on this since you've been touching related code and working off this PR?

anlumo · 2022-11-05T19:54:09Z

@anlumo, can I get a review from you on this since you've been touching related code and working off this PR?

So, sorry for taking so long. I already knew the code itself while adapting my PR to it, and now I've also checked out the documentation and the example. All of that seems fine to me.

alice-i-cecile · 2022-11-06T21:58:56Z

@MDeiml can you rebase this?

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.

Cargo.toml

alice-i-cecile · 2022-11-07T12:31:08Z

crates/bevy_render/src/lib.rs

-                    instance.create_surface(&handle)
+            let surface = {
+                let windows = app.world.resource_mut::<bevy_window::Windows>();
+                let raw_handle = windows.get_primary().and_then(|window| unsafe {


Needs a SAFETY comment :)

Oh, that's true. The file was missing the SAFETY comment before the change, but I'll add one.

Actually looking at it now it seems to me that this was never safe. In particular this comment

bevy/crates/bevy_window/src/raw_handle.rs

Lines 36 to 40 in 02fbf16

// SAFETY: [`RawHandleWrapper`] is just a normal "raw pointer", which doesn't impl Send/Sync. However the pointer is only

// exposed via an unsafe method that forces the user to make a call for a given platform. (ex: some platforms don't

// support doing window operations off of the main thread).

// A recommendation for this pattern (and more context) is available here:

// https://github.com/rust-windowing/raw-window-handle/issues/59

is wrong. The raw window and display handle are directly exposed as the attributes are pub and there are pub methods to access them.

Even without this problem, for my particular case to ensure safety, all windows in Windows have to be created through the main thread, but new windows can be created and added from any thread using Windows::add.

Same goes for

bevy/crates/bevy_render/src/view/window.rs

Lines 186 to 187 in 02fbf16

.or_insert_with(|| unsafe {

// NOTE: On some OSes this MUST be called from the main thread.

which is also missing a SAFETY comment. It says it has to be run on the main thread, but as far as I see there is nothing in place to ensure that.

If I understand this situation correctly this could be made safe by:

Making the attributes of RawHandleWrapper private and removing the methods to access them

Making Window::add unsafe and specifying that it has to be called from the main thread. Maybe add a comment that it is better to use CreateWindow anyways.

Making prepare_windows an exclusive system which if I remember correctly would make it run on the main thread

Making prepare_windows an exclusive system which if I remember correctly would make it run on the main thread

Ideally we would add a NonSend system parameter here, which forces main thread access without blocking parallelism fully.

Hm, it seems like changing the methods of Windows to ensure safety is unpractical, as there are also methods which expose mutable references, which also would need to be unsafe. So I'll make creating RawHandleWrapper unsafe instead.

Making prepare_windows an exclusive system which if I remember correctly would make it run on the main thread

Ideally we would add a NonSend system parameter here, which forces main thread access without blocking parallelism fully.

The marker was already there, I just missed it.

alice-i-cecile · 2022-11-07T12:34:52Z

examples/window/virtual.rs

+    });
+
+    let window_id = WindowId::new();
+    windows.add(Window::new_virtual(


This bit needs more comments; it's the point of the example.

alice-i-cecile

Some small feedback, but I'm liking the direction. I'd also be interested in seeing a headless rendering + virtual window example, but won't require it to be part of this PR. I suspect that it will be a very useful method for testing UI code without a GPU.

alice-i-cecile · 2023-09-29T08:06:54Z

Closed per rationale in #8278.

MDeiml force-pushed the virtual-windows branch from eee987b to bad1a8a Compare October 14, 2022 12:50

alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Oct 14, 2022

anlumo mentioned this pull request Oct 16, 2022

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

Closed

alice-i-cecile requested a review from aevyrie October 17, 2022 15:42

MDeiml force-pushed the virtual-windows branch from da84b1f to 460c467 Compare October 17, 2022 15:43

alice-i-cecile requested changes Oct 17, 2022

View reviewed changes

MDeiml force-pushed the virtual-windows branch from 460c467 to fef8f2a Compare October 17, 2022 15:45

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 5, 2022

MDeiml added 2 commits November 7, 2022 10:45

Introduce AbstractWindowHandle enum

006dd27

MDeiml force-pushed the virtual-windows branch from 7e2ec5d to 0c6caff Compare November 7, 2022 10:03

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 7, 2022

alice-i-cecile self-requested a review November 7, 2022 12:14

alice-i-cecile reviewed Nov 7, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

alice-i-cecile reviewed Nov 7, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 7, 2022

View reviewed changes

alice-i-cecile requested changes Nov 7, 2022

View reviewed changes

MDeiml force-pushed the virtual-windows branch from 0c6caff to e4bb697 Compare November 7, 2022 14:30

MDeiml added 3 commits November 7, 2022 16:41

Add example

5c03cb9

Fix doctest

ac6fa7d

Ensure safety of creating windows

4290e3b

MDeiml force-pushed the virtual-windows branch from e4bb697 to 4290e3b Compare November 7, 2022 15:41

haibane-tenshi mentioned this pull request Mar 31, 2023

Run Bevy in web worker context #8278

Draft

alice-i-cecile closed this Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable creating "virtual" windows without corresponding OS window #6256

Enable creating "virtual" windows without corresponding OS window #6256

MDeiml commented Oct 14, 2022

alice-i-cecile commented Oct 14, 2022

MDeiml commented Oct 14, 2022

alice-i-cecile commented Oct 14, 2022

MDeiml commented Oct 14, 2022

anlumo commented Oct 14, 2022

targrub commented Oct 15, 2022

MDeiml commented Oct 15, 2022

alice-i-cecile commented Oct 15, 2022

targrub commented Oct 16, 2022

anlumo commented Oct 16, 2022

MDeiml commented Oct 17, 2022

anlumo commented Oct 17, 2022

MDeiml commented Oct 17, 2022

anlumo commented Oct 17, 2022

alice-i-cecile left a comment

MDeiml commented Oct 17, 2022

alice-i-cecile commented Oct 18, 2022

anlumo commented Nov 5, 2022

alice-i-cecile commented Nov 6, 2022

alice-i-cecile Nov 7, 2022

MDeiml Nov 7, 2022

MDeiml Nov 7, 2022

alice-i-cecile Nov 7, 2022

MDeiml Nov 7, 2022

MDeiml Nov 7, 2022

alice-i-cecile Nov 7, 2022

alice-i-cecile left a comment

alice-i-cecile commented Sep 29, 2023

	// SAFETY: [`RawHandleWrapper`] is just a normal "raw pointer", which doesn't impl Send/Sync. However the pointer is only
	// exposed via an unsafe method that forces the user to make a call for a given platform. (ex: some platforms don't
	// support doing window operations off of the main thread).
	// A recommendation for this pattern (and more context) is available here:
	// https://github.com/rust-windowing/raw-window-handle/issues/59

	.or_insert_with(\|\| unsafe {
	// NOTE: On some OSes this MUST be called from the main thread.

Enable creating "virtual" windows without corresponding OS window #6256

Enable creating "virtual" windows without corresponding OS window #6256

Conversation

MDeiml commented Oct 14, 2022

Objective

Solution

Changelog

alice-i-cecile commented Oct 14, 2022

MDeiml commented Oct 14, 2022

alice-i-cecile commented Oct 14, 2022

MDeiml commented Oct 14, 2022

anlumo commented Oct 14, 2022

targrub commented Oct 15, 2022

MDeiml commented Oct 15, 2022

alice-i-cecile commented Oct 15, 2022

targrub commented Oct 16, 2022

anlumo commented Oct 16, 2022

MDeiml commented Oct 17, 2022

anlumo commented Oct 17, 2022

MDeiml commented Oct 17, 2022

anlumo commented Oct 17, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

MDeiml commented Oct 17, 2022

alice-i-cecile commented Oct 18, 2022

anlumo commented Nov 5, 2022

alice-i-cecile commented Nov 6, 2022

alice-i-cecile Nov 7, 2022

Choose a reason for hiding this comment

MDeiml Nov 7, 2022

Choose a reason for hiding this comment

MDeiml Nov 7, 2022

Choose a reason for hiding this comment

alice-i-cecile Nov 7, 2022

Choose a reason for hiding this comment

MDeiml Nov 7, 2022

Choose a reason for hiding this comment

MDeiml Nov 7, 2022

Choose a reason for hiding this comment

alice-i-cecile Nov 7, 2022

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Sep 29, 2023