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

Regression: image_sampler_pairs not created correctly in shader_desc #154

Closed
jeffkdev opened this issue Nov 8, 2024 · 13 comments
Closed
Assignees

Comments

@jeffkdev
Copy link

jeffkdev commented Nov 8, 2024

After transitioning my code to the 07-Nov-2024 update a couple of my shaders were running into this error on startup in sg_make_shader:

VALIDATE_SHADERDESC_IMAGE_NOT_REFERENCED_BY_IMAGE_SAMPLER_PAIRS: one or more images are note referenced by  (sg_shader_desc.vs|fs.image_sampler_pairs[].image_slot)
VALIDATE_SHADERDESC_SAMPLER_NOT_REFERENCED_BY_IMAGE_SAMPLER_PAIRS: one or more samplers are not referenced by image-sampler-pairs (sg_shader_desc.vs|fs.image_sampler_pairs[].sampler_slot)

I was able to trace this back to the shader_desc created by shdc missing a image_sampler_pairs entry. Both SG_BACKEND_GLCORE and SG_BACKEND_GLES3 backends had the same issue. Adding these lines below manually fixed it for me, so it isn't actively causing any issues on my end now. I can privately send you the shader files to reproduce it if you don't run into this issue upgrading any of your own projects. My only guess is it has something to do with using @include_block with image/samplers defined in it.

image

@floooh floooh self-assigned this Nov 9, 2024
@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Reminder: shader in DM.

@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Ok, I found the problem: the shader reflection code detects all three separate image samplers, but doesn't assign unique bindslots across shader stages. Both the waterColorTexture_waterSampler and the shadowMap_shadowMapSampler image-sampler-pair get bindslot 0 assigned, and the waterColor image sampler basically 'shadows' the shadowMap image-sampler during code generation.

Next I'll try to find out why exactly this regressed (it must be the different bindslot assignment code, all other resource types now have explicit bindslots assigned via the layout(binding=N) annotations, but the image-sampler-pairs still require an automatic bindslot assignment, and there must be a bug in that code.

Next I'll work on the fix :)

@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Ah bingo, it worked before because the image-sampler bindings were local to each shader stage before...

@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Testing the fix now on the sokol-samples... I'll actually also need to write a new sample which uses textures both on the vertex- and fragment-stage, this would have triggered the issue. But if the sokol-samples all work with the fix I'll update new binaries before that so that you can test too.

The generated bindings code looks like this now:

            desc.images[0].stage = SG_SHADERSTAGE_FRAGMENT;
            desc.images[0].image_type = SG_IMAGETYPE_2D;
            desc.images[0].sample_type = SG_IMAGESAMPLETYPE_DEPTH;
            desc.images[0].multisampled = false;
            desc.images[1].stage = SG_SHADERSTAGE_FRAGMENT;
            desc.images[1].image_type = SG_IMAGETYPE_2D;
            desc.images[1].sample_type = SG_IMAGESAMPLETYPE_FLOAT;
            desc.images[1].multisampled = false;
            desc.images[2].stage = SG_SHADERSTAGE_VERTEX;
            desc.images[2].image_type = SG_IMAGETYPE_2D;
            desc.images[2].sample_type = SG_IMAGESAMPLETYPE_FLOAT;
            desc.images[2].multisampled = false;
            desc.samplers[0].stage = SG_SHADERSTAGE_FRAGMENT;
            desc.samplers[0].sampler_type = SG_SAMPLERTYPE_COMPARISON;
            desc.samplers[1].stage = SG_SHADERSTAGE_FRAGMENT;
            desc.samplers[1].sampler_type = SG_SAMPLERTYPE_FILTERING;
            desc.samplers[2].stage = SG_SHADERSTAGE_VERTEX;
            desc.samplers[2].sampler_type = SG_SAMPLERTYPE_FILTERING;
            desc.image_sampler_pairs[0].stage = SG_SHADERSTAGE_VERTEX;
            desc.image_sampler_pairs[0].image_slot = 2;
            desc.image_sampler_pairs[0].sampler_slot = 2;
            desc.image_sampler_pairs[0].glsl_name = "waterColorTexture_waterSampler";
            desc.image_sampler_pairs[1].stage = SG_SHADERSTAGE_FRAGMENT;
            desc.image_sampler_pairs[1].image_slot = 0;
            desc.image_sampler_pairs[1].sampler_slot = 0;
            desc.image_sampler_pairs[1].glsl_name = "shadowMap_shadowMapSmp";
            desc.image_sampler_pairs[2].stage = SG_SHADERSTAGE_FRAGMENT;
            desc.image_sampler_pairs[2].image_slot = 1;
            desc.image_sampler_pairs[2].sampler_slot = 1;
            desc.image_sampler_pairs[2].glsl_name = "fogSkyTexture_fogSkySmp";

@jeffkdev
Copy link
Author

jeffkdev commented Nov 9, 2024

Sounds good, thanks! By the way, the new validation added this version helped too. I was missing a uniform bind in one of my shaders that was caught by the new sg_draw bindings check.

@floooh floooh closed this as completed in a7eabc4 Nov 9, 2024
@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Ok, fix is committed, binaries will be uptodate when this pipeline goes green: https://github.com/floooh/sokol-tools/actions/runs/11755942138

Many thanks for the bug report, that was an important one :)

(also if the fix still doesn't work for you please re-open this ticket - I'll try to write a new sample next which would trigger this problem, just to be save from similar regressions)

@jeffkdev
Copy link
Author

jeffkdev commented Nov 9, 2024

Cool will test when it finishes. Also while I'm thinking of it, there is a minor typo in error message: VALIDATE_SHADERDESC_IMAGE_NOT_REFERENCED_BY_IMAGE_SAMPLER_PAIRS (Sokol side change) "note" should be "not"

VALIDATE_SHADERDESC_IMAGE_NOT_REFERENCED_BY_IMAGE_SAMPLER_PAIRS: one or more images are note referenced by (sg_shader_desc.vs|fs.image_sampler_pairs[].image_slot)

@floooh
Copy link
Owner

floooh commented Nov 9, 2024

are thanks, will fix that right away :)

...ok the whole validation messages in that area need updating ...

@jeffkdev
Copy link
Author

jeffkdev commented Nov 9, 2024

The new shdc binary did fix the minimal reproduction I sent you. But it looks like it caused another issue so now the shader can't be created anymore. I haven't done any debugging or minifying the issue, but since it was working before (other than the shader_desc issue) I'll go ahead and DM you my whole uber shader file instead this time.

New error:

example.glsl:1:0: error: conflicting image-sampler definition found for 'shadowMap_shadowMapSmp'

@floooh floooh reopened this Nov 9, 2024
@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Ok, I think it's enough to just not do this over-eager image-sampler-pair merging across all shader programs. Such merged bindings are not actually needed anywhere (only for generating common bindslot constants, but the image-sampler-pairs don't have that).

Fix is on the way.

floooh added a commit that referenced this issue Nov 9, 2024
@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Ok new binaries in the oven: https://github.com/floooh/sokol-tools/actions/runs/11756324001

Let me know if that works :)

@jeffkdev
Copy link
Author

jeffkdev commented Nov 9, 2024

Shader generation and runtime are both working now. Thanks!

@jeffkdev jeffkdev closed this as completed Nov 9, 2024
@floooh
Copy link
Owner

floooh commented Nov 9, 2024

Nice, again thanks for the bug report. That was an important one.

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

No branches or pull requests

2 participants