Skip to content

Comments

DepthStencilState validation fixes#8840

Open
andyleiserson wants to merge 2 commits intogfx-rs:trunkfrom
andyleiserson:jj-branches/mxx
Open

DepthStencilState validation fixes#8840
andyleiserson wants to merge 2 commits intogfx-rs:trunkfrom
andyleiserson:jj-branches/mxx

Conversation

@andyleiserson
Copy link
Contributor

@andyleiserson andyleiserson commented Jan 8, 2026

The depth-related fields are optional in the WebGPU API, and may be omitted when they are not relevant to the configuration. In order to have wgpu-core provide a common implementation of this validation for all the JS clients, this PR makes them optional in wgpu.

Testing
Enables some relevant CTS tests.

Squash or Rebase? Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@andyleiserson
Copy link
Contributor Author

(Responding to discussion of a possible DepthState struct in matrix): The rules are that depthWriteEnabled must be provided if using a depth format; depthCompare must be provided if depthWriteEnabled is true or if stencil has an active depthFailOp. So while depthWriteEnabled could be mandatory in DepthState, depthCompare would still need to be optional. I think that makes it harder to justify a breaking change to introduce DepthState.

A data point: https://github.com/search?q=language%3ARust+RenderPipelineDescriptor+DepthStencilState&type=code finds 6.7k files 😬. Even the need to add .into() doesn't seem great, but I couldn't figure out an alternative that didn't involve duplicating or adding a type parameter to all the pipeline descriptors.

@andyleiserson
Copy link
Contributor Author

andyleiserson commented Jan 27, 2026

@cwfitzgerald you had proposed a different change that would create a separate DepthState struct. Does the issue of depthCompare needing to be optional even within DepthState (see #8840 (comment)) change your thinking at all?

(And I guess separately from that, there's also a question of whether to offer a migration via DepthStencilStateIdl like it is now, or to just inflict the full pain and change DepthStencilState to look the way we want.)

@cwfitzgerald
Copy link
Member

I'm definitely still on the team of "lets eat the breaking change now" - maybe we can talk about this at the meeting briefly, but I'm not sure what the ideal solution here is, I've offloaded all the context. I think this should probably just go ahead, not blocked on me.

These fields are optional in the WebGPU API. Having them optional in the
wgpu API as well keeps validation logic centralized in wgpu-core.

Fixes gfx-rs#8830
@andyleiserson
Copy link
Contributor Author

I removed DepthStencilStateIdl, i.e., updated the PR to take the breaking change all at once without extra compat/migration stuff.

I removed the DepthStencilState::new and DepthStencilState::depth constructors because they aren't simpler than the equivalent struct literals (the entire stencil state can be stubbed out with StencilState::default()). But I kept DepthStencilState::stencil because it seems marginally useful to be able to set up the stencil state without explicit values for the depth fields.

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