-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixed texture array multisampling validation #8566
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
Fixed texture array multisampling validation #8566
Conversation
andyleiserson
left a comment
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.
Might be worth a changelog mention.
wgpu-core/src/resource.rs
Outdated
| #[error("Sample count {0} is not supported by format {1:?} on this device. The WebGPU spec guarantees {2:?} samples are supported by this format. With the TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES feature your device supports {3:?}.")] | ||
| InvalidSampleCount(u32, wgt::TextureFormat, Vec<u32>, Vec<u32>), | ||
| #[error("1D/3D texture multisample count must be 1, got {0}")] | ||
| Non2DMultisampledTexture(u32), |
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.
Maybe InvalidMultisampledDimension to align with existing InvalidMultisampled* and Invalid*Dimension.
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
|
It seems like the CTS ( Running that test locally, I also got an assertion failure in Metal. It looks like the wgpu-hal metal backend does not map the texture type properly for the array multisampled case. |
This actually is not allowed by the code, because there are redundant checks related to multisampling. Depth-1 3D multisampled textures are disallowed in (I do think that redundant checks are bad because they result in exactly this kind of confusion, I filed #8568 about that.) |
|
@andyleiserson Thanks for the feedback. This might be more controversial than I assumed in that case, so maybe push it off until the next meeting or discuss further in the issue linked. |
Connections
Closes #8565
Description
Here the spec discusses validation of multisample state for textures. It says the following:
Clearly, this disallows multisampling for 1D and 3D textures but allows it for 2D textures and 2D texture arrays. The existing validation only checked that an image with multisampling >1 had an array length or depth of 1. This incorrectly allowed 1D textures and 3D textures with depth 1 to have multisampling, and incorrectly blocked 2D texture arrays.
Testing
Nothing new
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.