DirectXShaderCompiler icon indicating copy to clipboard operation
DirectXShaderCompiler copied to clipboard

Feature request, combinedImageSampler for texture arrays

Open Makogan opened this issue 1 year ago • 6 comments

Consider the following GLSL code:

#version 450

layout(location = 0) in vec3 normal;
layout(location = 1) in vec3 uv;

layout(location = 0) out vec4 outColor;

layout(binding = 1) uniform sampler2D textures[3];

void main()
{
    outColor = texture(textures[int(uv.z)], uv.xy);
}

An attempt to translate this into HLSL could look like:

[[vk::combinedImageSampler]][[vk::binding(1)]]
Texture2D<float4> textures[3];
[[vk::combinedImageSampler]][[vk::binding(1)]]
SamplerState samp[3];

float4 main(
    [[vk::location(0)]] float3 normal : NORMAL0,
    [[vk::location(1)]] float3 uv : UV0
) : SV_TARGET
{
  uint index = int(uv.z);
  return textures[index].Sample(samp[index], uv.xy) + float4(normal, 1) * 0.00001;
}

However, DXC does not support declaring combined image sampler arrays like this. I have also compiled the HLSL version with shaderc and it produced a shader that rendered the correct/expected output. That might be non-compliance or a bug on the end of shaderc, but that indicates that it is possible to compile this shader down into an homologous spirv representation as the GLSL version.

Among other things this would make it easier for developers to port existing GLSL codebases targetting vulkan into HLSL.

Makogan avatar Mar 09 '23 21:03 Makogan

I don't want to double down on the vk::combinedImageSampler attribute. After updating inline spir-v, I'll see how we can handle this. As I work through the inline spr-v, I'll make one of my goals to be able to define a class that can be made available in a header file that will be a combined image sampler.

Then I would to deprecate vk::combinedImageSampler.

s-perron avatar Jun 22 '23 18:06 s-perron

I was a little surprised this doesn't work. I guess it's not trivial. Any idea on when this will be supported?

BarabasGitHub avatar Feb 09 '24 22:02 BarabasGitHub

We are close to finishing the implementation for vk::SpirvOpaqueType. Once that is done, we will try to implement combined image sampler using that. It should allow for arrays.

The problem with the current design for the vk::combinedImageSampler is that it relies one SPIR-V opt to be able to guess as which textures and samplers should be combined, and there are a lot of corner cases where we cannot get it correct.

s-perron avatar Feb 12 '24 15:02 s-perron

@cassiebeckley Once the vk::SpirvOpaqueType lands please provide the syntax for people to use to define an array of combined texture samplers, and then close the issue.

s-perron avatar Feb 12 '24 15:02 s-perron

@s-perron I made this in godbolt https://godbolt.org/z/sxejh1q7W

I'll get @Przemog1 to reimplement GLSL-style samplerXXX and texture... functions and they'll live in an Apache licensed header here: https://github.com/Devsh-Graphics-Programming/Nabla/blob/master/include/nbl/builtin/hlsl/glsl_compat/core.hlsl

However I've found a bug in the implementationProp 0011, one can't "nest" vk::SpirvOpaqueType inside each other, this is a blocker because:

  1. You need to first create an OpTypeImage
  2. Then an OpTypeSampledImage from (1)
  3. Then an OpTypePointer in the UniformConstant storage class, and thats the actual type you declare your variable with

The error I get is that apparently vk::SpirvOpaqueType is an object and not a type

 is an object and cannot be used as a type parameter

Should I open a separate issue about that?

Should I open a separate issue about that?

Yes please do that.

s-perron avatar May 17 '24 15:05 s-perron

We are looking into supporting combined texture samplers through the recently added vk::SpirvOpaqueType. When that is done, the arrays should work.

s-perron avatar Aug 23 '24 15:08 s-perron