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

Fix the validation of vertex/index/instance ranges in render bundles #5144

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

nical
Copy link
Contributor

@nical nical commented Jan 26, 2024

Connections

Spec: https://gpuweb.github.io/gpuweb/#dom-gpurendercommandsmixin-draw

Description

The current bundle validation has two problems:

  • division by zero if the vertex stride is zero (we hit this in CTS runs in Firefox)
  • It is stricter than the specification by requesting that the buffer has space for stride * count whereas the specified formula is stride * (count - 1) + last_stride where last_stride is the space required to fit the last element (potentially smaller than the stride.

This PR addresses these issues by implementing the spec pretty much verbatim, but for render bundles only. Regular encoders are unfortunately still not up to spec. One way would be to use the same validation but it will be more expensive since it has to be done for each draw call while the current validation gets away with doing most of the calculation only when switching pipelines. I think that I can adapt that to match the spec but it's going to be more complicated.

I'm eager to get at least the render bundle part fixed so that we get rid of an outstanding CTS crash.

Checklist

  • Run cargo fmt.
  • Run cargo clippy.
  • Run cargo xtask test to run tests.

@nical nical requested a review from a team as a code owner January 26, 2024 13:46
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Looks solid!

@teoxoy teoxoy merged commit e2e9ef5 into gfx-rs:trunk Jan 26, 2024
27 checks passed
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