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:

• 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.

[asuessenbach]

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

[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 #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.

[asuessenbach]

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.

[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

[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.

[tomadamatkinson]

Works locally for me. Fits well with the HLSL/GLSL offline compilation branch. We can expand this in the future to allow for hot reloading as well!

[SaschaWillems]

LGTM

[gary-sweet]

How do I test this? There don't appear to be any samples here that are actually using anything other than GLSL

[gary-sweet]

You shouldn't need to update copyright if the file hasn't been changed in this PR

[Seweryn-Zielas-Mobica]

Fixed

[gary-sweet]

This doesn't seem intentional. A single blank line has been removed.

[Seweryn-Zielas-Mobica]

Fixed

[gary-sweet]

Nothing changed here but the copyright year?

[Seweryn-Zielas-Mobica]

Fixed

[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 - #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.