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.

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

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

Closed
7 of 8 tasks
Lokathor opened this issue Jul 23, 2019 · 52 comments
Closed
7 of 8 tasks

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

Lokathor opened this issue Jul 23, 2019 · 52 comments

Comments

@Lokathor
Copy link
Member

Lokathor commented Jul 23, 2019

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
Copy link

Osspial commented Jul 23, 2019

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
Copy link
Member Author

(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
Copy link

Osspial commented Jul 23, 2019

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

@Lokathor
Copy link
Member Author

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

@kvark
Copy link

kvark commented Jul 23, 2019

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

@Ralith
Copy link

Ralith commented Jul 23, 2019

@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
Copy link
Member Author

Lokathor commented Jul 23, 2019

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
Copy link

Ralith commented Jul 23, 2019

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
Copy link

zakarumych commented Jul 25, 2019

@kvark

It's like mint for windowing :)

Then we should also consider what mint did wrong.

@Lokathor
Copy link
Member Author

Could you elaborate on that one?

@msiglreith
Copy link

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

@icefoxen
Copy link
Contributor

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
Copy link

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

@zakarumych
Copy link

@Lokathor I don't see mint widely used

@kvark
Copy link

kvark commented Jul 25, 2019

@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
Copy link
Member Author

@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
Copy link

Osspial commented Jul 25, 2019

@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
Copy link

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
Copy link

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
Copy link

kvark commented Aug 10, 2019

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
Copy link

raphlinus commented Aug 10, 2019

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
Copy link

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
Copy link

Osspial commented Aug 11, 2019

@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 (rust-windowing/raw-window-handle#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
Copy link
Member Author

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
Copy link

Osspial commented Aug 11, 2019

@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
Copy link

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
Copy link

Ralith commented Aug 11, 2019

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
Copy link

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
Copy link
Member Author

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
Copy link

Osspial commented Aug 14, 2019

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

@AlexEne
Copy link
Member

AlexEne commented Aug 15, 2019

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
Copy link
Member Author

Lokathor commented Aug 15, 2019

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
Copy link
Member

AlexEne commented Aug 15, 2019

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

@Lokathor
Copy link
Member Author

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
Copy link
Member

AlexEne commented Aug 15, 2019

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 Lokathor changed the title Better windowing/graphics inter-operation [Tracker] Better windowing/graphics inter-operation Aug 15, 2019
@Lokathor
Copy link
Member Author

Can do

@Lokathor
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

Lokathor commented Aug 19, 2019

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
Copy link

eugene2k commented Aug 19, 2019

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
Copy link
Member Author

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
Copy link

Ralith commented Aug 19, 2019

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

@eugene2k
Copy link

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
Copy link

@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
Copy link

Discussion regarding ash integration in ash-rs/ash#232

@Osspial
Copy link

Osspial commented Aug 26, 2019

@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
Copy link

zakarumych commented Aug 27, 2019

@Osspial de ja vu.

Oh no, you actually wrote it 13 days ago.

@Osspial
Copy link

Osspial commented Aug 27, 2019

Oh welp, github had the comment saved. I thought I hadn't sent it :V

@Lokathor
Copy link
Member Author

The last pending checkbox (sdl2) has an issue placed for this in their repo, so I'm going to close this one up as "Success".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests