WIP: Redesign functionality of GLSLcompiler class #655

Szymon-Rajczakowski-Mobica · 2023-03-29T08:48:10Z

Description

Redesign functionality of GLSLcompiler class. Change name of this class to ShaderCompiler. Add support for HLSL, GLSL and SPV files.

General Checklist:

Please ensure the following points are checked:

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

I have tested the sample on at least one compliant Vulkan implementation
If the sample is vendor-specific, I have tagged it appropriately
I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
Any dependent assets have been merged and published in downstream modules
For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme

SaschaWillems · 2023-03-29T17:13:38Z

Not sure if this should be a separate sample. Imo it would be better if this was an addition to the existing basic raytracing sample, e.g. maybe a compile time define so people can toggle between GLSL and HLSL.

samples/extensions/raytracing_basic/raytracing_basic.cpp

shaders/khr_ray_tracing_basic/hlsl_closesthit.rchit

asuessenbach

I tried to run hlsl_shaders, but get the following error:

[error] [framework\common\vk_common.cpp:386] Failed to compile shader, Error: ERROR: #version: ES shaders for SPIR-V require version 310 or higher
ERROR: :20: '' :  syntax error, unexpected LEFT_BRACKET
ERROR: 2 compilation errors.  No code generated.

Would I need to update anything on my end?

Besides that: is there any means to switch between HSLS and GLSL now?

framework/common/vk_common.cpp

framework/shader_compiler.cpp

framework/shader_compiler.h

samples/extensions/raytracing_basic/raytracing_basic.cpp

CLAassistant · 2023-06-26T07:50:40Z

All committers have signed the CLA.

samples/extensions/raytracing_basic/raytracing_basic.cpp

framework/api_vulkan_sample.cpp

framework/common/vk_common.cpp

framework/shader_compiler.h

.copyrightignore

scripts/copyright.py

samples/extensions/raytracing_basic/CMakeLists.txt

SaschaWillems · 2023-07-03T17:00:44Z

framework/shader_compiler.h

@@ -33,7 +33,7 @@ namespace vkb
 {
 /// Helper class to generate SPIRV code from GLSL source


With the compiler now being able to support other languages, the comments need to be changed too. They still refer to GLSL only.

That comment still needs to be updated.

SaschaWillems · 2023-07-03T17:01:23Z

framework/shader_compiler.h

@@ -54,19 +54,21 @@ class GLSLCompiler
 	static void reset_target_environment();

 	/**
-	 * @brief Compiles GLSL to SPIRV code
+	 * @brief Compiles GLSL/HLSL to SPIRV code


Is this correct? If so, what tool is used to compile HLSL tp SPIR-V (dxc)? Or does it simply load offline compiled SPV? If it's the latter, the comment needs to be changed.

Glslang is used for compilation at runtime and for offline compilation DirectX Shader Compiler is being used.

SaschaWillems · 2023-07-03T17:02:29Z

samples/api/hlsl_shaders/hlsl_shaders.cpp

@@ -410,7 +410,7 @@ void HlslShaders::prepare_pipelines()

 	// Load shaders
 	std::array<VkPipelineShaderStageCreateInfo, 2> shader_stages{};
-	shader_stages[0] = load_hlsl_shader("hlsl_shaders/hlsl_shader.vert", VK_SHADER_STAGE_VERTEX_BIT);
+	shader_stages[0] = load_shader("hlsl_shaders/hlsl_shader.vert", VK_SHADER_STAGE_VERTEX_BIT, vkb::ShaderSourceLanguage::VK_HLSL);


This change doesn't seem correct. This is a standalone sample that is supposed to use it's own hlsl loading function, but this line would use the one from the framework instead. The change should be reverted.

This still needs to be reverted.

SaschaWillems · 2023-07-03T17:02:53Z

samples/api/hpp_hello_triangle/hpp_hello_triangle.cpp

@@ -1,4 +1,4 @@
-/* Copyright (c) 2021-2023, NVIDIA CORPORATION. All rights reserved.


I don't think it's okay to replace the existing copyright. If you add to a file, please append your copyright instead.

This is still open. This sample was done by Andreas (Nvidia).

I had to revert fix, it caused CI checks for copyrights to fail

SaschaWillems · 2023-07-03T17:14:44Z

I did a first quick review and added a few comments.

One thing I'm not sure about is the direction of this PR.

On one hand it seems to add HLSL support to the framework's compiler, but only through glslang, which is very very limited. And on the other hand it also adds direct loading of SPIR-V for e.g. the basic ray tracing sample.

Tbh. I think we should discuss a different approach that better fits our framework strategy.

We've been talking about HLSL support for quite some time and one option would be adding direct SPIR-V loading to the shader compiler (as a derived class maybe) so we can add HLSL to all samples with offline compiled SPIR-V files via DXC. The goal is also to not have to change every sample but to support shader language selection at framework level instead. And imo we should move into that direction.

I appreciate the work put in this PR, but it feels a bit like a stop gap towards our actual goal and we might revert a large part of that PR in the future.

I'll wait for other opinions, and I'd happily discuss this in the next samples call :)

Patryk-Jastrzebski-Mobica · 2023-07-17T12:26:43Z

Hi @SaschaWillems,
We have discussed this problem and think that the best solution will be to extend HLSL support (add raytracing support) in the glslang repository. At the moment @Dawid-Lorenz-Mobica is investigating how to add raytracing support for the HLSL language in the glslang repository.

SaschaWillems · 2023-07-17T15:03:24Z

Tbh I'm not sure if that's the right approach. HLSL in glslang is very limited and the actual reference compiler is DXC. So if we want to add HLSL support we should either integrate DXC or offline compile HLSL to SPIR-V and just use that.

Update README fix validation layers errors

…e the same as in setup_render_pass

Remove hlsl_raytracing_basic

to shader_compiler - Add ShaderSourceLanguage enum - Add src_language parameter to load_shader for support different shader languages - Change headers and few names in existing samples

to keep the order of arguments more meaningful - Add switch in UI to changing shaders language in raytracing_basic sample

fix typo bring HPP load_shader() and update_render_pass_flags() in line with non-hpp version use a single compile_to_spirv() with default source language rather than have 2 flavours

Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>

SaschaWillems · 2024-02-13T18:53:27Z

docs/build.adoc

@@ -208,13 +212,20 @@ build\windows\app\bin\Release\AMD64\vulkan_samples.exe
 * C{pp}14 Compiler
 * <<cmake-options,CMake Options>>
 * <<3d-models,3D models>>
+* Vulkan SDK


Can this be reverted? We don't require the SDK to build our samples.

SaschaWillems · 2024-02-13T18:53:38Z

docs/build.adoc

@@ -243,11 +254,18 @@ cmake --build build/linux --config Release --target vulkan_samples -j$(nproc)
 * https://vulkan.lunarg.com/doc/sdk/latest/mac/getting_started.html[Vulkan SDK] `./install_vulkan.py`
 * <<cmake-options,CMake Options>>
 * <<3d-models,3D models>>
+* Vulkan SDK


Same as above.

SaschaWillems · 2024-02-13T19:06:22Z

Also odd, I get three out-of-place hlsl related entries in the MSVC project browser now:

Probably left overs. Would be good if this PR did not include any HLSL shaders at all, but just the code required to load SPIR-V. We'll add the HLSL in separate PRs.

pawel-jastrzebski-mobica · 2024-02-15T10:35:57Z

@SaschaWillems : do you want to remove the whole off-line shader compilation functionality from the PR?

Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>

SaschaWillems · 2024-02-18T15:30:39Z

Nope, having a way to compile the HLSL shaders vie the build system is fine. Problem is where those projects are put in the project structure. They appear at the top level, but should instead be part of the sample itself.

unnecessary spv files.

from add_custom_command to execute_process.

for hpp_hello_triangle.cpp

Seweryn-Zielas-Mobica · 2024-03-11T12:35:20Z

I changed way of using dxc for offline compilation. I had problem with add_custom_target/add_custom_command that created targets in top level view of Visual Studio solution, I was only found way how to move it into other folders, but couldn't move it under other targets like specific samples. From what I red and understood, this is most probably not possible.
Instead of add_custom_target/add_custom_command is used execute_process which seems to work as good or maybe even better than previous solution, as now code for compiling shaders seems more straightforward and less bulky.

The difference in how it works now is rather not significant, before HLSL shaders were compiled at the start of executing build command, e.g. "cmake --build build/linux --config Release --target vulkan_samples -j$(nproc)"
Now they are compiled after running command for generating project, e.g. "cmake -G "Unix Makefiles" -Bbuild/linux -DCMAKE_BUILD_TYPE=Release"

I also updated cmake to only compile shaders if dxc executable is found, by searching for it under VULKAN_SDK path.

There is still glslang used for compilation HLSL shaders at sample runtime, and rather big modification of hpp_hello_triangle.cpp which I'm not sure if is needed.

There is still some work left to do, mostly stylistic like naming scheme and directories/folders paths, but core functionality of this PR that is compiling HLSL shaders seems to be done.

SaschaWillems

Can you undo the addition of HLSL shaders for the ray tracing basic sample. I'm already working on adding actual HLSL shaders, and to clearly divide work this PR should only add the offline compilation part.

Seweryn-Zielas-Mobica · 2024-03-18T09:49:47Z

@SaschaWillems As we discussed that we should only add offline compilation for HLSL, I've created created new PR that only do this, as it seemed easier and less misguiding when looking at this PR title.
New PR: #990

marty-johnson59 · 2024-04-08T15:22:38Z

Closing this as we're going a different direction

asuessenbach reviewed May 3, 2023

View reviewed changes

samples/extensions/raytracing_basic/raytracing_basic.cpp Outdated Show resolved Hide resolved

shaders/khr_ray_tracing_basic/hlsl_closesthit.rchit Outdated Show resolved Hide resolved

asuessenbach reviewed May 16, 2023

View reviewed changes

Szymon-Rajczakowski-Mobica force-pushed the hlsl_raytracing_basic branch from 0dfc743 to e9ba043 Compare June 19, 2023 11:16

Szymon-Rajczakowski-Mobica changed the title ~~WIP Hlsl raytracing basic~~ Redesign functionality of GLSLcompiler class Jun 19, 2023

Szymon-Rajczakowski-Mobica marked this pull request as ready for review June 19, 2023 12:06

lorna-mcneill-mobica force-pushed the hlsl_raytracing_basic branch from 358c127 to f8d8ba0 Compare June 26, 2023 07:33

asuessenbach reviewed Jun 27, 2023

View reviewed changes

asuessenbach reviewed Jul 3, 2023

View reviewed changes

.copyrightignore Outdated Show resolved Hide resolved

scripts/copyright.py Outdated Show resolved Hide resolved

asuessenbach previously approved these changes Jul 3, 2023

View reviewed changes

lorna-mcneill-mobica dismissed asuessenbach’s stale review via 8a92783 July 3, 2023 12:37

marty-johnson59 requested a review from tomadamatkinson July 3, 2023 15:33

SaschaWillems requested changes Jul 3, 2023

View reviewed changes

Dawid-Lorenz-Mobica mentioned this pull request Jul 10, 2023

Add new plugin: real time shader selection #733

Merged

18 tasks

lorna-mcneill-mobica mentioned this pull request Jul 10, 2023

Bring the non-hpp fw back in line with the hpp fw after they diverged when synchronization fixes were made to hpp api samples fw #747

Merged

6 tasks

Szymon-Rajczakowski-Mobica and others added 11 commits September 26, 2023 10:36

Add hlsl_raytacing_basic sample

950fcbe

Add hlsl_raytacing_basic sample

587e884

Update shaders

15f824f

Update README fix validation layers errors

Add missing flags in update_render_pass_flags function, they should b…

af3862f

…e the same as in setup_render_pass

Add define HLSL_SHADER to raytracing_basic

5f4adc7

Remove hlsl_raytracing_basic

- Change glsl_compiler class name

dd4f055

to shader_compiler - Add ShaderSourceLanguage enum - Add src_language parameter to load_shader for support different shader languages - Change headers and few names in existing samples

-Overloading the compile_to_spirv method

45bbaa9

to keep the order of arguments more meaningful - Add switch in UI to changing shaders language in raytracing_basic sample

Rebase and resolve commit problems

c387bb6

Fixed quality of code and copyrights

908a391

Fix build breaks

4d64cd7

Address review comments and fix copyright check issue

359a231

fix typo bring HPP load_shader() and update_render_pass_flags() in line with non-hpp version use a single compile_to_spirv() with default source language rather than have 2 flavours

Fix clang-format.

7aecbdb

Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>

Szymon-Rajczakowski-Mobica changed the title ~~Redesign functionality of GLSLcompiler class~~ WIP: Redesign functionality of GLSLcompiler class Feb 12, 2024

SaschaWillems reviewed Feb 13, 2024

View reviewed changes

Fix copyrights.

035a094

Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>

Merge branch 'main' into hlsl_raytracing_basic

900584f

SaschaWillems mentioned this pull request Mar 1, 2024

Add HLSL shaders #892

Closed

Seweryn-Zielas-Mobica added 14 commits March 4, 2024 14:39

Merge branch 'main' into hlsl_raytracing_basic

aa4d378

Working build without dxc and fixed usage of dxc - missing spirv flag

e855a79

Modify custom command creation for DXC compiler and remove

6aece42

unnecessary spv files.

Fix build issues after merge

594b2d3

Fix shaders names in ray tracing basic CMakeLists.txt

6b2255d

Fix Subpass dependencies for layout transitions

ca52111

Modify ray_tracing_basic to use real time shader selection plugin.

4365030

Fix handling shaders in hlsl_shaders and hpp_hlsl_shaders.

775eaa3

Udpate copyrights

4b54545

Fix clang-format

19152b1

Remove Vulkan SDK dependency from build.adoc

cc95ca3

Change usage of DirectX Shader Compiler in sample_helper.cmake

6cbf099

from add_custom_command to execute_process.

Update comment for ShaderCompiler class and update copyright

87791c5

for hpp_hello_triangle.cpp

Fix copyright in hpp_hello_triangle.cpp

04e5b0d

Merge branch 'main' into hlsl_raytracing_basic

6d347ed

SaschaWillems requested changes Mar 13, 2024

View reviewed changes

marty-johnson59 closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Redesign functionality of GLSLcompiler class #655

WIP: Redesign functionality of GLSLcompiler class #655

Szymon-Rajczakowski-Mobica commented Mar 29, 2023 •

edited

Loading

SaschaWillems commented Mar 29, 2023

asuessenbach left a comment

CLAassistant commented Jun 26, 2023 •

edited

Loading

SaschaWillems Jul 3, 2023

SaschaWillems Feb 3, 2024

Seweryn-Zielas-Mobica Mar 11, 2024

SaschaWillems Jul 3, 2023

Seweryn-Zielas-Mobica Mar 11, 2024

SaschaWillems Jul 3, 2023

SaschaWillems Feb 3, 2024

Seweryn-Zielas-Mobica Mar 11, 2024

SaschaWillems Jul 3, 2023

SaschaWillems Feb 3, 2024

Seweryn-Zielas-Mobica Mar 11, 2024

Seweryn-Zielas-Mobica Mar 11, 2024

SaschaWillems commented Jul 3, 2023 •

edited

Loading

Patryk-Jastrzebski-Mobica commented Jul 17, 2023

SaschaWillems commented Jul 17, 2023

SaschaWillems Feb 13, 2024

Seweryn-Zielas-Mobica Mar 11, 2024

SaschaWillems Feb 13, 2024

Seweryn-Zielas-Mobica Mar 11, 2024

SaschaWillems commented Feb 13, 2024 •

edited

Loading

pawel-jastrzebski-mobica commented Feb 15, 2024

SaschaWillems commented Feb 18, 2024

Seweryn-Zielas-Mobica commented Mar 11, 2024

SaschaWillems left a comment

Seweryn-Zielas-Mobica commented Mar 18, 2024

marty-johnson59 commented Apr 8, 2024

		@@ -33,7 +33,7 @@ namespace vkb
		{
		/// Helper class to generate SPIRV code from GLSL source

		@@ -1,4 +1,4 @@
		/* Copyright (c) 2021-2023, NVIDIA CORPORATION. All rights reserved.

WIP: Redesign functionality of GLSLcompiler class #655

WIP: Redesign functionality of GLSLcompiler class #655

Conversation

Szymon-Rajczakowski-Mobica commented Mar 29, 2023 • edited Loading

Description

General Checklist:

Sample Checklist

SaschaWillems commented Mar 29, 2023

asuessenbach left a comment

Choose a reason for hiding this comment

CLAassistant commented Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaWillems commented Jul 3, 2023 • edited Loading

Patryk-Jastrzebski-Mobica commented Jul 17, 2023

SaschaWillems commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaWillems commented Feb 13, 2024 • edited Loading

pawel-jastrzebski-mobica commented Feb 15, 2024

SaschaWillems commented Feb 18, 2024

Seweryn-Zielas-Mobica commented Mar 11, 2024

SaschaWillems left a comment

Choose a reason for hiding this comment

Seweryn-Zielas-Mobica commented Mar 18, 2024

marty-johnson59 commented Apr 8, 2024

Szymon-Rajczakowski-Mobica commented Mar 29, 2023 •

edited

Loading

CLAassistant commented Jun 26, 2023 •

edited

Loading

SaschaWillems commented Jul 3, 2023 •

edited

Loading

SaschaWillems commented Feb 13, 2024 •

edited

Loading