floooh / sokol-tools

Command line tools for use with sokol headers
MIT License
229 stars 57 forks source link

`image_sampler_pairs` is no longer continuous after the latest update #109

Closed terrybrash closed 12 months ago

terrybrash commented 1 year ago

It seems that Sokol SHDC is generating a sparse array of image_sampler_pairs on the latest version, from this shader:

uniform texture2D u_atlas_texture;
uniform texture2D u_atlas_outline;
uniform sampler u_atlas_sampler;

vec4 texture_out() {
  vec2 atlas_size = vec2(textureSize(sampler2D(u_atlas_texture, u_atlas_sampler), 0));
  vec2 v_texture_coord = (v_clip.xy + v_frag_coord * v_clip.zw) / atlas_size;
  vec4 fill = texture(sampler2D(u_atlas_texture, u_atlas_sampler), v_texture_coord);
  vec4 outline = texture(sampler2D(u_atlas_outline, u_atlas_sampler), v_texture_coord) * v_outline;
  return (fill * v_color) + outline * (1.0 - fill.a);
}

Previous Sokol SHDC Rust output:

// ... snip ...
desc.fs.images[0].used = true;
desc.fs.images[0].multisampled = false;
desc.fs.images[0].image_type = sg::ImageType::Dim2;
desc.fs.images[0].sample_type = sg::ImageSampleType::Float;
desc.fs.images[1].used = true;
desc.fs.images[1].multisampled = false;
desc.fs.images[1].image_type = sg::ImageType::Dim2;
desc.fs.images[1].sample_type = sg::ImageSampleType::Float;
desc.fs.samplers[0].used = true;
desc.fs.samplers[0].sampler_type = sg::SamplerType::Sample;
desc.fs.image_sampler_pairs[0].used = true;
desc.fs.image_sampler_pairs[0].image_slot = 0;
desc.fs.image_sampler_pairs[0].sampler_slot = 0;
desc.fs.image_sampler_pairs[1].used = true;
desc.fs.image_sampler_pairs[1].image_slot = 0;
desc.fs.image_sampler_pairs[1].sampler_slot = 0;
desc.fs.image_sampler_pairs[2].used = true;
desc.fs.image_sampler_pairs[2].image_slot = 1;
desc.fs.image_sampler_pairs[2].sampler_slot = 0;
// ... snip ...

Current Sokol SHDC Rust output:

// ... snip ...
desc.fs.images[0].used = true;
desc.fs.images[0].multisampled = false;
desc.fs.images[0].image_type = sg::ImageType::Dim2;
desc.fs.images[0].sample_type = sg::ImageSampleType::Float;
desc.fs.images[1].used = true;
desc.fs.images[1].multisampled = false;
desc.fs.images[1].image_type = sg::ImageType::Dim2;
desc.fs.images[1].sample_type = sg::ImageSampleType::Float;
desc.fs.samplers[0].used = true;
desc.fs.samplers[0].sampler_type = sg::SamplerType::Filtering;
desc.fs.image_sampler_pairs[0].used = true;
desc.fs.image_sampler_pairs[0].image_slot = 0;
desc.fs.image_sampler_pairs[0].sampler_slot = 0;
desc.fs.image_sampler_pairs[2].used = true;
desc.fs.image_sampler_pairs[2].image_slot = 1;
desc.fs.image_sampler_pairs[2].sampler_slot = 0;
// ... snip ...

Which fails validation on the most recent version of Sokol:

[sg][error][id:156] C:\Users\Terry\.cargo\git\checkouts\sokol-rust-d457e10df611d812\920164d\src\sokol\c\sokol_gfx.h(15713):
        VALIDATE_SHADERDESC_NO_CONT_IMAGE_SAMPLER_PAIRS: shader stage image-sampler-pairs must occupy continuous slots (sg_shader_desc.vs|fs.image_samplers[])

[sg][error][id:154] C:\Users\Terry\.cargo\git\checkouts\sokol-rust-d457e10df611d812\920164d\src\sokol\c\sokol_gfx.h(15753):
        VALIDATE_SHADERDESC_IMAGE_NOT_REFERENCED_BY_IMAGE_SAMPLER_PAIRS: shader stage: one 
or more images are note referenced by  (sg_shader_desc.vs|fs.image_sampler_pairs[].image_slot)

[sg][panic][id:258] C:\Users\Terry\.cargo\git\checkouts\sokol-rust-d457e10df611d812\920164d\src\sokol\c\sokol_gfx.h(15441):
        VALIDATION_FAILED: validation layer checks failed

ABORTING because of [panic]
floooh commented 1 year ago

Hmm this looks indeed wrong (the first output also isn't great because it has a duplicate image/sampler pair). Looks like I'm currently missing a test shader which has two identical samplers.

Thanks for the code snippet, I'll try to investigate tomorrow.

floooh commented 1 year ago

Ah ok, the culprit is definitely this new code which ignores image/sampler pairs with dummy samplers (SPIRVCross seems to generate such a dummy sampler for the textureSize() call).

https://github.com/floooh/sokol-tools/blob/9cdfb422bccd207b94cfde80c98d491e69c9814c/src/shdc/spirvcross.cc#L446-L449

Don't know yet though why this wasn't a problem before.

terrybrash commented 12 months ago

Looks like I'm currently missing a test shader which has two identical samplers.

Is there any way to avoid creating two identical samplers? I tried to assign them to a local variable in the shader but that didn't work because sampler2D is limited in where it can be called (which I don't really understand why 😅).

floooh commented 12 months ago

I think I found a solution that's more robust then before and also avoids the duplicate image/sampler pair which was present before. E.g. in the test shader it now looks like this:

      desc.fs.images[0].used = true;
      desc.fs.images[0].multisampled = false;
      desc.fs.images[0].image_type = SG_IMAGETYPE_2D;
      desc.fs.images[0].sample_type = SG_IMAGESAMPLETYPE_FLOAT;
      desc.fs.images[1].used = true;
      desc.fs.images[1].multisampled = false;
      desc.fs.images[1].image_type = SG_IMAGETYPE_2D;
      desc.fs.images[1].sample_type = SG_IMAGESAMPLETYPE_FLOAT;
      desc.fs.samplers[0].used = true;
      desc.fs.samplers[0].sampler_type = SG_SAMPLERTYPE_FILTERING;
      desc.fs.image_sampler_pairs[0].used = true;
      desc.fs.image_sampler_pairs[0].image_slot = 0;
      desc.fs.image_sampler_pairs[0].sampler_slot = 0;
      desc.fs.image_sampler_pairs[1].used = true;
      desc.fs.image_sampler_pairs[1].image_slot = 1;
      desc.fs.image_sampler_pairs[1].sampler_slot = 0;

I'll need to do a bit more testing though...

floooh commented 12 months ago

didn't work because sampler2D is limited in where it can be called (which I don't really understand why 😅)

Yeah, that's some weird GLSL limitation. Apparently it's not possible to store a GLSL combined image-sampler in a variable. The actual problem was the textureSize() call, which required to let SPIRVCross create dummy samplers for HLSL, MSL and WGSL output. I've solved that problem differently now by using a separate SPIRVCross GLSL output for parsing the reflection data.

floooh commented 12 months ago

Pull request: https://github.com/floooh/sokol-tools/pull/110

floooh commented 12 months ago

Ok fix is merged. The new executables will be available when this GH Action has finished: https://github.com/floooh/sokol-tools/actions/runs/6684866701

Please let me know if this works for you!

terrybrash commented 12 months ago

Works perfectly! Thanks again @floooh! 😁