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

GLSL Compiler #112

Closed
wants to merge 2 commits into from
Closed

Conversation

alexander-g
Copy link
Contributor

Included the GLSLang compiler as mentioned in #107.
The interface is a simple function, e.g: spirv = kp.compile_glsl_to_spirv(glsl)
The code is mostly adapted from the Vulkan-Samples repo.
Overhead is larger than I initially thought. The Python binary size increases from 1MB to 3.5MB.
Still I think it's very useful, especially for Python, but it's up to you whether or not to include it.

@axsaucedo
Copy link
Member

THank you for the contribution @alexander-g - this is very interesting, and I have to say that the glslang code to compile is super useful in itself, as there's very little out there that I have found which intutively covers how to compile shaders in a handful lines of code.

It does look like adding the logic to compile shaders may not be something we'd want to take on, as this could get quite complex given that the project in itself woudl be taken the maintenance overhead of being used to compile shaders.

Having said that, it would be good to discuss further particularly on a couple of thoughts:

  1. I actually thought your idea was to add the shaderc libraries as part of the installation, which would allow for all raw strings to be passed without having to do any explicit conversion
  2. Given that the way that it's currently used is to convert string shader to spirv bytes, would it not be easier to have this as a separate library that focuses just on this?
  3. Given it currently uses GLSL, it wouldn't support HLSL (as you would with something like shaderc) is there a reason why you went for GLSL instead of shaderc? Having said that shaderc woudl be an even bigger dependency so that could be even more complex.
  4. Have you tried just installing the shaderc libraries in your machine and just passing the raw string to confirm that this would be enough to fix it? If this is the case we could just include documentation on how to install shaderc in people's machines (as it would not require the extra step to convert the string to spirv bytes)

Let me know your thoughts on this.

Ultimately I think it would still be useful to have some of this code as a reference, but the main reasons why I think it probably is not something we'd want to add at the moment is:

  1. There would be a non-trivial maintenance overhead as people start using Kompute as their way to compile shaders,
  2. It does seem to add quite a bit of weight into the binary
  3. At this point I don't think it's clear to me that this would be a significant add as opposed to a nice-to-have
  4. Currently it would be limited to GLSL, and wouldn't support other languages like HSLS, so if we do look to bring this it probably would have to be with something like shaderc

But yeah, the glsl code is quite useful, I think we should add it into a snippets subfolder/repo as a separate piece of code, as it will definitely be useful potentially at some point.

@axsaucedo axsaucedo self-requested a review January 16, 2021 10:35
@axsaucedo axsaucedo added the enhancement New feature or request label Jan 16, 2021
@alexander-g
Copy link
Contributor Author

I haven't really given much thought to it. I thought since glslang is from Khronos so it must be the standard compiler. I've just installed pyshaderc, it compiles the shaders much faster but cannot compile many extensions (this might be because the python bindings are already several years old).
Regarding HLSL, glslang should be also able to deal with it, there is a flag which selects which one to use. I haven't tested it yet because I only use GLSL for now.

@axsaucedo
Copy link
Member

As discussed closing in favour of #154 and #153

@axsaucedo axsaucedo closed this Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants