Dragon: Properly handle state changes

Only reset states when necessary. Combine states between commands
and view in jobs and not on the render thread.

Change-Id: I2c45a794f56d5ca220190410814a9c0893fa4b7f
Reviewed-by: Andy Nichols <andy.nichols@qt.io>

7 files changed, 48 insertions, 77 deletions

diff --git a/src/runtime/dragon/dragonactivatedsurface.cpp b/src/runtime/dragon/dragonactivatedsurface.cpp
index e485759..7086f45 100644
--- a/src/runtime/dragon/dragonactivatedsurface.cpp
+++ b/src/runtime/dragon/dragonactivatedsurface.cpp
@@ -222,9 +222,6 @@ ActivatedSurface::ActivatedSurface(QSurface *surface,
 
     // TODO we might not want to do this repeatedly, put here now because we need them in multiple places
     m_glHelper = resolveHighestOpenGLFunctions();
-
-    // TODO reconsider resettings this on every frame
-    resetMasked(0xFFFFFFFFFFFF);
 }
 
 ActivatedSurface::~ActivatedSurface()
@@ -1356,15 +1353,6 @@ QOpenGLContext *ActivatedSurface::openGLContext() const
     return m_glContext;
 }
 
-void ActivatedSurface::setCurrentStateSet(const Immutable<RenderStateSet> &ss)
-{
-//    if (ss->stateMask() == m_currentStateSet->stateMask())
-//        return;
-
-    applyStateSet(ss);
-    m_currentStateSet = ss;
-}
-
 void ActivatedSurface::applyState(const StateVariant &stateVariant)
 {
     switch (stateVariant.type) {
@@ -1544,36 +1532,29 @@ void ActivatedSurface::resetMasked(qint64 maskOfStatesToReset)
         funcs->glLineWidth(1.0f);
 }
 
-void ActivatedSurface::applyStateSet(Immutable<RenderStateSet> ss)
+RenderStateSet ActivatedSurface::applyStateSet(const RenderStateSet &previous, const RenderStateSet &next)
 {
-//    const auto &previousStates = m_currentStateSet;
+    const StateMaskSet invOurState = ~next.stateMask();
 
-//    const StateMaskSet invOurState = ~ss->stateMask();
-    //     generate a mask for each set bit in previous, where we do not have
-    //     the corresponding bit set.
-
-//    StateMaskSet stateToReset = 0;
-//    stateToReset = previousStates->stateMask() & invOurState;
-//    qCDebug(Render::RenderStates) << "previous states "
-//                                  << QString::number(previousStates->stateMask(), 2);
-//    qCDebug(Render::RenderStates) << " current states " << QString::number(ss->stateMask(), 2)
-//                                  << "inverse " << QString::number(invOurState, 2)
-//                                  << " -> states to change:  " << QString::number(stateToReset, 2);
+    // generate a mask for each set bit in previous, where we do not have
+    // the corresponding bit set.
+    StateMaskSet stateToReset = previous.stateMask() & invOurState;
 
     // TODO Reset states that aren't active in the current state set
     // TODO I don't trust that this currently checks the values of the states,
     // hence, let's reset all just to make sure for now
-//    resetMasked(stateToReset);
-    resetMasked(0xFFFFFFFFFFFF);
-
-    //     Apply states that weren't in the previous state or that have
-    //     different values
-    const QVector<StateVariant> statesToSet = ss->states();
-    for (const StateVariant &ds : statesToSet) {
-//        if (previousStates->contains(ds))
-//            continue;
+    resetMasked(stateToReset);
+
+    // Apply states that weren't in the previous state or that have
+    // different values
+    for (const StateVariant &ds : next.states()) {
+        if (previous.contains(ds)) {
+            continue;
+        }
+
         applyState(ds);
     }
+    return next;
 }
 
 // TODO consider moving to Commands
diff --git a/src/runtime/dragon/dragonactivatedsurface_p.h b/src/runtime/dragon/dragonactivatedsurface_p.h
index dbccf57..20cd3b3 100644
--- a/src/runtime/dragon/dragonactivatedsurface_p.h
+++ b/src/runtime/dragon/dragonactivatedsurface_p.h
@@ -143,8 +143,6 @@ public:
     QOpenGLContext *openGLContext() const;
     // TODO make private
     void applyState(const StateVariant &stateVariant);
-    void applyStateSet(Immutable<RenderStateSet> ss);
-    void setCurrentStateSet(const Immutable<RenderStateSet> &ss);
 
     // TODO consider moving where they are used
     GLuint createRenderTarget(Qt3DCore::QNodeId renderTargetNodeId,
@@ -164,6 +162,7 @@ public:
     DrawContext beginDrawing(bool autoSwapBuffers);
 
     void blitFramebuffer(const BlitFramebufferInfo &blitFramebufferInfo, uint defaultFboId, const MutableContainer<GLTexture> &glTextures);
+    RenderStateSet applyStateSet(const RenderStateSet &previous, const RenderStateSet &next);
 private:
     // Cached clear data
     Optional<QColor> m_clearColor;
diff --git a/src/runtime/dragon/dragonrenderer.cpp b/src/runtime/dragon/dragonrenderer.cpp
index 826f04f..0745858 100644
--- a/src/runtime/dragon/dragonrenderer.cpp
+++ b/src/runtime/dragon/dragonrenderer.cpp
@@ -141,13 +141,6 @@ Renderer::Renderer(RenderType renderType)
     if (m_renderThread)
         m_renderThread->waitForStart();
 
-    // Failsafe state set if nothing else is set
-    m_defaultRenderStateSet = m_defaultRenderStateSet.modified([](RenderStateSet *stateSet){
-        stateSet->addState(StateVariant::createState<DepthTest>(GL_LESS));
-        stateSet->addState(StateVariant::createState<CullFace>(GL_BACK));
-        stateSet->addState(StateVariant::createState<ColorMask>(true, true, true, true));
-    });
-
     m_offscreenHelper = new OffscreenSurfaceHelper(this);
     m_offscreenHelper->moveToThread(QCoreApplication::instance()->thread());
 }
@@ -371,6 +364,9 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
         }
     }
 
+    // Assume that the frame starts with the default state set
+    RenderStateSet stateSet = RenderStateSet::defaultRenderStateSet();
+
     bool preparationsComplete = false;
     // TODO use an iterator for improved performance
     for (auto *surface : surfaceViews.keys()) {
@@ -622,17 +618,11 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
             const auto &commands = frameInput.renderCommands[leafNodeId]->commands;
             const auto &cameraMatrices = frameInput.renderViewCameraMatrices[leafNodeId];
 
-//            if (view->renderStateSet->stateMask() != 0)
-//                activeSurface.setCurrentStateSet(view->renderStateSet);
-//            else
-//                activeSurface.setCurrentStateSet(m_defaultRenderStateSet);
-
-            activeSurface.setCurrentStateSet(view->renderStateSet);
+            stateSet = activeSurface.applyStateSet(stateSet, view->renderStateSet.get());
 
             activeSurface.memoryBarrier(view->memoryBarrier);
             // TODO clear colors for individual buffers
 
-            // TODO handle render state set
             // TODO consider having this function return a render target that is passed to the following functions
             activeSurface.activateRenderTarget(frame.createdRenderTargets[view->renderTargetId].frameBufferObjectId, view->attachmentPack);
 
@@ -655,7 +645,8 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
                 // TODO prepare shader uniforms
                 // TODO check if anything is missing from commands (see old prepareCommandSubmission)
                 const auto &glShader = frame.uploadedShaders[command.m_shader->peerId()];
-                activeSurface.setCurrentStateSet(command.m_renderStateSet);
+
+                stateSet = activeSurface.applyStateSet(stateSet, command.m_renderStateSet.get());
 
                 // TODO execute command submission
 
@@ -696,7 +687,11 @@ Renderer::Frame Renderer::doRender(Renderer::Frame frame)
             // TODO consider blit framebuffer info
             // TODO bind framebuffer
         }
+
+        // Make sure to reset to defaults before finishing frame
+        stateSet = activeSurface.applyStateSet(stateSet, RenderStateSet::defaultRenderStateSet());
     }
+
     frame.number += 1;
     return frame;
 }
diff --git a/src/runtime/dragon/dragonrenderer_p.h b/src/runtime/dragon/dragonrenderer_p.h
index c040819..092694d 100644
--- a/src/runtime/dragon/dragonrenderer_p.h
+++ b/src/runtime/dragon/dragonrenderer_p.h
@@ -206,8 +206,6 @@ private:
 
     QSemaphore m_waitForInitializationToBeCompleted;
 
-    Immutable<RenderStateSet> m_defaultRenderStateSet;
-
     QElapsedTimer m_elapsedTimer;
 
     OffscreenSurfaceHelper *m_offscreenHelper;
diff --git a/src/runtime/dragon/jobs/dragonrenderviewjobs.cpp b/src/runtime/dragon/jobs/dragonrenderviewjobs.cpp
index bb5d961..35f8d75 100644
--- a/src/runtime/dragon/jobs/dragonrenderviewjobs.cpp
+++ b/src/runtime/dragon/jobs/dragonrenderviewjobs.cpp
@@ -135,6 +135,7 @@ RenderViews buildRenderViews(RenderViews renderViews,
         return renderViews;
 
     for (const auto &leafNodeId : frameGraph.leafNodes) {
+        bool hasRenderStateSet = false;
         RenderView renderView;
         renderView.leafNodeId = leafNodeId;
         const auto &ancestors = frameGraph.nodes[leafNodeId]->ancestors;
@@ -314,8 +315,8 @@ RenderViews buildRenderViews(RenderViews renderViews,
 
             case FrameGraphNode::StateSet: {
                 Q_ASSERT(node.can_convert<StateSetNode>());
+                hasRenderStateSet = true;
                 const auto &stateSet = node.as<StateSetNode>();
-
                 auto renderViewStateSet = *renderView.renderStateSet;
                 for (const Qt3DCore::QNodeId &stateId : stateSet->renderStates()) {
                     Q_ASSERT(renderStates.contains(stateId));
@@ -430,13 +431,8 @@ RenderViews buildRenderViews(RenderViews renderViews,
 
         // If no render states were set, assume the following defaults
         // TODO add documentation
-        if (renderView.renderStateSet->states().size() == 0){
-            auto renderStateSet = *renderView.renderStateSet;
-            renderStateSet.addState(StateVariant::createState<DepthTest>(GL_LESS));
-            renderStateSet.addState(StateVariant::createState<CullFace>(GL_BACK));
-            renderStateSet.addState(StateVariant::createState<ColorMask>(true, true, true, true));
-            renderView.renderStateSet = renderStateSet;
-        }
+        if (!hasRenderStateSet)
+            renderView.renderStateSet = RenderStateSet::defaultRenderStateSet();
 
         // TODO do not just assume camera has been set
         renderViews[leafNodeId] = renderView;
@@ -927,8 +923,7 @@ RenderCommands buildDrawRenderCommands(RenderCommands renderCommands,
                 // RenderPass { renderStates: [] } will use the states defined by
                 // StateSet in the FrameGraph
 
-                // Copy render state from view, as we only override this
-                RenderStateSet renderStateSet = *renderView->renderStateSet;
+                RenderStateSet renderStateSet = renderView->renderStateSet.get();
 
                 if (renderPass->hasRenderStates()) {
                     for (const Qt3DCore::QNodeId &stateId : renderPass->renderStates()) {
@@ -938,11 +933,8 @@ RenderCommands buildDrawRenderCommands(RenderCommands renderCommands,
                             renderStateSet.addState(renderStateNode->impl());
                     }
 
-                    // Merge RenderPass stateset with RenderView stateset
-                    //                    renderStateSet.merge(renderView->renderStateSet);
-
-                    // TODO add back change cost
-                    //                    command.changeCost = defaultRenderState->changeCost(renderStateSet);
+                    // TODO consider adding back change cost
+                    // command.changeCost = defaultRenderState->changeCost(renderStateSet);
                 }
 
                 command.m_renderStateSet = renderStateSet;
diff --git a/src/runtime/dragon/renderstates/dragonrenderstateset.cpp b/src/runtime/dragon/renderstates/dragonrenderstateset.cpp
index bc482a9..92a210d 100644
--- a/src/runtime/dragon/renderstates/dragonrenderstateset.cpp
+++ b/src/runtime/dragon/renderstates/dragonrenderstateset.cpp
@@ -52,7 +52,8 @@ RenderStateSet::~RenderStateSet()
 template<>
 void RenderStateSet::addState<StateVariant>(const StateVariant &ds)
 {
-    m_states.push_back(ds);
+    // Adding a state overrides any existing state of the same type
+    m_states[ds.type] = ds;
     m_stateMask |= ds.type;
 }
 
@@ -90,11 +91,6 @@ StateMaskSet RenderStateSet::stateMask() const
     return m_stateMask;
 }
 
-void RenderStateSet::merge(const Immutable<RenderStateSet> &other)
-{
-    m_stateMask |= other->stateMask();
-}
-
 bool RenderStateSet::contains(const StateVariant &ds) const
 {
     // trivial reject using the state mask bits
@@ -108,6 +104,15 @@ bool RenderStateSet::contains(const StateVariant &ds) const
     return false;
 }
 
+RenderStateSet RenderStateSet::defaultRenderStateSet()
+{
+    RenderStateSet stateSet;
+    stateSet.addState(StateVariant::createState<DepthTest>(GL_LESS));
+    stateSet.addState(StateVariant::createState<CullFace>(GL_BACK));
+    stateSet.addState(StateVariant::createState<ColorMask>(true, true, true, true));
+    return stateSet;
+}
+
 } // namespace Dragon
 } // namespace Qt3DRender
 
diff --git a/src/runtime/dragon/renderstates/dragonrenderstateset_p.h b/src/runtime/dragon/renderstates/dragonrenderstateset_p.h
index 4a9657a..bf1bfcc 100644
--- a/src/runtime/dragon/renderstates/dragonrenderstateset_p.h
+++ b/src/runtime/dragon/renderstates/dragonrenderstateset_p.h
@@ -79,9 +79,8 @@ public:
     int changeCost(RenderStateSet* previousState);
 
     StateMaskSet stateMask() const;
-    void merge(const Immutable<RenderStateSet> &other);
 
-    QVector<StateVariant> states() const { return m_states; }
+    const QHash<StateMask, StateVariant> &states() const { return m_states; }
 
     /**
      * @brief contains - check if this set contains a matching piece of state
@@ -89,9 +88,11 @@ public:
      * @return
      */
     bool contains(const StateVariant &ds) const;
+
+    static RenderStateSet defaultRenderStateSet();
 private:
     StateMaskSet m_stateMask;
-    QVector<StateVariant> m_states;
+    QHash<StateMask, StateVariant> m_states;
 };
 
 template<>