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

Rewrite Mesh Shader Output size calculation #2292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Jan 7, 2024

I found this section rather confusing to follow, so here's my attempt at making it clearer. I followed the style of the "Vertex Input Address Calculation" section, using separate bullet points for the variable definitions, and C code using longer variable names instead of the cryptic (IMO) mathematical ones.

I removed the use of the term "attribute", because that term is not as well defined compared to "location" and "component". The way that "attribute" was actually being used here, with the number of attributes defined as four times the number of locations, suggests that it's intended as a synonym for "component" here, which contradicts the rest of the Vulkan spec where "attribute" is synonymous with "location" (e.g. the "Vertex Attributes" section).

There are some potential further changes I think could be made. Instead of expressing memory size as number of components * 4, it might be more straightforward to express it simply as number of locations * 16. Then the second sentence in the section I modified here isn't really needed anymore, which further helps clarity. However, there is one point that probably needs clarifying: what if a single location is shared by both a per-vertex and per-primitive variable (using Component decorations)? Is it counted towards both types?

@Rua Rua marked this pull request as ready for review January 7, 2024 15:23
@pixeljetstream
Copy link

pixeljetstream commented Jan 10, 2024

reads much nicer indeed, thanks. Would keep it component based as you did so far.
I am not sure it would legal to overlap per-vertex and per-primitive outputs.

@oddhack
Copy link
Contributor

oddhack commented Jan 11, 2024

Just regarding the math markup, generally speaking we use short variable names in math expressions precisely because we think it's easier to read than very long, overly similar, and still arbitrary C pseudovariable names. Granted this is a stylistic choice and surely exceptions can be found, we aren't eager to see conversion to the other style.

@Rua
Copy link
Contributor Author

Rua commented Jan 20, 2024

I am not sure it would legal to overlap per-vertex and per-primitive outputs.

I haven't been able to find anything against it so far. Also, I realised that the same question applies to variables that depend on ViewIndex versus those that do not.

Just regarding the math markup, generally speaking we use short variable names in math expressions precisely because we think it's easier to read than very long, overly similar, and still arbitrary C pseudovariable names. Granted this is a stylistic choice and surely exceptions can be found, we aren't eager to see conversion to the other style.

I had the opposite experience, I found the one-letter variables much less self-explanatory and had to keep checking back with how they are defined while interpreting the formula. There is a reason that one-letter variables are avoided in programming after all?

@samuelhsw
Copy link

samuelhsw commented Jan 22, 2024

what if a single location is shared by both a per-vertex and per-primitive variable (using Component decorations)? Is it counted towards both types?

I think it is fine since the new change clarifies the number on component basis, and compilers should be able to alert if declaring per-vertex and per-primitive variables that use the same location while not different components within that location.

I had the opposite experience, I found the one-letter variables much less self-explanatory and had to keep checking back with how they are defined while interpreting the formula. There is a reason that one-letter variables are avoided in programming after all?

Either are good reasons to me. Perhaps we can bring up some other discussions about the stylistic choice in Vulkan WG if of enough interest, and just focus on clarifying calculation with less divergency in this PR?

@Rua
Copy link
Contributor Author

Rua commented Jan 22, 2024

compilers should be able to alert if declaring per-vertex and per-primitive variables that use the same location while not different components within that location.

I'm referring to the case where the variables share a location, but have different components and so do not overlap. In this case and under the current formulation of the spec, that location would be counted towards the total size twice: once in the per-vertex count, and once in the per-primitive count.

If dependence on ViewIndex is thrown into the mix, you have four possible combinations. Each component in the same location could belong to a different category, for example:

  • location 0 component 0: per vertex, doesn't depend on ViewIndex
  • location 0 component 1: per vertex, depends on ViewIndex
  • location 0 component 2: per primitive, doesn't depend on ViewIndex
  • location 0 component 3: per primitive, depends on ViewIndex

Counting location 0 towards all four categories would lead to it counting towards the total size four times. That doesn't seem correct, unless the implementation is expected to allocate entirely separate memory blocks for each category.

@samuelhsw
Copy link

I'm referring to the case where the variables share a location, but have different components and so do not overlap. In this case and under the current formulation of the spec, that location would be counted towards the total size twice: once in the per-vertex count, and once in the per-primitive count.

Oh I was trying to express it in the different way, but English failed me. Thanks for the elaboration.

Calculating the required size on component basis rather than location basis seems to get more accurate amount of output size to me after this modification no matter how a component were assigned to the corresponding category. 👍

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.

4 participants