Compile failing shaders once only and avoid asserting on failure

This way a material with a broken shader will fail once (and won't be
retried unless the shader code gets changed), and the application will
continue gracefully, without asserting.

Task-number: QTBUG-65936
Change-Id: I7003e8c6f7d9094280d7757c1020b485f93e3b37
Reviewed-by: Svenn-Arne Dragly <svenn-arne.dragly@qt.io>
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

4 files changed, 33 insertions, 10 deletions

diff --git a/src/render/graphicshelpers/graphicscontext.cpp b/src/render/graphicshelpers/graphicscontext.cpp
index 84163780d..0b6870c3a 100644
--- a/src/render/graphicshelpers/graphicscontext.cpp
+++ b/src/render/graphicshelpers/graphicscontext.cpp
@@ -511,14 +511,14 @@ void GraphicsContext::introspectShaderInterface(Shader *shader, QOpenGLShaderPro
 
 void GraphicsContext::loadShader(Shader *shader, ShaderManager *manager)
 {
-    QOpenGLShaderProgram *shaderProgram = m_shaderCache.getShaderProgramAndAddRef(shader->dna(), shader->peerId());
-    if (!shaderProgram) {
+    bool wasPresent = false;
+    QOpenGLShaderProgram *shaderProgram = m_shaderCache.getShaderProgramAndAddRef(shader->dna(), shader->peerId(), &wasPresent);
+    if (!shaderProgram && !wasPresent) {
         // No matching QOpenGLShader in the cache so create one
         shaderProgram = createShaderProgram(shader);
 
-        // Store in cache
-        if (shaderProgram)
-            m_shaderCache.insert(shader->dna(), shader->peerId(), shaderProgram);
+        // Store in cache (even when failed and shaderProgram is null)
+        m_shaderCache.insert(shader->dna(), shader->peerId(), shaderProgram);
     }
 
     // Ensure the Shader node knows about the program interface
diff --git a/src/render/materialsystem/shadercache.cpp b/src/render/materialsystem/shadercache.cpp
index 4ddf26799..ce29622ad 100644
--- a/src/render/materialsystem/shadercache.cpp
+++ b/src/render/materialsystem/shadercache.cpp
@@ -62,18 +62,28 @@ ShaderCache::~ShaderCache()
  *
  * \return A pointer to the shader program if it is cached, nullptr otherwise
  */
-QOpenGLShaderProgram *ShaderCache::getShaderProgramAndAddRef(ProgramDNA dna, Qt3DCore::QNodeId shaderPeerId)
+QOpenGLShaderProgram *ShaderCache::getShaderProgramAndAddRef(ProgramDNA dna, Qt3DCore::QNodeId shaderPeerId, bool *wasPresent)
 {
-    auto shaderProgram = m_programHash.value(dna, nullptr);
-    if (shaderProgram) {
+    auto shaderProgram = m_programHash.constFind(dna);
+
+    // Some callers may wish to differentiate between a result of null due to
+    // not having anything in the cache and a result of null due to the cache
+    // containing a program the shaders of which failed to compile.
+    if (wasPresent)
+        *wasPresent = shaderProgram != m_programHash.constEnd();
+
+    if (shaderProgram != m_programHash.constEnd()) {
         // Ensure we store the fact that shaderPeerId references this shader
         QMutexLocker lock(&m_refsMutex);
         QVector<Qt3DCore::QNodeId> &programRefs = m_programRefs[dna];
         auto it = std::lower_bound(programRefs.begin(), programRefs.end(), shaderPeerId);
         if (*it != shaderPeerId)
             programRefs.insert(it, shaderPeerId);
+
+        return *shaderProgram;
     }
-    return shaderProgram;
+
+    return nullptr;
 }
 
 /*!
diff --git a/src/render/materialsystem/shadercache_p.h b/src/render/materialsystem/shadercache_p.h
index 24a55876e..bda629ee5 100644
--- a/src/render/materialsystem/shadercache_p.h
+++ b/src/render/materialsystem/shadercache_p.h
@@ -71,7 +71,7 @@ class QT3DRENDERSHARED_PRIVATE_EXPORT ShaderCache
 public:
     ~ShaderCache();
 
-    QOpenGLShaderProgram *getShaderProgramAndAddRef(ProgramDNA dna, Qt3DCore::QNodeId shaderPeerId);
+    QOpenGLShaderProgram *getShaderProgramAndAddRef(ProgramDNA dna, Qt3DCore::QNodeId shaderPeerId, bool *wasPresent = nullptr);
     void insert(ProgramDNA dna, Qt3DCore::QNodeId shaderPeerId, QOpenGLShaderProgram *program);
     void removeRef(ProgramDNA dna, Qt3DCore::QNodeId shaderPeerId);
     void purge();
diff --git a/tests/auto/render/shadercache/tst_shadercache.cpp b/tests/auto/render/shadercache/tst_shadercache.cpp
index 1c70d4405..49628ef0f 100644
--- a/tests/auto/render/shadercache/tst_shadercache.cpp
+++ b/tests/auto/render/shadercache/tst_shadercache.cpp
@@ -131,6 +131,19 @@ void tst_ShaderCache::value()
     auto dnaC = ProgramDNA(54321);
     auto uncachedProgram = cache.getShaderProgramAndAddRef(dnaC, nodeIdB);
     QVERIFY(uncachedProgram == nullptr);
+
+    cache.clear();
+    // Test inserting nullptr.
+    cache.insert(dnaA, nodeIdA, nullptr);
+    bool wasPresent = false;
+    cachedProgramA = cache.getShaderProgramAndAddRef(dnaA, nodeIdA, &wasPresent);
+    QCOMPARE(wasPresent, true);
+    QCOMPARE(cachedProgramA, nullptr);
+    cache.clear();
+    // Test wasPresent==false.
+    cachedProgramB = cache.getShaderProgramAndAddRef(dnaB, nodeIdB, &wasPresent);
+    QCOMPARE(wasPresent, false);
+    QCOMPARE(cachedProgramB, nullptr);
 }
 
 void tst_ShaderCache::removeRef()