Fix Entity parenting hierarchy

When the initial Entity backend node hierarchy is created it skips over
any non-entity nodes to ensure that Entities are only parented to
other Entities. Calling QNode::setParent breaks this when reparenting
Entities to non-entity nodes.

Fix by sending a new "parentEntityUpdated" property update that backend
Entity nodes listen for. They keep the id of their new parent and flag
the need to rebuild the entity hierarchy.  This triggers a new job to
clear the children and parents of every backend Entity, then rebuilds
the hierarchy using the stored parent ID in each Entity. This is much
more forgiving of creation/parenting ordering issues and shouldn't be
less performant because any Entity reparent was previously marking
everything dirty anyway.

Add a new test from QTBUG-73905 that creates 4 cylinders and manipulates
the parents in different ways.

Add a new test to tst_nodes to reparent a QEntity to a QNode and ensure
the entity finds it's correct QEntity parent.

Add a new test to tst_entity to ensure backend nodes correctly handle
the new parenting events.

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

10 files changed, 250 insertions, 1 deletions

diff --git a/tests/auto/core/nodes/tst_nodes.cpp b/tests/auto/core/nodes/tst_nodes.cpp
index 3f7fb4a75..193d88c83 100644
--- a/tests/auto/core/nodes/tst_nodes.cpp
+++ b/tests/auto/core/nodes/tst_nodes.cpp
@@ -81,6 +81,7 @@ private slots:
     void removingChildEntitiesFromNode();
 
     void checkConstructionSetParentMix(); // QTBUG-60612
+    void checkParentingQEntityToQNode(); // QTBUG-73905
     void checkConstructionWithParent();
     void checkConstructionWithNonRootParent(); // QTBUG-73986
     void checkConstructionAsListElement();
@@ -1079,6 +1080,72 @@ void tst_Nodes::checkConstructionSetParentMix()
     QCOMPARE(lastEvent->addedNodeId(), subTreeRoot->id());
 }
 
+void tst_Nodes::checkParentingQEntityToQNode()
+{
+    // GIVEN
+    ObserverSpy spy;
+    Qt3DCore::QScene scene;
+    QScopedPointer<MyQNode> root(new MyQNode());
+
+    // WHEN
+    root->setArbiterAndScene(&spy, &scene);
+    root->setSimulateBackendCreated(true);
+
+    // THEN
+    QVERIFY(Qt3DCore::QNodePrivate::get(root.data())->scene() != nullptr);
+
+    // WHEN
+    auto subTreeRoot = new Qt3DCore::QEntity(root.data());
+    auto childEntity = new Qt3DCore::QEntity(subTreeRoot);
+    auto childNode = new Qt3DCore::QNode(subTreeRoot);
+
+    // THEN
+    QCoreApplication::processEvents();
+
+    // Ensure first event is subTreeRoot creation
+    const Qt3DCore::QNodeCreatedChangeBasePtr firstEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!firstEvent.isNull());
+    QCOMPARE(firstEvent->subjectId(), subTreeRoot->id());
+    QCOMPARE(firstEvent->parentId(), root->id());
+
+    // Ensure 2nd event is childEntity creation
+    const Qt3DCore::QNodeCreatedChangeBasePtr secondEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!secondEvent.isNull());
+    QCOMPARE(secondEvent->subjectId(), childEntity->id());
+    QCOMPARE(secondEvent->parentId(), subTreeRoot->id());
+
+    // Ensure 3rd event is childNode creation
+    const Qt3DCore::QNodeCreatedChangeBasePtr thirdEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!thirdEvent.isNull());
+    QCOMPARE(thirdEvent->subjectId(), childNode->id());
+    QCOMPARE(thirdEvent->parentId(), subTreeRoot->id());
+
+
+    // WHEN we reparent the childEntity to the childNode (QNode)
+
+    spy.events.clear();
+    childEntity->setParent(childNode);
+    // THEN we should get
+    // - one child removed change for childEntity->subTreeRoot,
+    // - one child added change for childEntity->childNode,
+    // - and one property updated event specifying the correct QEntity parent (subTreeRoot)
+    QCOMPARE(spy.events.size(), 3);
+
+    const auto removedEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeRemovedChange>();
+    QVERIFY(!removedEvent.isNull());
+    QCOMPARE(removedEvent->subjectId(), subTreeRoot->id());
+
+    const auto addedEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>();
+    QVERIFY(!addedEvent.isNull());
+    QCOMPARE(addedEvent->subjectId(), childNode->id());
+
+    const auto parentChangeEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyUpdatedChange>();
+    QVERIFY(!parentChangeEvent.isNull());
+    QCOMPARE(parentChangeEvent->subjectId(), childEntity->id());
+    QCOMPARE(parentChangeEvent->propertyName(), "parentEntityUpdated");
+    QCOMPARE(parentChangeEvent->value().value<Qt3DCore::QNodeId>(), subTreeRoot->id());
+}
+
 void tst_Nodes::checkConstructionWithParent()
 {
     // GIVEN
diff --git a/tests/auto/render/boundingsphere/tst_boundingsphere.cpp b/tests/auto/render/boundingsphere/tst_boundingsphere.cpp
index 992e643d2..b35c6d31a 100644
--- a/tests/auto/render/boundingsphere/tst_boundingsphere.cpp
+++ b/tests/auto/render/boundingsphere/tst_boundingsphere.cpp
@@ -55,6 +55,7 @@
 #include <Qt3DRender/private/calcboundingvolumejob_p.h>
 #include <Qt3DRender/private/calcgeometrytrianglevolumes_p.h>
 #include <Qt3DRender/private/loadbufferjob_p.h>
+#include <Qt3DRender/private/updateentityhierarchyjob_p.h>
 #include <Qt3DRender/private/buffermanager_p.h>
 #include <Qt3DRender/private/geometryrenderermanager_p.h>
 #include <Qt3DRender/private/sphere_p.h>
@@ -116,6 +117,10 @@ namespace {
 
 void runRequiredJobs(Qt3DRender::TestAspect *test)
 {
+    Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+    updateEntitiesJob.setManager(test->nodeManagers());
+    updateEntitiesJob.run();
+
     Qt3DRender::Render::UpdateWorldTransformJob updateWorldTransform;
     updateWorldTransform.setRoot(test->sceneRoot());
     updateWorldTransform.run();
diff --git a/tests/auto/render/entity/tst_entity.cpp b/tests/auto/render/entity/tst_entity.cpp
index 6ad958451..123a648d6 100644
--- a/tests/auto/render/entity/tst_entity.cpp
+++ b/tests/auto/render/entity/tst_entity.cpp
@@ -151,7 +151,7 @@ private slots:
         QVERIFY(!entity.componentsUuid<EnvironmentLight>().isEmpty());
         QVERIFY(!entity.componentUuid<Armature>().isNull());
         QVERIFY(entity.isBoundingVolumeDirty());
-        QVERIFY(!entity.childrenHandles().isEmpty());
+        QVERIFY(entity.childrenHandles().isEmpty());
         QVERIFY(!entity.layerIds().isEmpty());
         QVERIFY(renderer.dirtyBits() != 0);
         bool containsAll = entity.containsComponentsOfType<Transform,
@@ -162,6 +162,7 @@ private slots:
         entity.cleanup();
 
         // THEN
+        QVERIFY(entity.parentEntityId().isNull());
         QVERIFY(entity.componentUuid<Transform>().isNull());
         QVERIFY(entity.componentUuid<CameraLens>().isNull());
         QVERIFY(entity.componentUuid<Material>().isNull());
@@ -180,6 +181,122 @@ private slots:
         QVERIFY(!containsAll);
     }
 
+    void checkRebuildingEntityHierarchy()
+    {
+        // GIVEN
+        TestRenderer renderer;
+        NodeManagers nodeManagers;
+        Qt3DCore::QEntity frontendEntityA, frontendEntityB, frontendEntityC;
+
+        auto entityCreator = [&nodeManagers, &renderer](const Qt3DCore::QEntity &frontEndEntity) {
+            Entity *entity = nodeManagers.renderNodesManager()->getOrCreateResource(frontEndEntity.id());
+            entity->setNodeManagers(&nodeManagers);
+            entity->setRenderer(&renderer);
+            return entity;
+        };
+
+        auto backendA = entityCreator(frontendEntityA);
+        auto backendB = entityCreator(frontendEntityB);
+        auto backendC = entityCreator(frontendEntityC);
+
+        // THEN
+        QVERIFY(backendA->parentEntityId().isNull());
+        QVERIFY(backendB->parentEntityId().isNull());
+        QVERIFY(backendC->parentEntityId().isNull());
+
+        QVERIFY(backendA->parent() == nullptr);
+        QVERIFY(backendB->parent() == nullptr);
+        QVERIFY(backendC->parent() == nullptr);
+
+        QVERIFY(backendA->childrenHandles().isEmpty());
+        QVERIFY(backendB->childrenHandles().isEmpty());
+        QVERIFY(backendC->childrenHandles().isEmpty());
+
+        // WHEN
+        renderer.clearDirtyBits(0);
+        QVERIFY(renderer.dirtyBits() == 0);
+
+        auto sendParentChange = [&nodeManagers](const Qt3DCore::QEntity &entity) {
+            const auto parentChange = QPropertyUpdatedChangePtr::create(entity.id());
+            parentChange->setPropertyName("parentEntityUpdated");
+            auto parent = entity.parentEntity();
+            parentChange->setValue(QVariant::fromValue(parent ? parent->id() : Qt3DCore::QNodeId()));
+
+            Entity *backendEntity = nodeManagers.renderNodesManager()->getOrCreateResource(entity.id());
+            backendEntity->sceneChangeEvent(parentChange);
+        };
+
+        // reparent B to A and C to B.
+        frontendEntityB.setParent(&frontendEntityA);
+        sendParentChange(frontendEntityB);
+        frontendEntityC.setParent(&frontendEntityB);
+        sendParentChange(frontendEntityC);
+
+        // THEN
+        QVERIFY(renderer.dirtyBits() & AbstractRenderer::EntityHierarchyDirty);
+
+        QVERIFY(backendA->parentEntityId().isNull());
+        QVERIFY(backendB->parentEntityId() == frontendEntityA.id());
+        QVERIFY(backendC->parentEntityId() == frontendEntityB.id());
+
+        QVERIFY(backendA->parent() == nullptr);
+        QVERIFY(backendB->parent() == nullptr);
+        QVERIFY(backendC->parent() == nullptr);
+
+        QVERIFY(backendA->childrenHandles().isEmpty());
+        QVERIFY(backendB->childrenHandles().isEmpty());
+        QVERIFY(backendC->childrenHandles().isEmpty());
+
+        // WHEN
+        auto rebuildHierarchy = [](Entity *backend) {
+            backend->clearEntityHierarchy();
+            backend->rebuildEntityHierarchy();
+        };
+        rebuildHierarchy(backendA);
+        rebuildHierarchy(backendB);
+        rebuildHierarchy(backendC);
+
+        // THEN
+        QVERIFY(backendA->parent() == nullptr);
+        QVERIFY(backendB->parent() == backendA);
+        QVERIFY(backendC->parent() == backendB);
+
+        QVERIFY(!backendA->childrenHandles().isEmpty());
+        QVERIFY(!backendB->childrenHandles().isEmpty());
+        QVERIFY(backendC->childrenHandles().isEmpty());
+
+        // WHEN - reparent B to null.
+        frontendEntityB.setParent(static_cast<Qt3DCore::QNode *>(nullptr));
+        sendParentChange(frontendEntityB);
+        rebuildHierarchy(backendA);
+        rebuildHierarchy(backendB);
+        rebuildHierarchy(backendC);
+
+        QVERIFY(backendA->parentEntityId().isNull());
+        QVERIFY(backendB->parentEntityId().isNull());
+        QVERIFY(backendC->parentEntityId() == frontendEntityB.id());
+
+        QVERIFY(backendA->parent() == nullptr);
+        QVERIFY(backendB->parent() == nullptr);
+        QVERIFY(backendC->parent() == backendB);
+
+        QVERIFY(backendA->childrenHandles().isEmpty());
+        QVERIFY(!backendB->childrenHandles().isEmpty());
+        QVERIFY(backendC->childrenHandles().isEmpty());
+
+        // WHEN - cleanup
+        backendA->cleanup();
+        backendB->cleanup();
+        backendC->cleanup();
+
+        // THEN
+        QVERIFY(backendA->parentEntityId().isNull());
+        QVERIFY(backendB->parentEntityId().isNull());
+        QVERIFY(backendC->parentEntityId().isNull());
+
+        QVERIFY(renderer.dirtyBits() != 0);
+    }
+
     void shouldHandleSingleComponentEvents_data()
     {
         QTest::addColumn<QComponent*>("component");
diff --git a/tests/auto/render/layerfiltering/tst_layerfiltering.cpp b/tests/auto/render/layerfiltering/tst_layerfiltering.cpp
index eeffc69b2..9b8636f49 100644
--- a/tests/auto/render/layerfiltering/tst_layerfiltering.cpp
+++ b/tests/auto/render/layerfiltering/tst_layerfiltering.cpp
@@ -33,6 +33,7 @@
 #include <Qt3DRender/private/entity_p.h>
 #include <Qt3DRender/private/filterlayerentityjob_p.h>
 #include <Qt3DRender/private/updatetreeenabledjob_p.h>
+#include <Qt3DRender/private/updateentityhierarchyjob_p.h>
 #include <Qt3DRender/qlayer.h>
 #include <Qt3DRender/qlayerfilter.h>
 #include "testaspect.h"
@@ -632,6 +633,11 @@ private Q_SLOTS:
 
         // WHEN
         Qt3DRender::Render::Entity *backendRoot = aspect->nodeManagers()->renderNodesManager()->getOrCreateResource(entitySubtree->id());
+
+        Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+        updateEntitiesJob.setManager(aspect->nodeManagers());
+        updateEntitiesJob.run();
+
         Qt3DRender::Render::UpdateTreeEnabledJob updateTreeEnabledJob;
         updateTreeEnabledJob.setRoot(backendRoot);
         updateTreeEnabledJob.run();
diff --git a/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp b/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp
index 60b60eb6e..21f75b7a4 100644
--- a/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp
+++ b/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp
@@ -47,6 +47,7 @@
 #include <Qt3DRender/private/qrenderaspect_p.h>
 #include <Qt3DRender/private/pickboundingvolumejob_p.h>
 #include <Qt3DRender/private/pickboundingvolumeutils_p.h>
+#include <Qt3DRender/private/updateentityhierarchyjob_p.h>
 #include <Qt3DRender/private/updatemeshtrianglelistjob_p.h>
 #include <Qt3DRender/private/updateworldboundingvolumejob_p.h>
 #include <Qt3DRender/private/updateworldtransformjob_p.h>
@@ -107,6 +108,10 @@ namespace {
 
 void runRequiredJobs(Qt3DRender::TestAspect *test)
 {
+    Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+    updateEntitiesJob.setManager(test->nodeManagers());
+    updateEntitiesJob.run();
+
     Qt3DRender::Render::UpdateWorldTransformJob updateWorldTransform;
     updateWorldTransform.setRoot(test->sceneRoot());
     updateWorldTransform.run();
@@ -252,6 +257,10 @@ private Q_SLOTS:
         QVERIFY(root);
         QScopedPointer<Qt3DRender::TestAspect> test(new Qt3DRender::TestAspect(root.data()));
 
+        Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+        updateEntitiesJob.setManager(test->nodeManagers());
+        updateEntitiesJob.run();
+
         // THEN
         QList<Qt3DCore::QEntity *> frontendEntities;
         frontendEntities << qobject_cast<Qt3DCore::QEntity *>(root.data()) << root->findChildren<Qt3DCore::QEntity *>();
diff --git a/tests/auto/render/proximityfiltering/tst_proximityfiltering.cpp b/tests/auto/render/proximityfiltering/tst_proximityfiltering.cpp
index 7a5648271..7bb3c16a7 100644
--- a/tests/auto/render/proximityfiltering/tst_proximityfiltering.cpp
+++ b/tests/auto/render/proximityfiltering/tst_proximityfiltering.cpp
@@ -246,6 +246,11 @@ private Q_SLOTS:
 
         // WHEN
         Qt3DRender::Render::Entity *backendRoot = aspect->nodeManagers()->renderNodesManager()->getOrCreateResource(entitySubtree->id());
+
+        Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+        updateEntitiesJob.setManager(aspect->nodeManagers());
+        updateEntitiesJob.run();
+
         Qt3DRender::Render::UpdateTreeEnabledJob updateTreeEnabledJob;
         updateTreeEnabledJob.setRoot(backendRoot);
         updateTreeEnabledJob.run();
diff --git a/tests/auto/render/qcamera/tst_qcamera.cpp b/tests/auto/render/qcamera/tst_qcamera.cpp
index b630c447a..7aef2af7d 100644
--- a/tests/auto/render/qcamera/tst_qcamera.cpp
+++ b/tests/auto/render/qcamera/tst_qcamera.cpp
@@ -35,6 +35,7 @@
 
 #include <Qt3DCore/QEntity>
 #include <Qt3DCore/private/qnodecreatedchangegenerator_p.h>
+#include <Qt3DRender/private/updateentityhierarchyjob_p.h>
 #include <Qt3DCore/private/qaspectjobmanager_p.h>
 #include <Qt3DCore/qpropertyupdatedchange.h>
 #include <Qt3DCore/qnodecreatedchange.h>
@@ -99,6 +100,10 @@ namespace {
 
 void runRequiredJobs(Qt3DRender::TestAspect *test)
 {
+    Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+    updateEntitiesJob.setManager(test->nodeManagers());
+    updateEntitiesJob.run();
+
     Qt3DRender::Render::UpdateWorldTransformJob updateWorldTransform;
     updateWorldTransform.setRoot(test->sceneRoot());
     updateWorldTransform.run();
diff --git a/tests/auto/render/raycastingjob/tst_raycastingjob.cpp b/tests/auto/render/raycastingjob/tst_raycastingjob.cpp
index 4980bfc30..411bb9160 100644
--- a/tests/auto/render/raycastingjob/tst_raycastingjob.cpp
+++ b/tests/auto/render/raycastingjob/tst_raycastingjob.cpp
@@ -51,6 +51,7 @@
 #include <Qt3DRender/private/updateworldtransformjob_p.h>
 #include <Qt3DRender/private/expandboundingvolumejob_p.h>
 #include <Qt3DRender/private/calcboundingvolumejob_p.h>
+#include <Qt3DRender/private/updateentityhierarchyjob_p.h>
 #include <Qt3DRender/private/calcgeometrytrianglevolumes_p.h>
 #include <Qt3DRender/private/loadbufferjob_p.h>
 #include <Qt3DRender/private/buffermanager_p.h>
@@ -106,6 +107,10 @@ namespace {
 
 void runRequiredJobs(Qt3DRender::TestAspect *test)
 {
+    Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+    updateEntitiesJob.setManager(test->nodeManagers());
+    updateEntitiesJob.run();
+
     Qt3DRender::Render::UpdateWorldTransformJob updateWorldTransform;
     updateWorldTransform.setRoot(test->sceneRoot());
     updateWorldTransform.run();
diff --git a/tests/auto/render/renderer/tst_renderer.cpp b/tests/auto/render/renderer/tst_renderer.cpp
index 9321f5303..e67e0ed55 100644
--- a/tests/auto/render/renderer/tst_renderer.cpp
+++ b/tests/auto/render/renderer/tst_renderer.cpp
@@ -252,6 +252,30 @@ private Q_SLOTS:
         renderQueue->reset();
 
         // WHEN
+        renderer.markDirty(Qt3DRender::Render::AbstractRenderer::EntityHierarchyDirty, nullptr);
+        jobs = renderer.renderBinJobs();
+
+        // THEN
+        QCOMPARE(jobs.size(),
+                 1 + // EntityEnabledDirty
+                 1 + // EntityHierarchyJob
+                 1 + // WorldTransformJob
+                 1 + // UpdateWorldBoundingVolume
+                 1 + // UpdateShaderDataTransform
+                 1 + // ExpandBoundingVolumeJob
+                 1 + // CalculateBoundingVolumeJob
+                 1 + // UpdateEntityLayersJob
+                 1 + // updateLevelOfDetailJob
+                 1 + // updateSkinningPaletteJob
+                 1 + // cleanupJob
+                 1 + // sendBufferCaptureJob
+                 singleRenderViewJobCount +
+                 layerCacheJobCount);
+
+        renderer.clearDirtyBits(Qt3DRender::Render::AbstractRenderer::AllDirty);
+        renderQueue->reset();
+
+        // WHEN
         renderer.markDirty(Qt3DRender::Render::AbstractRenderer::AllDirty, nullptr);
         jobs = renderer.renderBinJobs();
 
@@ -259,6 +283,7 @@ private Q_SLOTS:
         // and ShaderGathererJob are not added here)
         QCOMPARE(jobs.size(),
                  1 + // EntityEnabledDirty
+                 1 + // EntityHierarchyDirty
                  1 + // WorldTransformJob
                  1 + // UpdateWorldBoundingVolume
                  1 + // UpdateShaderDataTransform
diff --git a/tests/auto/render/updateshaderdatatransformjob/tst_updateshaderdatatransformjob.cpp b/tests/auto/render/updateshaderdatatransformjob/tst_updateshaderdatatransformjob.cpp
index 67ddccd9b..4bab46423 100644
--- a/tests/auto/render/updateshaderdatatransformjob/tst_updateshaderdatatransformjob.cpp
+++ b/tests/auto/render/updateshaderdatatransformjob/tst_updateshaderdatatransformjob.cpp
@@ -29,6 +29,7 @@
 #include <QtTest/QTest>
 #include <Qt3DRender/private/updateshaderdatatransformjob_p.h>
 #include <Qt3DRender/private/updateworldtransformjob_p.h>
+#include <Qt3DRender/private/updateentityhierarchyjob_p.h>
 #include <Qt3DRender/private/nodemanagers_p.h>
 #include <Qt3DRender/private/managers_p.h>
 #include <Qt3DRender/qrenderaspect.h>
@@ -88,6 +89,10 @@ namespace {
 
 void runRequiredJobs(Qt3DRender::TestAspect *test)
 {
+    Qt3DRender::Render::UpdateEntityHierarchyJob updateEntitiesJob;
+    updateEntitiesJob.setManager(test->nodeManagers());
+    updateEntitiesJob.run();
+
     Qt3DRender::Render::UpdateWorldTransformJob updateWorldTransform;
     updateWorldTransform.setRoot(test->sceneRoot());
     updateWorldTransform.run();