Overv / VulkanTutorial

Tutorial for the Vulkan graphics and compute API
https://vulkan-tutorial.com
Creative Commons Attribution Share Alike 4.0 International
3.18k stars 519 forks source link

std::optional in queue families is confusing and seems unnecessary #132

Closed RealTrisT closed 5 years ago

RealTrisT commented 5 years ago

In the last part of /Drawing_a_triangle/Setup/Physical_devices_and_queue_families, this whole thing image could just simply be broken down into something like this

bool findQueueFamilies(VkPhysicalDevice dev, uint32_t* index = 0) {
    uint32_t queueFamilyCount = 0;

    vkGetPhysicalDeviceQueueFamilyProperties(dev, &queueFamilyCount, nullptr);

    if (!queueFamilyCount)return false;

    std::vector<VkQueueFamilyProperties> queueFamilies(queueFamilyCount);
    vkGetPhysicalDeviceQueueFamilyProperties(dev, &queueFamilyCount, queueFamilies.data());

    for (uint32_t i = 0; i < queueFamilyCount; i++) {
        if (queueFamilies[i].queueCount && queueFamilies[i].queueFlags & VK_QUEUE_GRAPHICS_BIT) {
            if(index)*index = i;
            return true;
        }
    }
    return false;
}

(with range-for I guess, you guys seem to really like those)

Maybe I'm just a bad programmer, but I had to go read into <optional> to understand this, and it seems really weird to go creating a struct, and do all this weird stuff, for something that is so simple.

I guess that's the whole point of <optional>, but it took me a bit of time to figure it out, I don't think it's doing much for the tutorial, especially given the style previously used. Just made it seem way more complicated than it was.

Overv commented 5 years ago

You'll see why we do it this way in the next chapter where we need the actual queue indices and not just a value that indicates whether they exist or not.

Can you explain what you found confusing about the concept of <optional>? I think it should be fairly self-explanatory even if you haven't seen it before (in other languages).

RealTrisT commented 5 years ago

You'll see why we do it this way in the next chapter where we need the actual queue indices and not just a value that indicates whether they exist or not.

I did in fact read it, that's why I put the optional pointer parameter there. The usage of mine would be something like this:

//Physical devices and queue families
bool isDeviceSuitable(VkPhysicalDevice device) {
    return findQueueFamilies(device);
}

//Logical device and queues (the next chapter)
uint32_t index = 0; 
findQueueFamilies(physicalDevice, &index);

VkDeviceQueueCreateInfo queueCreateInfo = {};
queueCreateInfo.sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO;
queueCreateInfo.queueFamilyIndex = index;
queueCreateInfo.queueCount = 1;

Can you explain what you found confusing about the concept of <optional>? I think it should be fairly self-explanatory even if you haven't seen it before (in other languages).

Nothing is inherently confusing to me about it, I just had never had to deal with it before (maybe because I prefer a more C-ish style), and all the sudden I'm seeing structures being declared and all these functions, and everything in my head gets way more complicated.

I also didn't find it self-explanatory, I've programmed a fair few languages, and what I find is common practice is to have an invalid value for the type, and if that's not possible (which is the case), just have something else tell you whether or not getting that value was successful. I can see optional being useful in a few scenarios, but I think it's a bit overkill for this particular one.

I guess I'm just being really neurotic about this, it's just that it could have taken me 2 seconds to understand, and it took me 2 minutes.

Overv commented 5 years ago

I did in fact read it, that's why I put the optional pointer parameter there.

Sorry for missing that part. As you will see in the Window surface chapter there are more queue families to find and return. I felt that having reference/pointer arguments for each of these was not a very scalable solution, which is why I introduced the struct approach.

The <optional> solution wasn't always there. Initially I stored the indexes into ints where -1 meant that the index was not found. This was changed some time ago because this approach required the variables to have an awkward signed type that required explicit casts to the type mandated in the spec (uint32_t). It would have been possible to instead use the maximum value of uint32_t as "invalid" value, but that too feels a bit dirty to me.

I hope it makes a bit more sense now what the motivation behind this particular approach is.

RealTrisT commented 5 years ago

You are totally right, I actually read on ahead into the window surface chapter on my way home, makes total sense. Sorry for bothering you with this, and thank you for being nice about it.

stored the indexes into ints where -1 meant that the index was not found (...) instead use the maximum value of uint32_t as "invalid" value

Signed -1 is the same as unsigned 0xFFFFFFFF. They are exactly same thing :D