Fix node de-registration from scene on deletion/reparenting

QNodePrivate::unsetSceneHelper() used to be broken, in that it set
the m_scene of the the item that the NodeVisitor functor was executed on.
Subsequent calls for child nodes would then have a m_scene == nullptr,
and could thus not be de-registered.

This patch fixes this behavior by treating each node's m_scene separately

Task-number: QTBUG-57571
Change-Id: I779caadd6957a97514fd105cd16935c3edc9f41f
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

2 files changed, 73 insertions, 7 deletions

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index e4bb5d4a3..8e7261be0 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -392,20 +392,22 @@ void QNodePrivate::setSceneHelper(QNode *root)
     Recursively unsets and remove nodes in the subtree of base node \a root from
     the scene. Also takes care of removing Components and Entities connections.
  */
-void QNodePrivate::unsetSceneHelper(QNode *root)
+void QNodePrivate::unsetSceneHelper(QNode *node)
 {
+    QNodePrivate *nodePrivate = QNodePrivate::get(node);
+
     // We also need to handle QEntity <-> QComponent relationships removal
-    if (QComponent *c = qobject_cast<QComponent *>(root)) {
+    if (QComponent *c = qobject_cast<QComponent *>(node)) {
         const QVector<QEntity *> entities = c->entities();
         for (QEntity *entity : entities) {
-            if (m_scene)
-                m_scene->removeEntityForComponent(c->id(), entity->id());
+            if (nodePrivate->m_scene)
+                nodePrivate->m_scene->removeEntityForComponent(c->id(), entity->id());
         }
     }
 
-    if (m_scene != nullptr)
-        m_scene->removeObservable(root);
-    root->d_func()->setScene(nullptr);
+    if (nodePrivate->m_scene != nullptr)
+        nodePrivate->m_scene->removeObservable(node);
+    nodePrivate->setScene(nullptr);
 }
 
 /*!
diff --git a/tests/auto/core/qscene/tst_qscene.cpp b/tests/auto/core/qscene/tst_qscene.cpp
index eabcc96fb..561a67765 100644
--- a/tests/auto/core/qscene/tst_qscene.cpp
+++ b/tests/auto/core/qscene/tst_qscene.cpp
@@ -48,6 +48,7 @@ private slots:
     void removeObservable();
     void removeNodeObservable();
     void addChildNode();
+    void deleteChildNode();
     void removeChildNode();
     void addEntityForComponent();
     void removeEntityForComponent();
@@ -280,6 +281,69 @@ void tst_QScene::addChildNode()
     }
 }
 
+void tst_QScene::deleteChildNode()
+{
+    // GIVEN
+    Qt3DCore::QScene *scene = new Qt3DCore::QScene;
+
+    QList<Qt3DCore::QNode *> nodes1, nodes2;
+
+    Qt3DCore::QNode *root1 = new tst_Node();
+    Qt3DCore::QNode *root2 = new tst_Node();
+    Qt3DCore::QNodePrivate::get(root1)->setScene(scene);
+    Qt3DCore::QNodePrivate::get(root2)->setScene(scene);
+
+    // WHEN
+    scene->addObservable(root1);
+    scene->addObservable(root2);
+    // THEN
+    QVERIFY(scene->lookupNode(root1->id()) == root1);
+    QVERIFY(scene->lookupNode(root2->id()) == root2);
+
+    // WHEN
+    for (int i = 0; i < 10; i++) {
+        Qt3DCore::QNode *child1 = new tst_Node();
+        child1->setParent(nodes1.isEmpty() ? root1 : nodes1.last());
+        nodes1.append(child1);
+
+        Qt3DCore::QNode *child2 = new tst_Node();
+        child2->setParent(nodes2.isEmpty() ? root2 : nodes2.last());
+        nodes2.append(child2);
+    }
+    QCoreApplication::processEvents();
+
+    // THEN
+    for (Qt3DCore::QNode *n : qAsConst(nodes1)) {
+        QVERIFY(scene->lookupNode(n->id()) == n);
+    }
+    for (Qt3DCore::QNode *n : qAsConst(nodes2)) {
+        QVERIFY(scene->lookupNode(n->id()) == n);
+    }
+
+    // gather node IDs
+    Qt3DCore::QNodeIdVector root1ChildIds;
+    for (Qt3DCore::QNode *n : qAsConst(nodes1))
+        root1ChildIds << n->id();
+
+    // WHEN
+    delete root1;
+    QCoreApplication::processEvents();
+
+    // THEN
+    for (Qt3DCore::QNodeId id : qAsConst(root1ChildIds)) {
+        QVERIFY(scene->lookupNode(id) == nullptr);
+    }
+
+    // WHEN
+    nodes2.first()->setParent(static_cast<Qt3DCore::QNode*>(nullptr));
+    QCoreApplication::processEvents();
+
+    // THEN
+    for (Qt3DCore::QNode *n : qAsConst(nodes2)) {
+        QVERIFY(scene->lookupNode(n->id()) == nullptr);
+    }
+}
+
 void tst_QScene::removeChildNode()
 {
     // GIVEN