Vulkan support

[Rua]

This includes Rust wrappers for SDL's Vulkan functions. I've also included an example program in the readme, much like the existing one for OpenGL.

edit by @Cobrand: Closes #784

[Cobrand]

Looks good to me! Can you just address the concern I mentioned here? Even if you don't address it right away, let's at least put it in the documentation if that's too much for a first step.

[Cobrand]

Do you have anything else in mind before merging or is this ready to merge?

[Rua]

If you think nothing else is needed, I think it's good to go.

[SamP20]

I've come across a potential issue with the vulkan example. In trying to port the example here to use rust-sdl2, I came across an issue where Surface<W> expects W to implement the trait Send (this doesn't become apparent until you try to call begin_render_pass).

Unfortunately I'm not sure what to use in place of window.context() when calling Surface::from_raw_surface. I can get the example to run by passing an empty tuple, however this crashes on exit (probably due to the window being dropped before the surface). This wasn't an issue with the original winit example since winit::Window implements Send.

It may be that this is an unnecessary restriction with vulkano in which case I'll raise an issue there instead.

[Rua]

I'm not really understanding how begin_render_pass is affected by the type W. What is your code and what error are you getting?

[SamP20]

I have uploaded my code here https://github.com/samp20/HelloTriangle/tree/9b6d066964c2c0b5501d9171182877e5ea374a73. You can swap my comments on lines 70/71 to remove the hacky fix.

Here are the errors below:

   Compiling hello_triangle v0.1.0 (file:///C:/Users/<snip>/hello_triangle)
warning: unused import: `sdl2::video::Window`
  --> src\main.rs:11:5
   |
11 | use sdl2::video::Window;
   |     ^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default

warning: unused import: `std::rc::Rc`
  --> src\main.rs:15:5
   |
15 | use std::rc::Rc;
   |     ^^^^^^^^^^^

warning: unused import: `std::sync::Mutex`
  --> src\main.rs:17:5
   |
17 | use std::sync::Mutex;
   |     ^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `std::rc::Rc<sdl2::video::WindowContext>: std::marker::Send` is not satisfied in `vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>`
   --> src\main.rs:255:14
    |
255 |             .begin_render_pass(framebuffers.as_ref().unwrap()[image_num].clone(), false,
    |              ^^^^^^^^^^^^^^^^^ `std::rc::Rc<sdl2::video::WindowContext>` cannot be sent between threads safely
    |
    = help: within `vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<sdl2::video::WindowContext>`
    = note: required because it appears within the type `vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>>`
    = note: required because it appears within the type `vulkano::swapchain::Swapchain<std::rc::Rc<sdl2::video::WindowContext>>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::swapchain::Swapchain<std::rc::Rc<sdl2::video::WindowContext>>>`
    = note: required because it appears within the type `vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>`
    = note: required because it appears within the type `((), std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>)`
    = note: required because it appears within the type `vulkano::framebuffer::Framebuffer<std::sync::Arc<vulkano::framebuffer::RenderPass<main::scope::CustomRenderPassDesc>>, ((), std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>)>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::framebuffer::Framebuffer<std::sync::Arc<vulkano::framebuffer::RenderPass<main::scope::CustomRenderPassDesc>>, ((), std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>)>>`

error[E0277]: `std::rc::Rc<sdl2::video::WindowContext>` cannot be shared between threads safely
   --> src\main.rs:255:14
    |
255 |             .begin_render_pass(framebuffers.as_ref().unwrap()[image_num].clone(), false,
    |              ^^^^^^^^^^^^^^^^^ `std::rc::Rc<sdl2::video::WindowContext>` cannot be shared between threads safely
    |
    = help: within `vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>`, the trait `std::marker::Sync` is not implemented for `std::rc::Rc<sdl2::video::WindowContext>`
    = note: required because it appears within the type `vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::swapchain::Surface<std::rc::Rc<sdl2::video::WindowContext>>>`
    = note: required because it appears within the type `vulkano::swapchain::Swapchain<std::rc::Rc<sdl2::video::WindowContext>>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::swapchain::Swapchain<std::rc::Rc<sdl2::video::WindowContext>>>`
    = note: required because it appears within the type `vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>`
    = note: required because it appears within the type `((), std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>)`
    = note: required because it appears within the type `vulkano::framebuffer::Framebuffer<std::sync::Arc<vulkano::framebuffer::RenderPass<main::scope::CustomRenderPassDesc>>, ((), std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>)>`
    = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<vulkano::framebuffer::Framebuffer<std::sync::Arc<vulkano::framebuffer::RenderPass<main::scope::CustomRenderPassDesc>>, ((), std::sync::Arc<vulkano::image::SwapchainImage<std::rc::Rc<sdl2::video::WindowContext>>>)>>`

Edited to reference specific commit

[Rua]

It looks like this is a possible oversight in Vulkano, so you may want to bring this up with them. I have no idea what the right solution would be, whether Surface should require W to be Send + Sync or that this restriction should be lifted for begin_render_pass. That is up to the Vulkano developers I suppose.

In any case, I wonder what is currently preventing WindowContext from being Send + Sync. At the very least, Window::context() ought to return Arc<WindowContext> instead of Rc<WindowContext>, but the problem is also VideoSubsystem not being multithreaded, which appears to be by design according to comments in its source code.

[Cobrand]

I can confirm that VideoSubsystem does not support Send on purpose, the Windows API (I think for direct3d?) crashes when calling from 2 different threads (even without race conditions). In any way, SDL was very clear about this: this part is not thread safe, but other parts may be (I think the audio parts can be thread safe for instance)

[SamP20]

I have created an issue here vulkano-rs/vulkano#994. Let me know if I've missed anything (It's rather late here).

A crude temporary fix is to pass an empty tuple into Surface::from_raw_surface, however this breaks Rust's safety and we now have to manually make sure Window outlives Surface

[Rua]

Should I remove the Vulkan example code?

[SamP20]

I came up with a fix for the Send+Sync issue. I created a small wrapper called Sendable that implements Send and Sync, but checks the current thread before allowing you to access the internal object. It also panic!s if you drop it from a different thread to where it was created. The updated code is here: https://github.com/samp20/HelloTriangle/tree/c9a3db54b0396d9ab4af9dfafda06cafe88024e8

Unfortunately with that fix I'm still getting a crash on close. The debugger shows the error occuring when SDL_DestroyWindow gets called:

Unloaded 'C:\Windows\System32\userenv.dll'.
Unloaded 'C:\Windows\System32\profapi.dll'.
Unloaded 'C:\Windows\System32\igd10iumd64.dll'.
Unloaded 'C:\Windows\System32\ncrypt.dll'.
Unloaded 'C:\Windows\System32\ntasn1.dll'.
Unloaded 'C:\Windows\System32\bcrypt.dll'.
Unloaded 'C:\Windows\System32\d3d10_1.dll'.
Unloaded 'C:\Windows\System32\d3d11.dll'.
Unloaded 'C:\Windows\System32\dxgi.dll'.
Unloaded 'C:\Windows\System32\d3d10_1core.dll'.
Unloaded 'C:\Windows\System32\dlumd9.dll'.
The thread 6508 has exited with code 0 (0x0).
The thread 11064 has exited with code 0 (0x0).
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
The thread 8568 has exited with code 0 (0x0).
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
The thread 9764 has exited with code 0 (0x0).
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
Exception thrown at 0x0000000051FB4100 (nvoglv64.dll) in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Unloaded 'C:\Windows\System32\nvoglv64.dll'.
Unloaded 'C:\Windows\System32\setupapi.dll'.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Unloaded 'C:\Windows\System32\wintrust.dll'.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Unloaded 'C:\Windows\System32\msasn1.dll'.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Unloaded 'C:\Windows\System32\crypt32.dll'.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.
Exception thrown at 0x0000000051FB4100 in hello_triangle.exe: 0xC0000005: Access violation executing location 0x0000000051FB4100.

It looks like something is trying to read a null pointer, although I'm not sure how to debug it further. I'm curious to see if anyone else can reproduce it. It might be that this is a bug with SDL2 itself, or my graphics drivers. The original winit example runs without any problems.

[Rua]

I think for now the only working solution is to pass () when creating the Vulkano Surface, and rely on Rust's drop order to guarantee that the Surface is dropped before the Window. I noticed just now that the order in which members of a struct are dropped is explicitly defined: rust-lang/rfcs#1857 . So you can guarantee the drop order even within a struct, the Vulkano Surface must appear in the struct definition before the SDL Window member.

[ghost]

Hi there,

Just wondering what the status is on getting this PR merged. Are we waiting for vulkano-rs/vulkano#994 to be resolved first? Should we be?

[Cobrand]

I'm not really following this closely so I don't really know if we should be, but in any case I'm against putting on master code that is potentially unsafe and/or unstable. If you can convince me otherwise that this PR is safe and stable for now (even if it's missing functionalities), I'm going to approve it. Otherwise, if there is something blocking it in another repo or whatever, I'm going to postpone this merge for when we have a solution.

[Rua]

The code itself is safe. The only unsafe part is when you try to mesh it with Vulkano, Vulkano is what forces you to use SDL in an unsafe way. And even then it can be used safely if you keep the destruction order in mind. Since that's a Vulkano detail and not an SDL detail, it shouldn't affect SDL.

[SamP20]

I know this is a closed PR, but I'd like to say that my previous issues in #785 (comment) seem to be fixed now. I've tested my https://github.com/SamP20/HelloTriangle example on my desktop (GeForce GTX 1070ti) and updated laptop drivers (GeForce 845M), and both work well now :)