Fix FrameGraph node parenting

Framegraph suffers from the same problem as Entities. When they are created
they pass their parent FrameGraph node, and not their parent QNode. When
reparenting them we need to make sure the same thing happens otherwise
you get backend FrameGraph nodes parented to non-framegraph nodes and
they are just dropped from backend.

Change-Id: I1b9cab2c9e869c690c4c43208e62a1044b3359a4
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

6 files changed, 57 insertions, 72 deletions

diff --git a/src/render/backend/managers.cpp b/src/render/backend/managers.cpp
index 3b1f8e910..26fd68600 100644
--- a/src/render/backend/managers.cpp
+++ b/src/render/backend/managers.cpp
@@ -72,7 +72,11 @@ FrameGraphNode* FrameGraphManager::lookupNode(Qt3DCore::QNodeId id) const
 
 void FrameGraphManager::releaseNode(Qt3DCore::QNodeId id)
 {
-    delete m_nodes.take(id);
+    auto node = m_nodes.take(id);
+    if (node) {
+        node->cleanup();
+        delete node;
+    }
 }
 
 void SkeletonManager::addDirtySkeleton(DirtyFlag dirtyFlag, HSkeleton skeletonHandle)
diff --git a/src/render/framegraph/framegraphnode.cpp b/src/render/framegraph/framegraphnode.cpp
index f2ce1a50a..47e8ec91e 100644
--- a/src/render/framegraph/framegraphnode.cpp
+++ b/src/render/framegraph/framegraphnode.cpp
@@ -40,9 +40,8 @@
 #include "framegraphnode_p.h"
 #include <Qt3DRender/private/renderer_p.h>
 #include <Qt3DRender/private/nodemanagers_p.h>
-#include <Qt3DCore/qpropertynoderemovedchange.h>
-#include <Qt3DCore/qpropertynodeaddedchange.h>
 #include <Qt3DRender/qframegraphnodecreatedchange.h>
+#include <Qt3DCore/qpropertyupdatedchange.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -101,28 +100,6 @@ void FrameGraphNode::setParentId(Qt3DCore::QNodeId parentId)
     }
 }
 
-void FrameGraphNode::appendChildId(Qt3DCore::QNodeId childId)
-{
-    if (!m_childrenIds.contains(childId)) {
-        FrameGraphNode *child = m_manager->lookupNode(childId);
-        if (child != nullptr) {
-            m_childrenIds.append(childId);
-            child->m_parentId = peerId();
-        }
-    }
-}
-
-void FrameGraphNode::removeChildId(Qt3DCore::QNodeId childId)
-{
-    if (m_childrenIds.contains(childId)) {
-        FrameGraphNode *child = m_manager->lookupNode(childId);
-        if (child != nullptr) {
-            child->m_parentId = Qt3DCore::QNodeId();
-        }
-        m_childrenIds.removeAll(childId);
-    }
-}
-
 Qt3DCore::QNodeId FrameGraphNode::parentId() const
 {
     return m_parentId;
@@ -155,17 +132,12 @@ void FrameGraphNode::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e)
 {
     switch (e->type()) {
 
-    case Qt3DCore::PropertyValueAdded: {
-       Qt3DCore::QPropertyNodeAddedChangePtr change = qSharedPointerCast<Qt3DCore::QPropertyNodeAddedChange>(e);
-        if (change->metaObject()->inherits(&QFrameGraphNode::staticMetaObject))
-            appendChildId(change->addedNodeId());
-        break;
-    }
-
-    case Qt3DCore::PropertyValueRemoved: {
-        Qt3DCore::QPropertyNodeRemovedChangePtr change = qSharedPointerCast<Qt3DCore::QPropertyNodeRemovedChange>(e);
-        if (change->metaObject()->inherits(&QFrameGraphNode::staticMetaObject))
-            removeChildId(change->removedNodeId());
+    case Qt3DCore::PropertyUpdated: {
+        auto change = qSharedPointerCast<Qt3DCore::QPropertyUpdatedChange>(e);
+        if (change->propertyName() == QByteArrayLiteral("parentFrameGraphUpdated")) {
+            auto newParent = change->value().value<Qt3DCore::QNodeId>();
+            setParentId(newParent);
+         }
         break;
     }
     default:
@@ -176,6 +148,12 @@ void FrameGraphNode::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e)
     BackendNode::sceneChangeEvent(e);
 }
 
+void FrameGraphNode::cleanup()
+{
+    setParentId({});
+}
+
+
 } // namespace Render
 } // namespace Qt3DRender
 
diff --git a/src/render/framegraph/framegraphnode_p.h b/src/render/framegraph/framegraphnode_p.h
index 19165f56f..0efd391c7 100644
--- a/src/render/framegraph/framegraphnode_p.h
+++ b/src/render/framegraph/framegraphnode_p.h
@@ -109,8 +109,6 @@ public:
     FrameGraphManager *manager() const;
 
     void setParentId(Qt3DCore::QNodeId parentId);
-    void appendChildId(Qt3DCore::QNodeId childHandle);
-    void removeChildId(Qt3DCore::QNodeId childHandle);
 
     Qt3DCore::QNodeId parentId() const;
     QVector<Qt3DCore::QNodeId> childrenIds() const;
@@ -118,6 +116,8 @@ public:
     FrameGraphNode *parent() const;
     QVector<FrameGraphNode *> children() const;
 
+    void cleanup();
+
     void sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e) override;
 
 protected:
diff --git a/src/render/framegraph/qframegraphnode.cpp b/src/render/framegraph/qframegraphnode.cpp
index 9acfd1209..d52b728a8 100644
--- a/src/render/framegraph/qframegraphnode.cpp
+++ b/src/render/framegraph/qframegraphnode.cpp
@@ -40,6 +40,7 @@
 #include "qframegraphnode.h"
 #include "qframegraphnode_p.h"
 #include <Qt3DRender/qframegraphnodecreatedchange.h>
+#include <Qt3DCore/qpropertyupdatedchange.h>
 
 #include <Qt3DCore/QNode>
 
@@ -248,9 +249,24 @@ QFrameGraphNode::QFrameGraphNode(QFrameGraphNodePrivate &dd, QNode *parent)
 
 Qt3DCore::QNodeCreatedChangeBasePtr QFrameGraphNode::createNodeCreationChange() const
 {
+    // connect to the parentChanged signal here rather than constructor because
+    // until now there's no backend node to notify when parent changes
+    connect(this, &QNode::parentChanged, this, &QFrameGraphNode::onParentChanged);
+
     return QFrameGraphNodeCreatedChangeBasePtr::create(this);
 }
 
+void QFrameGraphNode::onParentChanged(QObject *)
+{
+    const auto parentID = parentFrameGraphNode() ? parentFrameGraphNode()->id() : Qt3DCore::QNodeId();
+    auto parentChange = Qt3DCore::QPropertyUpdatedChangePtr::create(id());
+    parentChange->setPropertyName("parentFrameGraphUpdated");
+    parentChange->setValue(QVariant::fromValue(parentID));
+    const bool blocked = blockNotifications(false);
+    notifyObservers(parentChange);
+    blockNotifications(blocked);
+}
+
 } // namespace Qt3DRender
 
 QT_END_NAMESPACE
diff --git a/src/render/framegraph/qframegraphnode.h b/src/render/framegraph/qframegraphnode.h
index 1a15c4df6..57f4da181 100644
--- a/src/render/framegraph/qframegraphnode.h
+++ b/src/render/framegraph/qframegraphnode.h
@@ -63,6 +63,9 @@ protected:
     explicit QFrameGraphNode(QFrameGraphNodePrivate &dd, Qt3DCore::QNode *parent = nullptr);
     Qt3DCore::QNodeCreatedChangeBasePtr createNodeCreationChange() const override;
 
+private Q_SLOTS:
+    void onParentChanged(QObject *);
+
 private:
     Q_DECLARE_PRIVATE(QFrameGraphNode)
 };
diff --git a/tests/auto/render/framegraphnode/tst_framegraphnode.cpp b/tests/auto/render/framegraphnode/tst_framegraphnode.cpp
index 07ff4c0d9..22bd872dc 100644
--- a/tests/auto/render/framegraphnode/tst_framegraphnode.cpp
+++ b/tests/auto/render/framegraphnode/tst_framegraphnode.cpp
@@ -30,8 +30,7 @@
 #include <QtTest/QTest>
 #include <Qt3DRender/private/managers_p.h>
 #include <Qt3DRender/private/nodemanagers_p.h>
-#include <Qt3DCore/qpropertynodeaddedchange.h>
-#include <Qt3DCore/qpropertynoderemovedchange.h>
+#include <Qt3DCore/qpropertyupdatedchange.h>
 #include <Qt3DCore/private/qnodecreatedchangegenerator_p.h>
 #include "testrenderer.h"
 
@@ -108,24 +107,9 @@ private Q_SLOTS:
 
         // WHEN
         n->setParentId(parentId);
-        // THEN
-        QCOMPARE(n->parentId(), parentId);
-
-        // WHEN
-        const Qt3DCore::QNodeId childId = Qt3DCore::QNodeId::createId();
-        QScopedPointer<Qt3DRender::Render::FrameGraphNode> c(new MyFrameGraphNode());
-        setIdInternal(c.data(), childId);
-        manager->appendNode(childId, c.data());
-        n->appendChildId(childId);
-        // THEN
-        QCOMPARE(n->childrenIds().count(), 1);
 
-        // WHEN
-        n->appendChildId(childId);
         // THEN
-        QCOMPARE(n->childrenIds().count(), 1);
-
-        c.take();
+        QCOMPARE(n->parentId(), parentId);
     }
 
     void checkParentChange()
@@ -151,17 +135,8 @@ private Q_SLOTS:
         QVERIFY(child->parentId().isNull());
 
         // WHEN
-        parent1->appendChildId(childId);
-        // THEN
-        QCOMPARE(child->parentId(), parentId);
-        QCOMPARE(child->parent(), parent1);
-        QCOMPARE(parent1->childrenIds().count(), 1);
-        QCOMPARE(parent1->childrenIds().first(), childId);
-        QCOMPARE(parent1->children().count(), parent1->childrenIds().count());
-        QCOMPARE(parent1->children().first(), child);
+        child->setParentId(parentId);
 
-        // WHEN
-        parent1->appendChildId(childId);
         // THEN
         QCOMPARE(child->parentId(), parentId);
         QCOMPARE(child->parent(), parent1);
@@ -171,7 +146,8 @@ private Q_SLOTS:
         QCOMPARE(parent1->children().first(), child);
 
         // WHEN
-        parent1->removeChildId(childId);
+        child->setParentId(Qt3DCore::QNodeId());
+
         // THEN
         QVERIFY(child->parentId().isNull());
         QVERIFY(child->parent() == nullptr);
@@ -251,6 +227,7 @@ private Q_SLOTS:
         TestRenderer renderer;
 
         backendFGNode->setRenderer(&renderer);
+        backendFGChild->setRenderer(&renderer);
 
         setIdInternal(backendFGNode, fgNode1Id);
         setIdInternal(backendFGChild, frontendFGChild->id());
@@ -269,20 +246,27 @@ private Q_SLOTS:
 
         {
             // WHEN
-            const auto change = Qt3DCore::QPropertyNodeAddedChangePtr::create(Qt3DCore::QNodeId(), frontendFGChild);
-            backendFGNode->sceneChangeEvent(change);
+            renderer.clearDirtyBits(0);
+            const auto change = Qt3DCore::QPropertyUpdatedChangePtr::create(frontendFGChild->id());
+            change->setPropertyName("parentFrameGraphUpdated");
+            change->setValue(QVariant::fromValue(fgNode1Id));
+            backendFGChild->sceneChangeEvent(change);
 
             // THEN
             QCOMPARE(backendFGNode->childrenIds().size(), 1);
-            QCOMPARE(backendFGNode->childrenIds().first(), frontendFGChild->id());
+            QCOMPARE(backendFGChild->parentId(), fgNode1Id);
         }
         {
             // WHEN
-            const auto change = Qt3DCore::QPropertyNodeRemovedChangePtr::create(Qt3DCore::QNodeId(), frontendFGChild);
-            backendFGNode->sceneChangeEvent(change);
+            renderer.clearDirtyBits(0);
+            const auto change = Qt3DCore::QPropertyUpdatedChangePtr::create(frontendFGChild->id());
+            change->setPropertyName("parentFrameGraphUpdated");
+            change->setValue(QVariant::fromValue(Qt3DCore::QNodeId()));
+            backendFGChild->sceneChangeEvent(change);
 
             // THEN
             QCOMPARE(backendFGNode->childrenIds().size(), 0);
+            QVERIFY(backendFGChild->parentId().isNull());
         }
     }