WIP: Add new plugin: real time shader selection

[Dawid-Lorenz-Mobica]

Description

This PR adds a new plugin that enables dynamic shader switching in samples. The plugin can be enabled via the command line switch "--realtimeshaderselection". The sample must define the shaders that can be used by calling "ApiVulkanSample::store_shader" and implement the "change_shader" function, which notifies the sample about shader changes.

General Checklist:

Please ensure the following points are checked:

• [x] My code follows the coding style

• [ ] I have reviewed file licenses

• [ ] I have commented any added functions (in line with Doxygen)

• [ ] I have commented any code that could be hard to understand

• [ ] My changes do not add any new compiler warnings

• [ ] My changes do not add any new validation layer errors or warnings

• [ ] I have used existing framework/helper functions where possible

• [ ] My changes do not add any regressions

• [ ] I have tested every sample to ensure everything runs correctly

• [ ] This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

• [ ] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
• [ ] My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

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
• [ ] For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering

[CLAassistant]

All committers have signed the CLA.

[Dawid-Lorenz-Mobica]

Interesting approach! Did I get it right, that nothing is done when switching the language?

yes, currently only this sample "dynamic_uniform_buffers" can support this plugin

"args": [
    "sample",
    "dynamic_uniform_buffers"
    ,"--realtimeshaderselection"
]

and changing the shader won't change anything because the function " DynamicUniformBuffers::change_shader is empty (no shader replacement). I wasn't sure if it makes sense to implement this for this sample, because I know that the principle of loading shaders in this PR https://github.com/KhronosGroup/Vulkan-Samples/pull/655 will change a bit, so I also didn't want to generate conflicts. But if you think it's worth implementing this function to show the actions, no problem.

[Dawid-Lorenz-Mobica]

I found 2 more naming convention issues. Besides that, without actual usage of this new plugin, you don't know for sure that it's working. So I would vote for actually changing the shader in at least one sample to demonstrate it. Even though that might be changed by some future shader handling changes.

I added an example to dynamic_uniform_buffers sample (DynamicUniformBuffers::change_shader) The shader (base.frag.spv) has changed colors from the original (base.frag) file to make it easier to see the differences in the shaders.

[asuessenbach]

Looks good now, and works. Would be great, if you could adjust hpp_dynamic_uniform_buffers accordingly.

[Dawid-Lorenz-Mobica]

Hi @asuessenbach , thanks for noticing that there is no HPP example. During the implementation I noticed that we have two classes for the Drawer (ImGUi) class Drawer and class HPPDrawer Do we really need two implementations to use ImGui? In my opinion, this version of HPP should be removed. Because, The Drawer class isn't written in C anyway, it uses std::array and std::vector among others, so it's not C compatible anyway. CMake builds it in CPP also. Additional is more extensive, has several more options.

So my suggestion is to remove HPPDrawer and use only Drawer class. Because currently I don't see any logical example that we need to have two classes that do exactly the same thing and the only difference is in the types. Alternatively, if we don't want to remove HPPDrawer, I think we should make an Interface IDrawer that describes the functions that can be used in these classes.

Do you think that we should remove the HPPDrawer class and the ability to implement the Drawer class in functions in C and CPP, or creating an interface will be a better solution ?

[asuessenbach]

Totally agree, that Drawer and HPPDrawer are redundant. HPPDrawer was just introduced, when HPPGui was introduced, even though the Drawer is completely vulkan-agnostic. I think, the best approach would be to move that Drawer into its own files, use that from both Gui and HPPGui, and remove HPPDrawer.

[Dawid-Lorenz-Mobica]

As the last commit added quite a lot of changes, I will describe those that may be the most important

1. In file Vulkan-Samples\framework\common\hpp_vk_common.h:140 i changed the type for vk::PipelineShaderStageCreateInfo, previous you could only specify array[2] now it takes dynamic array/vector.

vk::ArrayProxyNoTemporaries<const vk::PipelineShaderStageCreateInfo> const &shader_stages

2. In file Vulkan-Samples\framework\drawer.h:29 i added

using HPPDrawer = Drawer;

because the samples from HPP used such a name and I didn't want to change it in a very large number of files, of course if we want to get rid of the HPPDrawer name, I can remove the "using" and change the class name in each of the files

3. ::create_pipeline It currently uses VkShaderStageFlagBits and for code written in Vulkan HPP it is not the default format, so for example in the file Vulkan-Samples\samples\api\hpp_dynamic_uniform_buffers\hpp_dynamic_uniform_buffers.cpp:208 I'm using a cast, possibly we can change it to:

a) Use a template, it will involve quite a lot of changes b) Create two variables one that stores c-style shaders and the other in cpp, easy to change but not very elegant c) Create a new internal type for shader types, then add the support in load_shader d) Or leave it as is

[asuessenbach]

After thinking about your changes yet another time, I would propose the following adjustments:

• As the available_shaders need to be in the vkb::Application (because vkb::Platform::get_app, called by plugins::RealTimeShaderSelection::on_update_ui_overlay can get you just that), you should have a non-virtual public function store_shaders and a virtual protected function change_language (or so). change_language would not need to get the vector of shaders, the vkb::ShaderSourceLanguage would suffice. vkb::Application::get_available_shaders should be const.
• To make that work with vkb::HPPVulkanSample, it could "hide" store_shaders and get_available_shaders by making them private, and provide corresponding functions using vk::ShaderStageFlagBits, calling the Application's function using the reinterpret_cast-trick.
• No need to have a copy of the shaders in plugins::RealTimeShaderSelection. You can always get them via vkb::Platform::get_app().get_available_shaders().

[Dawid-Lorenz-Mobica]

I did as you @asuessenbach suggested with a few changes

• virtual void change_shader is public, because RealTimeShaderSelection uses this function from the base class Application
• (no-virtual) get_available_shaders is public, because RealTimeShaderSelection uses this function from the base class Application. And for HPP we use casting HPPVulkanSample::get_available_shaders
• (no-virtual) void store_shaders is protected, There is no need to make this function public as it is only used by s sample

[marty-johnson59]

Hi @Dawid-Lorenz-Mobica, this looks ready to merge except for some branch conflicts. Can you resolve those and LMK when ready to merge? Thanks!

[pawel-jastrzebski-mobica]

@tomadamatkinson @asuessenbach @gary-sweet @SaschaWillems @JoseEmilio-ARM @gpx1000: I updated the branch to latest main. Since @Dawid-Lorenz-Mobica is no longer working on this issue and I cannot change the title to remove the "WIP" prefix, because I am not an author of this PR, please consider it as ready for review.

@SaschaWillems : I noticed that there is the VK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT error reported by the validation layer. We discussed it earlier that it should have already been solved by https://github.com/KhronosGroup/Vulkan-Samples/pull/775 :

[error] [framework\core\hpp_instance.cpp:48] -1303445259 - VUID-vkBeginCommandBuffer-commandBuffer-00050: Validation Error: [ VUID-vkBeginCommandBuffer-commandBuffer-00050 ] Object 0: handle = 0x169b0b7bc70, type = VK_OBJECT_TYPE_COMMAND_BUFFER; Object 1: handle = 0x27d60e0000000019, type = VK_OBJECT_TYPE_COMMAND_POOL; | MessageID = 0xb24f00f5 | Call to vkBeginCommandBuffer() on VkCommandBuffer 0x169b0b7bc70[] attempts to implicitly reset cmdBuffer created from VkCommandPool 0x27d60e0000000019[] that does NOT have the  bit set. The Vulkan spec states: If commandBuffer was allocated from a VkCommandPool which did not have the VK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT flag set, commandBuffer must be in the initial state (https://vulkan.lunarg.com/doc/view/1.3.216.0/windows/1.3-extensions/vkspec.html#VUID-vkBeginCommandBuffer-commandBuffer-00050)

[asuessenbach]

While merging with main, some error might have happen. In HPPDynamicUniformBuffers::prepare, some code has been duplicated. Instead of

	if (HPPApiVulkanSample::prepare(options))
	{
		prepare_camera();
		generate_cube();
		prepare_uniform_buffers();
		descriptor_set_layout = create_descriptor_set_layout();
		pipeline_layout       = get_device()->get_handle().createPipelineLayout({{}, descriptor_set_layout});
		pipeline              = create_pipeline(supported_shaders.begin()->first, supported_shaders.begin()->second);
		descriptor_pool       = create_descriptor_pool();
		descriptor_set        = vkb::common::allocate_descriptor_set(get_device()->get_handle(), descriptor_pool, descriptor_set_layout);
		update_descriptor_set();
		build_command_buffers();

		prepared = true;
	}

	vk::Device device = get_device()->get_handle();

	prepare_camera();
	generate_cube();
	prepare_uniform_buffers();

	descriptor_set_layout = create_descriptor_set_layout();
	pipeline_layout       = get_device()->get_handle().createPipelineLayout({{}, descriptor_set_layout});
	pipeline              = create_pipeline(supported_shaders.begin()->first, supported_shaders.begin()->second);
	descriptor_pool       = create_descriptor_pool();
	descriptor_set        = vkb::common::allocate_descriptor_set(get_device()->get_handle(), descriptor_pool, descriptor_set_layout);

	update_descriptor_set();
	build_command_buffers();
	prepared = true;
	return true;

it should just be

	if (HPPApiVulkanSample::prepare(options))
	{
		prepare_camera();
		generate_cube();
		prepare_uniform_buffers();
		descriptor_set_layout = create_descriptor_set_layout();
		pipeline_layout       = get_device()->get_handle().createPipelineLayout({{}, descriptor_set_layout});
		pipeline              = create_pipeline(supported_shaders.begin()->first, supported_shaders.begin()->second);
		descriptor_pool       = create_descriptor_pool();
		descriptor_set        = vkb::common::allocate_descriptor_set(get_device()->get_handle(), descriptor_pool, descriptor_set_layout);
		update_descriptor_set();
		build_command_buffers();

		prepared = true;
	}

	return prepared;

[pawel-jastrzebski-mobica]

@SaschaWillems

I'm not sure if we want to merge this in it's current state. The changes to the framework allowing to toggle between GLSL and SPIR-V are fine, but I don't think we want explicit shader selection in only a few samples (in this case the dynamic descriptor buffer one). Our goal is tho have GLSL and HLSL shaders that match 1:1, while this PR (at least judging by the screenshots) looks different when selecting SPV. And I can't see any HLSL files, so how are the new SPV files generated.

So to be in line with our framework discussions, this should only add SPV loading capabilities to the framework and not do any changes to an existing sample.

Probably need to discuss this in detail on the next call.

Without merging the PR #655 we need to limit this PR to selection between GLSL and SPV. When the appropriate mechanism for offline and/or online compilation of HLSL is added, the plugin added by this PR may be extended with HLSL. In other case it will be still blocked by HLSL compilation and we do not know when it will be possible to merge it.

[marty-johnson59]

Oops, needs header update with 2024 copyright..

[Seweryn-Zielas-Mobica]

@SaschaWillems

I'm not sure if we want to merge this in it's current state. The changes to the framework allowing to toggle between GLSL and SPIR-V are fine, but I don't think we want explicit shader selection in only a few samples (in this case the dynamic descriptor buffer one). Our goal is tho have GLSL and HLSL shaders that match 1:1, while this PR (at least judging by the screenshots) looks different when selecting SPV. And I can't see any HLSL files, so how are the new SPV files generated.

So to be in line with our framework discussions, this should only add SPV loading capabilities to the framework and not do any changes to an existing sample.

Probably need to discuss this in detail on the next call.

Hi Sascha, plugin related code from samples was removed, so there are no longer any changes to already existings samples.

About HLSL, this PR only add plugin for dynamic shader switching, so while there is "support" for HLSL shader in plugin itself, there is no support for HLSL shaders compilation in Vulkan Samples framework. Currently if anybody would try to use HLSL shader with plugin, he will get error: "The format is not supported" - link If I'm thinking correctly, HLSL shaders should work with this plugin without any issues, when support for their compilation will be added for framework in the future.

About mismatch in the shaders from screenshots - GLSL and SPIRV - I checked that with spirv-tools and it was made on purpose. There was a difference green channel in frag shader so you could see change of the shaders with your eyes, I also compiled GLSL into binary and replaced SPIRV shader with it to be sure. If used properly, there should be no difference in usage of different shaders.

I am open for any discussion about this and I'm waiting for your response.

[Seweryn-Zielas-Mobica]

I have tested every sample to ensure everything runs correctly - used "batch" command on Ubuntu 22.04.3 LTS. Didn't spot any new compiler warnings, validation errors/warnings or any other regression.

[Seweryn-Zielas-Mobica]

@gary-sweet About testing this plugin, I think easiest/quickest way would be to use patch that I attached to this comment, It is basically revert of commits that removed plugin support from Dynamic Uniform Buffers sample with resolved conflicts. realtime_shader_selection_uniform_buffers.patch.tar.gz

[gary-sweet]

@gary-sweet About testing this plugin, I think easiest/quickest way would be to use patch that I attached to this comment, It is basically revert of commits that removed plugin support from Dynamic Uniform Buffers sample with resolved conflicts. realtime_shader_selection_uniform_buffers.patch.tar.gz

@Seweryn-Zielas-Mobica That patch doesn't apply for me (error: git diff header lacks filename information when removing 1 leading pathname component (line 237))

Can you make a separate temporary PR that has that change on top of this one that I can checkout instead perhaps?

[Seweryn-Zielas-Mobica]

@gary-sweet I created temporary PR like you requested - https://github.com/KhronosGroup/Vulkan-Samples/pull/953 Also there is one fail and canceled checks here that looks like infrastructure issue, is there a way to make a re-run of CI checks?

[gary-sweet]

@gary-sweet I created temporary PR like you requested - #953 Also there is one fail and canceled checks here that looks like infrastructure issue, is there a way to make a re-run of CI checks?

I don't have that ability. It may be quicker to just push a small change to this PR to re-trigger them.

[marty-johnson59]

Looks like we have merge conflicts on this one. LMK when fixed and I'll merge. Thx

[Seweryn-Zielas-Mobica]

@marty-johnson59 I've fixed merge conflict, it was a small one in ".copyrightignore" file.