Skip to content

Conversation

toji
Copy link
Member

@toji toji commented Jul 30, 2025

Adds some initial tests for the primitive_id WGSL builtin. Currently uses enable chromium_experimental_primitive_id to enable, but that can change once the spec is fleshed out and accepted by the group. Feature is not exposed to compat mode.

Associated spec change is gpuweb/gpuweb#5273 I'm totally fine if we want to wait to land this till that is approved by the group and this can revert to using the final feature strings.

Issue: gpuweb/gpuweb#1786


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)
  • Test have be tested with compatibility mode validation enabled and behave as expected. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@toji toji requested a review from greggman July 30, 2025 22:21
Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me as a start but maybe some todos?

  • It's only testing that (num primitives rendered - 1) is written to the texture. It is not checking that primitive_id is ever anything other than (num primitives rendered - 1). In other words, if triCount = 5 and primitive_id was always 4 it would pass the test. Where as it's supposed to return 0 for primitive 0, 1 for primitive 1, etc....

  • It's not checking other topologies (point-list, line-list, line-strip, triangle-list)

@greggman
Copy link
Contributor

greggman commented Aug 4, 2025

Do we need to check behavior for these cases?

If your 2d vertex data is

0.1, 0.1    0.2, 0.1    0.1, 0.2   // normal triangle
0.3, 0.3    0.3, 0.3    0.3, 0.3   // zero size triangle
2.1, 2.1    2.2, 2.1    2.1, 2.2   // normal triangle but off screen
0.00001, 0.00001    0.00002, 0.00001   0.00001, 0.00002   // triangle whose area covers no pixels.
0.1, 0.1    0.1, 0.2   0.2, 0.1     // backface culling on, a culled triangle.

I believe all of these are supposed to be counted.

@toji
Copy link
Member Author

toji commented Aug 4, 2025

That's an excellent point! I had meant to add cases for culled or degenerate primitives but it slipped my mind when submitting. I'll add some.

(Been working on a couple of other related tasks in other areas, like the spec, which is why I haven't updated this yet. Will loop back around to it soon.)

@toji toji changed the title Add tests for the primitive_id builtin Add tests for the primitive_index builtin Aug 25, 2025
@toji
Copy link
Member Author

toji commented Aug 25, 2025

Please take another look. Coves more cases now, including all topology types, testing multiple primitive indices in a single draw, and degenerate/offscreen/reset primitives.

Copy link
Contributor

@greggman greggman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - dang I wish there was some diagrams. I had to manually graph everything 😅 - we should write a vscode cts extension where you can add like // CTS: vec2f and it reads the array below the comment and draws an SVG graph 😛

      //           0,2
      //            +
      //           /|
      //          /.|
      //         +--+--+
      //        /|..|  |
      //       /.|..|  |
      // -2,0 +--+--O--+--+ 2,0
      //         |  |  |
      //         |  |  |
      //         +--+--+
      //            |
      //            |
      //            +
      //           0,-2
       0, 0,
      -2, 0,
       0, 2,

      //           0,2
      //            +
      //            |\
      //            |.\
      //         +--+--+
      //         |  |..|\
      //         |  |..|.\
      // -2,0 +--+--O--+--+ 2,0
      //         |  |  |
      //         |  |  |
      //         +--+--+
      //            |
      //            |
      //            +
      //           0,-2
       0, 0,
       0, 2,
       2, 0,
       
      //           0,2
      //            +
      //            |
      //            |
      //         +--+--+
      //         |  |  |
      //         |  |  |
      // -2,0 +--+--O--+--+ 2,0
      //       \.|..|  |
      //        \|..|  |
      //         +--+--+
      //          \.|
      //           \|
      //            +
      //           0,-2
       0, 0,
      -2, 0,
       0,-2,
       
      //           0,2
      //            +
      //            |
      //            |
      //         +--+--+
      //         |  |  |
      //         |  |  |
      // -2,0 +--+--O--+--+ 2,0
      //         |  |..|./
      //         |  |..|/
      //         +--+--+
      //            |./
      //            |/
      //            +
      //           0,-2
       0, 0,
       0,-2,
       2, 0,


      //         v2
      //          +
      //          |
      //          |
      //       +--+--+
      //       |  |  |
      //       |v1|v4|
      // v0 +--+--.--+--+ v3
      // v7    |  |  |
      //       |  |  |
      //       +--+--+
      //          |
      //          |
      //          +
      //          v5,v6
      //
      //  #0  #1  #2  #3  #4  #5
      //   +  +   +   +-+ +   +-+
      //  /|  |\  |\  |/  |    \|
      // +-+  +-+ +-+ +   +     +
-2, 0, 0, 0, 0, 2, 2, 0, 0, 0, 0, -2, 0, 0, -2, 0

      //         v1,v5   v2,v6
      //       +---*---+---*---+
      //       |       |       |
      //       |       |       |
      //       |       |       |
      //       +-------o-------+
      //       |       |       |
      //       |       |       |
      //       |       |       |
      //       +---*---+---*---+
      //         v0,v4   v3,v7
-0.5, -1, -0.5, 1, 0.5, 1, 0.5, -1, -0.5, -1, -0.5, 1, 0.5, 1, 0.5, -1

      //         v1,v5   v2,v6
      //       +-------+-------+
      //       |       |       |
      //       |   *   |   *   |
      //       |       |       |
      //       +-------o-------+
      //       |       |       |
      //       |   *   |   *   |
      //       |       |       |
      //       +-------+-------+
      //         v0,v4   v3,v7

toji and others added 4 commits August 26, 2025 10:30
Adds some initial tests for the primitive_id WGSL builtin. Currently
uses `enable chromium_experimental_primitive_id` to enable, but that
can change once the spec is fleshed out and accepted by the group.
@toji
Copy link
Member Author

toji commented Aug 26, 2025

Added your awesome ASCII art to the PR when rebasing. Thanks!

@toji toji enabled auto-merge (squash) August 26, 2025 17:54
@toji toji merged commit 0558b19 into main Aug 26, 2025
1 check passed
@toji toji deleted the primitive_id branch August 26, 2025 18:27
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.

2 participants