Skip to content

Commit

Permalink
Address review
Browse files Browse the repository at this point in the history
  • Loading branch information
daxpedda committed Nov 16, 2023
1 parent 6303a3b commit 8b89667
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 31 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ Bottom level categories:

#### Safe `Surface` creation

It is now possible to safely create a `wgpu::Surface` with `Surface::create_surface()`. `Surface::create_surface_from_raw()` can be used to produce a `Surface<'static>`, which remains `unsafe`.
It is now possible to safely create a `wgpu::Surface` with `Surface::create_surface()` by letting `Surface` hold a lifetime to `window`.

Passing an owned value `window` to `Surface` will return a `Surface<'static>`. Shared ownership over `window` can still be achieved with e.g. an `Arc`. Alternatively a reference could be passed, which will return a `Surface<'window>`.

`Surface::create_surface_from_raw()` can be used to continue producing a `Surface<'static>` without any lifetime requirements over `window`, which also remains `unsafe`.

#### Naga

Expand Down
2 changes: 1 addition & 1 deletion examples/common/src/framework.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ struct ExampleContext {
}
impl ExampleContext {
/// Initializes the example context.
async fn init_async<'a, E: Example>(surface: &mut SurfaceWrapper, window: Arc<Window>) -> Self {
async fn init_async<E: Example>(surface: &mut SurfaceWrapper, window: Arc<Window>) -> Self {
log::info!("Initializing wgpu...");

let backends = wgpu::util::backend_bits_from_env().unwrap_or_default();
Expand Down
31 changes: 16 additions & 15 deletions examples/hello-windows/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,34 +1,34 @@
#![cfg_attr(target_arch = "wasm32", allow(dead_code))]

use std::collections::HashMap;
use std::{collections::HashMap, sync::Arc};
use winit::{
event::{Event, WindowEvent},
event_loop::EventLoop,
window::{Window, WindowId},
};

struct ViewportDesc<'window> {
window: &'window Window,
struct ViewportDesc {
window: Arc<Window>,
background: wgpu::Color,
surface: wgpu::Surface<'window>,
surface: wgpu::Surface<'static>,
}

struct Viewport<'window> {
desc: ViewportDesc<'window>,
struct Viewport {
desc: ViewportDesc,
config: wgpu::SurfaceConfiguration,
}

impl<'window> ViewportDesc<'window> {
fn new(window: &'window Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
let surface = instance.create_surface(window).unwrap();
impl ViewportDesc {
fn new(window: Arc<Window>, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
let surface = instance.create_surface(window.clone()).unwrap();
Self {
window,
background,
surface,
}
}

fn build(self, adapter: &wgpu::Adapter, device: &wgpu::Device) -> Viewport<'window> {
fn build(self, adapter: &wgpu::Adapter, device: &wgpu::Device) -> Viewport {
let size = self.window.inner_size();

let caps = self.surface.get_capabilities(adapter);
Expand All @@ -48,7 +48,7 @@ impl<'window> ViewportDesc<'window> {
}
}

impl Viewport<'_> {
impl Viewport {
fn resize(&mut self, device: &wgpu::Device, size: winit::dpi::PhysicalSize<u32>) {
self.config.width = size.width;
self.config.height = size.height;
Expand All @@ -62,11 +62,11 @@ impl Viewport<'_> {
}
}

async fn run(event_loop: EventLoop<()>, viewports: &[(Window, wgpu::Color)]) {
async fn run(event_loop: EventLoop<()>, viewports: Vec<(Arc<Window>, wgpu::Color)>) {
let instance = wgpu::Instance::default();
let viewports: Vec<_> = viewports
.iter()
.map(|(window, color)| ViewportDesc::new(window, *color, &instance))
.into_iter()
.map(|(window, color)| ViewportDesc::new(window, color, &instance))
.collect();
let adapter = instance
.request_adapter(&wgpu::RequestAdapterOptions {
Expand Down Expand Up @@ -180,6 +180,7 @@ fn main() {
.with_inner_size(winit::dpi::PhysicalSize::new(WINDOW_SIZE, WINDOW_SIZE))
.build(&event_loop)
.unwrap();
let window = Arc::new(window);
window.set_outer_position(winit::dpi::PhysicalPosition::new(
WINDOW_PADDING + column * WINDOW_OFFSET,
WINDOW_PADDING + row * (WINDOW_OFFSET + WINDOW_TITLEBAR),
Expand All @@ -200,7 +201,7 @@ fn main() {
}

env_logger::init();
pollster::block_on(run(event_loop, &viewports));
pollster::block_on(run(event_loop, viewports));
}
#[cfg(target_arch = "wasm32")]
{
Expand Down
18 changes: 10 additions & 8 deletions examples/uniform-values/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
//! The usage of the uniform buffer within the shader itself is pretty self-explanatory given
//! some understanding of WGSL.
use std::sync::Arc;
// We won't bring StorageBuffer into scope as that might be too easy to confuse
// with actual GPU-allocated WGPU storage buffers.
use encase::ShaderType;
Expand Down Expand Up @@ -83,9 +84,9 @@ impl Default for AppState {
}
}

struct WgpuContext<'window> {
pub window: &'window Window,
pub surface: wgpu::Surface<'window>,
struct WgpuContext {
pub window: Arc<Window>,
pub surface: wgpu::Surface<'static>,
pub surface_config: wgpu::SurfaceConfiguration,
pub device: wgpu::Device,
pub queue: wgpu::Queue,
Expand All @@ -94,12 +95,12 @@ struct WgpuContext<'window> {
pub uniform_buffer: wgpu::Buffer,
}

impl WgpuContext<'_> {
async fn new(window: &Window) -> WgpuContext {
impl WgpuContext {
async fn new(window: Arc<Window>) -> WgpuContext {
let size = window.inner_size();

let instance = wgpu::Instance::default();
let surface = instance.create_surface(window).unwrap();
let surface = instance.create_surface(window.clone()).unwrap();
let adapter = instance
.request_adapter(&wgpu::RequestAdapterOptions {
power_preference: wgpu::PowerPreference::HighPerformance,
Expand Down Expand Up @@ -223,8 +224,8 @@ impl WgpuContext<'_> {
}
}

async fn run(event_loop: EventLoop<()>, window: Window) {
let mut wgpu_context = Some(WgpuContext::new(&window).await);
async fn run(event_loop: EventLoop<()>, window: Arc<Window>) {
let mut wgpu_context = Some(WgpuContext::new(window).await);
// (6)
let mut state = Some(AppState::default());
let main_window_id = wgpu_context.as_ref().unwrap().window.id();
Expand Down Expand Up @@ -351,6 +352,7 @@ fn main() {
.with_inner_size(winit::dpi::LogicalSize::new(900, 900))
.build(&event_loop)
.unwrap();
let window = Arc::new(window);
#[cfg(not(target_arch = "wasm32"))]
{
env_logger::builder().format_timestamp_nanos().init();
Expand Down
26 changes: 20 additions & 6 deletions wgpu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,13 @@ impl Drop for Sampler {
pub type SurfaceConfiguration = wgt::SurfaceConfiguration<Vec<TextureFormat>>;
static_assertions::assert_impl_all!(SurfaceConfiguration: Send, Sync);

trait WgpuSurfaceRequirement: HasWindowHandle + HasDisplayHandle + WasmNotSend + WasmNotSync {}
/// This is used in [`Instance::create_surface`]. For a passed `window` to
/// fullfill the requirements, [`HasWindowHandle`], [`HasDisplayHandle`] (and
/// if not targetting Wasm [`Send`] and [`Sync`]) is required.
pub trait WgpuSurfaceRequirement:
HasWindowHandle + HasDisplayHandle + WasmNotSend + WasmNotSync
{
}

impl<T: HasWindowHandle + HasDisplayHandle + WasmNotSend + WasmNotSync> WgpuSurfaceRequirement
for T
Expand All @@ -397,7 +403,7 @@ impl<T: HasWindowHandle + HasDisplayHandle + WasmNotSend + WasmNotSync> WgpuSurf
/// serves a similar role.
pub struct Surface<'window> {
context: Arc<C>,
_surface: Option<Box<dyn 'window + WgpuSurfaceRequirement>>,
_surface: Option<Box<dyn WgpuSurfaceRequirement + 'window>>,
id: ObjectId,
data: Box<Data>,
// Stores the latest `SurfaceConfiguration` that was set using `Surface::configure`.
Expand All @@ -409,6 +415,8 @@ pub struct Surface<'window> {
config: Mutex<Option<SurfaceConfiguration>>,
}

// This custom implementation is required because [`Surface::_surface`] doesn't
// require [`Debug`](fmt::Debug), which we should not require from the user.
impl<'window> fmt::Debug for Surface<'window> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Surface")
Expand Down Expand Up @@ -1948,6 +1956,10 @@ impl Instance {
/// If the specified display and window handle are not supported by any of the backends, then the surface
/// will not be supported by any adapters.
///
/// If a reference is passed in `window`, the returned [`Surface`] will
/// hold a lifetime to it. Owned values will return a [`Surface<'static>`]
/// instead.
///
/// # Errors
///
/// - On WebGL2: Will return an error if the browser does not support WebGL2,
Expand All @@ -1958,10 +1970,7 @@ impl Instance {
/// - On macOS/Metal: will panic if not called on the main thread.
/// - On web: will panic if the `raw_window_handle` does not properly refer to a
/// canvas element.
pub fn create_surface<
'window,
W: 'window + HasWindowHandle + HasDisplayHandle + WasmNotSend + WasmNotSync,
>(
pub fn create_surface<'window, W: WgpuSurfaceRequirement + 'window>(
&self,
window: W,
) -> Result<Surface<'window>, CreateSurfaceError> {
Expand All @@ -1970,6 +1979,11 @@ impl Instance {
Ok(surface)
}

/// An alternative version to [`create_surface()`](Self::create_surface)
/// which has no lifetime requirements to `window` and doesn't require
/// [`Send`] or [`Sync`] (on non-Wasm targets). This makes it `unsafe`
/// instead and always returns a [`Surface<'static>`].
///
/// See [`create_surface()`](Self::create_surface) for more details.
///
/// # Safety
Expand Down

0 comments on commit 8b89667

Please sign in to comment.