Texture binding slots issue

[RandyGaul]

Currently if you write a shader and then comment out one of the textures, the slot bindings will be mismatched/counted by CF's implementation. For example:

layout(set = 2, binding = 1) uniform sampler2D tex1;
layout(set = 2, binding = 2) uniform sampler2D tex2;
layout(set = 2, binding = 3) uniform sampler2D tex3;

And then comment one out:

layout(set = 2, binding = 1) uniform sampler2D tex1;
//layout(set = 2, binding = 2) uniform sampler2D tex2;
layout(set = 2, binding = 3) uniform sampler2D tex3;

The third texture will get "missed" by CF's implementation. A proper solution would probably require a refactor of CF_ShaderInternal and potentially the glslang shader reflection to pull out shader slot bindings and store them in our own reflection structs.

This chunk of code can then use the new slot index instead of j index for assignments:

	// Bind images to all their respective slots.
	int sampler_count = shader->image_names.count();
	SDL_GPUTextureSamplerBinding* sampler_bindings = SDL_stack_alloc(SDL_GPUTextureSamplerBinding, sampler_count);
	int found_image_count = 0;
	for (int i = 0; found_image_count < sampler_count && i < material->fs.textures.count(); ++i) {
		const char* image_name = material->fs.textures[i].name;
		for (int j = 0; j < shader->image_names.size(); ++j) {
			if (shader->image_names[j] == image_name) {
				sampler_bindings[j].sampler = ((CF_TextureInternal*)material->fs.textures[i].handle.id)->sampler;
				sampler_bindings[j].texture = ((CF_TextureInternal*)material->fs.textures[i].handle.id)->tex;
				found_image_count++;
			}
		}
	}
	CF_ASSERT(found_image_count == sampler_count);
	SDL_BindGPUFragmentSamplers(pass, 0, sampler_bindings, (Uint32)found_image_count);

[bullno1]

I'm looking at the shader compiler and it could be wrong too: https://github.com/RandyGaul/cute_framework/blob/master/src/cute_shader/cute_shader.cpp#L385

SpvReflectDescriptorBinding::binding reflects the binding = 1 field in the layout attribute. It should be used instead of the loop variable because samplers might be declared out of order:

layout(set = 2, binding = 1) uniform sampler2D tex1;
layout(set = 2, binding = 3) uniform sampler2D tex3; // If we use the loop var, this would be 2
layout(set = 2, binding = 2) uniform sampler2D tex2;

Another thing is the way combined vs separate sampler are managed: https://github.com/RandyGaul/cute_framework/blob/master/src/cute_shader/cute_shader.cpp#L389C9-L393

I think they both need a name array, or rather an array of CF_ShaderLayoutInfo:

typedef struct CF_ShaderLayoutInfo {
    const char* name;
    int binding;
} CF_ShaderLayoutInfo;

[pusewicz]

Does this fix the issue? https://github.com/RandyGaul/cute_framework/compare/master...pusewicz:324-binfings?body=&expand=1

[bullno1]

Yeah, that's the general idea.

Even before this, I was already a bit confused about the separate vs combined sampler handling (i.e: The fall through without name). Technically, CF seems to support combined sampler only? Should the compiler just error out on separate image sampler?

There's also this: https://github.com/KhronosGroup/glslang/issues/591 where the distinction can be removed. Although I'm not sure how that would translate to different rendering backends.

[RandyGaul]

Feel free to put up a PR. It’s certainly worth fixing properly! Just requires a few test cases to assert correctness and we can merge in a solution after discussing it further.