-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable portability features on macOS #73
base: emulator2
Are you sure you want to change the base?
Enable portability features on macOS #73
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use vk::
prefix here as well
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)