Vulkan: using the same sampler in vertex and fragment shader causes to have no resource set for the sampler in the fragment shader

[adeisi]

Issue: Using Vulkan renderer (Dx12 works as expected) When using the same sampler in the vertex shader and fragment shader there is no resource set for the sampler in the fragment shader. Resource is set properly in vertex shader. Checked with RenderDoc.

To Reproduce: (41-tess) I have modfied the tesselation sample to simply read something from the same sampler used also in the fragment shader to reproduce this issue.

vs_terrain_render.sc

	// compute vertex location
	vec4 finalVertex = berp(v, a_texcoord0);

	finalVertex.z+= dmap(finalVertex.xy);
	
	v_texcoord0 = finalVertex.xy * 0.5 + 0.5;
// Added start
	vec2 s = texture2D(u_SmapSampler, v_texcoord0).xy; //using vec2(0,0) instead of texture2D(... makes no difference in the output
	finalVertex.x += s.y * 0.0001;
	finalVertex.z += s.y * 0.0001;
//Added end

Expected: No visual difference to the original rendering of the example

Using texture2D(u_SmapSampler... ):

Using vec2(0,0) instead of texture2D(u_SmapSampler... ):

Platform: Windows 10, VS 2019, latest nvidia driver 2070 super

[bkaradzic]

cc @pezcode

[pezcode]

That's a great repro 👍 Part of the issue is here:

https://github.com/bkaradzic/bgfx/blob/da555b072184d2831573719defcc5ebccb987448/src/renderer_vk.cpp#L5320

The code doesn't correctly merge the two shaders' m_bindInfo and m_bindings, but just uses m_bindInfo of the vertex shader if both access the same stage.

What complicates this a bit is that shaderc shifts the binding index around in the fragment shader:

https://github.com/bkaradzic/bgfx/blob/da555b072184d2831573719defcc5ebccb987448/tools/shaderc/shaderc_spirv.cpp#L766

Since the binding index is fixed inside SPIR-V and different for vertex and fragment, a bug fix either needs to:

• bind the texture twice in the same descriptor set (not a huge fan, but simple and backwards-compatible)
• hotpatch the SPIR-V at runtime (sounds horrible, but backwards-compatible)
• rework shaderc output to only shift UBO indices (cleanest long-term option but requires extra versioning to be backwards-compatible)

@rinthel What's the main motivation behind shifting the binding index for vertex and fragment? Is it safe to remove the offset for image, sampler, etc. and only keep it for uniforms?

[rinthel]

@pezcode I cannot remember every details, but yes, it's for uniform blocks, especially for several transformation matrices. If we can assure that images and samplers does not need offset, perhaps we can remove it

[hugoam]

I'll cross-post this here too. I've thought a little bit about this, and looking at the constraint that bgfx only has 16 fixed stages shared for every kind of binding (textures, images, buffers), I think this current code:

uint32_t bindingOffset = (stage == EShLanguage::EShLangFragment ? 48 : 0);
shader->setShiftBinding(glslang::EResUbo, bindingOffset);
shader->setShiftBinding(glslang::EResTexture, bindingOffset + 16);
shader->setShiftBinding(glslang::EResSampler, bindingOffset + 32);
shader->setShiftBinding(glslang::EResSsbo, bindingOffset + 16);
shader->setShiftBinding(glslang::EResImage, bindingOffset + 32);

Can probably be simplified safely to this:

uint32_t bindingOffset = 2; // Reserve two spots for the stage UBOs
shader->setShiftBinding(glslang::EResUbo, (stage == EShLanguage::EShLangFragment ? 1 : 0));
shader->setShiftBinding(glslang::EResTexture, bindingOffset);
shader->setShiftBinding(glslang::EResSampler, bindingOffset + 16);
shader->setShiftBinding(glslang::EResSsbo, bindingOffset);
shader->setShiftBinding(glslang::EResImage, bindingOffset);

We need two spots for the two stage UBOs, and we still need to shift the samplers, there is no way around that

[hugoam]

So I did try the change above on this branch https://github.com/bkaradzic/bgfx/compare/master...hugoam:spirv-bindings-simplify And it does look like it works perfectly in both VK and WebGPU, but shader versionning will be ugly, because you then have to conditionally use the old or new shifts on each of the lines I modified (I will have a try at this a bit later, and replace those magic numbers with some properly named constants too)

[hugoam]

@deity0815 Can you let us know if that PR fixed the issue for you ?

[adeisi]

@hugoam I have tried the latest version. If I have not made a mistake it seems to throw a VK validation error. I have used the following code before to make it work (include for vs and fs), this still works properly without validation error with the latest version:

#if PIXELShaderCompile == 1
SAMPLER2D(tex0,  8);
SAMPLER2D(tex1,  9);
#else
SAMPLER2D(tex0,  10);
SAMPLER2D(tex1,  11);
#endif

With the latest version I tried only using samplers below and only set required textures on cpu

SAMPLER2D(tex0,  8);
SAMPLER2D(tex1,  9);

This led to the following validation error: Device, Validation, 0: Validation Error: [ VUID-VkDescriptorSetLayoutCreateInfo-binding-00279 ] Object 0: handle = 0x1d157a63090, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0xf6f49662 | duplicated binding number in VkDescriptorSetLayoutBinding. The Vulkan spec states: The VkDescriptorSetLayoutBinding::binding members of the elements of the pBindings array must each have different values (https://vulkan.lunarg.com/doc/view/1.2.170.0/windows/1.2-extensions/vkspec.html#VUID-VkDescriptorSetLayoutCreateInfo-binding-00279)

[hugoam]

Ah, makes sense, the code is probably still binding it twice, but this should be a relatively simple fix now that we actually have the same binding index in both stages

[adeisi]

Vulkan: merge shared shader bindings (#2492) fixed it completely, thanks to @hugoam and @pezcode for fixing this issue. Great job 👍