bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Enable creating "virtual" windows without corresponding OS window

Open MDeiml opened this issue 3 years ago • 18 comments

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

MDeiml avatar Oct 14 '22 12:10 MDeiml

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.

alice-i-cecile avatar Oct 14 '22 13:10 alice-i-cecile

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.

MDeiml avatar Oct 14 '22 14:10 MDeiml

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

alice-i-cecile avatar Oct 14 '22 14:10 alice-i-cecile

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.

MDeiml avatar Oct 14 '22 15:10 MDeiml

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.

anlumo avatar Oct 14 '22 16:10 anlumo

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 . :)

targrub avatar Oct 15 '22 12:10 targrub

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.

MDeiml avatar Oct 15 '22 12:10 MDeiml

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.

alice-i-cecile avatar Oct 15 '22 14:10 alice-i-cecile

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!

targrub avatar Oct 16 '22 03:10 targrub

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.

anlumo avatar Oct 16 '22 22:10 anlumo

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. :sweat_smile:

MDeiml avatar Oct 17 '22 09:10 MDeiml

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.

anlumo avatar Oct 17 '22 14:10 anlumo

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?

MDeiml avatar Oct 17 '22 14:10 MDeiml

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.

anlumo avatar Oct 17 '22 14:10 anlumo

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

MDeiml avatar Oct 17 '22 15:10 MDeiml

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

alice-i-cecile avatar Oct 18 '22 23:10 alice-i-cecile

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

anlumo avatar Nov 05 '22 19:11 anlumo

@MDeiml can you rebase this?

alice-i-cecile avatar Nov 06 '22 21:11 alice-i-cecile

Closed per rationale in #8278.

alice-i-cecile avatar Sep 29 '23 08:09 alice-i-cecile