Fix OnDemand rendering with Scene3Dv5.14.0-rc2v5.14.0

When using OnDemand rendering, Scene3D would lock if nothing in
the scene were to change. By being blocked, it would also not
process jobs for other aspects (input, logics). That would prevent
things like a CameraController from running, which in turn would
make it impossible to move the camera and trigger a change in the
scene to request rendering.

Additionally, Scene3D would ignore whether the Qt3D renderer actually
needed rendering or not as it was watching on its own the changeArbiter
for changes to decide whether rendering was required or not. This would
ignore the case where Qt3D needs multiple frames to render a correct frame
(e.g loading buffers, shaders at frame n, rebuilding commands at frame n+1)

Scene3D now asks the Qt3D renderer by calling the shouldRender() function
to decide whether rendering is needed or not, in addition to watching the
changeArbiter.

Regardless of whether rendering is needed, it now let each aspect process
jobs. This ensures things like FrameAction/Input are processed.

Then, Scene3D decides whether full rendering is required or whether
it only has to be called to allow the Qt3D simulation loop to proceed for the
next frame. If the latter, it does it so as not to have QtQuick trigger a
redraw.

Change-Id: I870f773c224286d6b7ec0f6045319e51e09cbf8e
Task-number: QTBUG-80521
Reviewed-by: Mike Krus <mike.krus@kdab.com>

7 files changed, 42 insertions, 12 deletions

diff --git a/src/quick3d/imports/scene3d/scene3ditem.cpp b/src/quick3d/imports/scene3d/scene3ditem.cpp
index 5a1cec495..f0ebfbc38 100644
--- a/src/quick3d/imports/scene3d/scene3ditem.cpp
+++ b/src/quick3d/imports/scene3d/scene3ditem.cpp
@@ -433,12 +433,26 @@ void Scene3DItem::applyRootEntityChange()
 
 bool Scene3DItem::needsRender()
 {
+    // We need the dirty flag which is connected to the change arbiter
+    // receiving updates to know whether something in the scene has changed
+
+    // Ideally we would use shouldRender() alone but given that it becomes true
+    // only after the arbiter has sync the changes and might be reset before
+    // process jobs is completed, we cannot fully rely on it. It would require
+    // splitting processFrame in 2 parts.
+
+    // We only use it for cases where Qt3D render may require several loops of
+    // the simulation to fully process a frame (e.g shaders are loaded in frame
+    // n and we can only build render commands for the new shader at frame n +
+    // This is where renderer->shouldRender() comes into play as it knows
+    // whether some states remain dirty or not (even after processFrame is
+    // called)
+
     auto renderAspectPriv = static_cast<QRenderAspectPrivate*>(QRenderAspectPrivate::get(m_renderAspect));
     const bool dirty = m_dirty
             || (renderAspectPriv
                 && renderAspectPriv->m_renderer
-                && renderAspectPriv->m_renderer->settings()
-                && renderAspectPriv->m_renderer->settings()->renderPolicy() == QRenderSettings::Always);
+                && renderAspectPriv->m_renderer->shouldRender());
     m_dirty = false;
     return dirty;
 }
@@ -466,10 +480,6 @@ void Scene3DItem::onBeforeSync()
     if (!isVisible())
         return;
 
-    // Has anything in the 3D scene actually changed that requires us to render?
-    if (!needsRender())
-        return;
-
     Q_ASSERT(QThread::currentThread() == thread());
 
     // Since we are in manual mode, trigger jobs for the next frame
@@ -510,6 +520,7 @@ void Scene3DItem::onBeforeSync()
     // start rendering before this function has been called
     // We add in a safety to skip such frames as this could otherwise
     // make Qt3D enter a locked state
+    m_renderer->setSkipFrame(!needsRender());
     m_renderer->allowRender();
 
     // Note: it's too early to request an update at this point as
diff --git a/src/quick3d/imports/scene3d/scene3drenderer.cpp b/src/quick3d/imports/scene3d/scene3drenderer.cpp
index 1e322a615..fafeeedf4 100644
--- a/src/quick3d/imports/scene3d/scene3drenderer.cpp
+++ b/src/quick3d/imports/scene3d/scene3drenderer.cpp
@@ -161,6 +161,7 @@ Scene3DRenderer::Scene3DRenderer(Scene3DItem *item, Qt3DCore::QAspectEngine *asp
     , m_forceRecreate(false)
     , m_shouldRender(false)
     , m_dirtyViews(false)
+    , m_skipFrame(false)
     , m_allowRendering(0)
     , m_compositingMode(Scene3DItem::FBO)
 {
@@ -278,6 +279,19 @@ void Scene3DRenderer::beforeSynchronize()
         // We could otherwise enter a deadlock state
         if (!m_allowRendering.tryAcquire(std::max(m_allowRendering.available(), 1)))
             return;
+
+        // In the case of OnDemand rendering, we still need to get to this
+        // point to ensure we have processed jobs for all aspects.
+        // We also still need to call render() to allow proceeding with the
+        // next frame. However it won't be performing any 3d rendering at all
+        // so we do it here and return early. This prevents a costly QtQuick
+        // SceneGraph update for nothing
+        if (m_skipFrame) {
+            m_skipFrame = false;
+            static_cast<QRenderAspectPrivate*>(QRenderAspectPrivate::get(m_renderAspect))->renderSynchronous(false);
+            return;
+        }
+
         m_shouldRender = true;
 
         // Check size / multisampling
@@ -360,6 +374,11 @@ void Scene3DRenderer::setCompositingMode(Scene3DItem::CompositingMode mode)
     m_compositingMode = mode;
 }
 
+void Scene3DRenderer::setSkipFrame(bool skip)
+{
+    m_skipFrame = skip;
+}
+
 // Main Thread, Render Thread locked
 void Scene3DRenderer::setScene3DViews(const QVector<Scene3DView *> views)
 {
diff --git a/src/quick3d/imports/scene3d/scene3drenderer_p.h b/src/quick3d/imports/scene3d/scene3drenderer_p.h
index 4f3651cd3..08a2c60a3 100644
--- a/src/quick3d/imports/scene3d/scene3drenderer_p.h
+++ b/src/quick3d/imports/scene3d/scene3drenderer_p.h
@@ -87,7 +87,7 @@ public:
     void setCleanerHelper(Scene3DCleaner *cleaner);
     void allowRender();
     void setCompositingMode(Scene3DItem::CompositingMode mode);
-
+    void setSkipFrame(bool skip);
     void setScene3DViews(const QVector<Scene3DView *> views);
 
 public Q_SLOTS:
@@ -119,6 +119,7 @@ private:
     bool m_forceRecreate;
     bool m_shouldRender;
     bool m_dirtyViews;
+    bool m_skipFrame;
     QSemaphore m_allowRendering;
     Scene3DItem::CompositingMode m_compositingMode;
     QVector<Scene3DView *> m_views;
diff --git a/src/render/backend/abstractrenderer_p.h b/src/render/backend/abstractrenderer_p.h
index b618eda55..c8dd537c7 100644
--- a/src/render/backend/abstractrenderer_p.h
+++ b/src/render/backend/abstractrenderer_p.h
@@ -151,7 +151,7 @@ public:
 #if defined(QT_BUILD_INTERNAL)
     virtual void clearDirtyBits(BackendNodeDirtySet changes) = 0;
 #endif
-    virtual bool shouldRender() = 0;
+    virtual bool shouldRender() const = 0;
     virtual void skipNextFrame() = 0;
 
     virtual QVector<Qt3DCore::QAspectJobPtr> preRenderingJobs() = 0;
diff --git a/src/render/renderers/opengl/renderer/renderer.cpp b/src/render/renderers/opengl/renderer/renderer.cpp
index a530c26b0..bf9230079 100644
--- a/src/render/renderers/opengl/renderer/renderer.cpp
+++ b/src/render/renderers/opengl/renderer/renderer.cpp
@@ -1821,12 +1821,11 @@ void Renderer::clearDirtyBits(BackendNodeDirtySet changes)
 }
 #endif
 
-bool Renderer::shouldRender()
+bool Renderer::shouldRender() const
 {
     // Only render if something changed during the last frame, or the last frame
     // was not rendered successfully (or render-on-demand is disabled)
     return (m_settings->renderPolicy() == QRenderSettings::Always
-            || m_renderThread == nullptr // <==> we use Scene3D
             || m_dirtyBits.marked != 0
             || m_dirtyBits.remaining != 0
             || !m_lastFrameCorrect.loadRelaxed());
diff --git a/src/render/renderers/opengl/renderer/renderer_p.h b/src/render/renderers/opengl/renderer/renderer_p.h
index b2889bb0e..ee01754d3 100644
--- a/src/render/renderers/opengl/renderer/renderer_p.h
+++ b/src/render/renderers/opengl/renderer/renderer_p.h
@@ -207,7 +207,7 @@ public:
 #if defined(QT_BUILD_INTERNAL)
     void clearDirtyBits(BackendNodeDirtySet changes) override;
 #endif
-    bool shouldRender() override;
+    bool shouldRender() const override;
     void skipNextFrame() override;
 
     QVector<Qt3DCore::QAspectJobPtr> preRenderingJobs() override;
diff --git a/tests/auto/render/commons/testrenderer.h b/tests/auto/render/commons/testrenderer.h
index 8d27998a0..e1ee329f4 100644
--- a/tests/auto/render/commons/testrenderer.h
+++ b/tests/auto/render/commons/testrenderer.h
@@ -55,7 +55,7 @@ public:
     void doRender(bool swapBuffers) override { Q_UNUSED(swapBuffers); }
     void cleanGraphicsResources() override {}
     bool isRunning() const override { return true; }
-    bool shouldRender() override { return true; }
+    bool shouldRender() const override { return true; }
     void skipNextFrame() override {}
     QVector<Qt3DCore::QAspectJobPtr> preRenderingJobs() override { return QVector<Qt3DCore::QAspectJobPtr>(); }
     QVector<Qt3DCore::QAspectJobPtr> renderBinJobs() override { return QVector<Qt3DCore::QAspectJobPtr>(); }