-
Notifications
You must be signed in to change notification settings - Fork 86
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
default entry points to shader modules #3190
Conversation
91b8283
to
d929a05
Compare
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 but a bit more test coverage is needed
.combine('shaderModuleStage', ['compute', 'vertex', 'fragment'] as TShaderStage[]) | ||
.beginSubcases() | ||
.combine('provideEntryPoint', [true, false]) | ||
.combine('extraEntryPoint', [true, false]) |
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.
We also need to test different stages for the extraEntryPoint. For example a shader module with one vertex and one fragment stage should still be usable with default entry points.
This might be best as a separate test though, to avoid overparameterizing these tests for little extra coverage.
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.
Good catch!
d929a05
to
259242e
Compare
259242e
to
393dfa7
Compare
FYI Chromium CL has landed in Chrome Canary. See https://chromiumdash.appspot.com/commit/61e85e41a5335013b0b33334efd5646baa9d9b0d |
This PR adds tests for gpuweb/gpuweb#4387
Requirements for PR author:
.unimplemented()
./** documented */
and new helper files are found inhelper_index.txt
.They're passing locally with my Chromium build patched with https://chromium-review.googlesource.com/c/chromium/src/+/5057294.
Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.