microsoft / DirectXShaderCompiler

This repo hosts the source for the DirectX Shader Compiler which is based on LLVM/Clang.
Other
3.04k stars 677 forks source link

Feature request, combinedImageSampler for texture arrays #5092

Open Makogan opened 1 year ago

Makogan commented 1 year ago

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.

s-perron commented 1 year ago

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.

BarabasGitHub commented 7 months ago

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

s-perron commented 6 months ago

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 commented 6 months ago

@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.

devshgraphicsprogramming commented 4 months ago

@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?

s-perron commented 3 months ago

Should I open a separate issue about that?

Yes please do that.

s-perron commented 2 weeks ago

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