Skip to content
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

Passing raw GLSL string to Shader Module depricated so remove this method from supported approach #150

Closed
axsaucedo opened this issue Feb 13, 2021 · 3 comments · Fixed by #154

Comments

@axsaucedo
Copy link
Member

I have been able to confirm #85, specifically on why raw glsl/hlsl shaders are only supported in subset of devices. This is mainly as only some drivers actually support this, however the support for this has been explicitly dropped since Vulkan 1.2. This is explicitly outlined here: https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/VK_NV_glsl_shader.html

Because of this, the ability to pass shaders as string is no longer supported, and hence this should no longer be a suggested way to interact with the Kompute package. The solution for this would be to revisit #112, and add the option to add the GLSL dependency either fully as a utility module as part of the kompute library, or only part of test_kompute. There are tradeoffs on approaching each - mainly the disadvantage of making it part of the kompute library is that it would add an extra dependency to manage - there is also an extra bulk on the resulting binary, however it could be explored to use dynamic libraries as opposed to static.

Currently if only the test_kompute package supports it, would still be ok given that a different dependency could be suggested to be used in the python, such as pyshaderc.

Given that it is quite an improvement in experience when it comes to passing raw shaders, it may be more convenient to fully adopt it, but initially it may only be part of the tests. This will also help ensure all the tests are run as part of #121.

@axsaucedo
Copy link
Member Author

@alexander-g I have been researching this further, and I remember you mentioned that the glslang package actually is missing some capabilities that would result in non-fully supported shaders. Do you know if this is the case? I am currently looking to explore once again #112, initially to add it to the tests only (and use pyshaderc in the python tests), but it seems that glslangc which is part of shaderc is more desirable as it has further support for glsl syntax.

If this is the case would you be interested to explore updating your PR to use shaderc instead of glslang? Alternatively I would be able to have a further look at it. We could also use the dynamic instead of static library to avoid adding further size to the resulting kompute binary.

Here is an example of how using shaderc can be leveraged (although this project has many more helper functions) https://github.com/jbikker/lighthouse2/blob/master/lib/rendercore_vulkan_rt/vulkan_shader.cpp

@alexander-g
Copy link
Contributor

I don't remember what I had mentioned anymore, but my current experience is actually the other way around: pyshaderc is not able to compile some extensions (e.g. I had experimented with GL_EXT_shader_realtime_clock). However, this might be simply due to the package not being up-to date with the C library shaderc. glslang is from Khronos so I think it should support everything.
I nevertheless use pyshaderc in my vkJAX project because it compiles shaders much faster than glslang. This is more important atm because I compile a long chain of parameterized shaders during run-time.

Yes, I wanted to take a closer look at shaderc anyway, but I cannot promise how fast I can do it.

@axsaucedo
Copy link
Member Author

I see @alexander-g - makes sense. Ok I actually had a look at the pyshaderc repo, and submitted an issue as it seems pyshaderc doesn't compile in Windows atm and the repo is not really maintained - the shaderc binaries it uses are from 4 years ago. Because of this, I think at this point it would be better to take on this as it would make sure it uses the latest binaries, and these can be upgraded as we require.

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