From 6c00125769f2ab881401b085f8a42301346da1c5 Mon Sep 17 00:00:00 2001 From: slipher Date: Fri, 29 Aug 2025 04:34:23 -0500 Subject: [PATCH 1/7] Remove padding from end of material uniform struct Explicitly writing the padding seems pointless and the way the padding is calculated looks suspicious. --- src/engine/renderer/gl_shader.cpp | 16 ++++------------ src/engine/renderer/gl_shader.h | 5 ++--- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/engine/renderer/gl_shader.cpp b/src/engine/renderer/gl_shader.cpp index 28f1c1c64d..09a56c79d8 100644 --- a/src/engine/renderer/gl_shader.cpp +++ b/src/engine/renderer/gl_shader.cpp @@ -1545,9 +1545,8 @@ std::string GLShaderManager::RemoveUniformsFromShaderText( const std::string& sh return shaderMain; } -void GLShaderManager::GenerateUniformStructDefinesText( const std::vector& uniforms, const uint32_t padding, - const uint32_t paddingCount, const std::string& definesName, - std::string& uniformStruct, std::string& uniformDefines ) { +void GLShaderManager::GenerateUniformStructDefinesText( const std::vector& uniforms, + const std::string& definesName, std::string& uniformStruct, std::string& uniformDefines ) { for ( GLUniform* uniform : uniforms ) { uniformStruct += " " + ( uniform->_isTexture ? "uvec2" : uniform->_type ) + " " + uniform->_name; @@ -1569,12 +1568,6 @@ void GLShaderManager::GenerateUniformStructDefinesText( const std::vector_materialSystemUniforms, shader->padding, - 0, "materials[baseInstance & 0xFFF]", materialStruct, materialDefines ); + GenerateUniformStructDefinesText( shader->_materialSystemUniforms, + "materials[baseInstance & 0xFFF]", materialStruct, materialDefines ); materialStruct += "};\n\n"; @@ -2158,7 +2151,6 @@ void GLShader::PostProcessUniforms() { size = PAD( size, 4 ); std430Size += size; - padding = alignmentConsume; } _materialSystemUniforms = tmp; diff --git a/src/engine/renderer/gl_shader.h b/src/engine/renderer/gl_shader.h index ffaefc504b..e8b99273cd 100644 --- a/src/engine/renderer/gl_shader.h +++ b/src/engine/renderer/gl_shader.h @@ -186,7 +186,6 @@ class GLShader { const bool hasComputeShader; GLuint std430Size = 0; - uint32_t padding = 0; const bool worldShader; protected: @@ -462,8 +461,8 @@ class GLShaderManager { ShaderProgramDescriptor* out ); void SaveShaderBinary( ShaderProgramDescriptor* descriptor ); - void GenerateUniformStructDefinesText( const std::vector& uniforms, const uint32_t padding, - const uint32_t paddingCount, const std::string& definesName, + void GenerateUniformStructDefinesText( + const std::vector& uniforms, const std::string& definesName, std::string& uniformStruct, std::string& uniformDefines ); std::string RemoveUniformsFromShaderText( const std::string& shaderText, const std::vector& uniforms ); std::string ShaderPostProcess( GLShader *shader, const std::string& shaderText, const uint32_t offset ); From 8a64d1c4fef4db5f39bd1e8badbbef106988ed79 Mon Sep 17 00:00:00 2001 From: slipher Date: Fri, 29 Aug 2025 06:09:25 -0500 Subject: [PATCH 2/7] Don't use assert shorthands in headers 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. --- src/common/Assert.h | 6 ++++-- src/common/Math.h | 2 +- src/engine/framework/Resource.h | 2 +- src/engine/renderer/GLMemory.h | 2 +- src/engine/renderer/gl_shader.h | 32 ++++++++++++++++---------------- src/engine/renderer/tr_local.h | 16 ++++++++-------- src/engine/sys/sys_events.h | 2 +- 7 files changed, 32 insertions(+), 30 deletions(-) diff --git a/src/common/Assert.h b/src/common/Assert.h index 1a8c8620fa..47745e4ec8 100644 --- a/src/common/Assert.h +++ b/src/common/Assert.h @@ -42,7 +42,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * information to start debugging when the assert fires. * * In case of name clashes (with for example a testing library), you can define the - * DAEMON_SKIP_ASSERT_SHORTHANDS to only define the DAEMON_ prefixed macros. + * DAEMON_SKIP_ASSERT_SHORTHANDS to only define the DAEMON_ prefixed macros. Avoid using + * the shorthands in header files, so that the headers can be included in tests. * * These asserts feature: * - Logging of the error with file, line and function information. @@ -157,10 +158,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #define ASSERT_LE DAEMON_ASSERT_LE #define ASSERT_GT DAEMON_ASSERT_GT #define ASSERT_GE DAEMON_ASSERT_GE + #define ASSERT_UNREACHABLE DAEMON_ASSERT_UNREACHABLE #endif // You can put ASSERT_UNREACHABLE() in places that must never be reached. -#define ASSERT_UNREACHABLE() DAEMON_ASSERT_CALLSITE(false, , false, "Unreachable code hit."); UNREACHABLE(); +#define DAEMON_ASSERT_UNREACHABLE() DAEMON_ASSERT_CALLSITE(false, , false, "Unreachable code hit."); UNREACHABLE(); #ifdef DAEMON_ASSERTS_ENABLED // This stuff is so that the ASSERT_cc variants can be used on objects that don't have a diff --git a/src/common/Math.h b/src/common/Math.h index 2185fc0706..24c82e4ede 100644 --- a/src/common/Math.h +++ b/src/common/Math.h @@ -40,7 +40,7 @@ namespace Math { template inline WARN_UNUSED_RESULT T Clamp(T value, T min, T max) { - ASSERT_LE(min, max); + DAEMON_ASSERT_LE(min, max); if (!(value >= min)) return min; if (!(value <= max)) diff --git a/src/engine/framework/Resource.h b/src/engine/framework/Resource.h index 0117a062db..72298d5b18 100644 --- a/src/engine/framework/Resource.h +++ b/src/engine/framework/Resource.h @@ -61,7 +61,7 @@ namespace Resource { class Handle { public: Handle(std::shared_ptr value, const Manager* manager): value(value), manager(manager) { - ASSERT(!!value); // Should not be null + DAEMON_ASSERT(!!value); // Should not be null } // Returns a pointer to the resource, or to the default value if the diff --git a/src/engine/renderer/GLMemory.h b/src/engine/renderer/GLMemory.h index 9bead8c93a..4dbc12f2db 100644 --- a/src/engine/renderer/GLMemory.h +++ b/src/engine/renderer/GLMemory.h @@ -234,7 +234,7 @@ class GLVAO { uint32_t ofs = 0; for ( const vertexAttributeSpec_t* spec = attrBegin; spec < attrEnd; spec++ ) { vboAttributeLayout_t& attr = attrs[spec->attrIndex]; - ASSERT_NQ( spec->numComponents, 0U ); + DAEMON_ASSERT_NQ( spec->numComponents, 0U ); attr.componentType = spec->componentStorageType; if ( attr.componentType == GL_HALF_FLOAT && !glConfig.halfFloatVertexAvailable ) { attr.componentType = GL_FLOAT; diff --git a/src/engine/renderer/gl_shader.h b/src/engine/renderer/gl_shader.h index e8b99273cd..01f433613a 100644 --- a/src/engine/renderer/gl_shader.h +++ b/src/engine/renderer/gl_shader.h @@ -495,7 +495,7 @@ class GLUniformSampler : protected GLUniform { ShaderProgramDescriptor* p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } return p->uniformLocations[_locationIndex]; @@ -574,7 +574,7 @@ class GLUniform1i : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -619,7 +619,7 @@ class GLUniform1ui : protected GLUniform { ShaderProgramDescriptor* p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -663,7 +663,7 @@ class GLUniform1Bool : protected GLUniform { ShaderProgramDescriptor* p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -710,7 +710,7 @@ class GLUniform1f : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -759,7 +759,7 @@ class GLUniform1fv : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -794,7 +794,7 @@ class GLUniform2f : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -846,7 +846,7 @@ class GLUniform3f : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -898,7 +898,7 @@ class GLUniform4f : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -947,7 +947,7 @@ class GLUniform4fv : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -982,7 +982,7 @@ class GLUniformMatrix4f : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -1027,7 +1027,7 @@ class GLUniformMatrix32f : protected GLUniform { ShaderProgramDescriptor* p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -1065,7 +1065,7 @@ class GLUniformMatrix4fv : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -1099,7 +1099,7 @@ class GLUniformMatrix34fv : protected GLUniform ShaderProgramDescriptor *p = _shader->GetProgram(); if ( _global || !_shader->UseMaterialSystem() ) { - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); } if ( _shader->UseMaterialSystem() && !_global ) { @@ -1155,7 +1155,7 @@ class GLUniformBlock ShaderProgramDescriptor *p = _shader->GetProgram(); GLuint blockIndex = p->uniformBlockIndexes[_locationIndex]; - ASSERT_EQ( p, glState.currentProgram ); + DAEMON_ASSERT_EQ( p, glState.currentProgram ); if( blockIndex != GL_INVALID_INDEX ) { glBindBufferBase( GL_UNIFORM_BUFFER, blockIndex, buffer ); @@ -2795,7 +2795,7 @@ static colorModulation_t ColorModulateColorGen( if ( useMapLightFactor ) { - ASSERT_EQ( vertexOverbright, false ); + DAEMON_ASSERT_EQ( vertexOverbright, false ); colorModulation.lightFactor = tr.mapLightFactor; } diff --git a/src/engine/renderer/tr_local.h b/src/engine/renderer/tr_local.h index 7105b243eb..06a744829a 100644 --- a/src/engine/renderer/tr_local.h +++ b/src/engine/renderer/tr_local.h @@ -2255,13 +2255,13 @@ enum z = Math::Clamp( z, 0u, depth - 1 ); } - ASSERT_GE( x, 0u ); - ASSERT_GE( y, 0u ); - ASSERT_GE( z, 0u ); + DAEMON_ASSERT_GE( x, 0u ); + DAEMON_ASSERT_GE( y, 0u ); + DAEMON_ASSERT_GE( z, 0u ); - ASSERT_LT( x, width ); - ASSERT_LT( y, height ); - ASSERT_LT( z, depth ); + DAEMON_ASSERT_LT( x, width ); + DAEMON_ASSERT_LT( y, height ); + DAEMON_ASSERT_LT( z, depth ); return grid[z * width * height + y * width + x]; } @@ -2281,9 +2281,9 @@ enum index = Math::Clamp( index, 0u, size - 1 ); } - ASSERT_GE( index, 0U ); + DAEMON_ASSERT_GE( index, 0U ); - ASSERT_LT( index, size ); + DAEMON_ASSERT_LT( index, size ); return grid[index]; } diff --git a/src/engine/sys/sys_events.h b/src/engine/sys/sys_events.h index c6ad9d6990..b65bfaff2b 100644 --- a/src/engine/sys/sys_events.h +++ b/src/engine/sys/sys_events.h @@ -49,7 +49,7 @@ class EventBase { template const T& Cast() const { - ASSERT_EQ(this->type, T::ClassType()); + DAEMON_ASSERT_EQ(this->type, T::ClassType()); return *static_cast(this); } From 79858d7eac18d6e7198be85a426d953db711c3e3 Mon Sep 17 00:00:00 2001 From: slipher Date: Fri, 29 Aug 2025 03:25:54 -0500 Subject: [PATCH 3/7] Fix bad material SSBO uniform layout for multiple of 16 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. --- CMakeLists.txt | 2 +- src.cmake | 4 ++ src/engine/renderer/gl_shader.cpp | 4 +- src/engine/renderer/gl_shader_test.cpp | 76 ++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 src/engine/renderer/gl_shader_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 3572894f2d..93ff21b2b6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -952,7 +952,7 @@ if (BUILD_CLIENT) Flags ${WARNINGS} Files ${WIN_RC} ${BUILDINFOLIST} ${QCOMMONLIST} ${SERVERLIST} ${CLIENTBASELIST} ${CLIENTLIST} Libs ${LIBS_CLIENT} ${LIBS_CLIENTBASE} ${LIBS_ENGINE} - Tests ${ENGINETESTLIST} + Tests ${CLIENTTESTLIST} ) # generate glsl include files diff --git a/src.cmake b/src.cmake index 304b304a8e..df5b642eca 100644 --- a/src.cmake +++ b/src.cmake @@ -356,6 +356,10 @@ set(CLIENTLIST ${RENDERERLIST} ) +set(CLIENTTESTLIST ${ENGINETESTLIST} + ${ENGINE_DIR}/renderer/gl_shader_test.cpp +) + set(TTYCLIENTLIST ${ENGINE_DIR}/null/NullAudio.cpp ${ENGINE_DIR}/null/NullKeyboard.cpp diff --git a/src/engine/renderer/gl_shader.cpp b/src/engine/renderer/gl_shader.cpp index 09a56c79d8..c282f3c291 100644 --- a/src/engine/renderer/gl_shader.cpp +++ b/src/engine/renderer/gl_shader.cpp @@ -2128,14 +2128,14 @@ 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 = PAD( size, 4 ) - size; GLUniform* tmpUniform = _materialSystemUniforms[0]; tmp.emplace_back( _materialSystemUniforms[0] ); _materialSystemUniforms.erase( _materialSystemUniforms.begin() ); int uniform; - while ( ( alignmentConsume & 3 ) && _materialSystemUniforms.size() + while ( alignmentConsume && _materialSystemUniforms.size() && ( uniform = FindUniformForAlignment( _materialSystemUniforms, alignmentConsume ) ) != -1 ) { alignmentConsume -= _materialSystemUniforms[uniform]->_std430Size; diff --git a/src/engine/renderer/gl_shader_test.cpp b/src/engine/renderer/gl_shader_test.cpp new file mode 100644 index 0000000000..1d117ec30d --- /dev/null +++ b/src/engine/renderer/gl_shader_test.cpp @@ -0,0 +1,76 @@ +/* +=========================================================================== +Daemon BSD Source Code +Copyright (c) 2025, Daemon Developers +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are met: + * Redistributions of source code must retain the above copyright + notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above copyright + notice, this list of conditions and the following disclaimer in the + documentation and/or other materials provided with the distribution. + * Neither the name of the Daemon developers nor the + names of its contributors may be used to endorse or promote products + derived from this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL DAEMON DEVELOPERS BE LIABLE FOR ANY +DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +=========================================================================== +*/ + +#include + +#include "common/Common.h" + +#include "engine/renderer/gl_shader.h" + +namespace { + +class MaterialUniformPackingTestShaderBase : public GLShader +{ +public: + MaterialUniformPackingTestShaderBase() + : GLShader("MaterialUniformPackingTestShaderBase", 0, true, "", "") {} + + std::vector GetUniforms() + { + PostProcessUniforms(); + return _materialSystemUniforms; + } +}; + +template +GLUniform* Get(ShaderT& shader) +{ + // assume U has a single base class of GLUniform + return reinterpret_cast(static_cast(&shader)); +} + +TEST(MaterialUniformPackingTest, OneMatrix) +{ + class Shader1 : public MaterialUniformPackingTestShaderBase, + public u_ModelViewMatrix //mat4 + { + public: + Shader1() : u_ModelViewMatrix(this) {} + }; + + Shader1 shader1; + std::vector uniforms = shader1.GetUniforms(); + EXPECT_EQ(shader1.GetSTD430Size(), 16u); + ASSERT_EQ(uniforms.size(), 1); + EXPECT_EQ(uniforms[0], Get(shader1)); + EXPECT_EQ(uniforms[0]->_std430Size, 16u); +} + +} // namespace From 7246e4517ac38f30c8c0abd303055a4d33fc3443 Mon Sep 17 00:00:00 2001 From: slipher Date: Fri, 29 Aug 2025 05:10:16 -0500 Subject: [PATCH 4/7] Rewrite material system uniform SSBO packing Fix some bugs and add more unit tests. --- src/engine/renderer/gl_shader.cpp | 62 ++++++++++---------------- src/engine/renderer/gl_shader_test.cpp | 49 ++++++++++++++++++++ 2 files changed, 72 insertions(+), 39 deletions(-) diff --git a/src/engine/renderer/gl_shader.cpp b/src/engine/renderer/gl_shader.cpp index c282f3c291..33ead40d71 100644 --- a/src/engine/renderer/gl_shader.cpp +++ b/src/engine/renderer/gl_shader.cpp @@ -2092,14 +2092,14 @@ GLint GLShader::GetUniformLocation( const GLchar *uniformName ) const { return glGetUniformLocation( p->id, uniformName ); } -static int FindUniformForAlignment( std::vector& uniforms, const GLuint alignment ) { - for ( uint32_t i = 0; i < uniforms.size(); i++ ) { - if ( uniforms[i]->_std430Size <= alignment ) { - return i; +static auto FindUniformForOffset( std::vector& uniforms, const GLuint baseOffset ) { + for ( auto it = uniforms.begin(); it != uniforms.end(); ++it ) { + if ( 0 == ( ( (*it)->_std430Alignment - 1 ) & baseOffset ) ) { + return it; } } - return -1; + return uniforms.end(); } // Compute std430 size/alignment and sort uniforms from highest to lowest alignment @@ -2108,52 +2108,36 @@ void GLShader::PostProcessUniforms() { return; } + std::vector uniformQueue; for ( GLUniform* uniform : _uniforms ) { if ( !uniform->_global ) { - _materialSystemUniforms.emplace_back( uniform ); + uniformQueue.emplace_back( uniform ); } } - std::sort( _materialSystemUniforms.begin(), _materialSystemUniforms.end(), + std::stable_sort( uniformQueue.begin(), uniformQueue.end(), []( const GLUniform* lhs, const GLUniform* rhs ) { - return lhs->_std430Size > rhs->_std430Size; + return lhs->_std430Alignment > rhs->_std430Alignment; } ); // Sort uniforms from highest to lowest alignment so we don't need to pad uniforms (other than vec3s) - const uint numUniforms = _materialSystemUniforms.size(); - std::vector tmp; - while ( tmp.size() < numUniforms ) { - // Higher-alignment uniforms first to avoid wasting memory - GLuint size = _materialSystemUniforms[0]->_std430Size; - GLuint components = _materialSystemUniforms[0]->_components; - size = components ? PAD( size, 4 ) * components : size; - GLuint alignmentConsume = PAD( size, 4 ) - size; - - GLUniform* tmpUniform = _materialSystemUniforms[0]; - tmp.emplace_back( _materialSystemUniforms[0] ); - _materialSystemUniforms.erase( _materialSystemUniforms.begin() ); - - int uniform; - while ( alignmentConsume && _materialSystemUniforms.size() - && ( uniform = FindUniformForAlignment( _materialSystemUniforms, alignmentConsume ) ) != -1 ) { - alignmentConsume -= _materialSystemUniforms[uniform]->_std430Size; - - tmpUniform = _materialSystemUniforms[uniform]; - - tmp.emplace_back( _materialSystemUniforms[uniform] ); - _materialSystemUniforms.erase( _materialSystemUniforms.begin() + uniform ); - } - - if ( alignmentConsume ) { - tmpUniform->_std430Size += alignmentConsume; + GLuint align = 4; // mininum alignment since this will be used as an std140 array element + std430Size = 0; + _materialSystemUniforms.clear(); + while ( !uniformQueue.empty() || std430Size & ( align - 1 ) ) { + auto iterNext = FindUniformForOffset( uniformQueue, std430Size ); + if ( iterNext == uniformQueue.end() ) { + // add 1 unit of padding + ++std430Size; + ++_materialSystemUniforms.back()->_std430Size; + } else { + std430Size += ( *iterNext )->_std430Size; + align = std::max( align, ( *iterNext )->_std430Alignment ); + _materialSystemUniforms.push_back( *iterNext ); + uniformQueue.erase( iterNext ); } - - size = PAD( size, 4 ); - std430Size += size; } - - _materialSystemUniforms = tmp; } uint32_t GLShader::GetUniqueCompileMacros( size_t permutation, const int type ) const { diff --git a/src/engine/renderer/gl_shader_test.cpp b/src/engine/renderer/gl_shader_test.cpp index 1d117ec30d..a45e93f45e 100644 --- a/src/engine/renderer/gl_shader_test.cpp +++ b/src/engine/renderer/gl_shader_test.cpp @@ -73,4 +73,53 @@ TEST(MaterialUniformPackingTest, OneMatrix) EXPECT_EQ(uniforms[0]->_std430Size, 16u); } +TEST(MaterialUniformPackingTest, TwoFloats) +{ + class Shader1 : public MaterialUniformPackingTestShaderBase, + public u_DeformMagnitude, //float + public u_InverseGamma //float + { + public: + Shader1() : u_DeformMagnitude(this), u_InverseGamma(this) {} + }; + + Shader1 shader1; + std::vector uniforms = shader1.GetUniforms(); + EXPECT_EQ(shader1.GetSTD430Size(), 4u); + ASSERT_EQ(uniforms.size(), 2); + EXPECT_EQ(uniforms[0], Get(shader1)); + EXPECT_EQ(uniforms[0]->_std430Size, 1u); + EXPECT_EQ(uniforms[1], Get(shader1)); + EXPECT_EQ(uniforms[1]->_std430Size, 3u); +} + +TEST(MaterialUniformPackingTest, Vec3Handling) +{ + class Shader1 : public MaterialUniformPackingTestShaderBase, + public u_DeformMagnitude, //float + public u_SpecularExponent, //vec2 + public u_FogColor, //vec3 + public u_blurVec //vec3 + { + public: + Shader1() : u_DeformMagnitude(this), + u_SpecularExponent(this), + u_FogColor(this), + u_blurVec(this) {} + }; + + Shader1 shader1; + std::vector uniforms = shader1.GetUniforms(); + EXPECT_EQ(shader1.GetSTD430Size(), 12u); + ASSERT_EQ(uniforms.size(), 4); + EXPECT_EQ(uniforms[0], Get(shader1)); + EXPECT_EQ(uniforms[0]->_std430Size, 3u); + EXPECT_EQ(uniforms[1], Get(shader1)); + EXPECT_EQ(uniforms[1]->_std430Size, 1u); + EXPECT_EQ(uniforms[2], Get(shader1)); + EXPECT_EQ(uniforms[2]->_std430Size, 4u); + EXPECT_EQ(uniforms[3], Get(shader1)); + EXPECT_EQ(uniforms[3]->_std430Size, 4u); +} + } // namespace From 4a8596431801b0edc6e51f2429955aec14fbdd8f Mon Sep 17 00:00:00 2001 From: slipher Date: Fri, 29 Aug 2025 16:23:33 -0500 Subject: [PATCH 5/7] Add back padding to material uniforms struct --- src/engine/renderer/gl_shader.cpp | 6 ++++++ src/engine/renderer/gl_shader.h | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/engine/renderer/gl_shader.cpp b/src/engine/renderer/gl_shader.cpp index 33ead40d71..860aa3181a 100644 --- a/src/engine/renderer/gl_shader.cpp +++ b/src/engine/renderer/gl_shader.cpp @@ -1547,6 +1547,7 @@ std::string GLShaderManager::RemoveUniformsFromShaderText( const std::string& sh void GLShaderManager::GenerateUniformStructDefinesText( const std::vector& uniforms, const std::string& definesName, std::string& uniformStruct, std::string& uniformDefines ) { + int pad = 0; for ( GLUniform* uniform : uniforms ) { uniformStruct += " " + ( uniform->_isTexture ? "uvec2" : uniform->_type ) + " " + uniform->_name; @@ -1555,6 +1556,10 @@ void GLShaderManager::GenerateUniformStructDefinesText( const std::vector_std430Size - uniform->_std430BaseSize; p--; ) { + uniformStruct += "\tfloat _pad" + std::to_string( ++pad ) + ";\n"; + } + uniformDefines += "#define "; uniformDefines += uniform->_name; @@ -2132,6 +2137,7 @@ void GLShader::PostProcessUniforms() { ++std430Size; ++_materialSystemUniforms.back()->_std430Size; } else { + ( *iterNext )->_std430Size = ( *iterNext )->_std430BaseSize; std430Size += ( *iterNext )->_std430Size; align = std::max( align, ( *iterNext )->_std430Alignment ); _materialSystemUniforms.push_back( *iterNext ); diff --git a/src/engine/renderer/gl_shader.h b/src/engine/renderer/gl_shader.h index 01f433613a..da0b54b7c6 100644 --- a/src/engine/renderer/gl_shader.h +++ b/src/engine/renderer/gl_shader.h @@ -317,7 +317,8 @@ class GLUniform { const std::string _type; // In multiples of 4 bytes - GLuint _std430Size; + const GLuint _std430BaseSize; + GLuint _std430Size; // includes padding that depends on the other uniforms in the struct const GLuint _std430Alignment; const bool _global; // This uniform won't go into the materials UBO if true @@ -334,6 +335,7 @@ class GLUniform { const bool isTexture = false ) : _name( name ), _type( type ), + _std430BaseSize( std430Size ), _std430Size( std430Size ), _std430Alignment( std430Alignment ), _global( global ), From 5b7d7321966f59250929b50a7e942e12a9277249 Mon Sep 17 00:00:00 2001 From: slipher Date: Sat, 30 Aug 2025 12:51:10 -0500 Subject: [PATCH 6/7] Clarify function of material uniform struct packing 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. --- src/engine/renderer/Material.cpp | 2 +- src/engine/renderer/gl_shader.cpp | 23 +++++++++++++---------- src/engine/renderer/gl_shader.h | 22 ++++------------------ src/engine/renderer/gl_shader_test.cpp | 6 +++--- 4 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/engine/renderer/Material.cpp b/src/engine/renderer/Material.cpp index ee24c59751..2821ae45e3 100644 --- a/src/engine/renderer/Material.cpp +++ b/src/engine/renderer/Material.cpp @@ -1313,7 +1313,7 @@ void MaterialSystem::ProcessStage( MaterialSurface* surface, shaderStage_t* pSta material.bspSurface = surface->bspSurface; pStage->materialProcessor( &material, pStage, surface ); - pStage->paddedSize = material.shader->GetSTD430Size(); + pStage->paddedSize = material.shader->GetSTD140Size(); // HACK: Copy the shaderStage_t and MaterialSurface that we need into the material, so we can use it with glsl_restart material.refStage = pStage; diff --git a/src/engine/renderer/gl_shader.cpp b/src/engine/renderer/gl_shader.cpp index 860aa3181a..2329938584 100644 --- a/src/engine/renderer/gl_shader.cpp +++ b/src/engine/renderer/gl_shader.cpp @@ -1578,7 +1578,7 @@ void GLShaderManager::GenerateUniformStructDefinesText( const std::vectorstd430Size ) { + if ( !shader->std140Size ) { return shaderText; } @@ -1641,7 +1641,7 @@ std::string GLShaderManager::ShaderPostProcess( GLShader *shader, const std::str materialStruct += "};\n\n"; // 6 kb for materials - const uint32_t count = ( 4096 + 2048 ) / shader->GetSTD430Size(); + const uint32_t count = ( 4096 + 2048 ) / shader->GetSTD140Size(); std::string materialBlock = "layout(std140, binding = " + std::to_string( BufferBind::MATERIALS ) + ") uniform materialsUBO {\n" @@ -2084,8 +2084,8 @@ bool GLCompileMacro_USE_BSP_SURFACE::HasConflictingMacros(size_t permutation, co return false; } -uint32_t* GLUniform::WriteToBuffer( uint32_t* buffer ) { - return buffer; +uint32_t* GLUniform::WriteToBuffer( uint32_t * ) { + Sys::Error( "WriteToBuffer not implemented for GLUniform '%s'", _name ); } void GLShader::RegisterUniform( GLUniform* uniform ) { @@ -2107,7 +2107,9 @@ static auto FindUniformForOffset( std::vector& uniforms, const GLuin return uniforms.end(); } -// Compute std430 size/alignment and sort uniforms from highest to lowest alignment +// Compute std140 size/alignment and sort uniforms from highest to lowest alignment +// Note: using the std430 uniform size will give the wrong result for matrix types where +// the number of rows is not 4 void GLShader::PostProcessUniforms() { if ( !_useMaterialSystem ) { return; @@ -2128,17 +2130,18 @@ void GLShader::PostProcessUniforms() { // Sort uniforms from highest to lowest alignment so we don't need to pad uniforms (other than vec3s) GLuint align = 4; // mininum alignment since this will be used as an std140 array element - std430Size = 0; + std140Size = 0; _materialSystemUniforms.clear(); - while ( !uniformQueue.empty() || std430Size & ( align - 1 ) ) { - auto iterNext = FindUniformForOffset( uniformQueue, std430Size ); + while ( !uniformQueue.empty() || std140Size & ( align - 1 ) ) { + auto iterNext = FindUniformForOffset( uniformQueue, std140Size ); if ( iterNext == uniformQueue.end() ) { // add 1 unit of padding - ++std430Size; + ++std140Size; ++_materialSystemUniforms.back()->_std430Size; } else { + ASSERT_EQ( 0, ( *iterNext )->_components ); // array handling not implemented ( *iterNext )->_std430Size = ( *iterNext )->_std430BaseSize; - std430Size += ( *iterNext )->_std430Size; + std140Size += ( *iterNext )->_std430Size; align = std::max( align, ( *iterNext )->_std430Alignment ); _materialSystemUniforms.push_back( *iterNext ); uniformQueue.erase( iterNext ); diff --git a/src/engine/renderer/gl_shader.h b/src/engine/renderer/gl_shader.h index da0b54b7c6..293651fd9a 100644 --- a/src/engine/renderer/gl_shader.h +++ b/src/engine/renderer/gl_shader.h @@ -185,7 +185,7 @@ class GLShader { const bool hasFragmentShader; const bool hasComputeShader; - GLuint std430Size = 0; + GLuint std140Size = 0; const bool worldShader; protected: @@ -300,8 +300,8 @@ class GLShader { _vertexAttribs &= ~bit; } - GLuint GetSTD430Size() const { - return std430Size; + GLuint GetSTD140Size() const { + return std140Size; } bool UseMaterialSystem() const { @@ -317,6 +317,7 @@ class GLUniform { const std::string _type; // In multiples of 4 bytes + // FIXME: the uniform structs are actually std140 so it would be more relevant to provide std140 info const GLuint _std430BaseSize; GLuint _std430Size; // includes padding that depends on the other uniforms in the struct const GLuint _std430Alignment; @@ -690,11 +691,6 @@ class GLUniform1Bool : protected GLUniform { return sizeof( int ); } - uint32_t* WriteToBuffer( uint32_t *buffer ) override { - memcpy( buffer, ¤tValue, sizeof( bool ) ); - return buffer + _std430Size; - } - private: int currentValue = 0; }; @@ -772,11 +768,6 @@ class GLUniform1fv : protected GLUniform glUniform1fv( p->uniformLocations[ _locationIndex ], numFloats, f ); } - uint32_t* WriteToBuffer( uint32_t* buffer ) override { - memcpy( buffer, currentValue.data(), currentValue.size() * sizeof( float ) ); - return buffer + _components * _std430Size; - } - private: std::vector currentValue; }; @@ -1044,11 +1035,6 @@ class GLUniformMatrix32f : protected GLUniform { return 6 * sizeof( float ); } - uint32_t* WriteToBuffer( uint32_t* buffer ) override { - memcpy( buffer, currentValue, 6 * sizeof( float ) ); - return buffer + _std430Size * _components; - } - private: vec_t currentValue[6] {}; }; diff --git a/src/engine/renderer/gl_shader_test.cpp b/src/engine/renderer/gl_shader_test.cpp index a45e93f45e..f811cfdaac 100644 --- a/src/engine/renderer/gl_shader_test.cpp +++ b/src/engine/renderer/gl_shader_test.cpp @@ -67,7 +67,7 @@ TEST(MaterialUniformPackingTest, OneMatrix) Shader1 shader1; std::vector uniforms = shader1.GetUniforms(); - EXPECT_EQ(shader1.GetSTD430Size(), 16u); + EXPECT_EQ(shader1.GetSTD140Size(), 16u); ASSERT_EQ(uniforms.size(), 1); EXPECT_EQ(uniforms[0], Get(shader1)); EXPECT_EQ(uniforms[0]->_std430Size, 16u); @@ -85,7 +85,7 @@ TEST(MaterialUniformPackingTest, TwoFloats) Shader1 shader1; std::vector uniforms = shader1.GetUniforms(); - EXPECT_EQ(shader1.GetSTD430Size(), 4u); + EXPECT_EQ(shader1.GetSTD140Size(), 4u); ASSERT_EQ(uniforms.size(), 2); EXPECT_EQ(uniforms[0], Get(shader1)); EXPECT_EQ(uniforms[0]->_std430Size, 1u); @@ -110,7 +110,7 @@ TEST(MaterialUniformPackingTest, Vec3Handling) Shader1 shader1; std::vector uniforms = shader1.GetUniforms(); - EXPECT_EQ(shader1.GetSTD430Size(), 12u); + EXPECT_EQ(shader1.GetSTD140Size(), 12u); ASSERT_EQ(uniforms.size(), 4); EXPECT_EQ(uniforms[0], Get(shader1)); EXPECT_EQ(uniforms[0]->_std430Size, 3u); From 4b874042f4c637c814170f841365211735937fe6 Mon Sep 17 00:00:00 2001 From: slipher Date: Sun, 31 Aug 2025 17:15:27 -0500 Subject: [PATCH 7/7] Material uniform packing: add back arrays --- src/engine/renderer/gl_shader.cpp | 9 +++++++-- src/engine/renderer/gl_shader_test.cpp | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/engine/renderer/gl_shader.cpp b/src/engine/renderer/gl_shader.cpp index 2329938584..6bb6af97fc 100644 --- a/src/engine/renderer/gl_shader.cpp +++ b/src/engine/renderer/gl_shader.cpp @@ -2136,12 +2136,17 @@ void GLShader::PostProcessUniforms() { auto iterNext = FindUniformForOffset( uniformQueue, std140Size ); if ( iterNext == uniformQueue.end() ) { // add 1 unit of padding + ASSERT( !( *iterNext )->_components ); // array WriteToBuffer impls don't handle padding correctly ++std140Size; ++_materialSystemUniforms.back()->_std430Size; } else { - ASSERT_EQ( 0, ( *iterNext )->_components ); // array handling not implemented ( *iterNext )->_std430Size = ( *iterNext )->_std430BaseSize; - std140Size += ( *iterNext )->_std430Size; + if ( ( *iterNext )->_components ) { + ASSERT_GE( ( *iterNext )->_std430Alignment, 4 ); // these would need extra padding in a std130 array + std140Size += ( *iterNext )->_std430Size * ( *iterNext )->_components; + } else { + std140Size += ( *iterNext )->_std430Size; + } align = std::max( align, ( *iterNext )->_std430Alignment ); _materialSystemUniforms.push_back( *iterNext ); uniformQueue.erase( iterNext ); diff --git a/src/engine/renderer/gl_shader_test.cpp b/src/engine/renderer/gl_shader_test.cpp index f811cfdaac..359bf8057d 100644 --- a/src/engine/renderer/gl_shader_test.cpp +++ b/src/engine/renderer/gl_shader_test.cpp @@ -122,4 +122,21 @@ TEST(MaterialUniformPackingTest, Vec3Handling) EXPECT_EQ(uniforms[3]->_std430Size, 4u); } +TEST(MaterialUniformPackingTest, Array) +{ + class Shader1 : public MaterialUniformPackingTestShaderBase, + public u_Frustum //vec4[6] + { + public: + Shader1() : u_Frustum(this) {} + }; + + Shader1 shader1; + std::vector uniforms = shader1.GetUniforms(); + EXPECT_EQ(shader1.GetSTD140Size(), 24u); + ASSERT_EQ(uniforms.size(), 1); + EXPECT_EQ(uniforms[0], Get(shader1)); + EXPECT_EQ(uniforms[0]->_std430Size, 4u); +} + } // namespace