Shader fixes

- Make sure that shaders marked for destruction are un marked from
  destruction if recreated before having been destroyed.

- When loading shaders, make sure the shader wasn't already loaded
  when loading it. This can happen is a shader is abandoned and then
  re adopted.

Change-Id: I04597479d782bc6d31e4c7f78425c02c31217c7e
Reviewed-by: Mike Krus <mike.krus@kdab.com>

6 files changed, 83 insertions, 11 deletions

diff --git a/src/plugins/renderers/opengl/graphicshelpers/graphicscontext.cpp b/src/plugins/renderers/opengl/graphicshelpers/graphicscontext.cpp
index a4152449f..f52a358ad 100644
--- a/src/plugins/renderers/opengl/graphicshelpers/graphicscontext.cpp
+++ b/src/plugins/renderers/opengl/graphicshelpers/graphicscontext.cpp
@@ -233,7 +233,7 @@ void GraphicsContext::doneCurrent()
     m_glHelper = nullptr;
 }
 
-// Called by GL Command Thread
+// Called by GraphicsContext::loadShader itself called by Renderer::updateGLResources
 GraphicsContext::ShaderCreationInfo GraphicsContext::createShaderProgram(GLShader *shader)
 {
     QOpenGLShaderProgram *shaderProgram = shader->shaderProgram();
@@ -303,14 +303,17 @@ void GraphicsContext::loadShader(Shader *shaderNode,
 
     const QVector<Qt3DCore::QNodeId> sharedShaderIds = glShaderManager->shaderIdsForProgram(glShader);
     if (sharedShaderIds.size() == 1) {
-        // Shader in the cache hasn't been loaded yet
-        glShader->setGraphicsContext(this);
-        glShader->setShaderCode(shaderNode->shaderCode());
-        const ShaderCreationInfo loadResult = createShaderProgram(glShader);
-        shaderNode->setStatus(loadResult.linkSucceeded ? QShaderProgram::Ready : QShaderProgram::Error);
-        shaderNode->setLog(loadResult.logs);
-        // Loaded in the sense we tried to load it (and maybe it failed)
-        glShader->setLoaded(true);
+        // The Shader could already be loaded if we retrieved one
+        // that had been marked for destruction
+        if (!glShader->isLoaded()) {
+            glShader->setGraphicsContext(this);
+            glShader->setShaderCode(shaderNode->shaderCode());
+            const ShaderCreationInfo loadResult = createShaderProgram(glShader);
+            shaderNode->setStatus(loadResult.linkSucceeded ? QShaderProgram::Ready : QShaderProgram::Error);
+            shaderNode->setLog(loadResult.logs);
+            // Loaded in the sense we tried to load it (and maybe it failed)
+            glShader->setLoaded(true);
+        }
     } else {
         // Find an already loaded shader that shares the same QOpenGLShaderProgram
         for (const Qt3DCore::QNodeId sharedShaderId : sharedShaderIds) {
diff --git a/src/plugins/renderers/opengl/renderer/renderer.cpp b/src/plugins/renderers/opengl/renderer/renderer.cpp
index d01f22847..af43a4fe5 100644
--- a/src/plugins/renderers/opengl/renderer/renderer.cpp
+++ b/src/plugins/renderers/opengl/renderer/renderer.cpp
@@ -1178,6 +1178,10 @@ void Renderer::reloadDirtyShaders()
                 HShader shaderHandle = m_nodesManager->shaderManager()->lookupHandle(renderPass->shaderProgram());
                 Shader *shader = m_nodesManager->shaderManager()->data(shaderHandle);
 
+                // Shader could be null if the pass doesn't reference one yet
+                if (!shader)
+                    continue;
+
                 ShaderBuilder *shaderBuilder = nullptr;
                 for (const HShaderBuilder &builderHandle : activeBuilders) {
                     ShaderBuilder *builder = m_nodesManager->shaderBuilderManager()->data(builderHandle);
@@ -1205,7 +1209,7 @@ void Renderer::reloadDirtyShaders()
                     }
                 }
 
-                if (shader != nullptr && shader->isDirty())
+                if (shader->isDirty())
                     loadShader(shader, shaderHandle);
             }
         }
diff --git a/src/render/backend/managers_p.h b/src/render/backend/managers_p.h
index db12a8a7b..d7931c239 100644
--- a/src/render/backend/managers_p.h
+++ b/src/render/backend/managers_p.h
@@ -211,6 +211,21 @@ public:
         m_shaderIdsToCleanup.push_back(id);
     }
 
+    void removeShaderIdFromIdsToCleanup(Qt3DCore::QNodeId id)
+    {
+        m_shaderIdsToCleanup.removeAll(id);
+    }
+
+    bool hasShaderIdToCleanup(Qt3DCore::QNodeId id) const
+    {
+        return m_shaderIdsToCleanup.contains(id);
+    }
+
+    QVector<Qt3DCore::QNodeId> shaderIdsToCleanup() const
+    {
+        return m_shaderIdsToCleanup;
+    }
+
     // Called by RenderThread in updateGLResources (locked)
     QVector<Qt3DCore::QNodeId> takeShaderIdsToCleanup()
     {
diff --git a/src/render/materialsystem/shader.cpp b/src/render/materialsystem/shader.cpp
index 94e30a2c0..1e8568bf4 100644
--- a/src/render/materialsystem/shader.cpp
+++ b/src/render/materialsystem/shader.cpp
@@ -193,12 +193,18 @@ ShaderFunctor::ShaderFunctor(AbstractRenderer *renderer, ShaderManager *manager)
 QBackendNode *ShaderFunctor::create(const QNodeCreatedChangeBasePtr &change) const
 {
     Shader *backend = m_shaderManager->getOrCreateResource(change->subjectId());
+    // Remove from the list of ids to destroy in case we were added to it
+    m_shaderManager->removeShaderIdFromIdsToCleanup(change->subjectId());
     backend->setRenderer(m_renderer);
     return backend;
 }
 
 QBackendNode *ShaderFunctor::get(QNodeId id) const
 {
+    // If we are marked for destruction, return nullptr so that
+    // if we were to be recreated, create would be called again
+    if (m_shaderManager->hasShaderIdToCleanup(id))
+        return nullptr;
     return m_shaderManager->lookupResource(id);
 }
 
diff --git a/src/render/materialsystem/shader_p.h b/src/render/materialsystem/shader_p.h
index 5eccc7510..7af6ffe9d 100644
--- a/src/render/materialsystem/shader_p.h
+++ b/src/render/materialsystem/shader_p.h
@@ -141,7 +141,7 @@ inline QDebug operator<<(QDebug dbg, const Shader &shader)
 }
 #endif
 
-class ShaderFunctor : public Qt3DCore::QBackendNodeMapper
+class Q_AUTOTEST_EXPORT ShaderFunctor : public Qt3DCore::QBackendNodeMapper
 {
 public:
     explicit ShaderFunctor(AbstractRenderer *renderer,
diff --git a/tests/auto/render/shader/tst_shader.cpp b/tests/auto/render/shader/tst_shader.cpp
index a1f837010..212fdff4d 100644
--- a/tests/auto/render/shader/tst_shader.cpp
+++ b/tests/auto/render/shader/tst_shader.cpp
@@ -29,6 +29,7 @@
 #include <QtTest/QTest>
 #include <qbackendnodetester.h>
 #include <Qt3DRender/private/shader_p.h>
+#include <Qt3DRender/private/managers_p.h>
 #include <Qt3DRender/qshaderprogram.h>
 #include "testrenderer.h"
 
@@ -46,6 +47,7 @@ private slots:
     void checkSetRendererDirtyOnInitialization();
     void allowToChangeShaderCode_data();
     void allowToChangeShaderCode();
+    void checkShaderManager();
 };
 
 
@@ -278,6 +280,48 @@ void tst_RenderShader::allowToChangeShaderCode()
     renderer.resetDirty();
 }
 
+void tst_RenderShader::checkShaderManager()
+{
+    // GIVEN
+    Qt3DRender::QShaderProgram shader;
+    TestRenderer renderer;
+    Qt3DRender::Render::ShaderManager manager;
+    Qt3DRender::Render::ShaderFunctor creationFunctor(&renderer, &manager);
+
+    // THEN
+    QVERIFY(manager.shaderIdsToCleanup().isEmpty());
+
+    // WHEN
+    Qt3DCore::QNodeCreatedChangeBase changeObj(&shader);
+    Qt3DCore::QNodeCreatedChangeBasePtr changePtr(&changeObj, [](Qt3DCore::QNodeCreatedChangeBase *) {});
+    auto backend = creationFunctor.create(changePtr);
+
+    // THEN
+    QVERIFY(backend != nullptr);
+    QVERIFY(manager.shaderIdsToCleanup().isEmpty());
+
+    {
+        // WHEN
+        auto sameBackend = creationFunctor.get(shader.id());
+        // THEN
+        QCOMPARE(backend, sameBackend);
+    }
+
+    // WHEN
+    creationFunctor.destroy(shader.id());
+
+    // THEN -> Should be in list of ids to remove and return null on get
+    QVERIFY(manager.hasShaderIdToCleanup(shader.id()));
+    QVERIFY(creationFunctor.get(shader.id()) == nullptr);
+
+    // WHEN -> Should be removed from list of ids to remove
+    creationFunctor.create(changePtr);
+
+    // THEN
+    QVERIFY(manager.shaderIdsToCleanup().isEmpty());
+    QCOMPARE(creationFunctor.get(shader.id()), backend);
+}
+
 QTEST_APPLESS_MAIN(tst_RenderShader)
 
 #include "tst_shader.moc"