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.

Sign up for GitHub

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

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a28eb9
Enable creating "virtual" windows without corresponding OS window
MDeiml Oct 14, 2022
006dd27
Introduce `AbstractWindowHandle` enum
MDeiml Oct 14, 2022
5c03cb9
Add example
MDeiml Oct 17, 2022
ac6fa7d
Fix doctest
MDeiml Oct 19, 2022
4290e3b
Ensure safety of creating windows
MDeiml Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Introduce AbstractWindowHandle enum
  • Loading branch information
@MDeiml
MDeiml committed Nov 7, 2022
commit 006dd273c5a56782a788b45e8fc199b3d15fe01e
21 changes: 12 additions & 9 deletions crates/bevy_render/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub mod view;

use bevy_core::FrameCount;
use bevy_hierarchy::ValidParentCheckPlugin;
use bevy_window::AbstractWindowHandle;
pub use extract_param::Extract;

pub mod prelude {
Expand Down Expand Up @@ -142,17 +143,19 @@ impl Plugin for RenderPlugin {
.register_type::<Color>();

if let Some(backends) = options.backends {
let windows = app.world.resource_mut::<bevy_window::Windows>();
let instance = wgpu::Instance::new(backends);

let surface = windows
.get_primary()
.and_then(|window| window.raw_handle())
.map(|wrapper| unsafe {
let handle = wrapper.get_handle();
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 {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a SAFETY comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Making the attributes of RawHandleWrapper private and removing the methods to access them
  2. 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.
  3. Making prepare_windows an exclusive system which if I remember correctly would make it run on the main thread

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

match window.window_handle() {
AbstractWindowHandle::RawWindowHandle(handle) => {
Some(instance.create_surface(&handle.get_handle()))
}
AbstractWindowHandle::Virtual => None,
}
});

raw_handle
};
let request_adapter_options = wgpu::RequestAdapterOptions {
power_preference: options.power_preference,
compatible_surface: surface.as_ref(),
Expand Down
175 changes: 103 additions & 72 deletions crates/bevy_render/src/view/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use bevy_app::{App, Plugin};
use bevy_ecs::prelude::*;
use bevy_utils::{tracing::debug, HashMap, HashSet};
use bevy_window::{
CompositeAlphaMode, PresentMode, RawHandleWrapper, WindowClosed, WindowId, Windows,
AbstractWindowHandle, CompositeAlphaMode, PresentMode, RawHandleWrapper, WindowClosed,
WindowId, Windows,
};
use std::ops::{Deref, DerefMut};
use wgpu::TextureFormat;
Expand Down Expand Up @@ -41,7 +42,7 @@ impl Plugin for WindowRenderPlugin {

pub struct ExtractedWindow {
pub id: WindowId,
pub raw_handle: Option<RawHandleWrapper>,
pub handle: AbstractWindowHandle,
pub physical_width: u32,
pub physical_height: u32,
pub present_mode: PresentMode,
Expand Down Expand Up @@ -88,7 +89,7 @@ fn extract_windows(
.entry(window.id())
.or_insert(ExtractedWindow {
id: window.id(),
raw_handle: window.raw_handle(),
handle: window.window_handle(),
physical_width: new_width,
physical_height: new_height,
present_mode: window.present_mode(),
Expand Down Expand Up @@ -144,7 +145,7 @@ pub struct WindowSurfaces {

/// Creates and (re)configures window surfaces, and obtains a swapchain texture for rendering.
///
/// This will not handle [virtual windows](bevy_window::Window::new_virtual).
/// This will not handle [virtual windows](bevy_window::AbstractWindowHandle::Virtual).
///
/// NOTE: `get_current_texture` in `prepare_windows` can take a long time if the GPU workload is
/// the performance bottleneck. This can be seen in profiles as multiple prepare-stage systems all
Expand Down Expand Up @@ -176,77 +177,107 @@ pub fn prepare_windows(
render_adapter: Res<RenderAdapter>,
) {
for window in windows.windows.values_mut() {
if let Some(handle) = &window.handle {
let window_surfaces = window_surfaces.deref_mut();
let surface_data =
window_surfaces
.surfaces
.entry(window.id)
.or_insert_with(|| unsafe {
// NOTE: On some OSes this MUST be called from the main thread.
let surface = render_instance
.create_surface(&window.raw_handle.as_ref().unwrap().get_handle());
let format = *surface
.get_supported_formats(&render_adapter)
.get(0)
.unwrap_or_else(|| {
panic!(
"No supported formats found for surface {:?} on adapter {:?}",
surface, render_adapter
)
});
SurfaceData { surface, format }
let window_surfaces = window_surfaces.deref_mut();
let surface_data = match &window.handle {
AbstractWindowHandle::RawWindowHandle(handle) => window_surfaces
.surfaces
.entry(window.id)
.or_insert_with(|| unsafe {
// NOTE: On some OSes this MUST be called from the main thread.
let surface = render_instance
.create_surface(&window.raw_handle.as_ref().unwrap().get_handle());
let format = *surface
.get_supported_formats(&render_adapter)
.get(0)
.unwrap_or_else(|| {
panic!(
"No supported formats found for surface {:?} on adapter {:?}",
surface, render_adapter
)
});
SurfaceData { surface, format }
}),
AbstractWindowHandle::Virtual => continue,
};
let surface_data = window_surfaces
.surfaces
.entry(window.id)
.or_insert_with(|| unsafe {
// NOTE: On some OSes this MUST be called from the main thread.
let surface = render_instance
.create_surface(&window.raw_handle.as_ref().unwrap().get_handle());
let format = *surface
.get_supported_formats(&render_adapter)
.get(0)
.unwrap_or_else(|| {
panic!(
"No supported formats found for surface {:?} on adapter {:?}",
surface, render_adapter
)
});
SurfaceData { surface, format }
});

let surface_configuration = wgpu::SurfaceConfiguration {
format: surface_data.format,
width: window.physical_width,
height: window.physical_height,
usage: wgpu::TextureUsages::RENDER_ATTACHMENT,
present_mode: match window.present_mode {
PresentMode::Fifo => wgpu::PresentMode::Fifo,
PresentMode::Mailbox => wgpu::PresentMode::Mailbox,
PresentMode::Immediate => wgpu::PresentMode::Immediate,
PresentMode::AutoVsync => wgpu::PresentMode::AutoVsync,
PresentMode::AutoNoVsync => wgpu::PresentMode::AutoNoVsync,
},
alpha_mode: match window.alpha_mode {
CompositeAlphaMode::Auto => wgpu::CompositeAlphaMode::Auto,
CompositeAlphaMode::Opaque => wgpu::CompositeAlphaMode::Opaque,
CompositeAlphaMode::PreMultiplied => wgpu::CompositeAlphaMode::PreMultiplied,
CompositeAlphaMode::PostMultiplied => wgpu::CompositeAlphaMode::PostMultiplied,
CompositeAlphaMode::Inherit => wgpu::CompositeAlphaMode::Inherit,
},
};

// Do the initial surface configuration if it hasn't been configured yet. Or if size or
// present mode changed.
let frame = if window_surfaces.configured_windows.insert(window.id)
|| window.size_changed
|| window.present_mode_changed
{
render_device.configure_surface(&surface_data.surface, &surface_configuration);
surface_data
.surface
.get_current_texture()
.expect("Error configuring surface")
} else {
match surface_data.surface.get_current_texture() {
Ok(swap_chain_frame) => swap_chain_frame,
Err(wgpu::SurfaceError::Outdated) => {
render_device
.configure_surface(&surface_data.surface, &surface_configuration);
surface_data
.surface
.get_current_texture()
.expect("Error reconfiguring surface")
}
err => err.expect("Failed to acquire next swap chain texture!"),
let surface_configuration = wgpu::SurfaceConfiguration {
format: surface_data.format,
width: window.physical_width,
height: window.physical_height,
usage: wgpu::TextureUsages::RENDER_ATTACHMENT,
present_mode: match window.present_mode {
PresentMode::Fifo => wgpu::PresentMode::Fifo,
PresentMode::Mailbox => wgpu::PresentMode::Mailbox,
PresentMode::Immediate => wgpu::PresentMode::Immediate,
PresentMode::AutoVsync => wgpu::PresentMode::AutoVsync,
PresentMode::AutoNoVsync => wgpu::PresentMode::AutoNoVsync,
},
alpha_mode: match window.alpha_mode {
CompositeAlphaMode::Auto => wgpu::CompositeAlphaMode::Auto,
CompositeAlphaMode::Opaque => wgpu::CompositeAlphaMode::Opaque,
CompositeAlphaMode::PreMultiplied => wgpu::CompositeAlphaMode::PreMultiplied,
CompositeAlphaMode::PostMultiplied => wgpu::CompositeAlphaMode::PostMultiplied,
CompositeAlphaMode::Inherit => wgpu::CompositeAlphaMode::Inherit,
},
};

// Do the initial surface configuration if it hasn't been configured yet. Or if size or
// present mode changed.
let frame = if window_surfaces.configured_windows.insert(window.id)
|| window.size_changed
|| window.present_mode_changed
{
render_device.configure_surface(&surface_data.surface, &surface_configuration);
surface_data
.surface
.get_current_texture()
.expect("Error configuring surface")
} else {
match surface_data.surface.get_current_texture() {
Ok(swap_chain_frame) => swap_chain_frame,
Err(wgpu::SurfaceError::Outdated) => {
render_device.configure_surface(&surface_data.surface, &surface_configuration);
surface_data
.surface
.get_current_texture()
.expect("Error reconfiguring surface")
}
};
err => err.expect("Failed to acquire next swap chain texture!"),
}
};

window.swap_chain_texture = Some(TextureView::from(frame));
window.swap_chain_texture_format = Some(surface_data.format);
}
window.swap_chain_texture = Some(TextureView::from(frame));
window.swap_chain_texture_format = Some(surface_data.format);
}

let frame = match surface.get_current_texture() {
Ok(swap_chain_frame) => swap_chain_frame,
Err(wgpu::SurfaceError::Outdated) => {
render_device.configure_surface(surface, &swap_chain_descriptor);
surface
.get_current_texture()
.expect("Error reconfiguring surface")
}
err => err.expect("Failed to acquire next swap chain texture!"),
};

window.swap_chain_texture = Some(TextureView::from(frame));
}
40 changes: 22 additions & 18 deletions crates/bevy_window/src/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,21 @@ impl WindowResizeConstraints {
}
}

/// Handle used for creating surfaces in the render plugin
///
/// Either a raw handle to an OS window or `Virtual` to signify that there is no corresponding OS window.
#[derive(Clone, Debug)]
pub enum AbstractWindowHandle {
/// The window corresponds to an operator system window.
RawWindowHandle(RawWindowHandleWrapper),
/// The window does not to correspond to an operator system window.
///
/// It differs from a non-virtual window, in that the caller is responsible
/// for creating and presenting surface textures and inserting them into
/// [`ExtractedWindow`](https://docs.rs/bevy/*/bevy/render/view/struct.ExtractedWindow.html).
Virtual,
}

/// An operating system or virtual window that can present content and receive user input.
///
/// To create a window, use a [`EventWriter<CreateWindow>`](`crate::CreateWindow`).
Expand Down Expand Up @@ -287,7 +302,7 @@ pub struct Window {
cursor_visible: bool,
cursor_grab_mode: CursorGrabMode,
physical_cursor_position: Option<DVec2>,
raw_handle: Option<RawHandleWrapper>,
window_handle: AbstractWindowHandle,
focused: bool,
mode: WindowMode,
canvas: Option<String>,
Expand Down Expand Up @@ -431,7 +446,7 @@ impl Window {
cursor_grab_mode: window_descriptor.cursor_grab_mode,
cursor_icon: CursorIcon::Default,
physical_cursor_position: None,
raw_handle: Some(raw_handle),
window_handle: AbstractWindowHandle::RawWindowHandle(raw_handle),
focused: true,
mode: window_descriptor.mode,
canvas: window_descriptor.canvas.clone(),
Expand All @@ -443,11 +458,7 @@ impl Window {

/// Creates a new virtual [`Window`].
///
/// This window does not have to correspond to an operator system window.
///
/// It differs from a non-virtual window, in that the caller is responsible
/// for creating and presenting surface textures and inserting them into
/// [`ExtractedWindow`](https://docs.rs/bevy/*/bevy/render/view/struct.ExtractedWindow.html).
/// See [`AbstractWindowHandle::Virtual`].
pub fn new_virtual(
id: WindowId,
window_descriptor: &WindowDescriptor,
Expand All @@ -474,7 +485,7 @@ impl Window {
cursor_locked: window_descriptor.cursor_locked,
cursor_icon: CursorIcon::Default,
physical_cursor_position: None,
raw_window_handle: None,
window_handle: AbstractWindowHandle::Virtual,
focused: true,
mode: window_descriptor.mode,
canvas: window_descriptor.canvas.clone(),
Expand Down Expand Up @@ -866,16 +877,9 @@ impl Window {
self.focused
}

/// Get the [`RawWindowHandleWrapper`] corresponding to this window.
///
/// A return value of `None` signifies that this is a virtual window and does not
/// correspond to an OS window. The creator of the window is responsible
/// for creating and presenting surface textures and inserting them into
/// [`ExtractedWindow`](https://docs.rs/bevy/*/bevy/render/view/struct.ExtractedWindow.html).
///
/// See [`Self::new_virtual`].
pub fn raw_window_handle(&self) -> Option<RawHandleWrapper> {
self.raw_handle.clone()
/// Get the [`AbstractWindowHandle`] corresponding to this window.
pub fn window_handle(&self) -> AbstractWindowHandle {
self.window_handle.clone()
}

/// The "html canvas" element selector.
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_winit/src/winit_windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ impl WinitWindows {
inner_size.height,
scale_factor,
position,
Some(raw_handle),
raw_window_handle,
)
}

Expand Down