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

Possible incorrect handling of texture arrays in GLSL #4537

Open
spencerkohan opened this issue Oct 30, 2022 · 6 comments
Open

Possible incorrect handling of texture arrays in GLSL #4537

spencerkohan opened this issue Oct 30, 2022 · 6 comments
Labels
area: naga front-end kind: feature New feature or request lang: GLSL OpenGL Shading Language naga Shader Translator

Comments

@spencerkohan
Copy link

I'm using naga to create a module out of the following frgment shader:

            #version 450

            layout(location=2) in  vec2 uv;

            layout(set = 0, binding = 0) uniform texture2D glyphTextures[256];
            layout(set = 0, binding = 1) uniform sampler glyphSampler;

            void main() {
                vec4 tex = texture(
                    sampler2D(glyphTextures[0], glyphSampler),
                    uv
                );

                f_color = tex;
            }

If I inspect the resulting module, the type of the global variable named "glyphTexutures" is:

Type {
    name: None,
    inner: Array {
        base: [4],
        size: Constant(
            [5],
        ),
        stride: 0,
    },
}

According to the documentation, it looks like my type inner should be TypeInner::BindingArray rather than Array, right?

@teoxoy
Copy link
Member

teoxoy commented Nov 4, 2022

The initial implementation didn't cover all frontends (only the wgsl one). See gfx-rs/naga#1845 (comment) and #4313.

@teoxoy teoxoy added kind: feature New feature or request area: naga front-end lang: GLSL OpenGL Shading Language labels Dec 5, 2022
@cwfitzgerald cwfitzgerald transferred this issue from gfx-rs/naga Oct 25, 2023
@cwfitzgerald cwfitzgerald added the naga Shader Translator label Oct 25, 2023
@djeedai
Copy link

djeedai commented Feb 18, 2024

Just to confirm that I hit this in real-world code, which manifests as a panic on some unreachable!() (which is kind of scary, and not very explanatory).

  • naga::back::glsl::Writer::write_global() will get a global.space == AddressSpace::Handle which has an unreachable!() in the match case.
  • The caller only handles TypeInner::Image and TypeInner::Sampler, assuming that this is enough to handle all Handles, which is not the case for a binding array.

Original WGSL code which got translated had:

@group(0) @binding(13) var diffuse_environment_maps: binding_array<texture_cube<f32>, 8u>;

@cwfitzgerald
Copy link
Member

The problem you're hitting is a different one than the original issue. OpenGL doesn't support binding arrays, so we can't actually emit glsl for that binding.

@djeedai
Copy link

djeedai commented Feb 18, 2024

Oh sorry, I mixed texture arrays and binding arrays, wrong issue my bad.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Feb 18, 2024

I want to note the original issue is using binding arrays - this is just a GLSL frontend issue (we accept vulkan flavored glsl) whereas you're having a backend issue (we output OpenGL flavored glsl). OpenGL flavor glsl doesn't support binding arrays.

@djeedai
Copy link

djeedai commented Feb 19, 2024

Should I open a separate one for the fact the code reaches an unreachable!(), which even if GLSL doesn't support array bindings feels like a lack of error handling which at best shouldn't panic, or at least should assert with a dedicated condition and error message?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga front-end kind: feature New feature or request lang: GLSL OpenGL Shading Language naga Shader Translator
Projects
None yet
Development

No branches or pull requests

4 participants