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

Added support for specialization constants. They are now automaticall… #197

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NikolayKanchevski
Copy link

As commit name states, I added support for specialization constants. All the code written follows the already-established style of the SPIRV-Reflect codebase, and I have provided all the needed functions/fields, that such a feature would make sense to have.

Additions:

  1. Two new fields within SpvReflectShaderModule:

    • uint32_t specialization_constant_count;
    • SpvReflectSpecializationConstant* specialization_constants;

    As you can see, a new type SpvReflectSpecializationConstant has also been added. It includes the following members:

    • const char* name;
    • uint32_t spirv_id;
    • uint32_t constant_id;
    • uint32_t size;
    • SpvReflectSpecializationConstantType constant_type; where all possible types are:
      • SPV_REFLECT_SPECIALIZATION_CONSTANT_BOOL
      • SPV_REFLECT_SPECIALIZATION_CONSTANT_INT
      • SPV_REFLECT_SPECIALIZATION_CONSTANT_FLOAT
    • union { float float_value; uint32_t uint_bool_value } default_value;

    These fields can freely be accessed in SpvReflectShaderModule, just as all other types (push_constant_blocks, descriptor_bindings, etc.) are.

  2. Two new enumerating methods:

    • spvReflectEnumerateSpecializationConstants(p_module, p_count, pp_constants) - a static method in spirv-reflect.h, which works like any other spvReflectEnumerate... method.

    • ShaderModule::EnumerateSpecializationConstants(p_count, pp_constants) - a public member method of the ShaderModule class. Just like the rest of ShaderModule's member enumerating functions, it relies on its static counterpart in spirv-reflect.h under the hood (in this case - spvReflectEnumerateSpecializationConstants()).

  3. Last but not least, SpvReflectToYaml (aka the yamlizer) also picks up specialization constants and displays them same way all other members are shown. Example:

specialization_constant_count: 3, 
specialization_constants:
    - *sc0 # "ENABLE_SHADOWS"
      spirv_id: 443
      constant_id: 0
      size: 4
      default_value (as float): 1.4013e-45
      default_value (as int): 1
      ...
  1. There are a few other new internal functions (for example for parsing the specialization constants), but they, obviously, will not be available to the user.

Note that all the new additions are properly documented, once again, following the general styling of SPIRV-Reflect's documentation within the code.

…y picked and can be retrieved through `SpvReflectShaderModule.specialization_constants`, `ShaderModule.EnumerateSpecializationConstants()` or `spvReflectEnumerateSpecializationConstants()`.
@CLAassistant
Copy link

CLAassistant commented Jun 25, 2023

CLA assistant check
All committers have signed the CLA.

@NikolayKanchevski
Copy link
Author

NikolayKanchevski commented Jun 26, 2023

In the second commit, I made push constant fields' names to be their actual GLSL in-shader member names (not their type names, which what they were populated with before). See this link.

… type names, which is what they used to be until now) thanks to @sopyer
@shangjiaxuan
Copy link

shangjiaxuan commented Jul 12, 2023

Hello, I don't want to intrude, but what's the main difference of this pr from mine #154 aside from having yaml output but no 64 bit and evaluation?

@NikolayKanchevski
Copy link
Author

Hello, @shangjiaxuan, it seems I have glossed over your PR, as I had not seen it before. I wish I had - this would have made my life much simpler! Anyway, I see they are very similar now, so if you want to, I could close this PR. You can by the way take the YAML output bit and put it in yours, as it is the original one.

@shangjiaxuan
Copy link

@NikichaTV I'm sorry I didn't have a good description in the pr until yesterday. I didn't implement YAML because I'm not quite familiar with YAML, and because the tests currently tests YAML by string comparison against set result (Adding will fail tests. I also added a few to test my added features, and failing early on will mean it may not test my cases). I hope you can leave this open so that when maintainers decide to merge or integrate this feature, they can be merged together.

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

Successfully merging this pull request may close these issues.

3 participants