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

Wrong push constant size when using buffer references. #29

Closed
halli2 opened this issue Nov 6, 2022 · 2 comments · Fixed by #5
Closed

Wrong push constant size when using buffer references. #29

halli2 opened this issue Nov 6, 2022 · 2 comments · Fixed by #5

Comments

@halli2
Copy link

halli2 commented Nov 6, 2022

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

MarijnS95 commented Nov 6, 2022

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

Root-caused to:

_ => Ok(0),

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

@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 😅), 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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants