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

Feature request, combinedImageSampler for texture arrays #5092

Open
Makogan opened this issue Mar 9, 2023 · 6 comments
Open

Feature request, combinedImageSampler for texture arrays #5092

Makogan opened this issue Mar 9, 2023 · 6 comments
Assignees
Labels
enhancement Feature suggestion spirv Work related to SPIR-V

Comments

@Makogan
Copy link

Makogan commented Mar 9, 2023

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
Copy link
Collaborator

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.

SkillerRaptor added a commit to SkillerRaptor/hyper_engine that referenced this issue Aug 11, 2023
This will stay as long DXC has not implemented the equivalent for
bindless combined image samplers.

Issue: microsoft/DirectXShaderCompiler#5092
@BarabasGitHub
Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

@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

@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
Copy link
Collaborator

Should I open a separate issue about that?

Yes please do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature suggestion spirv Work related to SPIR-V
Projects
None yet
Development

No branches or pull requests

6 participants