Skip to content

Conversation

@bejado
Copy link
Member

@bejado bejado commented Aug 12, 2025

This updates LoadImageTest.UpdateImage2D to use SharedShaders.

  • Update SharedShaders to support different sampler types
  • Add a Builder to ScreenshotParams
  • Allow forcing an alpha value when comparing screenshots

@bejado bejado requested a review from mhoff12358 August 12, 2025 21:45
@bejado bejado added the internal Issue/PR does not affect clients label Aug 12, 2025
)";
}
case ShaderUniformType::Sampler: {
case ShaderUniformType::Sampler2D: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that WebGPU/WGSL does not support combined image samplers. We need to define and bind both the texture and the sampler for it and use both in the call to sample the color from the texture.

In pure WGSL it is:

...
@group(0) @binding(0) var mySampler : sampler;
@group(0) @binding(1) var myTexture : texture_2d<f32>;
...
let color = textureSample(myTexture, mySampler, uv);

textureLoad can be used to lookup data directly from a texture without a sampler.

We deal with this in our shader generation code using a SPIRV-OPT pass that splits combined image samplers (see (libs/filamat/src/GLSLPostProcessor.cpp ~line 574)[https://github.com/google/filament/blob/5daa0cfe4b9d74a489117d8df393d01540af1a0a/libs/filamat/src/GLSLPostProcessor.cpp#L574]: optimizer->RegisterPass(CreateSplitCombinedImageSamplerPass()); (thanks to a lot of help from David Neto). We also, do some tricks to remap the bindings for them, as can be seen from the rebindImageSamplerForWGSL(*spirv); call below the pass. We account for this when creating bind group layouts and bind groups on the driver/backend side (see filament/backend/src/webgpu/WebGPUDescriptorSetLayout.cpp ~lines 89-94):

// In WebGPU, textures and samplers are separate bindings.
/ We map the Filament binding index to two WebGPU binding indices:
// - texture: binding * 2
 // - sampler: binding * 2 + 1
wEntry.binding = fEntry.binding * 2;
entryInfo.binding = wEntry.binding;

I could be wrong, as I am not familiar with this side of the code, but I don't see where this is accounted for in the test shader generation. I think we need to modify the shader GLSL for the WebGPU case to handle this.

mAllowedPixelDeviations(numAllowedDeviations),
mPixelMatchThreshold(pixelMatchThreshold) {}
mPixelMatchThreshold(pixelMatchThreshold),
mForceAlphaValue(forceAlphaValue) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is forcing an alpha value necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Issue/PR does not affect clients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants