Enable portability features on macOS #73
Conversation
The-Minecraft-Scientist
changed the title
| @@ -4,6 +4,7 @@ use std::os::raw::c_char; | |||
| use std::sync::Arc; | |||
|
|
|||
| use ash::vk; | |||
| use ash::vk::{DeviceCreateFlags, PhysicalDeviceFeatures2, PhysicalDevicePortabilitySubsetFeaturesKHR}; | |||
There was a problem hiding this comment.
Use the vk:: prefix for all ash structures and don't import them directly. So instead of DeviceCreateFlags use vk::DeviceCreateFlags
| let mut portability_subset_features = PhysicalDevicePortabilitySubsetFeaturesKHR::default(); | ||
| #[cfg(target_os = "macos")] { | ||
| let mut dummy_features = PhysicalDeviceFeatures2::builder().push_next(&mut portability_subset_features); | ||
| unsafe {instance.vk().get_physical_device_features2(physical_device, &mut dummy_features);} |
There was a problem hiding this comment.
The problem with this is that if macos ever does add a first party vulkan implementation or if mvk doesn't need portability subset any more this will break. You should query if the extension is supported by the device and only then add it.
| unsafe {instance.vk().get_physical_device_features2(physical_device, &mut dummy_features);} | ||
| } | ||
| let device_create_info = device_create_info.queue_create_infos(queue_create_infos.as_slice()) | ||
| .push_next(&mut portability_subset_features); |
There was a problem hiding this comment.
This will break any system that doesn't support portability subset. Also this should be done in configure_device
| @@ -5,6 +5,7 @@ use std::str::Utf8Error; | |||
| use std::sync::Arc; | |||
|
|
|||
| use ash::vk; | |||
| use ash::vk::InstanceCreateFlags; | |||
| @@ -134,10 +135,23 @@ pub fn create_instance(config: InstanceCreateConfig) -> Result<Arc<InstanceConte | |||
| .engine_version(vk::make_api_version(0, BUILD_INFO.version_major, BUILD_INFO.version_minor, BUILD_INFO.version_patch)) | |||
| .api_version(max_api_version.into()); | |||
|
|
|||
| // this has to be outside of the if statement because it is prematurely dropped otherwise | |||
| let portability_name = CString::new("VK_KHR_portability_enumeration").unwrap(); | |||
| let request_portability = cfg!(target_os="macos"); | |||
There was a problem hiding this comment.
Instead of checking for macos and then forcing the extension it would be better to check if the extension is present and then enable it no matter the platform.
| required_extensions_str.push(portability_name.as_c_str().as_ptr()); | ||
| InstanceCreateFlags::ENUMERATE_PORTABILITY_KHR | ||
| } else { | ||
| InstanceCreateFlags::default() |
There was a problem hiding this comment.
Use ::empty() its not a functional difference but a bit clearer imo.
| let request_portability = cfg!(target_os="macos"); | ||
| if request_portability { | ||
| let portability_subset_name = CString::new("VK_KHR_portability_subset").unwrap(); | ||
| if !device.is_extension_supported(&portability_subset_name) { |
There was a problem hiding this comment.
If a device doesn't have portability subset that doesn't mean that it won't work but that it doesn't need portability subset. So instead of failing if the extension is missing you should instead only query the extension if it is present.
It works, but I'm certain there is a more elegant way to request the PortabilitySubsetFeatures than what I did (which is probably down to me not understanding how the pNext chains are setup)