This is a new version of the tutorial using RAII and SLang

[gpx1000]

This is a rewrite of the tutorial to use SLang and RAII. While it is complete and everything is working, this is a work in progress; PR is here to enable discussion and thought.

[SaschaWillems]

It looks like the line width changed for all adoc files. This makes it kinda hard to compare and in some cases also breaks links:

Is there a way to revert the line width/break changes?

[gpx1000]

There's not a good way to do that sadly :( The line width in the adoc pages didn't seem to work so I just grabbed the original markdown settings and used them.

[SaschaWillems]

We might need to check if Antora supports highlighting for slang. Afair it does glsl out of the box, but for hlsl I had to manually add in a syntax highlighter.

[gpx1000]

Slang isn't in the list for our antora site. Here's the list of languages we have a highlight script for. We should probably add SLang at the very least.

var hljs = require('highlight.js/lib/highlight')

hljs.registerLanguage('asciidoc', require('highlight.js/lib/languages/asciidoc'))
hljs.registerLanguage('bash', require('highlight.js/lib/languages/bash'))
hljs.registerLanguage('clojure', require('highlight.js/lib/languages/clojure'))
hljs.registerLanguage('cpp', require('highlight.js/lib/languages/cpp'))
hljs.registerLanguage('cs', require('highlight.js/lib/languages/cs'))
hljs.registerLanguage('css', require('highlight.js/lib/languages/css'))
hljs.registerLanguage('diff', require('highlight.js/lib/languages/diff'))
hljs.registerLanguage('dockerfile', require('highlight.js/lib/languages/dockerfile'))
hljs.registerLanguage('elixir', require('highlight.js/lib/languages/elixir'))
hljs.registerLanguage('glsl', require('highlight.js/lib/languages/glsl'))
hljs.registerLanguage('go', require('highlight.js/lib/languages/go'))
hljs.registerLanguage('groovy', require('highlight.js/lib/languages/groovy'))
hljs.registerLanguage('haskell', require('highlight.js/lib/languages/haskell'))
hljs.registerLanguage('hlsl', require('./hlsl.js'))
hljs.registerLanguage('java', require('highlight.js/lib/languages/java'))
hljs.registerLanguage('javascript', require('highlight.js/lib/languages/javascript'))
hljs.registerLanguage('json', require('highlight.js/lib/languages/json'))
hljs.registerLanguage('julia', require('highlight.js/lib/languages/julia'))
hljs.registerLanguage('kotlin', require('highlight.js/lib/languages/kotlin'))
hljs.registerLanguage('lua', require('highlight.js/lib/languages/lua'))
hljs.registerLanguage('markdown', require('highlight.js/lib/languages/markdown'))
hljs.registerLanguage('nix', require('highlight.js/lib/languages/nix'))
hljs.registerLanguage('none', require('highlight.js/lib/languages/plaintext'))
hljs.registerLanguage('objectivec', require('highlight.js/lib/languages/objectivec'))
hljs.registerLanguage('perl', require('highlight.js/lib/languages/perl'))
hljs.registerLanguage('php', require('highlight.js/lib/languages/php'))
hljs.registerLanguage('properties', require('highlight.js/lib/languages/properties'))
hljs.registerLanguage('puppet', require('highlight.js/lib/languages/puppet'))
hljs.registerLanguage('python', require('highlight.js/lib/languages/python'))
hljs.registerLanguage('ruby', require('highlight.js/lib/languages/ruby'))
hljs.registerLanguage('rust', require('highlight.js/lib/languages/rust'))
hljs.registerLanguage('scala', require('highlight.js/lib/languages/scala'))
hljs.registerLanguage('shell', require('highlight.js/lib/languages/shell'))
hljs.registerLanguage('sql', require('highlight.js/lib/languages/sql'))
hljs.registerLanguage('swift', require('highlight.js/lib/languages/swift'))
hljs.registerLanguage('xml', require('highlight.js/lib/languages/xml'))
hljs.registerLanguage('yaml', require('highlight.js/lib/languages/yaml'))

[SaschaWillems]

We might get away by duplicating the hlsl one, and adding in a few slang keywords ;)

[gpx1000]

Yep, that's what I'm thinking. I'll make a tracking issue on the vulkan-site repo and begin a pass at making a highlight.js. It might be worth it to investigate how to contribute that back upstream to help promote use of Slang?

[asuessenbach]

Just a couple of questions...

[asuessenbach]

There is no need to have the vk::raii objects wrapped by a std::unique_ptr. You could as well just use them as
vk::raii::Instance instance = nullptr;
Note that, as those classes don't have a default constructor, you would be forced to initialize them with nullptr.

[asuessenbach]

Maybe it's worth to use designated initializers with all the structs. Would make it a bit more verbose again, but maybe it's easier to digest then. Would look like

  constexpr vk::ApplicationInfo applicationInfo{ .pApplicationName   = "Hello Triangle",
                                                 .applicationVersion = VK_MAKE_VERSION( 1, 0, 0 ),
                                                 .pEngineName        = "No Engine",
                                                 .engineVersion      = VK_MAKE_VERSION( 1, 0, 0 ),
                                                 .apiVersion         = VK_API_VERSION_1_4 };

You would need to set VULKAN_HPP_NO_STRUCT_CONSTRUCTORS to have that feature available.

[gpx1000]

This is a really good idea. I'll work on getting it in.

[asuessenbach]

In case you want to switch from std::unique_ptr<vk::raii::Instance> to vk::raii::Instance, you could encode it here as
instance = vk::raii::Instance( context, createInfo );
or
instance = context.createInstance( createInfo );

[asuessenbach]

Maybe use std::ranges::find_if here as well?

[asuessenbach]

Maybe make the invalid case independent of the queueFamilyProperties, like ~0?

[asuessenbach]

This looks very different from the original.

[asuessenbach]

Maybe:
return std::ranges::contains( presentModes, vk::PresentModeKHR::eMailbox ) ? vk::PresentModeKHR::eMailbox : vk::PresentModeKHR::eFifo;

[asuessenbach]

Maybe
vk::DeviceCreateInfo deviceCreateInfo( {}, deviceQueueCreateInfo, {}, deviceExtensions );
or, with designated initializers

      vk::DeviceCreateInfo deviceCreateInfo{ .queueCreateInfoCount    = 1,
                                             .pQueueCreateInfos       = &deviceQueueCreateInfo,
                                             .enabledExtensionCount   = static_cast<uint32_t>( deviceExtensions.size() ),
                                             .ppEnabledExtensionNames = deviceExtensions.data() };

[asuessenbach]

No check for suitability of the device?

[SaschaWillems]

Remarks on the CMake build setup:

First, kudos for providing a proper CMakeLists.txt. That has always been one of my main issues with the current tutorial 👍🏻

On Windows though it does not yet work out of the box, as you have to manually specify folders for e.g. glfw and that does not seem to work with the glfw package you can download from their site.

For my own projects I moved to using CMake's FetchContent for such third party libraries, as that takes care of download, compiling and even installing these dependencies.

Would it be possible to use FetchContent for the tutorial too? That would simplify project setup even further 😄

[SaschaWillems]

This link is broken due to a whitespace.

[SaschaWillems]

I think we could remove those links. One of those hasn't been updated in years, the other one is also lagging behind.

[SaschaWillems]

This link is broken due to the line break.

[SaschaWillems]

This link is wrongly formatted (markdown style), which doesn't work for Asciidoc. Correct format would be url[caption], so https://vulkan.lunarg.com/[LunarG]

[SaschaWillems]

Same as above. This link is using the markdown format, and needs to be changed to asciidoc link format.

[SaschaWillems]

This link works fine, the link: is superflous though and can be removed (sorry for nitpicking ;) )

[SaschaWillems]

This link needs to be adjusted to the asciidoc link format too.

[SaschaWillems]

Minor nitpick: But I think we should remove indenation for standalone code blocks. Having snippets like these start every line with lots of empty space seems wasteful.

[SaschaWillems]

Might sound odd to a native speaker, but what does "NB" stand for?

[gpx1000]

Sorry, NB is latin, 'nota bene' or in English, "Note Well"
I'll change that to Note which basically means the same thing. There's subtle differences that don't really matter here.

[SaschaWillems]

Should be Slang instead of SLang, at least accordig to Slang

[SaschaWillems]

This link needs to be converted to asciidoc format

[SaschaWillems]

This link needs to be converted to asciidoc format

[SaschaWillems]

Should be updated to use the vulkan.hpp enums instead (e.g. vk::DebugUtilsMessageSeverityFlagBitsEXT::eWarning)

[SaschaWillems]

We should consider rewriting or removing this chapter all along and instead replace it with explicit device selection. This "device suitability check" has caused lots of confusion judging from community feedback.

[SaschaWillems]

This link needs to be changed to asciidoc format

[SaschaWillems]

Should be Slang instead of SLang

[SaschaWillems]

This seems to lack information on how SLANGC_EXECUTABLE is actually set/discovered.

[SaschaWillems]

Maybe remove OpenCL. This is confusing esp. to beginners, as the SPIR generated for Vulkan is not directly compatible with the one for OpenCL.

[SaschaWillems]

Should be Slang.

[SaschaWillems]

Should/could be changed to use the vulkan.hpp enums instead.

[SaschaWillems]

The list below should/could use vulkan.hpp enums instead

[SaschaWillems]

This should/could use the vulkan.hpp enums.

[SaschaWillems]

This should/could use the vulkan.hpp enums.

[SaschaWillems]

@gpx1000: One general issue I see is with initialization now. A lot of paragraphs explicitly refer to parameters by name, but the code now always passes parameters to constructurs.

E.g. this:

People have no way of connecting the parameters mentioned in the text with the code above.

For my projects that use vulkan.hpp I prefer designated initializers like this:

vk::PipelineInputAssemblyStateCreateInfo inputAssemblyState{ .topology = vk::PrimitiveTopology::eTriangleList };

While it's a bit more code, it's also much easier to understand and follow.

[gpx1000]

Yep, @asuessenbach said the same thing. I'm currently working on a new version that uses designated initializers. it's a big change will take a few days ;-).

[SaschaWillems]

Thanks 👍🏻

Somehow missed Andrea's comment during my review marathon.

[SaschaWillems]

With the tutorial now using slang, the glsl specific built-ins like gl_vertexIndex and gl_InstanceIndex should be replaced by the Slang variants.

[SaschaWillems]

LoC count might be revised, with vulkan.hpp i'ts far less than 900 ;)

[SaschaWillems]

Next line could be updated to constexpr

[SaschaWillems]

Another general thing we might want to discuss is the use of C constants. They're still used in a lot of places, and now often no longer match the code snippets. I think we should use vulkan.hpp enums instead everywhere, even in text paragraphs to make things less confusing for readers.

[SaschaWillems]

Not sure if we should keep that paragraph. Feels odd to use offsetof and yet warn people to maybe not use it.

[SaschaWillems]

While not part of this PR we should revise this chapter. I don't think manually flipping projection (see line 344+) is required anymore or if it still is there is a GLM define to change that. If this is changed, the whole "orginally for OpenGL" paragraph can be removed.

[SaschaWillems]

This chapter lacks a proper introduction. There is no real mention as to what a layout transition is and why it's used. Maybe add some context.

[SaschaWillems]

We might want to rephrase that a bit. This sounds like you MUST use a combined image sampler to sample an image in a shader, which isn't the case.

[SaschaWillems]

Might be worth to unify naming. The C++ code above e.g. has "pos" instead of "position".

[SaschaWillems]

Even though it may not be required, maybe explicitly add the binding slot, so people know it's something that might be required by Slang and Vulkan.

[SaschaWillems]

The code snippet here is still using glsl syntax and needs to be updated.

[SaschaWillems]

Any reason this was completely removed? I think we should at least keep the mention of tinyobj and where it's coming from.

[asuessenbach]

Maybe, the determination of those two indices could be formulated like this:

// look for a queueFamilyIndex that supports graphics and present
auto combinedIt = std::ranges::find_if( queueFamilyProperties,
                                        [&physicalDevice, &surface]( vk::QueueFamilyProperties const & qfp )
                                        {
                                          static uint32_t index = 0;
                                          return ( qfp.queueFlags & vk::QueueFlagBits::eGraphics ) && physicalDevice.getSurfaceSupportKHR( index++, surface );
                                        } );
if ( combinedIt != queueFamilyProperties.end() )
{
  uint32_t index = static_cast<uint32_t>( std::distance( queueFamilyProperties.begin(), combinedIt ) );
  return { index, index };  // the first index that supports graphics and present
}
else
{
  // there's no single index that supports both graphics and present -> look for separate ones
  auto graphicsIt =
    std::ranges::find_if( queueFamilyProperties, []( vk::QueueFamilyProperties const & qfp ) { return qfp.queueFlags & vk::QueueFlagBits::eGraphics; } );
  if ( graphicsIt != queueFamilyProperties.end() )
  {
    uint32_t graphicsIndex = static_cast<uint32_t>( std::distance( queueFamilyProperties.begin(), graphicsIt ) );
    auto     presentIt     = std::ranges::find_if( queueFamilyProperties,
                                           [&physicalDevice, &surface]( vk::QueueFamilyProperties const & )
                                           {
                                             static uint32_t index = 0;
                                             return physicalDevice.getSurfaceSupportKHR( index++, surface );
                                           } );
    if ( presentIt != queueFamilyProperties.end() )
    {
      uint32_t presentIndex = static_cast<uint32_t>( std::distance( queueFamilyProperties.begin(), presentIt ) );
      return { graphicsIndex, presentIndex };
    }
    else
    {
      throw std::runtime_error( "Could not find a queue family index that supports present -> terminating" );
    }
  }
  else
  {
    throw std::runtime_error( "Could not find a queue family index that supports graphics -> terminating" );
  }
}

[asuessenbach]

Maybe just
return device->createShaderModule( createInfo );

[asuessenbach]

Maybe perform this check only if enableValidationLayers is true?

[asuessenbach]

Is it intended to have this empty function to create the graphics pipeline?
In the tutorial named 08_graphics_pipeline?

[asuessenbach]

createGraphicsPipeline just creates a ShaderModule?

[asuessenbach]

As long as you don't have VULKAN_HPP_NO_EXCEPTIONS defined, vk::Result::eErrorOutOfDateKHR would be thrown as an exception.
That is, you would need to try acquireNextImage and either catch the specific vk::OutOfDateKHRError, or the general vk::SystemError.

[asuessenbach]

As above: you need to try/catch on vk::Result::eErrorOutOfDataKHR.

[asuessenbach]

Maybe a slightly different signature:
std::pair<vk::raii::Buffer, vk::raii::DeviceMemory> createBuffer( vk::DeviceSize size, vk::BufferUsageFlags usage, vk::MemoryPropertyFlags properties )

Would be called then like

  vk::raii::Buffer stagingBuffer(nullptr);
  vk::raii::DeviceMemory stagingBufferMemory(nullptr);
  std::tie( stagingBuffer, stagingBufferMemory ) =
    createBuffer( bufferSize, vk::BufferUsageFlagBits::eTransferSrc, vk::MemoryPropertyFlagBits::eHostVisible | vk::MemoryPropertyFlagBits::eHostCoherent );

or

  auto[ stagingBuffer, stagingBufferMemory ] =
    createBuffer( bufferSize, vk::BufferUsageFlagBits::eTransferSrc, vk::MemoryPropertyFlagBits::eHostVisible | vk::MemoryPropertyFlagBits::eHostCoherent );

[asuessenbach]

To make sure, a vk::raii::Object is generated empty, pass a nullptr in:
vk::raii::Buffer stagingBuffer(nullptr);

[asuessenbach]

Why do you use some temporary objects here? Can't you directly use the actual vertexBuffer?

[asuessenbach]

Maybe, instead of vk::IndexType::eUint16:
vk::IndexTypeValue<decltype(indices)::value_type>::value

It's substantially harder to read, but it makes clear where that value comes from (indices), and it stay correct in case the indices are changed to uint32_t, or so.

[asuessenbach]

Shouldn't this file be named 22_descriptor_set_layout.cpp?

[asuessenbach]

createImage takes uint32_t for width and height, but texWidth and texHeight are int. Maybe change them to uint32_t?

[asuessenbach]

Is it checked somewhere if samplerAnisotropy is supported?
The previous version checked it in pickPhysicalDevice via isDeviceSuitable

[asuessenbach]

Three lines above: VkDeviceSize -> vk::DeviceSize

[asuessenbach]

fragColor is not used.

[asuessenbach]

Maybe static_cast<uint32_t>(bindings.size())

[asuessenbach]

Maybe

auto formatIt = std::ranges::find_if(candidates, [&physicalDevice, &tiling, &features](auto const format){
    vk::FormatProperties props = physicalDevice->getFormatProperties(format);
    return (((tiling == vk::ImageTiling::eLinear) && ((props.linearTilingFeatures & features) == features)) ||
            ((tiling == vk::ImageTiling::eOptimal) && ((props.optimalTilingFeatures & features) == features)));
});
if ( formatIt == candidates.end())
{
    throw std::runtime_error("failed to find supported format!");
}
return *formatIt;

[asuessenbach]

What about all the other depth formats (like eD16Unorm, eX8D24UnormPack32, eD16UnormS8Uint,...)?

[asuessenbach]

What about the other stencil formats (like eS8Uint, eD16UnormS8Uint, eS8Uint,...)

[asuessenbach]

Maybe:

auto formatIt = std::ranges::find(availableFormats, vk::SurfaceFormatKHR(vk::Format::eB8G8R8A8Srgb, vk::ColorSpaceKHR::eSrgbNonlinear));
return (formatIt != availableFormats.end() ? *formatIt : availableFormats[0];

[asuessenbach]

Maybe the consts should be applied to the samples before as well.

[asuessenbach]

Maybe instead:

auto [vertexIt, inserted] = uniqueVertices.insert({vertex, static_cast<uint32_t>(vertices.size())});
if (inserted)
{
	vertices.push_back(vertex);
}
indices.push_back(vertexIt->second);

[asuessenbach]

Is it checked anywhere that samplerAnisotropy is supported?

[asuessenbach]

Is it checked anywhere that the deviceExtensions are supported?

[asuessenbach]

Those three members are already set in the constructor right above.

[asuessenbach]

What does it mean, that there are two directories, attachments and code, with nearly but not completely identical content?

[asuessenbach]

What if there's no queue family supporting both, graphics and compute?

[asuessenbach]

While presentQueue might be different from the graphicsQueue, computeQueue and graphicsQueue are the same. Why do you use them as separate entities?
Maybe you should at least use different queue indices for those two?

[asuessenbach]

VK_SUBPASS_EXTERNAL -> vk::SubpassExternal?

[asuessenbach]

Missed to remove that out-commented code?

[asuessenbach]

Not a big difference to createCommandBuffers. Maybe replace them by a function handling both cases?

[asuessenbach]

Is this file intentionally empty?

[asuessenbach]

VkDeviceSize -> vk::DeviceSize ?

[asuessenbach]

image and the queue family indices are already set in the constructor above.

[SaschaWillems]

What does it mean, that there are two directories, attachments and code, with nearly but not completely identical content?

There should only be an "attachment" folder. That's what Antora requires, all files from code need to be moved there.

[asuessenbach]

Is Vulkan still a "new" API?

[asuessenbach]

Why have you changed the background of this and all the following images?

[JoseEmilio-ARM]

missing auto here?