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

Validate shader location clashes #3613

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

emilk
Copy link
Contributor

@emilk emilk commented Mar 22, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description
Validate that no two attributes are assigned to the same shader location.

"Each attrib in the union of all GPUVertexAttribute across descriptor.buffers has a distinct attrib.shaderLocation value." (https://www.w3.org/TR/webgpu/#abstract-opdef-validating-gpuvertexstate)

(Thanks to @Wumpf for finding that quote, and for guiding me to making this PR)

Testing
cargo nextest run --no-fail-fast works the same as on master, which means one failure:

        FAIL [   0.064s]                  wgpu::wgpu-tests zero_init_texture_after_discard::discarding_depth_target_resets_texture_init_state_check_visible_on_copy_in_same_encoder

--- STDOUT:                               wgpu::wgpu-tests zero_init_texture_after_discard::discarding_depth_target_resets_texture_init_state_check_visible_on_copy_in_same_encoder ---

running 1 test

--- STDERR:                               wgpu::wgpu-tests zero_init_texture_after_discard::discarding_depth_target_resets_texture_init_state_check_visible_on_copy_in_same_encoder ---
-[AGXG13XFamilyCommandBuffer renderCommandEncoderWithDescriptor:]:380: failed assertion `A command encoder is already encoding to this command buffer'

@emilk emilk marked this pull request as ready for review March 22, 2023 13:31
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.

LGTM, thanks for the PR!

@teoxoy teoxoy added this to the WebGPU Specification V1 milestone Mar 22, 2023
@teoxoy
Copy link
Member

teoxoy commented Mar 22, 2023

Could you open an issue about the unrelated test failure and include some info about your hardware/setup?

@teoxoy teoxoy merged commit a89e35a into gfx-rs:master Mar 22, 2023
@@ -352,6 +352,8 @@ pub enum CreateRenderPipelineError {
location: wgt::ShaderLocation,
offset: wgt::BufferAddress,
},
#[error("Two or more attributes were assigned to the same shader lcoation {0}")]
Copy link
Member

@cwfitzgerald cwfitzgerald Mar 22, 2023

Choose a reason for hiding this comment

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

Unfortunately, missed a typo here (also added a few words while we're at it):

Suggested change
#[error("Two or more attributes were assigned to the same shader lcoation {0}")]
#[error("Two or more vertex attributes were assigned to the same location in the shader: {0}")]

@emilk
Copy link
Contributor Author

emilk commented Mar 22, 2023

I'll make a PR with the typo fixes

@emilk
Copy link
Contributor Author

emilk commented Mar 22, 2023

Could you open an issue about the unrelated test failure and include some info about your hardware/setup?

#3614

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