Spector.js icon indicating copy to clipboard operation
Spector.js copied to clipboard

Values for certain types of uniforms within UBOs are shown incorrectly

Open mixtur opened this issue 2 years ago • 5 comments

I was using Spector.js Chrome extension to debug our migration to UBOs and stumbled upon this.

We have the following uniform block:

layout(std140) uniform DirectLightShadowCastersUBO {
    float frustumSplits[3];
    int directShadowCastersMask[1];
    mat4 directShadowCastersTransforms[3];
};

And spector shows the following for frustumSplits image

The displayed value is 0.1683, 0, 0 which is wrong. The last two values shouldn't be zeroes. I think Spector is ignoring arrayStride here, because when I put some numbers into the buffer one after another I can actually see them in Spector.

mixtur avatar Aug 23 '23 09:08 mixtur

Yes, looks like it gets ignored (note how the offset is used without respecting arrayStride and matrixStride):

https://github.com/BabylonJS/Spector.js/blob/35385a5c015dee6fcd232ba253ad98d3d502f5ea/src/backend/states/drawCalls/drawCallState.ts#L453-L463

Edit: This might be part of offsets[i] retrieved from GL; I'm not sure about the specifics here, so I might be wrong. Even with the bug, the second and third element wouldn't be read as zero I believe. Did you try it on another machine with a different GL implementation already?

JannikGM avatar Aug 23 '23 11:08 JannikGM

Note layout(std140) in the shader code - it should make the layout implementation independent, but if you want, you can try it yourself https://cib3.webgears.app/3D-Engine/228e665dc1d4cd9fd12ab71a4dbcf5abd64e26e3/shadows.html (look for draw calls with PBR_MESH_SHADER in shader names) The link is only going to be alive for a few days maximum.

Though judging by the implementation of getUboValue it is expected that there are zeroes. https://github.com/BabylonJS/Spector.js/blob/35385a5c015dee6fcd232ba253ad98d3d502f5ea/src/backend/states/drawCalls/drawCallUboInputState.ts#L50-L75

In case of float x[3] the layout is the following:

[ 4 bytes x[0], 12 bytes to satisfy alignment,
  4 bytes x[1], 12 bytes to satisfy alignment,
  4 bytes x[2], 12 bytes padding,
  ]

The implementation simply does getBufferSubdata and then seems to do nothing with it. Assuming uboType.lengthMultiplier is 1 and size is 3. It should indeed read the first float and two zero-paddings immediately following it.

mixtur avatar Aug 23 '23 15:08 mixtur

I've tried to fix it, but it's still broken for matrices: https://github.com/JannikGM/Spector.js/tree/strides (and it still needs to be cleaned up). There's also a lack of optimizations still (like auto-reading objects with stride==0 / stride==elementSize)

JannikGM avatar Aug 23 '23 17:08 JannikGM

According to the spec https://registry.khronos.org/OpenGL/extensions/ARB/ARB_uniform_buffer_object.txt column-major matrices are treated as arrays of columns regardless of the dimensions. And array elements are vec4-aligned. So mat4x2 is taking as much space as mat4.

Row-major matrices are the same except they should be treated as arrays of rows.

Although today I discovered that when on NVidia GPUs + Chrome row-major case does something else. As well as Android on Mali G68. So yeah. It seems that this stuff is still implementation-defined to some extent. Or maybe I am using the wrong spec :)

mixtur avatar Aug 23 '23 20:08 mixtur

This is a really interesting one indeed :-)

@JannikGM, thanks a lot for your proposal here, this is going in the right direction and yes it was a complete oversight on my side.

sebavan avatar Aug 30 '23 22:08 sebavan