Fixed VkResult Error on macOS VulkanSDK 1.3.216

[NewSkyLine-dev]

Since VulkanSDK 1.3.216 it is mandatory to use the VK_KHR_PORTABILITY_subset extension. Fixed by adding

create_info.flags = VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR;

aswell as adding VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME and VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME to the extensions array.
Issue: #6101
Note:
Maybe considure changing the extensions array to a vector, for better readability and for easier adding new extensions.

[wolfpld]

A proper implementation needs to check for the availability of the VK_KHR_portability_subset extension, and does not rely on #ifdefs.

The intent is that this extension will be advertised only on implementations of the Vulkan 1.0 Portability Subset, and not on conformant implementations of Vulkan 1.0. Fully-conformant Vulkan implementations provide all the required capabilities, and so will not provide this extension. Therefore, the existence of this extension can be used to determine that an implementation is likely not fully conformant with the Vulkan spec.

If this extension is supported by the Vulkan implementation, the application must enable this extension.

Additionally, the application must enable the VK_KHR_portability_enumeration extension to be able to enumerate non-conformant devices.

This extension allows applications to control whether devices that expose the VK_KHR_portability_subset extension are included in the results of physical device enumeration. Since devices which support the VK_KHR_portability_subset extension are not fully conformant Vulkan implementations, the Vulkan loader does not report those devices unless the application explicitly asks for them. This prevents applications which may not be aware of non-conformant devices from accidentally using them, as any device which supports the VK_KHR_portability_subset extension mandates that the extension must be enabled if that device is used.

[NewSkyLine-dev]

Thanks for the feedback, but I honestly don't know what you mean. If I read the documentation of 1.3.216 right for macOS, it is now enforcing the user, to enable the VK_KHR_PORTABILITY_subset extension. I did it by adding a contexpr to check the VulkanSDK version, if yes, I #define VULKAN_HAS_KHR_PORTABILITY later in the code, it checks if it is defined, If yes, it includes VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME and VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME to the extensions array. Do I have to check if the extension is even supported?
Maybe could you explain it further so, that I correct my PR?
Would be very welcome :)

[wolfpld]

Do I have to check if the extension is even supported?

Yes, basically you need to check if the three extensions you are using are supported by the implementation, and only then enable them. It is possible that a future Mac Vulkan implementation will be fully conformant, in which case it will no longer advertise the portability subset, so requesting it will be an error. It is also possible that some other non-Apple implementation provides a non-conformant driver that you want to support here, but restrict behind an Apple-specific #ifdef.

The two functions that return a list of available extensions are vkEnumerateDeviceExtensionProperties and vkEnumerateInstanceExtensionProperties.

Things get even more complicated because Omar wants to support previous releases of MoltenVK (mentioned in #6101). While the VK_KHR_portability_subset extension was "always" required to enable, the VK_KHR_portability_enumeration extension is a recent addition. So you may have to resort to some kind of #ifdefing based on the header version anyway.

Technically, it would be nice to extend the driver selection process to select a non-conforming implementation only if there are no conforming implementations. See the current selection logic below:

imgui/examples/example_sdl_vulkan/main.cpp

Lines 117 to 130 in 5c8041b

	// If a number >1 of GPUs got reported, find discrete GPU if present, or use first one available. This covers
	// most common cases (multi-gpu/integrated+dedicated graphics). Handling more complicated setups (multiple
	// dedicated GPUs) is out of scope of this sample.
	int use_gpu = 0;
	for (int i = 0; i < (int)gpu_count; i++)
	{
	VkPhysicalDeviceProperties properties;
	vkGetPhysicalDeviceProperties(gpus[i], &properties);
	if (properties.deviceType == VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU)
	{
	use_gpu = i;
	break;
	}
	}

[ocornut]

Maybe considure changing the extensions array to a vector, for better readability and for easier adding new extensions.

I agree. You could use a ImVector<const char*> there and copy result from the first call, then amend the vector in the new code-path.

Things get even more complicated because Omar wants to support previous releases of MoltenVK

In most cases we try to support old versions of SDK, but Apple ecosystems carrying a culture of aggressive-updating and MoltenVK (along with Vulkan) being known to be frequently updated I am OK if we don't support this if it can simplify examples and/or backends.

I can't comment on the other specifics here. Happy to merge when it feels like we have a concensus.

[wolfpld]

In most cases we try to support old versions of SDK, but Apple ecosystems carrying a culture of aggressive-updating and MoltenVK (along with Vulkan) being known to be frequently updated I am OK if we don't support this if it can simplify examples and/or backends.

Unfortunately, availability of header defines for these extensions ripples to other platforms as well. For example, both VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME and VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR first appeared only in Vulkan SDK 1.3.208, which would be a new requirement on all platforms, if these extensions would to be supported everywhere.

An alternative would be to provide own definitions for missing things. For example, if VK_KHR_portability_enumeration preprocessor macro is not defined, add the following:

#define VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR 0x00000001
#define VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME "VK_KHR_portability_enumeration"

But this is not a solution I would prefer.

[ocornut]

Note that we afaik are talking about the examples/ application, for which it would be more acceptable to raise version requirements. As opposed to Vulkan backend itself for which ideally we’d accept older versions (as people updating their version of dear imgui and relying on vanilla backends wouldn’t enjoy having to update an SDK to update dear imgui).

[zao]

This doesn't perform the correct test. The preprocessor doesn't know about constexpr variables and substitutes the not-defined name desired_version with an integer zero in the comparison.

[NewSkyLine-dev]

Updated the whole commit. Now there is a proper function, that checks if the extension has to be enabled. Also modernized the code with few C++ aspects(std::cerr, std::vector<>). Maybe check if out and give some feedback. :) Compiles on Windows, as well as on macOS.

[ocornut]

Also modernized the code with few C++ aspects(std::cerr, std::vector<>)

Please don't. This has turned this PR from "need some smal tweaks to merge" into "impossible to merge" :(
There are multiple examples to keep in sync, multi-viewport in docking branch which is a larger beast, and we have an explicit policy to avoid C++ std library as much as we can. So those changes are unfortunately moving us backward.

I just need the minimum amount of changes (closer to 32da8f3 but ok to refactor slightly to use ImVector<> exactly in that spot to simplify adding extension) that also happens to be correct.

Thank you for your patience.

[NewSkyLine-dev]

Alright. I understand. So I will change the modernized part to the old one, and change the extension part, to the new one. Is that correct so that it is “merge ready”, if there are no more conflicts?

[ocornut]

Right now there are hundreds of noisy changes in the PR so I would suggest to get back to square one.
I don't know what will be correct but I imagine I would follow wolfpld feedback above.

[NewSkyLine-dev]

Alright, changed it to the original one, only added the advantages of the extension vector and the function that checks if the enumeration extension has to be enabled.

[ocornut]

This is still using C++ library std::vector instead of ImVector.
I might be wrong as I am merely glancing at the changes but it also looks incorrect, as the IMGUI_VULKAN_DEBUG_REPORT path would override previous extension, there's duplicate code (create_into fields filled twice), they are unusual variables (e.g. enablePortabilitySubset), checkExtensions() seems insufficiently designed/named.

[wolfpld]

Why is this check disabling portability subset? If the extension is available, it must be enabled.

[NewSkyLine-dev]

I see your point. Now that I'm looking over everything again, I see that it would probably be easier to create two functions. One that checks the flag for VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR and one that checks if the VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME is enabled. And of course both check if the OS is macOS and the Vulkan version is >= 1.3.216. Is this the best solution? Maybe you know a better one. I am always grateful for better ideas.

[wolfpld]

Stop making unnecessary checks. What does the OS or driver version has to do with anything here? Read what the extension specification actually says, and follow that to the letter.

[NewSkyLine-dev]

Alright, I will look over it again, and will try to terminate all the C++ stuff and will change it to the ImVec version. And will look at the function again. It may very well be that something is not be as it should. Sry for the troubles.

[zao]

I've only looked over the code from a legacy SDK standpoint, haven't tried building it or looked at what older SDKs define.

[zao]

This test would fail for future major or minor version bumps such as 1.4.0 or 2.0.0 where the lesser version components reset to lower numbers.

[zao]

Are all these *_NAME and *_BIT_KHR names defined in all older Vulkan SDKs that we support, or do they need to be guarded by the preprocessor, conditionally defined by us, or use our own string literals and constants?

[NewSkyLine-dev]

I would have to look it up, if all *_NAME and *_BIT_KHR are in older versions. I only looked at the newest version. Thanks for pointing this out.

[NewSkyLine-dev]

So now I reduced everything to one function:

bool is_instance_extension_enabled(const char *extension_name)
{
    uint32_t extension_count = 0;
    VkResult result = vkEnumerateInstanceExtensionProperties(nullptr, &extension_count, nullptr);
    if (result != VK_SUCCESS)
    {
        return false;
    }

    ImVector<VkExtensionProperties> extensions;
    extensions.resize(extension_count);
    result = vkEnumerateInstanceExtensionProperties(nullptr, &extension_count, extensions.Data);
    if (result != VK_SUCCESS)
    {
        return false;
    }

    for (int i = 0; i < extensions.Size; i++)
    {
        if (strcmp(extensions[i].extensionName, extension_name) == 0)
        {
            return true;
        }
    }
    return false;
}

It checks if an instance extension is enabled, and returns true, if so, and false, if not.
Now the parts that changed in the code itself:

#ifdef __APPLE__
if (is_instance_extension_enabled(VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME))
    create_info.flags = VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR;
#endif

ImVector<const char*> extensions;
uint32_t extensions_count = 0;
const char** glfw_extensions = glfwGetRequiredInstanceExtensions(&extensions_count);
for (uint32_t i = 0; i < extensions_count; i++)
    extensions.push_back(glfw_extensions[i]);
#ifdef __APPLE__
if (is_instance_extension_enabled(VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME))
    extensions.push_back(VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME);
#endif 

I haven't committed it yet, because I don't know if these solutions are any good.
@wolfpld or @ocornut, would you be so kind and check if these functions are as requested, or if they even do what they should do? :) I will commit as soon as there is an OK from your sites. :)

[ocornut]

Easier to commit so we can review.
Right now I put this aside for a bit, I'll look at it in details later when I can test myself on a Mac.

[NewSkyLine-dev]

Accidentally closed PR. New is open on #6172 Sry for the trouble