Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

gpx1000
Copy link

@gpx1000 gpx1000 commented Mar 18, 2025

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.

gpx1000 added 7 commits March 17, 2025 12:46
Streamline formatting, clarify key setup steps, and consolidate redundant information in Vulkan development guide. Focus on improving readability while maintaining cross-platform support for Vulkan SDK, dependencies, and build tools like CMake and GLFW.
Refactor code to replace raw Vulkan handles with RAII wrappers from Vulkan-Hpp for better resource management and safety. Key changes include RAII usage for buffers, images, and device memory, along with modernized function calls and parameter handling. Code readability and alignment with updated Vulkan practices were also improved.
Migrated codebase to use `vk::` RAII wrappers, enhanced type safety, and updated Vulkan API conventions. Made textual corrections in documentation for grammatical consistency and clarity. Adjusted resource creation and rendering process to comply with Vulkan's multi-sampling requirements.
Corrected punctuation, grammar, and terminology for clarity and accuracy. Improved consistency in formatting, adopted RAII style for Vulkan handles, and refined examples by updating to modern Vulkan C++ bindings for better maintainability.
Fix typos and improve clarity in compute shader documentation

Corrected grammatical mistakes and improved phrasing for better readability in compute shader documentation. Adjusted links to use `.adoc` extension and reformatted lines for consistency and clarity. No functional changes were made to the content.
```
Updated tutorial text to use inclusive language ("we" instead of "I") and improved readability. Modernized Vulkan instance creation using RAII and cleaner initialization code in compliance with up-to-date Vulkan best practices.
@SaschaWillems SaschaWillems self-requested a review March 19, 2025 20:48
@SaschaWillems
Copy link
Collaborator

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
Copy link
Author

gpx1000 commented Mar 19, 2025

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
Copy link
Collaborator

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

@gpx1000
Copy link
Author

gpx1000 commented Mar 19, 2025

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?

Copy link

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of questions...


return availableFormats[0];
static vk::Format chooseSwapSurfaceFormat(const std::vector<vk::SurfaceFormatKHR>& availableFormats) {
return ( availableFormats[0].format == vk::Format::eUndefined ) ? vk::Format::eB8G8R8A8Unorm : availableFormats[0].format;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very different from the original.

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Mar 20, 2025

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 😄

* `VK_DEBUG_UTILS_MESSAGE_SEVERITY_INFO_BIT_EXT`: Informational message like the creation of a resource
* `VK_DEBUG_UTILS_MESSAGE_SEVERITY_WARNING_BIT_EXT`: Message about behavior that is not necessarily an error, but very likely a bug in your application
* `VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT`: Message about behavior that is invalid and may cause crashes
* `VK_DEBUG_UTILS_MESSAGE_SEVERITY_INFO_BIT_EXT`: Informational message
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it with the C version in the tutorial text on purpose with the idea that we can link directly to the spec such that the reader will know to look for the spec's version instead of the C++ version. That might be confusing though, maybe we should change it to C++ everywhere?

}
----

The next section will introduce the first requirements that we'll check for in the `isDeviceSuitable` function.
As we'll start using more Vulkan features in the later chapters we will also extend this function to include more checks.

== Base device suitability checks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be good for after the merge to revisit. I agree, this needs to be revisited.

@gpx1000
Copy link
Author

gpx1000 commented Jun 20, 2025

If possible, everyone take another look. There's several comments here that I have left open and haven't yet addressed. However, this PR is getting pretty complicated to work with and I'd like to get it merged so we can address any short comings in their own PR. So unless you see anything drastically as a bug, let's go ahead and merge this and then circle back for improvements.

@SaschaWillems
Copy link
Collaborator

One thing that I might've missed initially: Base requirement is now Vulkan1.4.

Not sure if that's what we want. Android is still far behind, and I fear we would leave too many people behind?

@gpx1000
Copy link
Author

gpx1000 commented Jun 20, 2025 via email

Introduce `install_dependencies_windows.bat` and `install_dependencies_linux.sh` to simplify setup for Vulkan projects. Updated documentation with instructions for using the scripts and clarified Vulkan SDK installation steps.
@gpx1000
Copy link
Author

gpx1000 commented Jun 20, 2025

I went ahead and made two install scripts one for Linux and one for Windows. The Windows one is using vcpkg. While I might not use vcpkg anywhere, enough people have asked for it, and it does make writing the windows install script easier.
I wanted to avoid fetchcontent and submodules because both of those solutions would only capture the dependencies at a specific version and we'd have to update over time. This way, it's a system wide install that should work everywhere in all platforms. Try it out and lemme know if it works for you (I only have my Linux machine up so I haven't tested the Windows install).

@SaschaWillems
Copy link
Collaborator

Awesome 👍🏻 That simplifies things a lot. CMake is now almost working out of the box. I'm still seeing these errors:

CMake Error at CMakeLists.txt:123 (file):
file COPY cannot find
"V:/Vulkan-Docs-Site/Vulkan-Tutorial-review/attachments/../resources/viking_room.obj":
No error.
Call Stack (most recent call first):
CMakeLists.txt:219 (add_chapter)

(multiple similar errors)

and

CMake Error at V:/github/vcpkg/scripts/buildsystems/vcpkg.cmake:598 (_add_executable):
Cannot find source file:

11_render_passes.cpp

Call Stack (most recent call first):
CMakeLists.txt:99 (add_executable)
CMakeLists.txt:154 (add_chapter)

(miltuple simial errors)

The first one can be fixed by adjusting the path for the resources (they're now located right besides the CMake file)

The second one seems to be caused by files still being included that are no longer present (due to using dynamic rendering).

Here is a patch file for that (not sure if it works outside of windows):
patch_file.patch

@SaschaWillems
Copy link
Collaborator

A few chapters won't compile for me (Windows 11, Visual Studio 2022) due to syntax errors:

06_swap_chain_creation.cpp(263,29): error C2039: 'contains': is not a member of 'std::ranges'e formal list?)
06_swap_chain_creation.cpp(263,39): error C2065: 'presentModes': undeclared identifier
06_swap_chain_creation.cpp(263,29): error C3861: 'contains': identifier not found

05_window_surface.cpp(176,49): error C2187: syntax error: ')' was unexpected here
05_window_surface.cpp(176,49): error C2059: syntax error: ')'

07_image_views.cpp(234,14): error C2039: 'enabledExtensionCount': is not a member of 'vk::DeviceQueueCreateInfo'
07_image_views.cpp(234,61): error C2059: syntax error: ';'
07_image_views.cpp(235,13): error C2059: syntax error: '.'
07_image_views.cpp(238,9): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
07_image_views.cpp(238,36): error C2065: 'physicalDevice': undeclared identifier
07_image_views.cpp(238,52): error C2065: 'deviceCreateInfo': undeclared identifier
07_image_views.cpp(239,9): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
07_image_views.cpp(239,50): error C2065: 'graphicsIndex': undeclared identifier
07_image_views.cpp(240,9): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
07_image_views.cpp(240,49): error C2065: 'presentIndex': undeclared identifier
07_image_views.cpp(241,5): error C2059: syntax error: '}'
07_image_views.cpp(241,5): error C2143: syntax error: missing ';' before '}'
07_image_views.cpp(243,28): error C2143: syntax error: missing ';' before '{'
07_image_views.cpp(243,28): error C2447: '{': missing function header (old-style formal list?)

27_depth_buffering.cpp(1002,16): error C1075: '(': no matching token found

It also looks like MSVC is having some serious issues with the Vulkan module, most samples look like this:

image

They do compile, but code highlighting is completely broken. Matches my former experience with modules and MSVC :(

@SaschaWillems
Copy link
Collaborator

So I got at least some of the chapters working. Did test e.g. the compute shader (since I wrote that one), but it didn't work as expected.

First, it looks very different:

image
image

Top is what it should look like, bottom is what it now looks like. Should be easy to fix.

But the sample now also crashes when trying to resize the window:

vk::Queue::presentKHR: ErrorOutOfDateKHR

gpx1000 added 2 commits June 21, 2025 11:25
…ctory didn't fully make it over. This should compile everything again, I'm still working on compute_shader.
Updated swap chain format to use `vk::SurfaceFormatKHR` for better type clarity and consistency. Refined format and color space references throughout the code. Improved `chooseSwapSurfaceFormat` logic for desirable format selection and adjusted pipeline blending constants for correctness. Removed unused shader modules in the graphics pipeline setup.
@gpx1000
Copy link
Author

gpx1000 commented Jun 21, 2025

I'll have to look at the color blending again when there's more time. The cause of that change is escaping me but I'm not getting a crash when I resize. I'll have to wait until after the move when I can setup my windows machine again to test if there's something I'm missing there that might cause the resize issue. Thanks for testing @SaschaWillems

@SaschaWillems
Copy link
Collaborator

Thanks for fixing.

I do get a compilation error in the base code project, but that looks totally bogus and I think it's a bug with MSVC and not the actual code.

All other chapters now compile fine 👍🏻

@SaschaWillems
Copy link
Collaborator

Did a build of the docs site with this PR and read through all chapters. I'm happy with the changes, and I only noticed a few minotr things like some chapters still linking to the GLSL shaders or some broken links.

But those can be easily fixed after the merge.

Maybe we can discuss this PR on the next docs call and decide on how to move forward.

[]( vk::QueueFamilyProperties const & qfp ) { return qfp.queueFlags & vk::QueueFlagBits::eGraphics; } );
// get the first index into queueFamilyProperties which supports graphics
auto graphicsQueueFamilyProperty = std::ranges::find_if( queueFamilyProperties, []( auto const & qfp )
{ return (qfp.queueFlags & vk::QueueFlagBits::eGraphics) != static_cast<vk::QueueFlags>(0); } );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:
return (qfp.queueFlags & vk::QueueFlagBits::eGraphics);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler error complaining that the type is not a bool. I could cast it, but I bet the compiler would give the same asm output from casting as simply comparing against 0.

Copy link

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments...

std::vector<VkDynamicState> dynamicStates = {
VK_DYNAMIC_STATE_VIEWPORT,
VK_DYNAMIC_STATE_SCISSOR
std::vector dynamicStates = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a std::vector is this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang lint has started suggesting when the compiler can figure out what the types are automatically. This is an instance of trying that suggestion out. If you feel this loses clarity to the tutorial, we can certainly be more verbose.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file deleted?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaikr because the tutorial now uses dynamic rendering, so no more need for render passes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, same for the framebuffers. Those go away. Once we merge, I'll renumber the chapters. I also am trying to hold back a new chapter on Mobile and using Vulkan prior to 1.4 and determining when that's needed. But we're already pretty complicated PR as it stands so I don't want to add until it can be an atomic add.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file deleted?

gpx1000 added 3 commits June 26, 2025 12:15
Refactor device extension handling and physical device selection logic

Unified device extension references to `requiredDeviceExtension` for clarity and consistency. Improved physical device selection by incorporating checks for Vulkan 1.3 features and necessary dynamic rendering capabilities. Enhanced code readability with structured initialization and modern Vulkan C++ practices.
Improved device extension handling and shader input/output structures. Enhanced readability with structured initialization. Updated device selection to validate Vulkan 1.3 features and required extensions. Refined pipeline blending and color attachment setup for clarity and correctness.
@gpx1000
Copy link
Author

gpx1000 commented Jun 27, 2025

@SaschaWillems I'm still not getting a crash on resize for 31_compute_shader. However, the bug was in the slang shader code. It took me a bit to figure out how to correctly replace gl_PointCoord with the slang equivalent. That should work now. I'm going to see if I can test on a few more platforms and get a reproduce on the resize crashbug.

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Jun 28, 2025

Interesting. I still get that with the latest change, and even more interestingly, it does NOT happen in the swap chain code of the sample.

If I add a try..catch, I do run into the catch block:

            try {
                result = presentQueue.presentKHR(presentInfo);
                if (result == vk::Result::eErrorOutOfDateKHR || result == vk::Result::eSuboptimalKHR || framebufferResized) {
                    framebufferResized = false;
                    recreateSwapChain();
                }
                else if (result != vk::Result::eSuccess) {
                    throw std::runtime_error("failed to present swap chain image!");
                }
            }
            catch (const std::exception& e) {
                throw std::runtime_error("failed to present swap chain image!");
            }

So maybe the raii variant of vulkan-hpp defaults to exception raising for me?

Visually the sample now looks as expected 👍🏻

But anyway, it's something we can safely fix after this PR is merged ;)

@SaschaWillems
Copy link
Collaborator

Also need to find out how to make this work with MSVC's intellisense. It does not find anything inside the vulkan module for me, and it seems that MSVC needs to build a special file for intellisense to work with modules: https://developercommunity.visualstudio.com/t/Additional-intermediate-build-files-for-/10642332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants