Finish 0.4 update

[Friz64]

This PR picks up where #67 left off, cleans up the documentation and implements all remaining TODOs and concerns. #68 was also backported.

---

• Closes New release?  #58
• Closes Use separate types for each platform #60
• Closes Proposal: Remove all the cfg guards. #63
• Closes Proposal: Revert Option<NonNull<c_void>> back to *mut c_void #69

[Lokathor]

I will have a close look at this the next time i have some time for Rust

[madsmtm]

This looks nice, but I think you should keep the commit history from #67 - maybe merge that first, and rebase this on top

[Friz64]

but I think you should keep the commit history from #67 - maybe merge that first, and rebase this on top

Open to doing this if desired.

[Lokathor]

I normally don't place a strong value on git history stuff, as long as the code that's published is correct that's good enough for me.

However, if you know how to merge the history (or whatever you call it) and it's easy enough to do, then it seems like a good idea, since it was asked for.

[Lokathor]

or, if you need me to actually merge #67 to make it work then i can do that.

[Friz64]

or, if you need me to actually merge #67 to make it work then i can do that.

Yes, i think we should do that and i'll rebase this as suggested.

[Lokathor]

alright, you're set.

[madsmtm]

Have a few minor nits, but doesn't matter much, approved from my side

[Friz64]

I also pushed a commit which moves the "Safety guarantees" documentation to the crate root, making it more prominent. In the previous location, it was easy to miss it.

[Lokathor]

Hmm, but when viewing in rustdoc you usually look at the page for the item itself. In this case the page for the trait. Myself, I rarely look at module level documentation to find out info on a specific item. Also, the rust-analyzer tooltip shows the item's docs not the docs of the module the item goes in.

We could have a message in both places, but i think the trait should have the full safety message and the module should have a note to please see the docs of the trait for the rules.

[Friz64]

We could have a message in both places, but i think the trait should have the full safety message and the module should have a note to please see the docs of the trait for the rules.

This is the best solution, thanks!

[i509VCB]

I'm guessing from_has_raw_window_handle exists over just implementing From<H: HasRawWindowHandle> to make trustng a window handle more explicit?

[Lokathor]

Yeah, because there's unsafe stuff going on we didn't want to just use a standard conversion trait. You should have to pay just a little more attention about what's happening.

[Lokathor]

If everyone is happy I'll merge this in a few hours.

[maroider]

I just have a minor issue with naming consistency. There are a couple of types/enum variants which derive their names from some other underlying technology/library whose name contains an acronym and is typically written in a stylized way:

• UIKit
  • RawWindowHandle::UIKit
  • UIKitHandle
• XCB
  • RawWindowHandle::Xcb
  • XcbHandle
• WinRT
  • RawWindowHandle::WinRT
  • WinRTHandle
• The Android NDK
  • RawWindowHandle::AndroidNDK
  • AndroidNDKHandle

Out of these, RawWindowHandle::Xcb and XcbHandle are outliers, as the name of the type/enum variant doesn't follow the same pattern as the others.

I see three options here, and I'm not entirely happy with any of them.

1. Make everything follow the regular capitalization rules for Rust types, i.e. acronyms only get one capital letter.
   This does admittedly look quite ugly, but AndroidNDKHandle is going to look ugly to me no matter which way it's written.
2. Use RawWindowHandle::XCB, XCBHandle
3. Don't do anything. Keep everything as-is.
   I sometimes get hung up on the most inconsequential of details, and this isn't that important in the end.

[Friz64]

I am in favor of option 1. While none of these are perfect, option 1 is the only correct one.

[madsmtm]

I agree with @Friz64.

So it would be UiKit[Handle], Xcb[Handle], WinRt[Handle] and AndroidNdk[Handle], right?

[Lokathor]

I have no strong opinion here, (1) seems reasonable enough.

[Friz64]

So it would be UiKit[Handle], Xcb[Handle], WinRt[Handle] and AndroidNdk[Handle], right?

Yes exactly.

And regarding the changelog: Is it complete now?

[Lokathor]

Yes, I believe we're all set. I'll merge later if no one speaks up.

[i509VCB]

So a lifetime question regarding the trusted handle. Per the documentation the TrustedWindowHandle says you assert the handle is trusted when you insert it into the struct.

The idea of trusted is very vague, I'd guess trusted means you refer to all required capabilities to refer to a window completely.

The trusted handle is not Cloneable, so we can rely on that living as long as the scope it was initialized for. However I don't think this garuntees the backing resources of the handle will live longer than the handle itself.

I may consider what vulkano does with their surface object, which also takes a window object, be it a winit window or an Arc to act as a sort of anchor for making the trusted handle live long enough.

[madsmtm]

Yeah, you do have a point - Maybe the API should be completely changed to something like:

pub trait HasTrustedWindowHandle {
    fn trusted_window_handle<'a>(&'a self) -> TrustedWindowHandle<'a>;
}

#[derive(...)]
pub struct TrustedWindowHandle<'a> {
    raw: RawWindowHandle,
    _phantom: PhantomData<&'a ()>,
}

impl<'a> TrustedWindowHandle<'a> {
    pub const unsafe fn from_raw(raw: RawWindowHandle) -> Self { ... }
    pub fn as_raw(&self) -> RawWindowHandle { ... }
}

impl From<TrustedWindowHandle<'a>> for RawWindowHandle { ... }

TrustedWindowHandle would now be the main way to interact with this library.

(Actually, we could rename TrustedWindowHandle -> WindowHandle here).

But this still leaves open what TrustedWindowHandle really guarantees: For example, is the window guaranteed to be able to be interacted with by calling OS apis? And is this even possible to guarantee this for all platforms (e.g. when closing the window, is it still valid to render into?).

[Lokathor]

The trusted part is supposed to be indicating that the pointers contained, if non-null, are valid (at least when you make the value).

[madsmtm]

But I don't think that is a strong enough guarantee to help e.g. gfx::hal::Instance::create_surface become safe?

[Lokathor]

they just say the handle has to be valid i their safety rules.

if there are other undocumented requirements then that should be documented.

[madsmtm]

Well, I don't know much about gfx, but I think they actually have the requirement that the window may not be deallocated - for example, on MacOS, dropping winit::Window results in closing it and it being deallocated (after AppKit has processed the event). So now, a previously valid TrustedWindowHandle would become invalid.

So the following example would cause undefined behaviour, because things created from the TrustedWindowHandle was allowed to live longer than the data:

let window: winit::Window = ...;
let handle = TrustedWindowHandle::from_has_raw_window_handle(&window);
// Changed to take the current TrustedWindowHandle, which would make it "safe"
let surface = backend::Instance::create_surface(handle)?;
drop(window);
// Use `surface` later on, causes undefined behaviour because the window was deallocated.

[Lokathor]

So my position on this sort of thing has usually been that you can have an unsafe constructor and then part of the unsafe promise is that you won't use the value (or things based on it in this case) for "too long". So if from_has_... is an unsafe call, then i think we could justify the rest of things.

Because the alternative is that we can never ever claim safe surface creation, when we know that the creation step itself isn't causing issues.

probably the surface and window should be bundled together by some code that knows how to make the surface bot outlive the window.

[maroider]

probably the surface and window should be bundled together by some code that knows how to make the surface bot outlive the window.

I've been meaning to bring this up, but the gfx-rs folks have been thinking about this as well in gfx-rs/wgpu#1463. As I understand it, the thing that's most interesting to guarantee is that the handles you get stay valid after you've got a hold of them.

In gfx-rs/wgpu#1463, it was suggested that things (in this case: wgpu::Surface) which need a window (T: HasRawWindowHandle) should take it by value, provide methods to get the window back as a reference or by value, and that raw-window-handle should add the following implementations for HasRawWindowHandle:

impl<'a, T: HasRawWindowHandle> HasRawWindowHandle for &'a T { /* .. */ }
impl<T: HasRawWindowHandle> HasRawWindowHandle for Rc<T> { /* .. */ }
impl<T: HasRawWindowHandle> HasRawWindowHandle for Arc<T> { /* .. */ }

With this in mind, I don't see where TrustedWindowHandle would fit in.

[madsmtm]

So if from_has_... is an unsafe call, then i think we could justify the rest of things.

It would, but that defeats the bigger purpose of TrustedWindowHandle, which is to allow user code to be completely safe.

probably the surface and window should be bundled together by some code that knows how to make the surface bot outlive the window.

This would be possible on MacOS at least (by retaining the NSWindow), don't know about the other platforms, but it would still be unsound without a lifetime parameter on TrustedWindowHandle:

let window: winit::Window = ...;
let handle = TrustedWindowHandle::from_has_raw_window_handle(&window);
drop(window);

// Later on when the window has been deallocated:
let surface = backend::Instance::create_surface(handle)?;

[madsmtm]

In gfx-rs/wgpu#1463, it was suggested that things (in this case: wgpu::Surface) which need a window (T: HasRawWindowHandle) should take it by value

Thanks for the reference! This is definitely a path forward, though I think it's pretty unergonomic that the user would no longer have full control over the window.

But then again, it might be unsafe if they did, or if the surface didn't somehow take mutable ownership (e.g. attempting to create two surfaces on the same window would need to be guarded against, since it would very likely cause undefined behaviour).

[Lokathor]

it would still be unsound without a lifetime parameter on TrustedWindowHandle

Kinda, you'd have to not have a surface and window separate, they'd have to be combined so that you can't drop them in the wrong order. Then the end user code would only handle the bundle and not interact with the inner fields directly.

to tell you the truth I don't remember why TrustedWindowHandle was developed, because it's not really needed for end users to have all safe code.

[msiglreith]

Would it be an option to keep the Option<NonNull<_>> variant for Android at least? Tbh not sure atm how to resolve this.
Context: rust-windowing/winit#1624 (comment)

[Lokathor]

@msiglreith I'm not sure I fully understand the problem. The winit folks just don't want to return a null pointer when a handle isn't available? Because that is definitely what should happen.

And we took out all the Option<NonNull<c_void>> stuff because Rust makes it a royal pain in the butt to manually be converting between ONN and a pointer all the time.

[msiglreith]

yeah.. well I guess I'm fine with it as I can workaround that part

[Lokathor]

We could maybe make the docs more clear about the expectations, and then you could use that as the basis for a PR to winit to just return the darn null pointer, if that would help you.

[DJMcNab]

I want TrustedWindowHandle for bevy_window to ensure that it is not locked to a specific windowing library (to make using e.g. rust-sdl2 easier/a drop in replacement)

In this case, it would be the job of bevy_winit to remove the TrustedWindowHandle component (or even the entire window 👀) before it drops the winit window.

If the conversation here is correct, it seems that completely decoupling these is going to be difficult, and need some more thinking.

I think the most viable choice is BevyWindow(Arc<dyn HasRawWindowHandle>) with an unsafe constructor guaranteeing it's possible to create a swapchain from it.

[maroider]

In any case, I think the decision on the safety around HasRawWindowHandle shouldn't block this PR, since this PR doesn't touch RawWindowHandle, TrustedWindowHandle, or the version number (despite the title).

[Lokathor]

Ah, yes, right.

I'll merge as is and we can discuss further if needed before a release.

[maroider]

we can discuss further if needed before a release.

To that end, I've opened a separate issue (#71).

[pythonesque]

I want TrustedWindowHandle for bevy_window to ensure that it is not locked to a specific windowing library (to make using e.g. rust-sdl2 easier/a drop in replacement)

In this case, it would be the job of bevy_winit to remove the TrustedWindowHandle component (or even the entire window 👀) before it drops the winit window.

If the conversation here is correct, it seems that completely decoupling these is going to be difficult, and need some more thinking.

I think the most viable choice is BevyWindow(Arc<dyn HasRawWindowHandle>) with an unsafe constructor guaranteeing it's possible to create a swapchain from it.

FWIW, if the HasRawWindowHandle implementation for Arc goes through, I think what you proposed should work without needing anything special. However, due to Arc::make_mut, I'm not sure the Arc implementation is actually safe :| I need to think about this more. A Pin version definitely is possible though, you'd have Pin<Arc<UnpinWrapper<dyn HasRawWindowHandle>>> in that case.

I'm not sure why you feel you need an unsafe constructor here, though. If you need the actual window elsewhere, the way you construct the dyn HasRawWindowHandle version is actually as a typecast from the generic version, so you can have Arc<W> and Arc<dyn HasRawWindowHandle> at the same time pointing to the same thing (same applies to the pinned version), or you could make some trait like trait WindowTrait : HasRawWindowHandle { fn create_swapchain(&self) -> ... } and use Arc<dyn WindowTrait>. Am I missing where the unsafe should be needed?

[a1phyr]

However, due to Arc::make_mut, I'm not sure the Arc implementation is actually safe :|

It is not a issue because Arc::make_mut requires T: Clone and therefore T: Sized which is not the case for dyn HasRawWindowHandle

[pythonesque]

However, due to Arc::make_mut, I'm not sure the Arc implementation is actually safe :|

It is not a issue because Arc::make_mut requires T: Clone and therefore T: Sized which is not the case for dyn HasRawWindowHandle

From what I have heard, this isn't necessarily something that can be relied on going forward to prevent moves, while Pin can.

[Lokathor]

Can someone explain the Arc::make_mut concern in more detail? Is the idea that someone could run a mem::swap using the &mut RawWindowHandle to change the value out from under you?