KhronosGroup / SPIRV-Reflect

SPIRV-Reflect is a lightweight library that provides a C/C++ reflection API for SPIR-V shader bytecode in Vulkan applications.
Apache License 2.0
672 stars 147 forks source link

YAML built_in for structs is unclear #269

Open dpwiz opened 2 months ago

dpwiz commented 2 months ago

For individual members it can be one of the SPIRV builtins. But for structures it turns into a CSV-string with ids :exploding_head: Either it should be a proper array, or a null, since there are no built-in structures (I hope).

spencer-lunarg commented 2 months ago

But for structures it turns into a CSV-string with ids

to be clear on the issue, can you give an example?

it should be a proper array, or a null

Are you talking about the string output printed if you run spirv-reflect.exe or when using the spirv-reflect.h C API?

dpwiz commented 2 months ago

In the test YAML files:

    built_in: 0, 1, 3, 4 # [Position, PointSize, ClipDistance, CullDistance]

Here's the part that generates it:

  //   SpvBuiltIn                          built_in;
  os << t1 << "built_in: ";
  if (iv.decoration_flags & SPV_REFLECT_DECORATION_BLOCK) {
    for (uint32_t i = 0; i < iv.member_count; i++) {
      os << iv.members[i].built_in;
      if (i < (iv.member_count - 1)) {
        os << ", ";
      }
    }
  } else {
    os << iv.built_in;
  }
  os << " # " << ToStringSpvBuiltIn(iv, false) << std::endl;
spencer-lunarg commented 2 months ago

So the 0, 1, 3, 4 represent the values of these in the SPIRV-Headers, the # [Position, PointSize, ClipDistance, CullDistance] is just there to make it easier for people to know what they are (without having to look them up)

I guess what would you think the correct output should be instead of

    built_in: 0, 1, 3, 4 # [Position, PointSize, ClipDistance, CullDistance]
dpwiz commented 2 months ago

The ids and labels are okay (but the current format isn't a list of ints, it's a string node).

I'm confused about member built_in values leaking into the parent structure. The members are IVs themselves and carry their own built_in field and value (and a label). So, a parser should add a provision to detect it and treat as null instead, because the numbers are not for the IV being parsed.

now:

built_in: 0, 1, 3, 4

better:

built_in: [0, 1, 3, 4]

best:

😄