Fix Light Uniform handling

* OpenGL Renderer:
When we parse shaders we record standard uniform names (e.g mvp),
light uniform names and other unknown uniform names in dedicated lists.
However, when setting the light uniform values, instead of using the light uniform names lists
to check whether a value could be set or not, we were using the unknown uniform names list and
therefore, we ended up not setting the value.

* RHI Renderer:
When parsing the shaders to record uniform names, even though we iterate through uniform struct
arrays and members, we were not recording the member names as uniform names. This prevented
from ever being able to set a the value of a struct or array member uniform.
Additionally, the way we handled introspecting and uploading UBOs made up of arrays of structs
was incorrect.

Task-number: QTBUG-95650
Task-number: QTBUG-100402
Change-Id: I7e33575906c1bb376fe8059174c09cda553930e4
Reviewed-by: Mike Krus <mike.krus@kdab.com>
(cherry picked from commit 3e71f4904ddcb9cac06027a0ade35acb02a6b67e)
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

4 files changed, 44 insertions, 22 deletions

diff --git a/src/plugins/renderers/opengl/renderer/renderview.cpp b/src/plugins/renderers/opengl/renderer/renderview.cpp
index 78622366b..a97889d34 100644
--- a/src/plugins/renderers/opengl/renderer/renderview.cpp
+++ b/src/plugins/renderers/opengl/renderer/renderview.cpp
@@ -1210,11 +1210,11 @@ void RenderView::setShaderStorageValue(ShaderParameterPack &uniformPack,
 }
 
 void RenderView::setDefaultUniformBlockShaderDataValue(ShaderParameterPack &uniformPack,
-                                                       const GLShader *shader,
+                                                       const std::vector<int> &uniformsNamesIds,
                                                        ShaderData *shaderData,
                                                        const QString &structName) const
 {
-    UniformBlockValueBuilder builder(shader->uniformsNamesIds(),
+    UniformBlockValueBuilder builder(uniformsNamesIds,
                                      m_manager->shaderDataManager(),
                                      m_manager->textureManager(),
                                      m_viewMatrix);
@@ -1258,7 +1258,7 @@ void RenderView::applyParameter(const Parameter *param,
         if (uniformValue.valueType() == UniformValue::NodeId &&
                 (shaderData = m_manager->shaderDataManager()->lookupResource(*uniformValue.constData<Qt3DCore::QNodeId>())) != nullptr) {
             // Try to check if we have a struct or array matching a QShaderData parameter
-            setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader, shaderData, StringToInt::lookupString(nameId));
+            setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader->uniformsNamesIds(), shaderData, StringToInt::lookupString(nameId));
         }
         break;
     }
@@ -1414,8 +1414,8 @@ void RenderView::updateLightUniforms(RenderCommand *command, const Entity *entit
                 if (worldTransform)
                     shaderData->updateWorldTransform(*worldTransform);
 
-                setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader, shaderData, GLLights::LIGHT_STRUCT_NAMES[lightIdx]);
-                setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader, shaderData, GLLights::LIGHT_STRUCT_UNROLL_NAMES[lightIdx]);
+                setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader->lightUniformsNamesIds(), shaderData, GLLights::LIGHT_STRUCT_NAMES[lightIdx]);
+                setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader->lightUniformsNamesIds(), shaderData, GLLights::LIGHT_STRUCT_UNROLL_NAMES[lightIdx]);
                 ++lightIdx;
             }
         }
@@ -1448,7 +1448,8 @@ void RenderView::updateLightUniforms(RenderCommand *command, const Entity *entit
     if (m_environmentLight && m_environmentLight->isEnabled()) {
         ShaderData *shaderData = m_manager->shaderDataManager()->lookupResource(m_environmentLight->shaderData());
         if (shaderData) {
-            setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader, shaderData, QStringLiteral("envLight"));
+            // EnvLight isn't part of the light uniform name ids
+            setDefaultUniformBlockShaderDataValue(command->m_parameterPack, shader->uniformsNamesIds(), shaderData, QStringLiteral("envLight"));
             auto irr =
                     shaderData->properties()["irradiance"].value.value<Qt3DCore::QNodeId>();
             auto spec =
diff --git a/src/plugins/renderers/opengl/renderer/renderview_p.h b/src/plugins/renderers/opengl/renderer/renderview_p.h
index d67a04b8c..b2fb7c69f 100644
--- a/src/plugins/renderers/opengl/renderer/renderview_p.h
+++ b/src/plugins/renderers/opengl/renderer/renderview_p.h
@@ -392,7 +392,7 @@ private:
                                const ShaderStorageBlock &block,
                                const UniformValue &value) const;
     void setDefaultUniformBlockShaderDataValue(ShaderParameterPack &uniformPack,
-                                               const GLShader *shader,
+                                               const std::vector<int> &uniformsNamesIds,
                                                ShaderData *shaderData,
                                                const QString &structName) const;
     void applyParameter(const Parameter *param,
diff --git a/src/plugins/renderers/rhi/renderer/pipelineuboset.cpp b/src/plugins/renderers/rhi/renderer/pipelineuboset.cpp
index 750956b1b..54c02a8b1 100644
--- a/src/plugins/renderers/rhi/renderer/pipelineuboset.cpp
+++ b/src/plugins/renderers/rhi/renderer/pipelineuboset.cpp
@@ -620,29 +620,34 @@ void PipelineUBOSet::uploadUBOsForCommand(const RenderCommand &command,
 
         for (const RHIShader::UBO_Member &member : qAsConst(uboBlock.members)) {
             const QShaderDescription::BlockVariable &blockVariable = member.blockVariable;
+
+            // Array
             if (!blockVariable.arrayDims.empty()) {
-                if (!blockVariable.structMembers.empty()) {
-                    const int arr0 = blockVariable.arrayDims[0];
-                    for (int i = 0; i < arr0; i++) {
-                        for (const RHIShader::UBO_Member &structMember : member.structMembers) {
+                if (!blockVariable.structMembers.empty()) { // Array of structs
+                    // we treat structMembers as arrayMembers when we are dealing with an array of structs´
+                    const size_t arr0 = size_t(blockVariable.arrayDims[0]);
+                    const size_t m = std::max(arr0, member.structMembers.size());
+                    for (size_t i = 0; i < m; ++i) {
+                        const RHIShader::UBO_Member &arrayMember = member.structMembers[i];
+                        for (const RHIShader::UBO_Member &arrayStructMember : arrayMember.structMembers) {
                             uploadUniform(uniforms, ubo,
-                                          structMember,
+                                          arrayStructMember,
                                           distanceToCommand,
                                           i * blockVariable.size / arr0);
                         }
                     }
-                } else {
+                } else { // Array of scalars
                     uploadUniform(uniforms, ubo,
                                   member, distanceToCommand);
                 }
             } else {
-                if (!blockVariable.structMembers.empty()) {
+                if (!blockVariable.structMembers.empty()) { // Struct
                     for (const RHIShader::UBO_Member &structMember : member.structMembers) {
                         uploadUniform(uniforms, ubo,
                                       structMember,
                                       distanceToCommand);
                     }
-                } else {
+                } else { // Scalar
                     uploadUniform(uniforms, ubo,
                                   member, distanceToCommand);
                 }
diff --git a/src/plugins/renderers/rhi/renderer/rhishader.cpp b/src/plugins/renderers/rhi/renderer/rhishader.cpp
index e76fc2fd3..6509746ab 100644
--- a/src/plugins/renderers/rhi/renderer/rhishader.cpp
+++ b/src/plugins/renderers/rhi/renderer/rhishader.cpp
@@ -332,7 +332,14 @@ void RHIShader::recordAllUniforms(UBO_Member &uboMember,
         // We iterate through all the [l][n][m] by building [0][0][0] and incrementing
         forEachArrayAccessor(member.arrayDims, [&](const QString &str) {
             // "foo.bar[1][2]"
-            m_unqualifiedUniformNames << (fullMemberName + str);
+            const QString unqualifiedMemberName = (fullMemberName + str);
+            m_unqualifiedUniformNames << unqualifiedMemberName;
+
+            // Record as an individual uniform
+            m_uniformsNames.push_back(unqualifiedMemberName);
+            const int nameId = StringToInt::lookupId(unqualifiedMemberName);
+            m_uniformsNamesIds.push_back(nameId);
+
             // Question : does it make sense to also record foo[0], foo[0][0], etc...
             // if there are e.g. 3 dimensions ?
         });
@@ -344,20 +351,29 @@ void RHIShader::recordAllUniforms(UBO_Member &uboMember,
             m_structNamesIds.push_back(StringToInt::lookupId(m_structNames.back()));
         });
 
-        // Record the struct members
-        for (const QShaderDescription::BlockVariable& bv : member.structMembers) {
-            forEachArrayAccessor(member.arrayDims, [&] (const QString& str) {
+        // Record the array times the struct members => entry[i].struct_member
+        forEachArrayAccessor(member.arrayDims, [&] (const QString& str) {
+            UBO_Member arrayMember {StringToInt::lookupId(fullMemberName + str), {}, {}};
+            // Record all struct member into the array member[i]
+            for (const QShaderDescription::BlockVariable& bv : member.structMembers) {
                 //recordAllUniforms("baz", "foo.bar[1][2].")
                 const QString structMemberNamePrefix = fullMemberName + str + QLatin1Char('.');
                 UBO_Member innerMember {StringToInt::lookupId(structMemberNamePrefix), bv, {}};
                 recordAllUniforms(innerMember, structMemberNamePrefix);
-                uboMember.structMembers.push_back(innerMember);
-            });
-        }
+                arrayMember.structMembers.push_back(innerMember);
+            }
+            // When dealing with an array of structs, we treat structMembers as arrayMembers
+            uboMember.structMembers.push_back(arrayMember);
+        });
     } else {
         // Final member (not array or struct)
         // Replace nameId with final nameId name
         uboMember.nameId = StringToInt::lookupId(fullMemberName);
+
+        // Record as an individual uniform
+        m_uniformsNames.push_back(fullMemberName);
+        const int nameId = StringToInt::lookupId(fullMemberName);
+        m_uniformsNamesIds.push_back(nameId);
     }
 }