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

Issues with textureGather on texture_2d<u32> #2135

Closed
JMS55 opened this issue Nov 26, 2022 · 4 comments · Fixed by #2138
Closed

Issues with textureGather on texture_2d<u32> #2135

JMS55 opened this issue Nov 26, 2022 · 4 comments · Fixed by #2138
Labels
area: front-end Input formats for conversion kind: bug Something isn't working lang: WGSL WebGPU shading language

Comments

@JMS55
Copy link
Contributor

JMS55 commented Nov 26, 2022

Edit: Can't reproduce on naga master. However, now a different validation error is produced in wgpu:

Caused by:
    In Device::create_compute_pipeline
      note: label = `ambient_occlusion_denoise_pipeline`
    error matching shader requirements against the pipeline
    unable to filter the texture (ResourceBinding { group: 0, binding: 1 }) by the sampler (ResourceBinding { group: 1, binding: 0 })
    integer textures can't be sampled

Caused by the following match ignoring the gather field in naga:

naga/src/valid/analyzer.rs

Lines 571 to 594 in df4e403

E::ImageSample {
image,
sampler,
gather: _,
coordinate,
array_index,
offset: _,
level,
depth_ref,
} => {
let image_storage = GlobalOrArgument::from_expression(expression_arena, image)?;
let sampler_storage = GlobalOrArgument::from_expression(expression_arena, sampler)?;
match (image_storage, sampler_storage) {
(GlobalOrArgument::Global(image), GlobalOrArgument::Global(sampler)) => {
self.sampling_set.insert(SamplingKey { image, sampler });
}
_ => {
self.sampling.insert(Sampling {
image: image_storage,
sampler: sampler_storage,
});
}
}


According to the spec, this should be a valid operation. However, I get the following error in my project:

var depth_differences: texture_2d<u32>;

error: Entry point denoise at Compute is invalid
    ┌─ wgsl:106:18
    │
106 │     let edges0 = textureGather(0, depth_differences, point_clamp_sampler, uv);
    │                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ naga::Expression [17]
    │
    = Expression [17] is invalid
    = Unable to operate on image class Sampled { kind: Uint, multi: false }

Taking a look at naga's source code, it looks like this is an InvalidImageClass Err. There's only 3 places it's created, of which the following seems most likely:

let image_depth = match class {
crate::ImageClass::Sampled {
kind: crate::ScalarKind::Float,
multi: false,
} => false,
crate::ImageClass::Depth { multi: false } => true,
_ => return Err(ExpressionError::InvalidImageClass(class)),
};

It looks like originally this match statement assumes a float texture only, which is a valid assumption when this was originally used for only regular samples, but when gathers were added, this assumption was broken.

@JMS55 JMS55 changed the title WGSL mistakenly rejects textureGather on texture_2d<u32> Issues with textureGather on texture_2d<u32> Nov 26, 2022
@teoxoy teoxoy added kind: bug Something isn't working lang: WGSL WebGPU shading language area: front-end Input formats for conversion labels Nov 26, 2022
@teoxoy teoxoy added this to the WGSL Specification V1 milestone Nov 26, 2022
@JMS55
Copy link
Contributor Author

JMS55 commented Dec 15, 2022

@teoxoy this commit 9ddf38e turned out to be necessary for my shader https://github.com/JMS55/bevy/blob/gtao/crates/bevy_pbr/src/ssao/denoise_ssao.wgsl#L25. The tests I wrote just didn't catch it :/.

@teoxoy
Copy link
Member

teoxoy commented Dec 15, 2022

What is the error exactly?

@JMS55
Copy link
Contributor Author

JMS55 commented Dec 15, 2022

Ah sorry, the error is:

Caused by:
    In Device::create_compute_pipeline
      note: label = `ssao_denoise_pipeline`
    error matching shader requirements against the pipeline
    unable to filter the texture (ResourceBinding { group: 0, binding: 1 }) by the sampler (ResourceBinding { group: 1, binding: 0 })
    integer textures can't be sampled

@teoxoy
Copy link
Member

teoxoy commented Dec 16, 2022

This looks like it's caused by this validation error over in wgpu: https://github.com/gfx-rs/wgpu/blob/f14bee6740b8755afa362c3be6ef4b225209871a/wgpu-core/src/validation.rs#L1071

I think we should allow those through if the SamplerBindingType is NonFiltering.

Please open an issue/PR on the wgpu repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: front-end Input formats for conversion kind: bug Something isn't working lang: WGSL WebGPU shading language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants