Replace `winit` with `raw-window-handle` in backend crates #2967

jamsch0 · 2019-08-20T17:36:22Z

Closes #2956
PR checklist:

make succeeds (on *nix)
make reftests succeeds
tested examples with the following backends: dx11, dx12, vulkan, wgl
rustfmt run on changed code

Currently Android is not supported in raw-window-handle and therefore unimplemented in gfx-backend-vulkan - waiting on rust-windowing/raw-window-handle#12 to be resolved.

Android support is currently missing.

Only applies to `wgl` feature.

kvark

Thank you, amazing work!
Aside from the comments on the review, I wonder how we should go about publishing these changes.
Several choices are available:

in "hal-0.3" branch, add support for raw-window-handle in addition to winit, so that it's not breaking and users can test it out
back-port this change directly to "hal-0.3" but also keep a dummy "winit" feature that changes nothing: users who had "winit" enabled and used to pass "winit::Window" would still have that code working, right? Unless they were using "winit" version that didn't have "raw-window-handle" integration itself, yet, hmm
wait for "hal-0.4" release, potentially making it sooner rather than later

kvark · 2019-08-20T18:44:14Z

src/backend/dx11/src/lib.rs

+    ) -> Surface {
+        let hwnd = match has_handle.raw_window_handle() {
+            raw_window_handle::RawWindowHandle::Windows(handle) => handle.hwnd,
+            _ => unreachable!(),


I think cases like this should be panic!, because they will easily be reachable once the enum gets new variants

Yes of course, I hadn't considered that. Do you have any suggestions for the error message(s)?

It actually sounds like we need an error code returned for that mismatch?..

It actually sounds like we need an error code returned for that mismatch?..

Could you explain that a bit further, please?

Just adding something like struct InitError in window.rs module, so that all create_surface_from_raw calls would return Result<Surface, window::InitError> instead of panicking.

I thought that was what you were implying, just wanted to double check :)

src/backend/vulkan/src/window.rs

jamsch0 · 2019-08-20T20:39:10Z

src/backend/gl/src/window/wgl.rs

-
-        let hwnd = window.hwnd();
-        self.create_surface_from_hwnd(hwnd as *mut _)
+    #[cfg(windows)]


Not quite sure about this, or if the entire wgl feature should be cfg(target_os = windows), or whether the unreachable should instead be something like panic!("WGL is not supported for this target")?

latter sounds good to me. @msiglreith ?

jamsch0 · 2019-08-20T20:48:18Z

Wrt. how to land this change, I see now that it would be a breaking change as you mention. If you wanted to go with option 1, would that be achieved by adding create_surface(&impl HasRawWindowHandle) alongside create_surface(&winit::Window), both cfg'd out as appropriate and mutually exclusive?

kvark · 2019-08-20T20:54:00Z

Wrt. how to land this change, I see now that it would be a breaking change as you mention. If you wanted to go with option 1, would that be achieved by adding create_surface(&impl HasRawWindowHandle) alongside create_surface(&winit::Window), both cfg'd out as appropriate and mutually exclusive?

I think we can make them exclusive, but only if winit takes precedence (in case of both features enabled), so that the existing code works.

Although, a simpler (and better) path would just be to just expose it under a different name. We'll break the API in the next version anyway.

jamsch0 · 2019-08-20T21:12:45Z

Although, a simpler (and better) path would just be to just expose it under a different name. We'll break the API in the next version anyway.

Ok, in that case I'll go ahead and do that, maybe with the name create_surface_from_raw for now? (Obviously feel free to bikeshed.)

kvark · 2019-08-20T21:15:36Z

Although, a simpler (and better) path would just be to just expose it under a different name. We'll break the API in the next version anyway.

Ok, in that case I'll go ahead and do that, maybe with the name create_surface_from_raw for now? (Obviously feel free to bikeshed.)

Sounds great, thank you!

(to clarify, this would need to be done in a separate PR targeting hal-0.3 branch and bumping the patch versions)

jamsch0 · 2019-08-20T22:15:25Z

I've now opened #2968 with these changes as additive against the hal-0.3 branch. Will address your remaining feedback there.

kvark · 2019-08-20T22:41:21Z

@jchapman127 would you mind landing this PR though? We still want this very change in master.

jamsch0 · 2019-08-20T22:46:01Z

@jchapman127 would you mind landing this PR though? We still want this very change in master.

Does that include changing create_surface to return Result<InitError>?

kvark · 2019-08-20T22:49:36Z

@jchapman127 would you mind landing this PR though? We still want this very change in master.

Does that include changing create_surface to return Result<InitError>?

That would be great, yeah! Especially, since you've done this for hal-0.3 already, so it shouldn't be difficult to port back here.

2968: [hal-0.3] Add support for `raw-window-handle` r=kvark a=jchapman127 Fixes #2956. Closes #2967. PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [ ] tested examples with the following backends: - [x] `rustfmt` run on changed code Non-breaking change version of #2967. Currently Android is not supported in raw-window-handle and therefore unimplemented in gfx-backend-vulkan - waiting on rust-windowing/raw-window-handle#12 to be resolved. Co-authored-by: James Chapman <[email protected]>

2968: [hal-0.3] Add support for `raw-window-handle` r=kvark a=jchapman127 Closes #2956. PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [ ] tested examples with the following backends: - [x] `rustfmt` run on changed code Non-breaking change version of #2967. Currently Android is not supported in raw-window-handle and therefore unimplemented in gfx-backend-vulkan - waiting on rust-windowing/raw-window-handle#12 to be resolved. Co-authored-by: James Chapman <[email protected]>

jamsch0 added 5 commits August 20, 2019 18:25

Replace winit with raw-window-handle for gfx-backend-empty

ba6acee

Replace winit with raw-window-handle for gfx-backend-dx11

196984c

Replace winit with raw-window-handle for gfx-backend-dx12

3654d46

Replace winit with raw-window-handle for gfx-backend-metal

9712bcc

Replace winit with raw-window-handle for gfx-backend-vulkan

6e9a03c

Android support is currently missing.

jamsch0 mentioned this pull request Aug 20, 2019

Please add support for the new raw-window-handle crate #2956

Closed

jamsch0 added 2 commits August 20, 2019 18:43

rustfmt

9c8e881

Replace winit with raw-window-handle for gfx-backend-gl

135c97a

Only applies to `wgl` feature.

kvark requested changes Aug 20, 2019

View reviewed changes

jamsch0 commented Aug 20, 2019

View reviewed changes

Refactor Vulkan backend create_surface into single match expression

862c2db

jamsch0 mentioned this pull request Aug 20, 2019

[hal-0.3] Add support for raw-window-handle #2968

Merged

4 tasks

jamsch0 closed this Aug 20, 2019

kvark reopened this Aug 20, 2019

Return Result::Err(InitError) instead of panicing

48636aa

Loosen raw-window-handle version requirement to 0.1

a6b353d

kvark mentioned this pull request Oct 9, 2019

[hal-0.3] Upgrade to raw-window-handle v0.3.0 #3037

Closed

4 tasks

antonok-edm mentioned this pull request Oct 9, 2019

Replace winit with raw-window-handle in backend crates #3038

Merged

4 tasks

bors bot closed this in fb76260 Oct 9, 2019

bors bot closed this in #3038 Oct 9, 2019

Replace winit with raw-window-handle in backend crates #2967

Replace winit with raw-window-handle in backend crates #2967

Uh oh!

Conversation

jamsch0 commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

jamsch0 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

kvark Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

jamsch0 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

kvark Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

jamsch0 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamsch0 Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

jamsch0 commented Aug 20, 2019

Uh oh!

kvark commented Aug 20, 2019

Uh oh!

jamsch0 commented Aug 20, 2019

Uh oh!

kvark commented Aug 20, 2019

Uh oh!

jamsch0 commented Aug 20, 2019

Uh oh!

kvark commented Aug 20, 2019

Uh oh!

jamsch0 commented Aug 20, 2019

Uh oh!

kvark commented Aug 20, 2019

Uh oh!

Uh oh!

Replace `winit` with `raw-window-handle` in backend crates #2967

Replace `winit` with `raw-window-handle` in backend crates #2967

jamsch0 commented Aug 20, 2019 •

edited

Loading

jamsch0 Aug 20, 2019 •

edited

Loading