Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Parse 'centroid' and 'noperspective' modifiers.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:interpolation_modifiers into master
2 unresolved threads

Signed-off-by: Nikolay Sivov nsivov@codeweavers.com

Merge request reports

Checking pipeline status.

Approved by

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Oct 9, 2023 9:10pm UTC)

Merge details

  • Changes merged into master with 3af629cf.
  • Deleted the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 90efe7a4
  • 391 393 semantic = &field->semantic;
    392 394 elem_semantic_index = semantic->index;
    393 395 loc = &field->loc;
    396 element_modifiers |= field->storage_modifiers;
    • This logic needs tests, I think. It should be easy to add tests for nointerpolation.

    • Author Developer

      Could you suggest a good reference about this feature? I can easily check compiler output, but looking at nointerpolation test, it requires more than that.

    • Author Developer

      I repurposed existing tests to show that field modifiers are actually used. Please take a look.

    • The test is structured how I was imagining, but what I'm ideally looking for is a test that shows this really should be |= (and not something more complicated). I.e. if you specify nointerpolation on the struct itself, is that propagated onto all struct fields? What if there's conflicting modifiers?

    • Author Developer

      Yes, the rules are more complicated apparently.

    • Author Developer

      Or maybe they are not. The order of precedence is 'nointerpolation' -> 'sample' -> 'centroid' or 'noperspective'. Conflicts are tested only at the same level - on fields or variables.

    • Author Developer

      Added some more tests.

    • Please register or sign in to reply
  • Interpolation doesn't matter for outputs in sm4, but it does in sm6 and GLSL, so can you please add an equivalent of 3/4 for outputs?

  • Giovanni Mascellani
    Giovanni Mascellani @giomasce started a thread on commit ed026e5b
  • 4184 {
    4185 { HLSL_MODIFIER_CENTROID | HLSL_MODIFIER_NOPERSPECTIVE, VKD3DSIM_LINEAR_NOPERSPECTIVE_CENTROID },
    4186 { HLSL_MODIFIER_NOPERSPECTIVE, VKD3DSIM_LINEAR_NOPERSPECTIVE },
    4187 { HLSL_MODIFIER_CENTROID, VKD3DSIM_LINEAR_CENTROID },
    4188 };
    4189 unsigned int i;
    4190
    4191 for (i = 0; i < ARRAY_SIZE(modes); ++i)
    4192 {
    4193 if ((var->storage_modifiers & modes[i].modifiers) == modes[i].modifiers)
    4194 {
    4195 mode = modes[i].mode;
    4196 break;
    4197 }
    4198 }
    4199 }
  • Could you suggest a good reference about this feature? I can easily check compiler output, but looking at nointerpolation test, it requires more than that.

    The GLSL spec at https://registry.khronos.org/OpenGL/specs/gl/GLSLangSpec.4.60.html#interpolation-qualifiers is fairly decent.

  • Nikolay Sivov added 10 commits

    added 10 commits

    • ed026e5b...90d4529f - 5 commits from branch wine:master
    • f6490a93 - vkd3d-shader/hlsl: Parse 'centroid' and 'noperspective' modifiers.
    • f6cb51f5 - vkd3d-shader/hlsl: Allow interpolation modifiers on structure fields.
    • 0e53d28f - vkd3d-shader/hlsl: Propagate structure fields modifiers when copying shader inputs.
    • bea5af93 - vkd3d-shader/tpf: Output interpolation modifiers for input declarations.
    • b7791521 - tests: Add a test for interpolation modifiers specified on structure fields.

    Compare with previous version

    • Resolved by Nikolay Sivov

      Please, when you rebase and change your commits at the same time, do not force-push everything together. First do a force-push with the rebase, then another force-push for the genuine changes to the MR. It makes reviewing easier with the GitLab tools (and also with plain git commands).

      Also, don't introduce the tests at the end of the MR. Rather, introduce tests at the beginning of the MR, mark them as todo and remove the todo marking when the tests start passing. This also makes the review easier. Please fix this second thing in this MR.

  • Nikolay Sivov added 34 commits

    added 34 commits

    • b7791521...ca05e57e - 29 commits from branch wine:master
    • 7023c884 - tests: Add a test for interpolation modifiers specified on structure fields.
    • ac8b6c11 - vkd3d-shader/hlsl: Parse 'centroid' and 'noperspective' modifiers.
    • 5b0f88e8 - vkd3d-shader/hlsl: Allow interpolation modifiers on structure fields.
    • 2592a0d2 - vkd3d-shader/hlsl: Propagate structure fields modifiers when copying shader inputs.
    • 60f04fe8 - vkd3d-shader/tpf: Output interpolation modifiers for input declarations.

    Compare with previous version

  • I can't say there are too many tests: 3/5 and 5/5 could also have some tests. But I'll say that the MR is good enough.

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Nikolay Sivov added 92 commits

    added 92 commits

    • 60f04fe8...014960b6 - 87 commits from branch wine:master
    • 8c4068e3 - tests: Add a test for interpolation modifiers specified on structure fields.
    • 7cbe0938 - vkd3d-shader/hlsl: Parse 'centroid' and 'noperspective' modifiers.
    • 56d0d073 - vkd3d-shader/hlsl: Allow interpolation modifiers on structure fields.
    • de2d64e3 - vkd3d-shader/hlsl: Propagate structure fields modifiers when copying shader inputs.
    • 3e680001 - vkd3d-shader/tpf: Output interpolation modifiers for input declarations.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading