Traverse-Research / rspirv-reflect

🦀 Minimal SPIR-V reflection library.
https://traverseresearch.nl
Apache License 2.0
41 stars 10 forks source link

Wrong push constant size when using buffer references. #29

Closed halli2 closed 1 year ago

halli2 commented 2 years ago

Using push constant for buffer references gives wrong push constant range size.

Glsl shader:

#version 460

#extension GL_EXT_buffer_reference:require
#extension GL_EXT_buffer_reference2:require

struct Mesh {
    vec4 position;
    vec2 uv;
};

layout(std430, buffer_reference, buffer_reference_align = 32) readonly buffer MeshBuffer {
    Mesh mesh[];
};
layout(std430, buffer_reference) readonly buffer IndexBuffer {
    uint index[];
};

layout(push_constant) uniform registers {
    MeshBuffer mesh_buffer;
    IndexBuffer index_buffer;
} Registers;

layout(location = 0) out vec2 uv;

void main(){
    uint index = registers.index_buffer.index[gl_VertexIndex];
    Mesh mesh = registers.mesh_buffer.mesh[index];
    gl_Position = mesh.position;
    uv = mesh.uv;
}

Should give size of 16, but reflection gives size of 8.

MarijnS95 commented 2 years ago

Your example does not compile, lowercase registers and Registers should be swapped in the layout(push_constants) uniform <type> { ... } <identifier>; declaration.

Root-caused to: https://github.com/Traverse-Research/rspirv-reflect/blob/8894d3570cd738c61e26b08349f55277092b9e3c/src/lib.rs#L602

Turns out we were treating any unknown type as zero-sized, when we should obviously panic on encountering something unknown (so that it can be reported and implemented). In your case it was OpTypePointer, which I hardcoded to 8 for now in #5.

MarijnS95 commented 1 year ago

@halli2 as it turns out this isn't a supported use-case by upstream SPIR-V Reflect either. It just "happens to work" because they round up a struct to SPIRV_DATA_ALIGNMENT (which we don't do in this crate :sweat_smile:), but we can very easily break it. Consider the following GLSL snippet:

layout(push_constant) uniform Registers
{
    uint test; // off=0, size=4
    MeshBuffer mesh_buffer; // off=8, size=8?
    IndexBuffer index_buffer; // off=16, size=8?
}
registers;

This bit of code:

https://github.com/KhronosGroup/SPIRV-Reflect/blob/a7c7b8a99f8fa7e21ec37f591a427196262659c4/spirv_reflect.c#L2398-L2408

Simply finds the offset of index_buffer at 16, doesn't know a size, and finally rounds up to a multiple of 16 and returns a push-constant size of 16 instead of 24 :)

If you don't mind I'd like to ask about this on the SPIRV-Reflect repo before merging #5 and releasing it :)