Vulkan-Tutorial icon indicating copy to clipboard operation
Vulkan-Tutorial copied to clipboard

Lesson 5 text & code mismatch

Open TomServeaux opened this issue 5 months ago • 3 comments

In lesson 5, the section "Querying for Presentation Support" when it talks about how to calculate presentIndex within the body of createLogicalDevice() it says:

Add a call to it in the same loop as the VK_QUEUE_GRAPHICS_BIT

There's no such loop in the code (by which I mean the code linked to at the bottom of the page, in the repo called 05_window_surface.cpp) and no such VK_QUEUE_GRAPHICS_BIT. I'm guessing that in an old version of the tutorial it used the C API and found graphicsIndex through a loop rather than the ranges:: stuff?

And then in the very next section "Creating the Presentation Queue" it has the full code for the upgraded createLogicalDevice() where it calculates presentIndex for real using different code than (briefly) mentioned above. However, even this full version of createLogicalDevice() doesn't match what's in the code. I'm looking at the bottom of the function where (in the text) it uses C++20 designated initializers inconsistently and also seems to be manually setting up the pointer structure that was obviated by the structure chain in the previous lesson.

I'm probably botching the explanation, just look at the function in the tutorial and the actual code.

TomServeaux avatar Jul 22 '25 18:07 TomServeaux

Also, the assert statement previously in createLogicalDevice() is removed here without comment, presumably because it's handled by added exception-throwing. I suggest that 1) when removing something from a previous lesson say why, people may leave it in by accident (which is harmless here but might not always be?) and 2) stick with the exception-throwing throughout instead of asserts or other ad hoc error displays since that's what main is testing for anyway.

TomServeaux avatar Jul 22 '25 18:07 TomServeaux

AND I think there might be an error in the code for createLogicalDevice() that is not present in the (apparently older) text version.

Text:

auto presentIndex = physicalDevice.getSurfaceSupportKHR( graphicsIndex, *surface )
                                       ? graphicsIndex
                                       : static_cast<uint32_t>( queueFamilyProperties.size() );
if ( presentIndex == queueFamilyProperties.size() )
...

Code:

auto presentIndex = physicalDevice.getSurfaceSupportKHR( graphicsIndex, *surface )
                                      ? graphicsIndex
                                      : ~0;
if ( presentIndex == queueFamilyProperties.size() )
...

Isn't the code version wrong? Doesn't it only work if queueFamilyProperties.size() is 0?

TomServeaux avatar Jul 22 '25 18:07 TomServeaux

moreover correct me if im wrong but, we specificly query for the case where presentIndex != graphicsIndex then throw it all out of the window when creating the queues,

// query for Vulkan 1.3 features
vk::StructureChain<vk::PhysicalDeviceFeatures2, vk::PhysicalDeviceVulkan13Features, vk::PhysicalDeviceExtendedDynamicStateFeaturesEXT> featureChain = {
    {},                               // vk::PhysicalDeviceFeatures2
    {.dynamicRendering = true },      // vk::PhysicalDeviceVulkan13Features
    {.extendedDynamicState = true }   // vk::PhysicalDeviceExtendedDynamicStateFeaturesEXT
};

// create a Device
float                     queuePriority = 0.0f;
vk::DeviceQueueCreateInfo deviceQueueCreateInfo{ .queueFamilyIndex = graphicsIndex, .queueCount = 1, .pQueuePriorities = &queuePriority };
vk::DeviceCreateInfo      deviceCreateInfo{ .pNext = &featureChain.get<vk::PhysicalDeviceFeatures2>(),
                                            .queueCreateInfoCount = 1,
                                            .pQueueCreateInfos = &deviceQueueCreateInfo,
                                            .enabledExtensionCount = static_cast<uint32_t>(requiredDeviceExtension.size()),
                                            .ppEnabledExtensionNames = requiredDeviceExtension.data() };

device = vk::raii::Device( physicalDevice, deviceCreateInfo );
graphicsQueue = vk::raii::Queue( device, graphicsIndex, 0 );
presentQueue = vk::raii::Queue( device, presentIndex, 0 );

like we explicitly handle the case where presentIndex != graphicsIndex,

if ( presentIndex == queueFamilyProperties.size() ) {
      // there's nothing like a single family index that supports both graphics and present -> look for another
      // family index that supports present
      for ( size_t i = 0; i < queueFamilyProperties.size(); i++) {
          if ( physicalDevice.getSurfaceSupportKHR( static_cast<uint32_t>( i ), *surface ) ) {
              presentIndex = static_cast<uint32_t>( i );
              break;
          }
      }
  }

then during queue creation just create one queue for graphics. the creation code should look something like this,

    vk::StructureChain<vk::PhysicalDeviceFeatures2, vk::PhysicalDeviceVulkan13Features,
                       vk::PhysicalDeviceExtendedDynamicStateFeaturesEXT>
        featureChain = {
            {},
            {.dynamicRendering = true},
            {.extendedDynamicState = true},
        };

    std::set<uint32_t> uniqueFamilies = {
        graphicsIndex,
        presentIndex,
    };

    float queuePriority = 0.0f;
    std::vector<vk::DeviceQueueCreateInfo> queueCreateInfos;
    for (auto index : uniqueFamilies) {
        queueCreateInfos.push_back({
            .queueFamilyIndex = index,
            .queueCount = 1,
            .pQueuePriorities = &queuePriority,
        });
    }
    vk::DeviceCreateInfo deviceCreateInfo{
        .pNext = &featureChain.get(),
        .queueCreateInfoCount = static_cast<uint32_t>(queueCreateInfos.size()),
        .pQueueCreateInfos = queueCreateInfos.data(),
        .enabledExtensionCount = deviceExtensions.size(),
        .ppEnabledExtensionNames = deviceExtensions.data(),
    };

    device = vk::raii::Device(physicalDevice, deviceCreateInfo);
    queues.graphics = vk::raii::Queue(device, graphicsIndex, 0);
    queues.present = vk::raii::Queue(device, presentIndex, 0);

I-ABD-I avatar Jul 26 '25 16:07 I-ABD-I