Fix material system uniform SSBO struct packing

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

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

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

src/common/Math.h

@@ -40,7 +40,7 @@ namespace Math {
     template<typename T> 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))

src/engine/framework/Resource.h

@@ -61,7 +61,7 @@ namespace Resource {
     class Handle {
         public:
             Handle(std::shared_ptr<T> value, const Manager<T>* 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

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;

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;

src/engine/renderer/gl_shader.cpp

@@ -1545,9 +1545,9 @@ std::string GLShaderManager::RemoveUniformsFromShaderText( const std::string& sh
 	return shaderMain;
 }
 
-void GLShaderManager::GenerateUniformStructDefinesText( const std::vector<GLUniform*>& 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<GLUniform*>& 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;
 
@@ -1556,6 +1556,10 @@ void GLShaderManager::GenerateUniformStructDefinesText( const std::vector<GLUnif
 		}
 		uniformStruct += ";\n";
 
+		for (int p = uniform->_std430Size - uniform->_std430BaseSize; p--; ) {
+			uniformStruct += "\tfloat _pad" + std::to_string( ++pad ) + ";\n";
+		}
+
 		uniformDefines += "#define ";
 		uniformDefines += uniform->_name;
 
@@ -1569,18 +1573,12 @@ void GLShaderManager::GenerateUniformStructDefinesText( const std::vector<GLUnif
 		uniformDefines += "\n";
 	}
 
-	// Array of structs is aligned to the largest member of the struct
-	for ( uint32_t i = 0; i < padding; i++ ) {
-		uniformStruct += "	int uniform_padding" + std::to_string( i + paddingCount );
-		uniformStruct += ";\n";
-	}
-
 	uniformDefines += "\n";
 }
 
 // This will generate all the extra code for material system shaders
 std::string GLShaderManager::ShaderPostProcess( GLShader *shader, const std::string& shaderText, const uint32_t offset ) {
-	if ( !shader->std430Size ) {
+	if ( !shader->std140Size ) {
 		return shaderText;
 	}
 
@@ -1637,13 +1635,13 @@ std::string GLShaderManager::ShaderPostProcess( GLShader *shader, const std::str
 
 	std::string materialStruct = "\nstruct Material {\n";
 	std::string materialDefines;
-	GenerateUniformStructDefinesText( shader->_materialSystemUniforms, shader->padding,
-		0, "materials[baseInstance & 0xFFF]", materialStruct, materialDefines );
+	GenerateUniformStructDefinesText( shader->_materialSystemUniforms,
+		"materials[baseInstance & 0xFFF]", materialStruct, materialDefines );
 
 	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"
@@ -2086,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 ) {
@@ -2099,69 +2097,61 @@ GLint GLShader::GetUniformLocation( const GLchar *uniformName ) const {
 	return glGetUniformLocation( p->id, uniformName );
 }
 
-static int FindUniformForAlignment( std::vector<GLUniform*>& 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<GLUniform*>& 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
+// 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;
 	}
 
+	std::vector<GLUniform*> 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<GLUniform*> 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 = 4 - size % 4;
-
-		GLUniform* tmpUniform = _materialSystemUniforms[0];
-		tmp.emplace_back( _materialSystemUniforms[0] );
-		_materialSystemUniforms.erase( _materialSystemUniforms.begin() );
-
-		int uniform;
-		while ( ( alignmentConsume & 3 ) && _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
+	std140Size = 0;
+	_materialSystemUniforms.clear();
+	while ( !uniformQueue.empty() || std140Size & ( align - 1 ) ) {
+		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 {
+			( *iterNext )->_std430Size = ( *iterNext )->_std430BaseSize;
+			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 );
 		}
-
-		size = PAD( size, 4 );
-		std430Size += size;
-		padding = alignmentConsume;
 	}
-
-	_materialSystemUniforms = tmp;
 }
 
 uint32_t GLShader::GetUniqueCompileMacros( size_t permutation, const int type ) const {