rust-gamedev / wg

Coordination repository of the Game Development Working Group
514 stars 10 forks source link

[Tracker] Better windowing/graphics inter-operation #26

Closed Lokathor closed 4 years ago

Lokathor commented 5 years ago

Update: Most work is done! Unfortunately this didn't help the GL API as much as it helped the newstyle APIs like Vulkan/Metal/gfx-hal/wgpu, but it was a good improvement.

Windowing Side

Graphical Side

original issue post follows:

gfx-hal (technically the Instance type in gfx-backend-foo for one of gl, vulkan, dx12, etc) takes a &Window, but all types actually secretly have their version number as part of their type so it takes a &Window-v0.19.0. That's fine and all except there's a new 0.20.0-alpha2 out and people are supposed to be trying out that new alpha but they can't draw a picture because it's the wrong version so you get a type mismatch error. We can hardly alpha test the library if we can't even draw a picture. The backends have a plan for what to do: ignore their winit integration, possibly disable the winit feature entirely, and then use the create_surface_from_THING version for whatever raw window thing you have. This is annoying, but you can do it. The gfx-backend-vulkan crate has support for xlib, xcb, wayland, and even an existing vulkan surface. Honestly, all that create_surface does with the &Window is look inside the Window for the raw window data (using the right platform specific extension trait), and then call one of the other constructors anyway.

What we should do is cut out the whole middle step where gfx-hal has to know about the unstable winit crate, and we let it rely on a very stable interface that both winit and gfx-hal and other window libs and other graphical libs can all agree on.

We'll call the crate raw-window-handle (bikeshed here). The crate only needs a single trait and a support enum for the return value of the single method on that trait:

pub unsafe trait HasRawWindowHandle {
  fn raw_window_handle(&self) -> RawHandleData;
}

pub enum RawHandleData {
  Win32{ hwnd: *mut c_void},
  XLib{ display: *mut c_void, window: u64 },
  Cocoa{ .. } \
  // etc, one for each windowing system,
  // we prefer c_void and pointer casting instead of the "real" pointer
  // types when possible so that it builds easily on all platforms.
}

Then winit implements this for Window, and gfx-hal backends can write create_surface(win: &impl HasRawWindowHandle), and end users trying to get the two to talk to each other stop having to match their versions up quite so tightly. The raw-window-handle crate can just be one of those "0.1 crates that basically never updates because it does the job fine as is" like libc, perhaps to become 1.0 once we've really kicked the tires a bit., perhaps never to even bother with 1.0 at all.

And that's something we could get done in just 1 month.

Osspial commented 5 years ago

I'll agree that a crate exposing some sort of HasRawWindowHandle trait is a great idea! You asked for bikeshedding, so I'll provide it:

I don't think it makes sense to expose RawHandleData as an enum, since only one variant will ever be created on any given platform (with the exception of Wayland/X11). Exposing an enum forces downstream users to create match branches that will never be executed on whatever target is getting compiled, which hardly seems ergonomic. I'd rather expose each platform's pointer as a separate function on HasRawWindowHandle that gets conditionally compiled, like so:

pub unsafe trait HasRawWindowHandle {
    #[cfg(target_os = "windows")]
    fn raw_win32_handle(&self) -> *mut c_void;
    #[cfg(any(
        target_os = "linux",
        target_os = "dragonfly",
        target_os = "freebsd",
        target_os = "netbsd",
        target_os = "openbsd"
    ))]
    fn raw_unix_handle(&self) -> UnixHandle;
    #[cfg(target_os = "macos")]
    fn raw_cocoa_handle(&self) -> CocoaHandle;
    /* and more, as necessary */
}

#[cfg(any(
    target_os = "linux",
    target_os = "dragonfly",
    target_os = "freebsd",
    target_os = "netbsd",
    target_os = "openbsd"
))]
pub enum UnixHandle {
    X11 { .. },
    Wayland { .. },
}

#[cfg(target_os = "macos")]
pub struct CocoaHandle {
    pub ns_window: *mut c_void,
    pub ns_view: *mut c_void,
}

Also, I'm not entirely happy with the HasRawWindowHandle/raw-window-handle name, but I'm having a hard time coming up with a better one. I'll post one if I can think of one, though.

WRT stabilization - I think releasing this as 0.1, then letting it sit and get tested for a while is the best approach. Once we're sure we have the right design, we can use the semver trick to release 1.0 without breaking everything.

Lokathor commented 5 years ago

(note that i removed the first part so now you're replying to text that doesn't exist so you might care to also remove your rely to said first part)

Osspial commented 5 years ago

Whoops, I drafted that before you removed it. I've edited that out.

Lokathor commented 5 years ago

Note that the Linux Framebuffer naturally would need to end up on such a list thing.

kvark commented 5 years ago

@Lokathor this would be a good thing to have for both gfx-hal and wgpu-rs. It's like mint for windowing :)

Ralith commented 5 years ago

@Osspial Alternatively, @Lokathor's design could be used, but with the enum cases conditionally compiled out. That would likely reduce the amount of conditional compilation needed in downstream crates, as only the code that actually manipulates the raw handles would need to be platform-specific, rather than all code which obtains, passes, or stores them.

Lokathor commented 5 years ago

So ideally we'd want to use #[non_exhaustive] on the enum once stable so that everyone handles all the cases they can and just accepts ahead of time that there's new stuff that might happen that they should just gracefully error on instead of failure to build.

EDIT: once that attribute is stable that is

Ralith commented 5 years ago

I'm not sure that's appropriate. It's difficult to gracefully report an error to the user when you can't display anything, and if the error is "your platform is not supported" then the package probably shouldn't have been distributed to that platform to begin with, which a compile-time provides for.

zakarumych commented 5 years ago

@kvark

It's like mint for windowing :)

Then we should also consider what mint did wrong.

Lokathor commented 5 years ago

Could you elaborate on that one?

msiglreith commented 5 years ago

From last meeting: @Osspial mentioned that they will open a repository for this in rust-windowing within the next days

icefoxen commented 5 years ago

From the point of view of a crate that integrates both, this issue is more a nuisance than anything else, 'cause ideally you're using the most-stable versions of winit and gfx anyway and those are pretty stable.

From the point of view of someone who occasionally submits patches to winit and/or gfx, it would be pretty nice though. I question the need for another crate for indirection though, you could just have platform specific functions in winit to expose the window handles, since they seem to mostly be void pointers anyway, and functions in gfx that take them. A crate for it may be more convenient, I'm just asking "do we really need this?" before we make a 30-line crate.

zakarumych commented 5 years ago

@icefoxen the thing is, no one wants to write platform specific code. Although platform-specific methods exist on both sides already.

zakarumych commented 5 years ago

@Lokathor I don't see mint widely used

kvark commented 5 years ago

@omni-viral it's used in ggez, three-rs, mathbench, and other things.

@icefoxen the API contract (trait or enum) has to live somewhere. If it lives in winit, then it will not be accessible to SDL/GLFW, it will be breaking as often as winit, sp it's not going to be useful. If it lives in gfx-hal, then we'd get a circular dependency (gfx-hal -> winit -> windowing (in gfx-hal)), which isn't pretty either. Everything points to the fact it has to be a standalone crate, which doesn't change as often as any of the users.

Lokathor commented 5 years ago

@icefoxen "those" are absolutely not pretty stable :P

anyway the end user doesn't even see a difference

before

Instance::from_window(&Window)

after

Instance::from_window(&impl HasRawWindowHandle)

So before their code said

let i = back::Instance::from_window(&window);

and now it still says

let i = back::Instance::from_window(&window);
Osspial commented 5 years ago

@icefoxen Another potential upside of this is that it would in theory let us fully decouple Winit and Glutin's versioning, which has been something of a pain point in the past. I'd like us to try implementing we publish the first version on crates.io.

Anyhow, initial version is available here: https://github.com/rust-windowing/raw-window-handle. I haven't added any docs yet, but I'm not sure that's necessary. A readme will be coming soonish.

I prioritized being consistent internally, consistent with Winit and std::os, and forwards compatibility more than anything else, so the resulting design is a bit different than what was discussed here. If anyone feels it takes the wrong steps feel free to bring it up.

EDIT: Important note - the version on crates.io is a squatted, stub crate, not the actual implementation.

crlf0710 commented 5 years ago

About windowing/graphics, i wonder if it's feasible to provide more smooth winit/piet or raw-window-handle/piet integration. I think that will improve the rust graphics ecosystem a lot. @raphlinus

raphlinus commented 5 years ago

In the longer term, I'd very much like to see that. Eventually, we will have native GPU-accelerated piet implementations. Until then, you either want to use Direct2D (on Windows) or wire up a software renderer. The former is, as I understand, not all that well supported on winit; generally you want do do presentation with dxgi rather than gl-style.

So basically I'm unlikely to work on this myself any time soon (other than druid-shell integration, where we can fine-tune as needed), but definitely would want to encourage others.

kvark commented 5 years ago

Until then, you either want to use Direct2D (on Windows) or wire up a software renderer. The former is, as I understand, not all that well supported on winit; generally you want do do presentation with dxgi rather than gl-style.

Could you elaborate on that? winit should not be aligned to "gl-style" in any way, and it should not prevent anybody to use Direct2D on Windows.

raphlinus commented 5 years ago

It's only a sense I got from the winit discussion on smooth resize. I don't have solid experience on winit so will defer to other people who know more.

crlf0710 commented 5 years ago

Personally, i care most about availability at the current stage. I just hope for something just like libgdiplus but in pure rust available on all three desktop platforms (and even better if mobile platforms are included too). Maybe such functionality already exists in some crate, i don't know. But obviously there's coordinating missing here to make it an obvious reasonable choice, so never got the necessary polishing to reach the fore-said availability.

Maybe saying this is a little over casual application oriented rather than game-oriented. But i still believe it's none the less important for the whole ecosystem.

Osspial commented 5 years ago

@raphlinus I was mainly talking about the OpenGL case there because that's the environment I'm personally familiar with. Winit doesn't do anything to inherently tie itself to OpenGL, and it's very much usable with other graphics APIs.


Anyhow, update on raw-window-handle - I just merged a change that simplifies the design into a single enum instead of the complex arrangement of extension traits it was initially. I just opened a PR adding some basic documentation (https://github.com/rust-windowing/raw-window-handle/pull/8), so this should mostly be good to release once that's merged!

I've got one unresolved question, though - should we expose some sort of is_valid or is_destroyed method? That would allow HasRawWindowHandle to be used on windows that aren't necessarily owned by Rust applications, such as for DSP audio plugins.

Lokathor commented 5 years ago

I can't immediately say "yes" to that, so I think (at least to start) we should not do that, and then presumably the person can still use whatever OS call to check for themselves.

Osspial commented 5 years ago

@Lokathor The issue with telling users to use OS calls is that they may not necessarily be reliable. For instance, the Windows API is explicitly allowed to re-use window handles after a window has been destroyed, which can potentially result in the validity-checking call returning true when the original window no longer exists.

crlf0710 commented 5 years ago

For windows this problem is actually solvable using SetWinEventHook, see this blog, but i'm not sure it's worth it, or there's similiar mechanism on other platforms.

Ralith commented 5 years ago

Determining whether a window is closed is usually something you do by processing events from the windowing system. This is something an application will generally need to do anyway, and adding it to the trait would probably require implementers to maintain additional internal state. It's not clear to me why anything extra is needed even for non-owned windows, since an application will still presumably be processing events.

msiglreith commented 5 years ago

I would consider this difficult for cases where the window handle might come from FFI, which puts additional constraints, where it might not be possible to provide this information.

Lokathor commented 5 years ago

Yeah, my SDL2 wrapper crate has no way at all to provide such an interface. It just reports what SDL2 says, and SDL2 just makes OS calls, so

Osspial commented 5 years ago

Alright, we'll leave that API out for now. I've pushed the initial version to crates.io.

EDIT: I tried integrating this with Winit, and it looks like window handle structs are currently impossible to instantiate due to the exact method we use to make them non-exhaustive. I will publish an update shortly fixing this.

zakarumych commented 5 years ago

Can't find WebHadler

Osspial commented 5 years ago

@omni-viral That crate doesn't have a WebHandle yet because I honestly have no idea what that's supposed to look like! I'd be happy for someone with the relevant expertise to step in and add that, though.

Anyhow, 0.1.2 is on crates.io and makes raw-window-handle usable. I've made a PR implementing that in Winit which should get merged soon: https://github.com/rust-windowing/winit/pull/1105.

Osspial commented 5 years ago

Winit support for raw-window-handle is on Crates.io. Update to alpha 3 to use it.

AlexEne commented 5 years ago

I didn't pay too much attention to the discussions here, but the way this issue was opened looks like something that should have been in the winit repo?

As winit was fixed and published on crates, can we please clarify what we're looking to achieve with this thread that's not winit specific? And let's put that in the issue so any new participant understands what this is about.

Lokathor commented 5 years ago

No, this is the opposite of winit specific. This is about getting a direct winit dependency out of gfx-hal, ash, wgpu, etc

EDIT: in other words, this issue is being used as a point of communication between various stakeholders in the gamedev ecosystem. Exactly as the WG is intended to do.

AlexEne commented 5 years ago

So after that merge it was fixed? Or are still things that need to be done for this to be achieved?

Lokathor commented 5 years ago

This can be considered a tracking issue for a time. no graphics libs use the new api yet, so there's more to get done first.

AlexEne commented 5 years ago

If we have consensus about the next steps maybe opening issues that track the work in various libs is a better approach. Otherwise this risks becoming a status update on work in various crates more or less.

Actually, even as a tracking issue the next step should probably be to open issues that it can track from the various crates, then it's clear to any new participant what the state of the discussion is. (Agreed upon , some work done, needs this other work done)

Lokathor commented 5 years ago

Can do

Lokathor commented 5 years ago

Core graphical libs have had issues filed. Once they've got stuff in place we can also tell people using those core graphical libs to update their versions as necessary.

eugene2k commented 5 years ago

I'm curious why this design was chosen. Would it not be simpler to have just a number of traits with a single function? Something like

trait RawWin32Handle {
    fn raw_win32_handle(&self) -> Win32Handle
}
trait RawXlibHandle {
    fn raw_xlib_handle(&self) -> XlibHandle
}
Lokathor commented 5 years ago

because then you can't pass the window to one function no matter what window system like this:

let surface = instance::create_surface(window).unwrap();

there's already functions for window system specific creations available, if you want the end user to manually sort that out on their side.

eugene2k commented 5 years ago

Sorry, I still don't get what's wrong with providing a number of common traits. create_surface is platform-specific, window is also platform-specific. It shouldn't be a problem to change create_surface signature based on whether it's compiled for one window system or another nor should it be a problem having the Window object implement platform-specific traits on platforms that support them.

Lokathor commented 5 years ago

Some platforms (Linux) have more than one windowing system available, you don't know which until runtime, so you can't always cfg create_surface it to accept exactly one type of handle-giving thing.

Ralith commented 5 years ago

It's also a pain to deal with a bunch of different traits when just one will do.

eugene2k commented 5 years ago

Hmm... I guess it's ultimately a stylistic choice. It just seems to me that with a set of RawWindowHandle traits the crate itself wouldn't need so many conditional compilation flags.

zakarumych commented 5 years ago

@eugene2k it would. And user code would. And code that just passes raw handle to code that matches it. With this design end-user (create_surface(window) caller) doesn't think about it at all.

msiglreith commented 5 years ago

Discussion regarding ash integration in https://github.com/MaikKlein/ash/issues/232

Osspial commented 5 years ago

@omni-viral That crate doesn't have a WebHandle yet because I honestly have no idea what that's supposed to look like! I'd be happy for someone with the relevant expertise to step in and add that, though.

zakarumych commented 5 years ago

@Osspial de ja vu.

Oh no, you actually wrote it 13 days ago.