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

YAML built_in for structs is unclear #269

Open
dpwiz opened this issue Jun 30, 2024 · 4 comments
Open

YAML built_in for structs is unclear #269

dpwiz opened this issue Jun 30, 2024 · 4 comments

Comments

@dpwiz
Copy link

dpwiz commented Jun 30, 2024

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

@spencer-lunarg
Copy link
Contributor

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
Copy link
Author

dpwiz commented Jun 30, 2024

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
Copy link
Contributor

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
Copy link
Author

dpwiz commented Jul 1, 2024

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:

😄

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

No branches or pull requests

2 participants