Fix material system uniform SSBO struct packing #1777

slipher · 2025-08-29T18:19:12Z

Fix the code for laying out the std430 struct containing the uniforms for material system GLSL shaders. There is no functional change given the exact sets of uniforms we have on our GLSL shaders right now, but adding a new one could cause things to blow up as demonstrated at #1696 (comment).

I'm not attempting to run the added unit tests on CI for now as the client test does full client initialization including opening a window. So it wouldn't easily run in a headless environment.

Explicitly writing the padding seems pointless and the way the padding is calculated looks suspicious.

Use DAEMON_ASSERT_xyz instead of ASSERT_xyz in headers, so that the headers are compatible with Googletest. Also document this requirement in Assert.h and provide a non-shorthand version of ASSERT_UNREACHABLE.

If the uniform size is divisible by 16 it wrongly thought that 16 bytes of padding (instead of 0) were needed. Add a unit test demonstrating the case that was broken.

VReaperV · 2025-08-29T20:08:03Z

Remove padding from end of material uniform struct
Explicitly writing the padding seems pointless and the way
the padding is calculated looks suspicious.

The point is that I can copy it from shader source to buffer view in Nsight and immediately see the values read by the shader.

slipher · 2025-08-29T20:15:37Z

The point is that I can copy it from shader source to buffer view in Nsight and immediately see the values read by the shader.

I don't get it, why would adding dummy fields at the end make this more effective?

VReaperV · 2025-08-29T20:46:51Z

The point is that I can copy it from shader source to buffer view in Nsight and immediately see the values read by the shader.

I don't get it, why would adding dummy fields at the end make this more effective?

Because otherwise I'd need to add them manually.

slipher · 2025-08-29T21:39:15Z

OK I found that in the documentation:

Alignments are not considered; all types are considered packed.

Does the latest commit work? I would try it but the last time I tried to update the software, it broke - it can no longer establish the socket connection.

The original code seemed strange since it could only add padding at the end. I assume you need to add it anywhere else there are holes too? That's what I did.

VReaperV · 2025-08-29T22:05:16Z

Thx, I'll try that in a bit.

Yeah, the original code might be wrong on that.

VReaperV · 2025-08-30T16:01:05Z

Does the latest commit work? I would try it but the last time I tried to update the software, it broke - it can no longer establish the socket connection.

That does work, yes.

The change in struct packing does, however, break when the material uniforms add up to less than 4 components/16 bytes. In that case the size is calculated as if they're tightly packed, when in the actual buffer interface they're rounded up to a vec4. The std430Size naming is misleading at this point, since the only buffer interfaces that use the generated structs are now using std140. The related variables functions can be renamed or a boolean flag added to round up the alignment to vec4. Personally though, I don't have any plans to add buffers with generated struct arrays and std430 interfaces.

Also, I believe the implementation in this pr doesn't calculate the size for arrays correctly as the component count is never taken into account.

Fix some bugs and add more unit tests.

It's actually std140 not std430, so fix comment and GLShader member names to reflect that. (GLUniform still has the old _std430XXX names though.) Since it cannot handle arrays, add an assert that the uniforms are not arrays. Also delete WriteToBuffer implementations that assume std430 layout for arrays.

slipher · 2025-08-30T18:01:22Z

The change in struct packing does, however, break when the material uniforms add up to less than 4 components/16 bytes. In that case the size is calculated as if they're tightly packed, when in the actual buffer interface they're rounded up to a vec4.

I didn't realize it actually uses std140, not std430. Fixed. I renamed some of the variable/function names in GLShader to reflect this.

Also, I believe the implementation in this pr doesn't calculate the size for arrays correctly as the component count is never taken into account.

The code was not prepared to handle arrays anyway since the WriteToBuffer functions all assumed std430 packing, plus they would have multiplied any padding at the end by the number of elements. So I added an assert that arrays are not used and NUKED WriteToBuffer implementations that are not compatible with std140.

VReaperV · 2025-08-30T18:10:58Z

The code was not prepared to handle arrays anyway since the WriteToBuffer functions all assumed std430 packing, plus they would have multiplied any padding at the end by the number of elements. So I added an assert that arrays are not used and NUKED WriteToBuffer implementations that are not compatible with std140.

I have it working fine in #1684 for u_Frustum. It's enough to check that the underlying value is 16 bytes and multiply by the array size when calculating struct size. Padding is never needed for such a uniform since it's always aligned.

The change to GLUniform1Bool seems erroneous?

slipher · 2025-08-31T22:19:32Z

I have it working fine in #1684 for u_Frustum. It's enough to check that the underlying value is 16 bytes and multiply by the array size when calculating struct size. Padding is never needed for such a uniform since it's always aligned.

OK I added pack support for 4-aligned arrays.

The change to GLUniform1Bool seems erroneous?

It's not strictly related to lack of array support, but I deleted it in passing since it also looks wrong. It only writes 1 byte out of 4 and even if you can get away with that, it's definitely not endian-aware.

DolceTriade · 2025-09-01T00:43:02Z

Does this fix the edge case where if you add a uniform that is 4 byte aligned, it messes up the alignment of the struct and causes view corruption? I hit that in some of my testing..

I hacked it like this:

@@ -2135,7 +2135,7 @@ void GLShader::PostProcessUniforms() {
                GLuint size = _materialSystemUniforms[0]->_std430Size;
                GLuint components = _materialSystemUniforms[0]->_components;
                size = components ? PAD( size, 4 ) * components : size;
-               GLuint alignmentConsume = 4 - size % 4;
+               GLuint alignmentConsume = (size % 4 == 0) ? 0 : 4 - size % 4;
 
                GLUniform* tmpUniform = _materialSystemUniforms[0];
                tmp.emplace_back( _materialSystemUniforms[0] );

(which is probably wrong, but it fixed my corruption)

slipher · 2025-09-01T01:04:47Z

Does this fix the edge case where if you add a uniform that is 4 byte aligned, it messes up the alignment of the struct and causes view corruption? I hit that in some of my testing..

I hacked it like this:
@@ -2135,7 +2135,7 @@ void GLShader::PostProcessUniforms() {
                GLuint size = _materialSystemUniforms[0]->_std430Size;
                GLuint components = _materialSystemUniforms[0]->_components;
                size = components ? PAD( size, 4 ) * components : size;
-               GLuint alignmentConsume = 4 - size % 4;
+               GLuint alignmentConsume = (size % 4 == 0) ? 0 : 4 - size % 4;
 
                GLUniform* tmpUniform = _materialSystemUniforms[0];
                tmp.emplace_back( _materialSystemUniforms[0] );
(which is probably wrong, but it fixed my corruption)

Yes. The Fix bad material SSBO uniform layout for multiple of 16 commit gets that one.

VReaperV · 2025-09-02T11:48:08Z

src/engine/renderer/gl_shader.cpp

-		if ( uniforms[i]->_std430Size <= alignment ) {
-			return i;
+static auto FindUniformForOffset( std::vector<GLUniform*>& uniforms, const GLuint baseOffset ) {
+	for ( auto it = uniforms.begin(); it != uniforms.end(); ++it ) {


You can just do it : uniforms if you're making it an iterator instead of an index.

I don't see how - you just get a reference to the element that way, not an iterator.

VReaperV · 2025-09-02T11:48:37Z

src/engine/renderer/gl_shader.cpp

+		} else {
+			( *iterNext )->_std430Size = ( *iterNext )->_std430BaseSize;
+			if ( ( *iterNext )->_components ) {
+				ASSERT_GE( ( *iterNext )->_std430Alignment, 4 ); // aligment less than 4 not compatible with std130


Clarified the comment.

VReaperV · 2025-09-02T11:50:31Z

It's not strictly related to lack of array support, but I deleted it in passing since it also looks wrong. It only writes 1 byte out of 4 and even if you can get away with that, it's definitely not endian-aware.

I see. #1685 should fix that once merged, and will get rid of the WriteToBuffer() overloads.

slipher added 3 commits August 29, 2025 04:34

Remove padding from end of material uniform struct

6c00125

Explicitly writing the padding seems pointless and the way the padding is calculated looks suspicious.

Don't use assert shorthands in headers

8a64d1c

Use DAEMON_ASSERT_xyz instead of ASSERT_xyz in headers, so that the headers are compatible with Googletest. Also document this requirement in Assert.h and provide a non-shorthand version of ASSERT_UNREACHABLE.

Fix bad material SSBO uniform layout for multiple of 16

79858d7

If the uniform size is divisible by 16 it wrongly thought that 16 bytes of padding (instead of 0) were needed. Add a unit test demonstrating the case that was broken.

slipher mentioned this pull request Aug 29, 2025

Relief mapping doesn't work when the material system is used #1696

Closed

slipher added 3 commits August 30, 2025 12:55

Rewrite material system uniform SSBO packing

7246e45

Fix some bugs and add more unit tests.

Add back padding to material uniforms struct

4a85964

slipher force-pushed the structfix branch from e609f4b to 5b7d732 Compare August 30, 2025 17:57

VReaperV approved these changes Sep 2, 2025

View reviewed changes

Material uniform packing: add back arrays

4b87404

slipher force-pushed the structfix branch from e91fca0 to 4b87404 Compare September 4, 2025 13:43

slipher merged commit 1649189 into DaemonEngine:master Sep 4, 2025
9 checks passed

slipher deleted the structfix branch September 4, 2025 16:56

Fix material system uniform SSBO struct packing #1777

Fix material system uniform SSBO struct packing #1777

Uh oh!

Conversation

slipher commented Aug 29, 2025

Uh oh!

VReaperV commented Aug 29, 2025

Uh oh!

slipher commented Aug 29, 2025

Uh oh!

VReaperV commented Aug 29, 2025

Uh oh!

slipher commented Aug 29, 2025

Uh oh!

VReaperV commented Aug 29, 2025

Uh oh!

VReaperV commented Aug 30, 2025

Uh oh!

slipher commented Aug 30, 2025

Uh oh!

VReaperV commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slipher commented Aug 31, 2025

Uh oh!

DolceTriade commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slipher commented Sep 1, 2025

Uh oh!

VReaperV Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

slipher Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

VReaperV Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

slipher Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

VReaperV commented Sep 2, 2025

Uh oh!

Uh oh!

Uh oh!

VReaperV commented Aug 30, 2025 •

edited

Loading

DolceTriade commented Sep 1, 2025 •

edited

Loading