ShaderData: refactoring

Only store the original properties (does not store transformed values anymore
as these can change on a per thread basis and lead to races in the
ShaderData).

Instead introduce a method to check if a property needs to be transformed and
a method which given a viewMatrix returns the transformed value. This can
then be used concurrently by each UniformBlockValueBuilder without introducing
races.

Also since building UBO from a ShaderData has been disabled since 5.7, remove
all the nested ShaderData update logic.

Unit tests updated.

Change-Id: Id0e5956b9f7d93f8d019c3d8999268fc3ad68e79
Task-number: QTBUG-54818
Reviewed-by: Kevin Ottens <kevin.ottens@kdab.com>

6 files changed, 132 insertions, 140 deletions

diff --git a/src/render/backend/renderview.cpp b/src/render/backend/renderview.cpp
index 9226f8fac..64f8bacca 100644
--- a/src/render/backend/renderview.cpp
+++ b/src/render/backend/renderview.cpp
@@ -654,10 +654,8 @@ void RenderView::setDefaultUniformBlockShaderDataValue(ShaderParameterPack &unif
     UniformBlockValueBuilder *builder = m_localData.localData();
     builder->activeUniformNamesToValue.clear();
 
-    // updates transformed properties;
-    // Fix me: this will lead to races when having multiple cameras
-    shaderData->updateViewTransform(m_data.m_viewMatrix);
-
+    // Set the view matrix to be used to transform "Transformed" properties in the ShaderData
+    builder->viewMatrix = m_data.m_viewMatrix;
     // Force to update the whole block
     builder->updatedPropertiesOnly = false;
     // Retrieve names and description of each active uniforms in the uniform block
@@ -802,6 +800,9 @@ void RenderView::setShaderAndUniforms(RenderCommand *command, RenderPass *rPass,
                         setUniformValue(command->m_parameterPack, LIGHT_COLOR_NAMES[lightIdx], QVector3D(1.0f, 1.0f, 1.0f));
                         setUniformValue(command->m_parameterPack, LIGHT_INTENSITY_NAMES[lightIdx], 0.5f);
 
+                        // There is no risk in doing that even if multithreaded
+                        // since we are sure that a shaderData is unique for a given light
+                        // and won't ever be referenced as a Component either
                         QMatrix4x4 *worldTransform = lightEntity->worldTransform();
                         if (worldTransform)
                             shaderData->updateWorldTransform(*worldTransform);
diff --git a/src/render/jobs/renderviewjobutils.cpp b/src/render/jobs/renderviewjobutils.cpp
index f81615959..760e931dd 100644
--- a/src/render/jobs/renderviewjobutils.cpp
+++ b/src/render/jobs/renderviewjobutils.cpp
@@ -428,7 +428,7 @@ UniformBlockValueBuilder::~UniformBlockValueBuilder()
 {
 }
 
-void UniformBlockValueBuilder::buildActiveUniformNameValueMapHelper(const QString &blockName, const QString &qmlPropertyName, const QVariant &value)
+void UniformBlockValueBuilder::buildActiveUniformNameValueMapHelper(ShaderData *currentShaderData, const QString &blockName, const QString &qmlPropertyName, const QVariant &value)
 {
     // In the end, values are either scalar or a scalar array
     // Composed elements (structs, structs array) are simplified into simple scalars
@@ -461,20 +461,29 @@ void UniformBlockValueBuilder::buildActiveUniformNameValueMapHelper(const QStrin
         QString varName = blockName + QLatin1Char('.') + qmlPropertyName;
         if (uniforms.contains(varName)) {
             qCDebug(Shaders) << "UBO scalar member " << varName << " set for update";
-            activeUniformNamesToValue.insert(StringToInt::lookupId(varName), value);
+
+            // If the property needs to be transformed, we transform it here as
+            // the shaderdata cannot hold transformed properties for multiple
+            // thread contexts at once
+            if (currentShaderData->isPropertyToBeTransformed(qmlPropertyName))
+                activeUniformNamesToValue.insert(StringToInt::lookupId(varName),
+                                                 currentShaderData->getTransformedProperty(qmlPropertyName, viewMatrix));
+            else
+                activeUniformNamesToValue.insert(StringToInt::lookupId(varName), value);
         }
     }
 }
 
 void UniformBlockValueBuilder::buildActiveUniformNameValueMapStructHelper(ShaderData *rShaderData, const QString &blockName, const QString &qmlPropertyName)
 {
-    const QHash<QString, QVariant> &properties = updatedPropertiesOnly ? rShaderData->updatedProperties() : rShaderData->properties();
+    const QHash<QString, QVariant> &properties = rShaderData->properties();
     QHash<QString, QVariant>::const_iterator it = properties.begin();
     const QHash<QString, QVariant>::const_iterator end = properties.end();
 
     while (it != end) {
         const auto prefix = qmlPropertyName.isEmpty() ? QLatin1String("") : QLatin1String(".");
-        buildActiveUniformNameValueMapHelper(blockName + prefix + qmlPropertyName,
+        buildActiveUniformNameValueMapHelper(rShaderData,
+                                             blockName + prefix + qmlPropertyName,
                                              it.key(),
                                              it.value());
         ++it;
diff --git a/src/render/jobs/renderviewjobutils_p.h b/src/render/jobs/renderviewjobutils_p.h
index 7f70690d7..fa133ef4c 100644
--- a/src/render/jobs/renderviewjobutils_p.h
+++ b/src/render/jobs/renderviewjobutils_p.h
@@ -55,6 +55,7 @@
 #include <Qt3DCore/qnodeid.h>
 #include <QtCore/qhash.h>
 #include <QtCore/qvariant.h>
+#include <QMatrix4x4>
 
 QT_BEGIN_NAMESPACE
 
@@ -157,7 +158,8 @@ struct Q_AUTOTEST_EXPORT UniformBlockValueBuilder
     UniformBlockValueBuilder();
     ~UniformBlockValueBuilder();
 
-    void buildActiveUniformNameValueMapHelper(const QString &blockName,
+    void buildActiveUniformNameValueMapHelper(ShaderData *currentShaderData,
+                                              const QString &blockName,
                                               const QString &qmlPropertyName,
                                               const QVariant &value);
     void buildActiveUniformNameValueMapStructHelper(ShaderData *rShaderData,
@@ -168,6 +170,7 @@ struct Q_AUTOTEST_EXPORT UniformBlockValueBuilder
     QHash<QString, ShaderUniform> uniforms;
     UniformBlockValueBuilderHash activeUniformNamesToValue;
     ShaderDataManager *shaderDataManager;
+    QMatrix4x4 viewMatrix;
 };
 
 } // namespace Render
diff --git a/src/render/materialsystem/shaderdata.cpp b/src/render/materialsystem/shaderdata.cpp
index 1f68bed13..d6873e7dc 100644
--- a/src/render/materialsystem/shaderdata.cpp
+++ b/src/render/materialsystem/shaderdata.cpp
@@ -94,7 +94,6 @@ void ShaderData::initializeFromPeer(const QNodeCreatedChangeBasePtr &change)
         const QVariant &propertyValue = entry.second;
         const QString propertyName = QString::fromLatin1(entry.first);
 
-        m_properties.insert(propertyName, propertyValue);
         m_originalProperties.insert(propertyName, propertyValue);
 
         // We check if the property is a QNodeId or QVector<QNodeId> so that we can
@@ -108,14 +107,15 @@ void ShaderData::initializeFromPeer(const QNodeCreatedChangeBasePtr &change)
         }
     }
 
-    // We look for transformed properties
-    QHash<QString, QVariant>::iterator it = m_properties.begin();
-    const QHash<QString, QVariant>::iterator end = m_properties.end();
+    // We look for transformed properties once the complete hash of
+    // originalProperties is available
+    QHash<QString, QVariant>::iterator it = m_originalProperties.begin();
+    const QHash<QString, QVariant>::iterator end = m_originalProperties.end();
 
     while (it != end) {
         if (static_cast<QMetaType::Type>(it.value().type()) == QMetaType::QVector3D) {
             // if there is a matching QShaderData::TransformType propertyTransformed
-            QVariant value = m_properties.value(it.key() + QLatin1String("Transformed"));
+            QVariant value = m_originalProperties.value(it.key() + QLatin1String("Transformed"));
             // if that's the case, we apply a space transformation to the property
             if (value.isValid() && value.type() == QVariant::Int)
                 m_transformedProperties.insert(it.key(), static_cast<TransformType>(value.toInt()));
@@ -138,38 +138,39 @@ ShaderData *ShaderData::lookupResource(QNodeId id)
 // Call by cleanup job (single thread)
 void ShaderData::clearUpdatedProperties()
 {
-    m_updatedProperties.clear();
-    const QHash<QString, QVariant>::const_iterator end = m_nestedShaderDataProperties.end();
-    QHash<QString, QVariant>::const_iterator it = m_nestedShaderDataProperties.begin();
-
-    while (it != end) {
-        if (it.value().userType() == QMetaType::QVariantList) {
-            const auto values = it.value().value<QVariantList>();
-            for (const QVariant &v : values) {
-                ShaderData *nested = lookupResource(v.value<QNodeId>());
-                if (nested != nullptr)
-                    nested->clearUpdatedProperties();
-            }
-        } else {
-            ShaderData *nested = lookupResource(it.value().value<QNodeId>());
-            if (nested != nullptr)
-                nested->clearUpdatedProperties();
-        }
-        ++it;
-    }
+    // DISABLED: Is only useful when building UBO from a ShaderData, which is disable since 5.7
+    //    const QHash<QString, QVariant>::const_iterator end = m_nestedShaderDataProperties.end();
+    //    QHash<QString, QVariant>::const_iterator it = m_nestedShaderDataProperties.begin();
+
+    //    while (it != end) {
+    //        if (it.value().userType() == QMetaType::QVariantList) {
+    //            const auto values = it.value().value<QVariantList>();
+    //            for (const QVariant &v : values) {
+    //                ShaderData *nested = lookupResource(v.value<QNodeId>());
+    //                if (nested != nullptr)
+    //                    nested->clearUpdatedProperties();
+    //            }
+    //        } else {
+    //            ShaderData *nested = lookupResource(it.value().value<QNodeId>());
+    //            if (nested != nullptr)
+    //                nested->clearUpdatedProperties();
+    //        }
+    //        ++it;
+    //    }
 }
 
 void ShaderData::cleanup(NodeManagers *managers)
 {
-    for (Qt3DCore::QNodeId id : qAsConst(m_updatedShaderData)) {
-        ShaderData *shaderData = ShaderData::lookupResource(managers, id);
-        if (shaderData)
-            shaderData->clearUpdatedProperties();
-    }
+    Q_UNUSED(managers)
+    // DISABLED: Is only useful when building UBO from a ShaderData, which is disable since 5.7
+    //    for (Qt3DCore::QNodeId id : qAsConst(m_updatedShaderData)) {
+    //        ShaderData *shaderData = ShaderData::lookupResource(managers, id);
+    //        if (shaderData)
+    //            shaderData->clearUpdatedProperties();
+    //    }
     m_updatedShaderData.clear();
 }
 
-// Called by renderview jobs (several concurrent threads)
 /*!
   \internal
   Lookup if the current ShaderData or a nested ShaderData has updated properties.
@@ -179,94 +180,38 @@ void ShaderData::cleanup(NodeManagers *managers)
   \note This needs to be performed for every top level ShaderData every time it is used.
   As we don't know if the transformed properties use the same viewMatrix for all RenderViews.
  */
-bool ShaderData::updateViewTransform(const QMatrix4x4 &viewMatrix)
-{
-    // We can't perform this only once as we don't know if we would be call as the root or a
-    // nested ShaderData
-    QMutexLocker lock(&m_mutex);
 
-    // Update transformed properties
-    // We check the matrices and decide if the transform has changed since the previous call to needsUpdate
-    if (m_viewMatrix != viewMatrix) {
-        m_viewMatrix = viewMatrix;
-        const QHash<QString, TransformType>::const_iterator transformedEnd = m_transformedProperties.end();
-        QHash<QString, TransformType>::const_iterator transformedIt = m_transformedProperties.begin();
-
-        while (transformedIt != transformedEnd) {
-            if (transformedIt.value() == ModelToEye) {
-                m_updatedProperties.insert(transformedIt.key(), m_viewMatrix * m_worldMatrix * m_originalProperties.value(transformedIt.key()).value<QVector3D>());
-                m_properties.insert(transformedIt.key(), m_viewMatrix * m_worldMatrix * m_originalProperties.value(transformedIt.key()).value<QVector3D>());
-            }
-            ++transformedIt;
-        }
-    }
-    const QHash<QString, QVariant>::const_iterator end = m_nestedShaderDataProperties.end();
-    QHash<QString, QVariant>::const_iterator it = m_nestedShaderDataProperties.begin();
+bool ShaderData::isPropertyToBeTransformed(const QString &name) const
+{
+    return m_transformedProperties.contains(name);
+}
 
-    while (it != end) {
-        const int userType = it.value().userType();
-
-        if (userType == QMetaType::QVariantList) {
-            QVariantList updatedNodes;
-            bool nestedNeedsUpdate = false;
-            const QVariantList values = variant_value<QVariantList>(it.value());
-            for (const QVariant &v : values) {
-                if (v.userType() != qNodeIdTypeId)
-                    continue;
-
-                const Qt3DCore::QNodeId nestedId = variant_value<Qt3DCore::QNodeId>(v);
-                ShaderData *nested = lookupResource(nestedId);
-                if (nested != nullptr) {
-                    // We need to add the nested nodes to the updated property list
-                    // as we need to maintain order
-                    // if node[0] doesn't need update but node[1] does,
-                    // if we only have a single element, the renderer would update element [0]
-                    nestedNeedsUpdate |= nested->updateViewTransform(viewMatrix);
-                    updatedNodes << v;
-                }
-            }
-            // Of course we only add all the nodes if at least one of the nested nodes required and update
-            if (nestedNeedsUpdate && !updatedNodes.empty())
-                m_updatedProperties.insert(it.key(), updatedNodes);
-        } else if (userType == qNodeIdTypeId) {
-            const Qt3DCore::QNodeId nestedId = variant_value<Qt3DCore::QNodeId>(it.value());
-            ShaderData *nested = lookupResource(nestedId);
-            if (nested != nullptr && nested->updateViewTransform(viewMatrix))
-                m_updatedProperties.insert(it.key(), it.value());
+QVariant ShaderData::getTransformedProperty(const QString &name, const QMatrix4x4 &viewMatrix)
+{
+    // Note protecting m_worldMatrix at this point as we assume all world updates
+    // have been performed when reaching this point
+    auto it = m_transformedProperties.find(name);
+    if (it != m_transformedProperties.end()) {
+        const TransformType transformType = it.value();
+        switch (transformType) {
+        case ModelToEye:
+            return QVariant::fromValue(viewMatrix * m_worldMatrix * m_originalProperties.value(name).value<QVector3D>());
+        case ModelToWorld:
+            return QVariant::fromValue(m_worldMatrix * m_originalProperties.value(it.key()).value<QVector3D>());
+        case ModelToWorldDirection:
+            return QVariant::fromValue((m_worldMatrix * QVector4D(m_originalProperties.value(it.key()).value<QVector3D>(), 0.0f)).toVector3D());
         }
-        ++it;
     }
-    return m_updatedProperties.size() > 0;
+    return QVariant();
 }
 
-bool ShaderData::updateWorldTransform(const QMatrix4x4 &worldMatrix)
+// Called by FramePreparationJob or by RenderView when dealing with lights
+void ShaderData::updateWorldTransform(const QMatrix4x4 &worldMatrix)
 {
-    // TODO: Factor this out into a job that populates data in the corresponding
-    // renderview or other intermediate data structure. See QTBUG-54818
     QMutexLocker lock(&m_mutex);
     if (m_worldMatrix != worldMatrix) {
         m_worldMatrix = worldMatrix;
-
-        const QHash<QString, TransformType>::const_iterator transformedEnd = m_transformedProperties.end();
-        QHash<QString, TransformType>::const_iterator transformedIt = m_transformedProperties.begin();
-
-        while (transformedIt != transformedEnd) {
-            if (transformedIt.value() == ModelToEye) {
-                m_updatedProperties.insert(transformedIt.key(), m_viewMatrix * m_worldMatrix * m_originalProperties.value(transformedIt.key()).value<QVector3D>());
-                m_properties.insert(transformedIt.key(), m_viewMatrix * m_worldMatrix * m_originalProperties.value(transformedIt.key()).value<QVector3D>());
-            } else if (transformedIt.value() == ModelToWorldDirection) {
-                auto localDirection = QVector4D(m_originalProperties.value(transformedIt.key()).value<QVector3D>(), 0.0f);
-                auto worldDirection = (m_worldMatrix * localDirection).toVector3D();
-                m_updatedProperties.insert(transformedIt.key(), worldDirection);
-                m_properties.insert(transformedIt.key(), worldDirection);
-            } else {
-                m_updatedProperties.insert(transformedIt.key(), m_worldMatrix * m_originalProperties.value(transformedIt.key()).value<QVector3D>());
-                m_properties.insert(transformedIt.key(), m_worldMatrix * m_originalProperties.value(transformedIt.key()).value<QVector3D>());
-            }
-            ++transformedIt;
-        }
     }
-    return m_updatedProperties.size() > 0;
 }
 
 // This will add the ShaderData to be cleared from updates at the end of the frame
@@ -297,19 +242,7 @@ void ShaderData::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e)
 
         // Note we aren't notified about nested QShaderData in this call
         // only scalar / vec properties
-        if (m_properties.contains(propertyName)) {
-            // If this is a Transformed property, we need to multiply against the correct
-            // matrices
-            m_originalProperties.insert(propertyName, propertyValue);
-            if (m_transformedProperties.contains(propertyName)) {
-                if (m_transformedProperties[propertyName] == ModelToEye)
-                    propertyValue = m_viewMatrix * m_worldMatrix * propertyValue.value<QVector3D>();
-                else
-                    propertyValue = m_worldMatrix * propertyValue.value<QVector3D>();
-            }
-            m_properties.insert(propertyName, propertyValue);
-            m_updatedProperties.insert(propertyName, propertyValue);
-        }
+        m_originalProperties.insert(propertyName, propertyValue);
         BackendNode::markDirty(AbstractRenderer::AllDirty);
     }
 
diff --git a/src/render/materialsystem/shaderdata_p.h b/src/render/materialsystem/shaderdata_p.h
index 46dd26faf..90568eaea 100644
--- a/src/render/materialsystem/shaderdata_p.h
+++ b/src/render/materialsystem/shaderdata_p.h
@@ -79,15 +79,16 @@ public:
     ShaderData();
     ~ShaderData();
 
-    QHash<QString, QVariant> properties() const { return m_properties; }
-    QHash<QString, QVariant> updatedProperties() const { return m_updatedProperties; }
+    QHash<QString, QVariant> properties() const { return m_originalProperties; }
 
     // Called by FramePreparationJob
-    bool updateWorldTransform(const QMatrix4x4 &worldMatrix);
+    void updateWorldTransform(const QMatrix4x4 &worldMatrix);
 
     // Call by RenderViewJob
     void markDirty();
-    bool updateViewTransform(const QMatrix4x4 &viewMatrix);
+
+    bool isPropertyToBeTransformed(const QString &name) const;
+    QVariant getTransformedProperty(const QString &name, const QMatrix4x4 &viewMatrix);
 
     // Called by FrameCleanupJob
     static void cleanup(NodeManagers *managers);
@@ -98,17 +99,18 @@ protected:
     void initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr &change) Q_DECL_OVERRIDE;
     void sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e) Q_DECL_OVERRIDE;
 
+    PropertyReaderInterfacePtr m_propertyReader;
+
     // 1 to 1 match with frontend properties, modified only by sceneChangeEvent
     QHash<QString, QVariant> m_originalProperties;
-    // 1 to 1 match with frontend properties apart from Transformed
-    // properties which contain the matrices product
-    QHash<QString, QVariant> m_properties;
-    // only updated properties, Transformed properties have the same
-    // value as in m_properties
-    QHash<QString, QVariant> m_updatedProperties;
-    PropertyReaderInterfacePtr m_propertyReader;
+
+    // Contains properties thar are of type ShaderData
     QHash<QString, QVariant> m_nestedShaderDataProperties;
+
+    // Contains property that are defined like: postionTransformed: ModelToEye
     QHash<QString, TransformType> m_transformedProperties;
+
+
     QMutex m_mutex;
     static QVector<Qt3DCore::QNodeId> m_updatedShaderData;
     QMatrix4x4 m_worldMatrix;
diff --git a/tests/auto/render/renderviewutils/tst_renderviewutils.cpp b/tests/auto/render/renderviewutils/tst_renderviewutils.cpp
index df68a61d4..b3ef1ba74 100644
--- a/tests/auto/render/renderviewutils/tst_renderviewutils.cpp
+++ b/tests/auto/render/renderviewutils/tst_renderviewutils.cpp
@@ -47,6 +47,7 @@ private Q_SLOTS:
     void topLevelStructValue_data();
     void topLevelStructValue();
     void topLevelDynamicProperties();
+    void transformedProperties();
     void shouldNotifyDynamicPropertyChanges();
 
 private:
@@ -496,6 +497,49 @@ void tst_RenderViewUtils::topLevelDynamicProperties()
              shaderData->property("array"));
 }
 
+void tst_RenderViewUtils::transformedProperties()
+{
+    // GIVEN
+    QScopedPointer<Qt3DRender::QShaderData> shaderData(new Qt3DRender::QShaderData());
+    QScopedPointer<Qt3DRender::Render::ShaderDataManager> manager(new Qt3DRender::Render::ShaderDataManager());
+
+    // WHEN
+    const QVector3D position = QVector3D(15.0f, -5.0f, 10.0f);
+    QMatrix4x4 worldMatrix;
+    QMatrix4x4 viewMatrix;
+
+    worldMatrix.translate(-3.0f, 2.0f, 7.5f);
+    viewMatrix.translate(9.0f, 6.0f, 12.0f);
+
+    shaderData->setProperty("position0", position);
+    shaderData->setProperty("position1", position);
+    shaderData->setProperty("position2", position);
+    shaderData->setProperty("position3", position);
+    shaderData->setProperty("position1Transformed", Qt3DRender::Render::ShaderData::ModelToEye);
+    shaderData->setProperty("position2Transformed", Qt3DRender::Render::ShaderData::ModelToWorld);
+    shaderData->setProperty("position3Transformed", Qt3DRender::Render::ShaderData::ModelToWorldDirection);
+    initBackendShaderData(shaderData.data(), manager.data());
+
+    // THEN
+    Qt3DRender::Render::ShaderData *backendShaderData = manager->lookupResource(shaderData->id());
+    QVERIFY(backendShaderData != nullptr);
+    QVERIFY(!backendShaderData->isPropertyToBeTransformed(QStringLiteral("position0")));
+    QVERIFY(backendShaderData->isPropertyToBeTransformed(QStringLiteral("position1")));
+    QVERIFY(backendShaderData->isPropertyToBeTransformed(QStringLiteral("position2")));
+    QVERIFY(backendShaderData->isPropertyToBeTransformed(QStringLiteral("position3")));
+
+    // WHEN
+    backendShaderData->updateWorldTransform(worldMatrix);
+    const QVector3D position1Value = backendShaderData->getTransformedProperty(QStringLiteral("position1"), viewMatrix).value<QVector3D>();
+    const QVector3D position2Value = backendShaderData->getTransformedProperty(QStringLiteral("position2"), viewMatrix).value<QVector3D>();
+    const QVector3D position3Value = backendShaderData->getTransformedProperty(QStringLiteral("position3"), viewMatrix).value<QVector3D>();
+
+    // THEN
+    QCOMPARE(position1Value, viewMatrix * worldMatrix * position);
+    QCOMPARE(position2Value, worldMatrix * position);
+    QCOMPARE(position3Value, (worldMatrix * QVector4D(position, 0.0f)).toVector3D());
+}
+
 void tst_RenderViewUtils::shouldNotifyDynamicPropertyChanges()
 {
     // GIVEN