Recursive include resolution for shader

[Lachei]

Is your feature request related to a problem? Please describe. Recently I tried compiling a shader using the vsg::ShaderCompiler and ran into the problem of non resolved includes when having includes in included files

Describe the solution you'd like My current solution is to add a recursive call to insertIncludes(). Also by using stringstream the readShaderSource() method becomes: https://github.com/vsg-dev/VulkanSceneGraph/blob/e68c7cec727bc4832fb556b799fdceba65bfae72/src/vsg/vk/ShaderCompiler.cpp#L540

std::string ShaderCompiler::readShaderSource(const Path& filename, const Paths& paths){ 
    Path foundFile = findFile(filename, paths);
    if (foundFile.empty()) return std::string();

    std::ifstream fin(foundFile);
    std::stringstream buffer;
    buffer << fin.rdbuf();
    std::string ret = insertIncludes(buffer.str(), paths); //recursiveley resolve includes
    return ret;
}

Which is a shorter code for include resolution including recursive includes. The code above using stringstream for string conversion is tested under Linux and Windows.

Cheers, Josef

[robertosfield]

There is include support built into ShaderCompiler, but the includes are done during the compilation process. not resolved at load time. The read isn't supposed to resolve the includes with the current implementation, instead it's supposed to just load the file you ask it to.

Does the VSG work fine on your shares out of the box? If not could you please provide an example that isn't working. If it things work, but you just have a personal preference for resolving the includes then you can just manually call ShaderCompile::insertIncludes(..)

[Lachei]

I am aware that the includes are loaded when the shader is compiled. The ShaderCompiler::readShaderSource() was changed as it appeared to be the easiest way to recursively resolve the includes. With resolve i mean that the include source is loaded and put into the place of the #include "header" line in the shader.

An example for a shader file having such a recursive include structure can be found in this raygen shader: https://github.com/Lachei/VulkanPBRT/blob/936366f4fb5d1074e137acc4bb3695252edad1ab/shaders/ptRaygen.rgen#L21 Which refers to this shader file: https://github.com/Lachei/VulkanPBRT/blob/936366f4fb5d1074e137acc4bb3695252edad1ab/shaders/camera.glsl Which then includes https://github.com/Lachei/VulkanPBRT/blob/936366f4fb5d1074e137acc4bb3695252edad1ab/shaders/sampling.glsl And so on....

For these shaders the runtime compilation via the ShaderCompiler does not work. The line i added to the ShaderCompiler::readShaderSource() automatically loads and resolves all included header files in the currently loaded header file (in ShaderCompiler::insertIncludes() ShaderCompiler::readShaderSource() is automatically called for all includes present). With this small fix I was able to get the shader compiler to also include the recursive includes on shader compilation. And while seeing the function i also changed the parsing of the file to the stringstream variant as it is easier to read. However after seeing benchmarks on the different string parsings I realized that the current solution is faster than the stringstream version. So for speed the current implementation is better.

If it things work, but you just have a personal preference for resolving the includes then you can just manually call ShaderCompile::insertIncludes(..)

While i defenitely could do that, i thought that often a bit more complex shaders with seperated shader files will have such recursive include structures and thus support for these shaders would be preferred.

Also when dealing with includes, the debug print on compilation error for a shader source is a bit misleading. The shader compiler itself gets the source file with the includes in the shader string and prints the error messages with line number according to that shader string, while the debug out prints the unresolved shader source resulting in a mismatch in line numbers for the errors.

[robertosfield]

I'm away from my dev system so can't look into details. I'd like to add a general clarification, for normal VSG usage the shaders will be automatically compiled for you by the CompileTraversal that is invoked by vsg::Viewer::compile(..). With this normal usage users would never need to call ShaderCompier directly themselves - the assignment of the shader related objects to vsg::GraphicsPipeline/ComputePipeline is all that will be required.

Only in special cases would I expect users to need to invoke ShaderCompiler themselves.

[Lachei]

Yes, under normal circumstances this would be absoluteley correct. However i do online the shader compilation to runtime because i have to set defines in the shader dynamically. This means i have to hand the defines over to the shader compiler. Unfortunateley currently for all pipelines where the compilation for the shader stages is handled no defines are given to the shader compiler [shaderCompiler->compile(stage); // may need to map defines and paths in some fashion](https://github.com/vsg-dev/VulkanSceneGraph/blob/e68c7cec727bc4832fb556b799fdceba65bfae72/src/vsg/state/ComputePipeline.cpp#L83)

And as far as i can see the "problem" with recursive includes would still exist in this cenario.

[robertosfield]

It should be possible to inject the required defines via the ShaderCompileSettings structure that you can assign the ShaderModule:

https://github.com/vsg-dev/VulkanSceneGraph/blob/master/include/vsg/state/ShaderModule.h

Could you provide example code that illustrates your usage case, it might be that the VSG needs to be tweaked, or perhaps just your usage, or a combination of the two.

[Lachei]

Oh yes, i defenitely overlooked the ShaderCompileSettingsStructure. However the custom defines work just fine and i had no issue with them. The cpp side code for my usage would be here: https://github.com/Lachei/VulkanPBRT/blob/b14c334ce33f17f434a12a6a61caa94f807b3ba5/PBRTPipeline.cpp#L201

The shader to be compiled is then the one stated above.

[robertosfield]

@Lachei I'm now back at my dev station and looking into the recursive includes support. I'll look at build VulkanPBRT tp give me a bit of insight for your usage case. I see that there is a dependency on OpenEXR, in what form do you install it? Build from source? Install from repo?

I see that you have copied in the VSG into your code base to build, this will add an extra hurdle for pulling in any fixes I make. My recommendation would be to use an approach such as the one taken in vsgFramework to pull in projects and build them locally, with a local install as well. The advantage of this approach is you can update to new versions of the VSG etc. more easily,

FYI, the VSG version number is now at 0.1.13 and will contain lots of changes since you copied in the VSG code base, which unfortunately will mean a few API changes that could make the update more awkward,

Over the next 3-4 months my aim is to get the VSG to 1.0 , so will be doing a wide review of the codebase, fixing design and implementation issues required. Getting the VSG into a solid place for 1.0 requires as much testing of the codebase as we can get - catching things like the recursive includes is a good example. Testing in real-world applications is really important in this, and VulkanPBRT would be a good type of project to test things against as it'll stress different parts of the VSG than most projects. Happy to coordinate on this.

[Lachei]

I install OpenEXR by using standard apt package manager on Linux. Currently the project needs a pre 3.0 version of OpenEXR (current version in apt is 2.5 i think). If the version is newer, the linking has to be updated to also include the now seperate Imf math library.

Yes, i know about the changes and have already thought about using externally pulled in projects. I simply set my focus on other things...

I also keep track of the version changes and know that there have been a few significant changes (eg. rescoping of Variables, ImageInfo, BufferInfo updates), however these changes never affected the shader compiler (except the changes for raytracing shader compilation, which are included in my local copy).

In order to make it easier for you to check the recursive inlcude example i will be setting up a clean linux distro and gonna use the current vsg to make a small test application showing my solution.

For VulkanPBRT i will currently stay on my local copy of vsg as there will be coming quite extensive modifications to the raytracing structures to be able to properly support things like volumetrics tracing, large scale on the fly update of acceleration structures and more. I am planning to maybe reincorporate the found solutions back into the vsg when these parts of the project are properly tested and debugged.

[robertosfield]

Which openexr package do you install? Just installed libopenexr-dev and now VulkanPBRT cmake runs fine. get a build error though:

[ 9%] Building CXX object CMakeFiles/VulkanPBRT.dir/AccumulationBuffer.cpp.o /home/robert/Dev/VulkanPBRT/AccumulationBuffer.cpp: In member function ‘void AccumulationBuffer::updateImageLayouts(vsg::Context&)’: /home/robert/Dev/VulkanPBRT/AccumulationBuffer.cpp:35:59: error: ‘VK_ACCESS_NONE_KHR’ was not declared in this scope; did you mean ‘VK_THREAD_DONE_KHR’? 35 | auto prevIlluLayout = vsg::ImageMemoryBarrier::create(VK_ACCESS_NONE_KHR, VK_ACCESS_SHADER_WRITE_BIT, VK_IMAGE_LAYOUT_UNDEFINED, | ^~~~~~ | VK_THREAD_DONE_KHR

[robertosfield]

I've done a search through the VulkanSDK's I have downloaded and VK_ACCESS_NONE_KHR exists in 1.2.176.1 onwards. I had defaulted to using 1.2.162.0 for recent testing of the VSG. Updating to 1.2.176.1 lets build progress.

FYI, A recent cmake macro addition to the VSG allows you to check the Vulkan version so you can set minimum versions.

[robertosfield]

As a general comment on the VSG's ray tracing support. It is now based on the Khronos extensions rather than NVIdia ones. The classes are all pretty similar, so nothing has fundamentally changed. I'm now happy with the general design though, but haven't spent enough time just focused on the topic to know what the best replacement would be. The original RTX ray tracing code in the VSG was written by Tom Hogarth, and I haven't done much beyond maintain the related codebase. Before VSG-1.0 I want to review and refactor the ray tracing functionality.

I'm happy to take suggestions/collaborate on refining the ray tracing classes. This will have to wait till December or later though as have lots of other tasks to get on with first.

[Lachei]

It is now based on the Khronos extensions rather than NVIdia ones

Yes, I have been the one who provided the port (Josef Stumpfegger). I also mentioned in the discussion in goolge groups that i would recommend to create a ShaderBindingTable class to better be able to properly assign ray tracing shader groups to the different shader stages and make the whole process easier to understand. There is a ShaderBindingTable class inside the VulkanPBRT vsg library that demonstrates just that. There should be maybe a few small changes but the overall concept is imo a very convenient one for easier usage. My opinion on the overall structure of the raytracing classes is that they are well made and fit also the Khronos extension raytracing quite well (Only a few additions might be good).

[Lachei]

In order to make it easier for you to check the recursive inlcude example i will be setting up a clean linux distro and gonna use the current vsg to make a small test application showing my solution.

I will setup the example today in the evening.

[robertosfield]

Joseph, Thanks for reminding me that it was you that submitted the port to Khronos raytracing extensions. W.r.t improvements when I arrive at a point that I can dive into ray tracing side I'll ping you for your input.

I've been reviewing the present vsg::ShaderCompiler code and feel that having the support for inlining within ShaderCompiler::readShaderSource would be an elegent way to handle included shaders themselves having shaders, using recursion in this instance would be reasonable. I think I'll add this this afternoon.

Reviewing this code tells me that it makes the assumption that the included shaders are on the local filesystem and can be accessed with an ifstream opening a file. This limits the flexibility of where users can place files, so I will look to see if I can refine the code to leverage the vsg::read() support for reading and customizing finding and reading of files.

[robertosfield]

I have merged 01fab69a9b6a06f8fb115f703f142c2aff0dc958 to add support for nested shader includes via recursion.

I have done some brief testing with some modified vsgclip shaders, but paths were not being resolved so I had to hack the code a bit, clearly the paths aren't be managed correctly for the vsgclip usage case. I need to come up with another fix for this separate problem. It may make sense for me to just rewrite this part of ShaderCompiler to better handle reading the nested shaders in a more standard way i.e. leveraging vsg::read(), but will need to do a wider review to figure this out.

[Lachei]

but paths were not being resolved so I had to hack the code a bit, clearly the paths aren't be managed correctly for the vsgclip usage case

Ahh yes, now i remember why i did not use the ShaderCompileSettingsStructure . To find the include files I had to fill the paths variable. And thus i had to call the compile function for the ShaderCompiler directly...

[robertosfield]

Thanks that's a helpful insight. There a couple of routes I could go to implement resolution of the paths, if I get it right I think it should be possible to provide avoid the need for explictly compiling the shaders in PBRTPipeline::setupPipeline(..) and instead rely upon vsg::ShaderCompileSettings.

One route might be to add a vsg::Options object to the ShaderCompileSettings so that this can be used to doing the file path checks and reading.

Another approach would be to have the load process of the root shader file check for includes then load them and assign them to the ShaderCompileSettings as a map<std::string, std::string>, this map would be essentially be a <include_filename, shader_source>. This would avoid the shader compilation having to worry about finding and loading shaders. It would also give applications another way of injection source i.e. a bit like defines, but easier to interchange.

Given I don't yet have a strong preference of how to go about this I think I'll create a one or two branches and just experiment and see how the code looks in the end. I'll provide I link in this thread if/when I check some possible code in.

[robertosfield]

I started the map<std::string, std::string> but then had second thoughts, it might be still be the best approach, but getting everything to work together will need some great reflection on how the loaders of the shaders and the compile interact.

I think the most straight forward approach would be to cache an vsg::Options object in the vsg::ShaderCompileSettings or in vsg::ShaderModule. However, this wouldn't be serialisable in a straight forward manner.

I will leave this for tomorrow to resolve. perhaps sleeping on it will give me greater clarity.

[Lachei]

The map implementation would also have the advantage of directly supporting defines with a value. Currently one can only inject a define and check if a name is defined. It would however be very practical to be able to set a string value which is also added to the define line. An example for this use case would be to insert a image layout (needed for storage images such as rgba8 ...) without having to make a case distinction in shaders. Of course this type of defines can also be added to the current solution by changing the define check to only check for the first "part" of a define (so defines would need to be split in the shader compiler into a define part and a value part) and then adding the define and the value if the define is imported via the pragma.

[Lachei]

A current work around in my project to not use strange define case distinction but a combination of a define with a value can be seen in: https://github.com/Lachei/VulkanPBRT/blob/b14c334ce33f17f434a12a6a61caa94f807b3ba5/shaders/formatConverter.comp Which is set on cpu side via: https://github.com/Lachei/VulkanPBRT/blob/b14c334ce33f17f434a12a6a61caa94f807b3ba5/UtilityPipelines/FormatConverter.cpp This solution is however also inconvenient as i have to always pre define which values can be inserted with an include and have to add all possible values to the #pragma import_defines line...

[robertosfield]

On Mon, 15 Nov 2021 at 18:25, Lachei @.***> wrote:

The map implementation would also have the advantage of directly supporting defines with a value. Currently one can only inject a define and check if a name is defined. It would however be very practical to be able to set a string value which is also added to the define line. An example for this use case would be to insert a image layout (needed for storage images such as rgba8 ...) without having to make a case distinction in shaders. Of course this type of defines can also be added to the current solution by changing the define check to only check for the first "part" of a define (so defines would need to be split in the shader compiler into a define part and a value part) and then adding the define and the value if the define is imported via the pragma.

I had the later in mind originally. The OSG does the former w.r.t substitutions.

Given there is an overlap between the pragmatic shader compilation #define support and the #include, having one set of solutions is attractive.

[Lachei]

Also maybe a feature for later can be seen in line 29: https://github.com/Lachei/VulkanPBRT/blob/b14c334ce33f17f434a12a6a61caa94f807b3ba5/UtilityPipelines/FormatConverter.cpp#L29 I have added a reflection part to my local vsg library, as this allows for a very fast pipeline initialization and easier shader editing. The current implementation in the repo is not optimal however.

[robertosfield]

On Mon, 15 Nov 2021 at 18:31, Lachei @.***> wrote:

Also maybe a feature for later can be seen in line 29:

https://github.com/Lachei/VulkanPBRT/blob/b14c334ce33f17f434a12a6a61caa94f807b3ba5/UtilityPipelines/FormatConverter.cpp#L29 I have added a reflection part to my local vsg library, as this allows for a very fast pipeline initialization and better handling. The current implementation in the repo is not optimal however.

Do you have a commit for the other changes you made? Knowing what code you've changed will help me understand what you've done and how it's intended to be used.

[Lachei]

Here are the consecutive commits which implement the reflection: https://github.com/Lachei/VulkanPBRT/commit/43308e429eb839cc4c0709037bcee13a32501415 https://github.com/Lachei/VulkanPBRT/commit/0b23a424dfe2b30f710c0555ad6db6b01f1b24af https://github.com/Lachei/VulkanPBRT/commit/e799f6a34e5eb512a41c80fd36876dc2b96e1a84 https://github.com/Lachei/VulkanPBRT/commit/b38121c934792321e092f1814fca516fb004359e And a small final bugfix: https://github.com/Lachei/VulkanPBRT/commit/cda19617726cec7f28e2657673d1655226077181

For a final version i would make the BindingMap a separate class which has the method to get the descriptor set binding and descriptor set index inside it to have the method at a more intuitive place. Also the mergeBindingMaps method should go into that class. I would however keep the internal way to store the bindings, as the creation of descriptor set layouts does not have to be change because the DescriptorSetLayoutBindings can be handed over by simply access it in the BindingMap as it is done in our current implementation.

[robertosfield]

On Mon, 15 Nov 2021 at 20:07, Lachei @.***> wrote:

Here are the consecutive commits which implement the reflection: @. https://github.com/Lachei/VulkanPBRT/commit/43308e429eb839cc4c0709037bcee13a32501415 @. https://github.com/Lachei/VulkanPBRT/commit/0b23a424dfe2b30f710c0555ad6db6b01f1b24af @. https://github.com/Lachei/VulkanPBRT/commit/e799f6a34e5eb512a41c80fd36876dc2b96e1a84 @. https://github.com/Lachei/VulkanPBRT/commit/b38121c934792321e092f1814fca516fb004359e And a small final bugfix: @.*** https://github.com/Lachei/VulkanPBRT/commit/cda19617726cec7f28e2657673d1655226077181

For a final version i would make the BindingMap a separate class which has the method to get the descriptor set binding and descriptor set index inside it to have the method at a more intuitive place. Also the mergeBindingMaps method should go into that class. I would however keep the internal way to store the bindings, as the creation of descriptor set layouts does not have to be change because the DescriptorSetLayoutBindings can be handed over by simply access it in the BindingMap as it is done in our current implementation.

Thanks for the links, I'll have a look over once I've resolved the include issue.

On the include front I have come to the conclusion that the includes have to be resolved at the same time as the load, the easiest would be to insert all the includes and place them in the ShaderModule::source string. This isn't perfect but would at least resolve one part of the jigsaw without spreading complexity out over the code base. I may still opt for some form of substitution map, but may leave that for a future rev.

I would like to resolve how to handle the compile error side of things but again not sure I should try to resolve everything today. Tomorrow I'm back on client for the Wendsday-Friday, and Monday-Wednesday so don't want to get part way through a big refactor.

--

On a different topic, I see that you have a OpenEXR ReaderWriter in your version of vsgXchange. This looks like it could be moved into the main vsgXchange library, both the licenses are MIT so are compatible. Thoughts?

[Lachei]

On a different topic, I see that you have a OpenEXR ReaderWriter in your version of vsgXchange. This looks like it could be moved into the main vsgXchange library, both the licenses are MIT so are compatible. Thoughts?

Yes, i need openEXR file loading for being able to process externally produced renderings including gBuffer information and forward them to the denoisers implemented. As we are also currently working on automatic image generation for things like neural network training we also need to export these, so i have implemented already the writer for that... As for moving the to the main vsgXchange library: If you want to include openEXR support I would be glad to provide the code, no problems with me. I only would suggest to wait for a few weeks, as i have not yet checked the implementation thoroughly enough. Also, the openEXR library is soon to be updated for all platforms to the 3.x version which means that the math library is moved out of the standard openexr library and thus has to be included and linked different.

[robertosfield]

Will wait on merging OpenEXR then.

I have now merged my changes for recursive includes to VulkanSceneGraph and vsgXchange respectively:

https://github.com/vsg-dev/VulkanSceneGraph/pull/363 https://github.com/vsg-dev/vsgXchange/pull/64

The ShaderCompile::insertIncludes() has been moved to vsg::insertIncludes(), it and the ShaderCompile::compile(..) methods now take a ref_ptr options rather Paths, this enables far more flexible handling of finding and reading files.

vsgXchange also has been updated to cal vsg::insertIncludes() in the glsl ReaderWriter. This insures that all the paths are properly resolved. It does however mean that the source is full expanded and no longer matches the original shader file, as it'll include all the included parts, including any nested includes.

For your own code base I'd recommend just using the vsg::read_cast("myshader.vert") and assigning a vsg::ShaderCompileSettings to the ShaderModule when you need to inject #defines.

I'll now have a look through your links/have a think about improving the defines side.

[Lachei]

It does however mean that the source is full expanded and no longer matches the original shader file, as it'll include all the included parts, including any nested includes.

While it is of course a pity that it does not resemble the original shader file, this is actually very useful for debugging if there have been compilation errors. The shader compiler always uses the fully expanded source and thus the compile errors are printed wrt this source. Now the debug print of the shader does correspond correctly to the errors produced. Also, according to my experience, it is very easy to deduce where the different parts of the full shader come frome due to the //begin xxx header //end xx header comments which are automatically added.

Maybe another intereseting thing to add to the vulkan scenegraph would be a fly navigation as an alternative for the trackball. I happen to also have a first implementation for that in my repo: https://github.com/Lachei/VulkanPBRT/blob/master/vsg/src/vsg/viewer/FlyNavigation.cpp

[robertosfield]

I have now done a code review of your changes to ShaderStage which add the bindings reflection. I now understand the motivation behind the changes, but at this point haven't had to time ponder the topic to know the best path forward.

My instinct would be to have a dedicated class that provides the list of bindings and associated names, and the push constant ranges, then this could be populated by parsing the shader source or by using SPIRV-Reflect as you have done, I'd rather not add another dependency to the core VSG. The binding class could then be associated with a ShaderModule for applications that need to look up the bindings, i.e. ShaderModule "has a".ref_ptr. One might want to inject the bindings to the shader source, rather than read them from the shader, though this is something that can be done with #define's.

For VulaknPBRT I think you could create such a Bindings class and populate it independently from ShaderModule/ShaderStage and just associated it with one of these via the VSG's meta data object system. You could possibly even use this more directly and leverage it to map the names to indices by doing shaderModuke->setValue("bindingName", 0) & getValue("bindingName", value);

Avoiding medications to the core VSG would make it easier to update your own code base with changes to the VSG.

W.r.t FlyNavigation, is that what VulkanPBRT uses by default?

[Lachei]

W.r.t FlyNavigation, is that what VulkanPBRT uses by default?

No, trackball is default. By adding "--fly" to the command line arguments the fly navigation is used

[Lachei]

if(useFlyNavigation)
  viewer->addEventHandler(vsg::FlyNavigation::create(camera));
else
  viewer->addEventHandler(vsg::Trackball::create(camera));

[robertosfield]

Just tired:

./VulkanPBRT -i ~/Data/glTF-Sample-Models/2.0/Sponza/glTF/Sponza.gltf --fly

For navigating this model it works much better than the vsg::Trackball. I feel both could be improved/learn from the other to make them better.

I will need to return to this topic as I've taken a bit of left turn from the path of the work I was planning for this Monday/Tuesday. I'll close this Issue for now. The outstanding topics would be good to return to, vsg-users google group is probably be best venue for discussions on this. In a couple of weeks I'll start up a discussion on outstanding items for VSG-1.0, so this would be ideal to drop in there.