Wrong logic in physical device selection

[BlauHimmel]

https://github.com/inexorgame/vulkan-renderer/blob/3bb0e9d9ec9982c0d3d9052de7253c7b4c17d305/src/vulkan-renderer/settings_decision_maker.cpp#L346

Typo or intended?

Maybe it should be VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU instead of VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU here...

---

https://github.com/inexorgame/vulkan-renderer/blob/3bb0e9d9ec9982c0d3d9052de7253c7b4c17d305/src/vulkan-renderer/settings_decision_maker.cpp#L445

When more than one suitable graphics card exist, it is the PhysicalDevices that are 'suitable' rather than all PhysicalDevices that are enumerated by vkEnumeratePhysicalDevices should be looped to calculate the score.

[IAmNotHanni]

Hello there!

That is a bug indeed. If had to look at the line above:

if (gpu_type_1 == VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU || gpu_type_2 == VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU) {
    discrete_gpu_exists = true;
}

if (gpu_type_1 == VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU || gpu_type_2 == VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU) {
    integrated_gpu_exists = true;
}

Even just from the logic this can't make any sense at all. Thank you for finding this!

Solution

We could fix this, but I would suggest that we throw away the entire device selection code and replace it with a much simpler device selection code. The current code I wrote is completely over engineered, as pointed out by the Khronos foum as well: https://community.khronos.org/t/feedback-on-device-selection-mechanism/105337

A much much simpler approach would be to replace this with a simple std::sort algorithm, as pointed out by @yeetari. It would basically sort the available physical devices (gpus) according to the following rules:

• gpu type (VK_PHYSICAL_DEVICE_TYPE_DISCRETE_GPU > VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU > VK_PHYSICAL_DEVICE_TYPE_CPU> VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU > VK_PHYSICAL_DEVICE_TYPE_OTHER
• It would also filter out certain physical device types entirely, so maybe VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU is not available at all
• Sort by gpu memory size maybe?
• It would also allow the user to maybe pass a lambda into the gpu selection mechanism, so the user can specify extra criteria for the gpus, like only accept gpus that support geometry shaders.

Btw that is the following ticket: https://github.com/inexorgame/vulkan-renderer/issues/457 (When writing the ticket, we didn't know about this bug)

It would be awesome if you wanted to refactor this and to open a pull request :) Feel encouraged to join our Discord server as well: https://discord.com/invite/acUW8k7

best regards Hanni

[IAmNotHanni]

So that lambda I have in mind could look something like this btw:

bool accept_physical_device = [&](const VkPhysicalDeviceFeatures features) {
    // Check if anisotropic filtering is available!
    if (features.samplerAnisotropy != VK_TRUE) {
        // Not available? Sorry that is a knockout criteria...
        return false;
   }
   // add more fancy checks here...
   // nothing to complain about? well then this physical device is a candidate
   return true;
};

so then we could do something like this:

auto physical_device = select_physical_device(accept_physical_device);

The lambda is of course also allowed to be empty, meaning the user does not care.

[IAmNotHanni]

Reminder for me

• [x] Update reference page as well when updating the code

[IAmNotHanni]

I made a pull request for this.