Add external memory support for vulkan on windows & linux #275
Conversation
|
I've verified in a personal project that this code works. I'd like to get a review of it so we can hopefully get this merged! Going to ping @Jasper-Bekkers, hopefully this isn't too much of a hassle. No rush here just want to make sure you see it! |
src/vulkan/mod.rs
Outdated
| } | ||
|
|
||
| impl MemoryBlock { | ||
| #[allow(clippy::too_many_arguments, clippy::fn_params_excessive_bools)] |
There was a problem hiding this comment.
The refactor here should be to make a MemoryBlockCreateDesc that has these parameters.
| // On other platforms you can't create an external capable allocator, so this would be unreachable | ||
| #[cfg(any(windows, all(unix, not(target_vendor = "apple"))))] |
There was a problem hiding this comment.
I think on other platforms then we should fail with an error rather then silently failing to create an exportable texture.
There was a problem hiding this comment.
The error would occur elsewhere, see lines 843-845
| #[cfg(windows)] | ||
| let mut export_info = vk::ExportMemoryAllocateInfoKHR::default() | ||
| .handle_types(vk::ExternalMemoryHandleTypeFlags::OPAQUE_WIN32_KHR); | ||
| #[cfg(all(unix, not(target_vendor = "apple")))] | ||
| let mut export_info = vk::ExportMemoryAllocateInfoKHR::default() | ||
| .handle_types(vk::ExternalMemoryHandleTypeFlags::OPAQUE_FD_KHR); |
There was a problem hiding this comment.
Would you mind replacing these #[cfg] compilation guards with if cfg!()? None of this ash code is guarded behind conditional compilation, and that way all this code is modifiable and compile-testable even on platforms where that cfg!() expression evaluates to false.
There was a problem hiding this comment.
Trouble here is that they are different types, and they must exist until the allocation is actually done. So I could switch to if cfg!(), but I think the code would look worse (probably you would end up having an option for each type). Let me know if you still want me to make the change
|
I think I've addressed most comments |
This has been tested by my own project, though no in tree testing is there yet(not sure how I'd add that). Also, the testing is only for vulkan Linux but I suspect it would also work on Windows in its current state.
This short PR adds support for memory allocated for external use. The code probably can't be accepted in its current state. For example, it has a
#[allow(clippy::too_many_arguments, clippy::fn_params_excessive_bools)]attribute you probably wouldn't want in production code.Putting this out here for now and for review.
Based on discussion in #274, also going to ping @Jasper-Bekkers :)
Implementation details: the memory is only allocated with support for exporting it later. The user must manually export the memory later, though this isn't very difficult. Used as a reference was this article from vulkan docs.