-
Notifications
You must be signed in to change notification settings - Fork 100
Test GPUBuffer in GPUBindingResource #4390
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
023a862 to
54eb9da
Compare
| .fn(t => { | ||
| const { offset, size, bufferSize, bindBufferResource } = t.params; | ||
|
|
||
| const bufferData = new Uint8Array(bufferSize); |
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.
| const bufferData = new Uint8Array(bufferSize); | |
| const bufferData = new Uint32Array(bufferSize / 4); |
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.
optional nit
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.
This forces me to add / 4 in other places so if that's okay, I'll leave it as is.
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.
Sounds good.
| ) | ||
| .paramsSubcasesOnly(u => | ||
| u // | ||
| .combine('bindBufferResource', [false, true] as const) |
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 could avoid generating the subcases with offset / size when bindBufferResource is true. Also bufferSize could be extraBufferSize: [0, 8] and the computation done in the test.
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.
Done
This PR reflects changes from the following spec PRs: * gpuweb/gpuweb#5193 * gpuweb/gpuweb#5160 Please publish new version after it's merged so that we can add tests in the CTS as in gpuweb/cts#4390
|
just a nit but I'd never guess to look for binding buffer tests in |
|
Ah yeah that's super odd. |
|
Where would you move them? Something like a new file called |
|
Sure that would work. |
|
@Kangz Done |
|
@greggman I'd like to roll gpuweb/types#182 into the CTS but I'd like to avoid defining texture formats for texture-formats-tier1. Do you know if there's a way to override GPUTextureFormats type to say "exclude r16unorm, etc" for now? |
That doesn't appear to be possible and it's not entirely clear we want to make it possible. There are compile time checks that all the GPUTextureFormat formats have corresponding data. We'd have to removing those checks to make it possible. I'll try to put up a PR that rolls the types and adds what's needed to keep running. |
|
pr to roll types #4400 |
…` r=#webgpu-reviewers! This change to upstream IDL happened at [gpuweb#5193](gpuweb/gpuweb#5193). Test coverage was added in [gpuweb/cts#4390](gpuweb/cts#4390). Differential Revision: https://phabricator.services.mozilla.com/D257634
…` r=webgpu-reviewers,webidl,teoxoy,emilio This change to upstream IDL happened at [gpuweb#5193](gpuweb/gpuweb#5193). Test coverage was added in [gpuweb/cts#4390](gpuweb/cts#4390). Differential Revision: https://phabricator.services.mozilla.com/D257634
…` r=webgpu-reviewers,webidl,teoxoy,emilio This change to upstream IDL happened at [gpuweb#5193](gpuweb/gpuweb#5193). Test coverage was added in [gpuweb/cts#4390](gpuweb/cts#4390). Differential Revision: https://phabricator.services.mozilla.com/D257634
Spec PR: gpuweb/gpuweb#5193
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.