Skip to content

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
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@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