-
Notifications
You must be signed in to change notification settings - Fork 96
Add tests for the primitive_index builtin #4435
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
Conversation
There was a problem hiding this 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)
Do we need to check behavior for these cases? If your 2d vertex data is
I believe all of these are supposed to be counted. |
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.) |
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. |
There was a problem hiding this 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
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.
Added your awesome ASCII art to the PR when rebasing. Thanks! |
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:
.unimplemented()
./** documented */
and new helper files are found inhelper_index.txt
.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.