Skip to content
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

Test maxBindGroupsPlusVertexBuffers limits #3368

Merged
merged 6 commits into from
Feb 7, 2024

Conversation

greggman
Copy link
Contributor

@greggman greggman commented Feb 6, 2024

Here's an updated test for maxBindGroupPlusVertexBuffers. I can't easily test with explicit pipeline layouts because it's currently impossible to create sparse explicit pipeline layouts but the code is there for when it becomes possible which seems like a future plan.


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.)

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.

@greggman
Copy link
Contributor Author

greggman commented Feb 6, 2024

Note: All of the tests are skipped on Chrome since Chrome doesn't currently set maxBindGroupsPlusVertexBuffers high enough to be able to test anything.

On Safari Technology Preview the tests that are within the limits pass, those that are past the limit fail as not generating an error. For the draw tests, they current fail because Safari disallows calling setVertexBuffer(ndx, null). If you comment out the sections that do that then, as above, they pass when things are within the limit and fail with "no error when one was expected" when over the limit.

@mwyrzykowski
Copy link

On Safari Technology Preview the tests that are within the limits pass, those that are past the limit fail as not generating an error. For the draw tests, they current fail because Safari disallows calling setVertexBuffer(ndx, null). If you comment out the sections that do that then, as above, they pass when things are within the limit and fail with "no error when one was expected" when over the limit.

I'll try this locally since Safari Technology Preview doesn't include validation for these tests yet, I just noticed it while adding it locally. I have a PR open to add the validation, it hasn't merged yet.

@greggman greggman force-pushed the test-maxBindGroupsPlusVertexBuffers branch from 56e2325 to 48d1d69 Compare February 6, 2024 22:46
@greggman
Copy link
Contributor Author

greggman commented Feb 6, 2024

I updated to use empty bindGroupLayouts as suggested by @kainino0x so the explicit pipeline layouts path is now enabled.

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

LGTM

@greggman greggman force-pushed the test-maxBindGroupsPlusVertexBuffers branch from 0bce1e1 to d787abf Compare February 7, 2024 18:34
@greggman greggman enabled auto-merge (squash) February 7, 2024 18:35
@greggman greggman merged commit 75d8f8a into gpuweb:main Feb 7, 2024
1 check passed
@greggman greggman deleted the test-maxBindGroupsPlusVertexBuffers branch February 7, 2024 18:39
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.

3 participants