ChangeArbiter/SceneChange: use id as change subject

Instead of having a QNode* or QObservable as the subject of a change, we use
it's unique NodeId instead. That will prevent the ChangeArbiter from trying to
distribute changes by looking at a QNode/QObservable id when the
QNode/QObservable might have been destroyed in the meantime.

Change-Id: Ia419d5b841434fd65522c8c65de552089cfe97cf
Task-number: QTBUG-44628
Reviewed-by: Laszlo Agocs <laszlo.agocs@theqtcompany.com>
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

11 files changed, 38 insertions, 81 deletions

diff --git a/src/core/nodes/qcomponent.cpp b/src/core/nodes/qcomponent.cpp
index a05b9980b..2827db9f9 100644
--- a/src/core/nodes/qcomponent.cpp
+++ b/src/core/nodes/qcomponent.cpp
@@ -59,7 +59,7 @@ void QComponentPrivate::addEntity(QEntity *entity)
     // We notify only if we have a QChangeArbiter
     if (m_changeArbiter != Q_NULLPTR) {
         Q_Q(QComponent);
-        QScenePropertyChangePtr e(new QScenePropertyChange(ComponentAdded, q));
+        QScenePropertyChangePtr e(new QScenePropertyChange(ComponentAdded, QSceneChange::Node, q->id()));
         e->setPropertyName("entity");
         e->setValue(QVariant::fromValue(entity->id()));
         notifyObservers(e);
@@ -71,7 +71,7 @@ void QComponentPrivate::removeEntity(QEntity *entity)
     // We notify only if we have a QChangeArbiter
     if (m_changeArbiter != Q_NULLPTR) {
         Q_Q(QComponent);
-        QScenePropertyChangePtr e(new QScenePropertyChange(ComponentRemoved, q));
+        QScenePropertyChangePtr e(new QScenePropertyChange(ComponentRemoved, QSceneChange::Node, q->id()));
         e->setPropertyName("entity");
         e->setValue(QVariant::fromValue(entity->id()));
         notifyObservers(e);
diff --git a/src/core/nodes/qentity.cpp b/src/core/nodes/qentity.cpp
index a0f980b5a..0e8c596d0 100644
--- a/src/core/nodes/qentity.cpp
+++ b/src/core/nodes/qentity.cpp
@@ -117,7 +117,7 @@ void QEntity::addComponent(QComponent *comp)
         // Sending a full fledged component in the notification as we'll need
         // to know which type of component it was and its properties to create
         // the backend object
-        QScenePropertyChangePtr propertyChange(new QScenePropertyChange(ComponentAdded, this));
+        QScenePropertyChangePtr propertyChange(new QScenePropertyChange(ComponentAdded, QSceneChange::Node, id()));
         propertyChange->setPropertyName("component");
         propertyChange->setValue(QVariant::fromValue(QNodePtr(QNode::clone(comp), &QNodePrivate::nodePtrDeleter)));
         d->notifyObservers(propertyChange);
@@ -137,7 +137,7 @@ void QEntity::removeComponent(QComponent *comp)
         // Sending just the component id as it is the only part needed to
         // cleanup the backend object. This way we avoid a clone which might
         // fail in the case of large scenes.
-        QScenePropertyChangePtr propertyChange(new QScenePropertyChange(ComponentRemoved, this));
+        QScenePropertyChangePtr propertyChange(new QScenePropertyChange(ComponentRemoved, QSceneChange::Node, id()));
         propertyChange->setValue(QVariant::fromValue(comp->id()));
         propertyChange->setPropertyName("componentId");
         d->notifyObservers(propertyChange);
diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index c4b6088e6..4de2f85a5 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -82,8 +82,7 @@ void QNodePrivate::addChild(QNode *childNode)
 
     // We notify only if we have a QChangeArbiter
     if (m_changeArbiter != Q_NULLPTR) {
-        Q_Q(QNode);
-        QScenePropertyChangePtr e(new QScenePropertyChange(NodeCreated, q));
+        QScenePropertyChangePtr e(new QScenePropertyChange(NodeCreated, QSceneChange::Node, m_id));
         e->setPropertyName("node");
         // We need to clone the parent of the childNode we send
         QNode *parentClone = QNode::clone(q_func());
@@ -109,8 +108,7 @@ void QNodePrivate::removeChild(QNode *childNode)
 
     // Notify only if child isn't a clone
     if (m_changeArbiter != Q_NULLPTR) {
-        Q_Q(QNode);
-        QScenePropertyChangePtr e(new QScenePropertyChange(NodeAboutToBeDeleted, q));
+        QScenePropertyChangePtr e(new QScenePropertyChange(NodeAboutToBeDeleted, QSceneChange::Node, m_id));
         e->setPropertyName("node");
         // We need to clone the parent of the childNode we send
         //        QNode *parentClone = QNode::clone(childNode->parentNode());
@@ -229,8 +227,7 @@ void QNodePrivate::notifyPropertyChange(const char *name, const QVariant &value)
     if (m_blockNotifications)
         return;
 
-    Q_Q(QNode);
-    QScenePropertyChangePtr e(new QScenePropertyChange(NodeUpdated, q));
+    QScenePropertyChangePtr e(new QScenePropertyChange(NodeUpdated, QSceneChange::Node, m_id));
     e->setPropertyName(name);
     e->setValue(value);
     notifyObservers(e);
diff --git a/src/core/qbackendscenepropertychange.cpp b/src/core/qbackendscenepropertychange.cpp
index cdbce641a..7d33414f7 100644
--- a/src/core/qbackendscenepropertychange.cpp
+++ b/src/core/qbackendscenepropertychange.cpp
@@ -52,8 +52,8 @@ QBackendScenePropertyChangePrivate::~QBackendScenePropertyChangePrivate()
 {
 }
 
-QBackendScenePropertyChange::QBackendScenePropertyChange(ChangeFlag type, QBackendNode *subject, QSceneChange::Priority priority)
-    : QScenePropertyChange(*new QBackendScenePropertyChangePrivate(this), type, subject->d_func(), priority)
+QBackendScenePropertyChange::QBackendScenePropertyChange(ChangeFlag type, const QNodeId &subjectId, QSceneChange::Priority priority)
+    : QScenePropertyChange(*new QBackendScenePropertyChangePrivate(this), type, Observable, subjectId, priority)
 {
 }
 
@@ -61,6 +61,7 @@ QBackendScenePropertyChange::~QBackendScenePropertyChange()
 {
 }
 
+// TO DO get rid off setTargetNode, use the subject instead ??
 void QBackendScenePropertyChange::setTargetNode(const QNodeId &id)
 {
     Q_D(QBackendScenePropertyChange);
@@ -78,8 +79,8 @@ QBackendScenePropertyChange::QBackendScenePropertyChange(QBackendScenePropertyCh
 {
 }
 
-QBackendScenePropertyChange::QBackendScenePropertyChange(QBackendScenePropertyChangePrivate &dd, ChangeFlag type, QBackendNode *subject, QSceneChange::Priority priority)
-    : QScenePropertyChange(dd, type, subject->d_func(), priority)
+QBackendScenePropertyChange::QBackendScenePropertyChange(QBackendScenePropertyChangePrivate &dd, ChangeFlag type, const QNodeId &subjectId, QSceneChange::Priority priority)
+    : QScenePropertyChange(dd, type, Observable, subjectId, priority)
 {
 }
 
diff --git a/src/core/qbackendscenepropertychange.h b/src/core/qbackendscenepropertychange.h
index 8f26f9627..382418500 100644
--- a/src/core/qbackendscenepropertychange.h
+++ b/src/core/qbackendscenepropertychange.h
@@ -50,7 +50,7 @@ class QBackendNode;
 class QT3DCORESHARED_EXPORT QBackendScenePropertyChange : public QScenePropertyChange
 {
 public:
-    QBackendScenePropertyChange(ChangeFlag type, QBackendNode *subject,
+    QBackendScenePropertyChange(ChangeFlag type, const QNodeId &subjectId,
                                 Priority priority = Standard);
     virtual ~QBackendScenePropertyChange();
 
@@ -60,8 +60,7 @@ public:
 protected:
     Q_DECLARE_PRIVATE(QBackendScenePropertyChange)
     QBackendScenePropertyChange(QBackendScenePropertyChangePrivate &dd);
-    QBackendScenePropertyChange(QBackendScenePropertyChangePrivate &dd, ChangeFlag type,
-                                QBackendNode *subject, Priority priority = Standard);
+    QBackendScenePropertyChange(QBackendScenePropertyChangePrivate &dd, ChangeFlag type, const QNodeId &subjectId, Priority priority = Standard);
 };
 
 typedef QSharedPointer<QBackendScenePropertyChange> QBackendScenePropertyChangePtr;
diff --git a/src/core/qchangearbiter.cpp b/src/core/qchangearbiter.cpp
index 86ddc7d72..1d111d642 100644
--- a/src/core/qchangearbiter.cpp
+++ b/src/core/qchangearbiter.cpp
@@ -124,8 +124,7 @@ void QChangeArbiter::distributeQueueChanges(QChangeQueue *changeQueue)
         switch (change->observableType()) {
 
         case QSceneChange::Observable: {
-            QObservableInterface *subject = change->subject().m_observable;
-            QNodeId nodeId = m_scene->nodeIdFromObservable(subject);
+            const QNodeId nodeId = change->subjectId();
             if (m_nodeObservations.contains(nodeId)) {
                 QObserverList &observers = m_nodeObservations[nodeId];
                 Q_FOREACH (const QObserverPair&observer, observers) {
@@ -139,9 +138,9 @@ void QChangeArbiter::distributeQueueChanges(QChangeQueue *changeQueue)
         }
 
         case QSceneChange::Node: {
-            QNode *subject = change->subject().m_node;
-            if (m_nodeObservations.contains(subject->id())) {
-                QObserverList &observers = m_nodeObservations[subject->id()];
+            const QNodeId nodeId = change->subjectId();
+            if (m_nodeObservations.contains(nodeId)) {
+                QObserverList &observers = m_nodeObservations[nodeId];
                 Q_FOREACH (const QObserverPair&observer, observers) {
                     if ((change->type() & observer.first))
                         observer.second->sceneChangeEvent(change);
diff --git a/src/core/qscenechange.cpp b/src/core/qscenechange.cpp
index c5bb1622d..20218f04c 100644
--- a/src/core/qscenechange.cpp
+++ b/src/core/qscenechange.cpp
@@ -67,26 +67,15 @@ QSceneChangePrivate::~QSceneChangePrivate()
 {
 }
 
-QSceneChange::QSceneChange(ChangeFlag type, QObservableInterface *observable, QSceneChange::Priority priority)
+QSceneChange::QSceneChange(ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, QSceneChange::Priority priority)
     : d_ptr(new QSceneChangePrivate(this))
 {
     Q_D(QSceneChange);
     d->m_type = type;
     d->m_priority = priority;
     d->m_timestamp = QDateTime::currentMSecsSinceEpoch();
-    d->m_subject.m_observable = observable;
-    d->m_subjectType = Observable;
-}
-
-QSceneChange::QSceneChange(ChangeFlag type, QNode *node, QSceneChange::Priority priority)
-    : d_ptr(new QSceneChangePrivate(this))
-{
-    Q_D(QSceneChange);
-    d->m_type = type;
-    d->m_priority = priority;
-    d->m_timestamp = QDateTime::currentMSecsSinceEpoch();
-    d->m_subject.m_node = node;
-    d->m_subjectType = Node;
+    d->m_subjectId = subjectId;
+    d->m_subjectType = observableType;
 }
 
 QSceneChange::~QSceneChange()
@@ -99,26 +88,15 @@ QSceneChange::QSceneChange(QSceneChangePrivate &dd)
 {
 }
 
-QSceneChange::QSceneChange(QSceneChangePrivate &dd, ChangeFlag type, QObservableInterface *observable, QSceneChange::Priority priority)
-    : d_ptr(&dd)
-{
-    Q_D(QSceneChange);
-    d->m_type = type;
-    d->m_priority = priority;
-    d->m_timestamp = QDateTime::currentMSecsSinceEpoch();
-    d->m_subject.m_observable = observable;
-    d->m_subjectType = Observable;
-}
-
-QSceneChange::QSceneChange(QSceneChangePrivate &dd, ChangeFlag type, QNode *node, QSceneChange::Priority priority)
+QSceneChange::QSceneChange(QSceneChangePrivate &dd, ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, QSceneChange::Priority priority)
     : d_ptr(&dd)
 {
     Q_D(QSceneChange);
     d->m_type = type;
     d->m_priority = priority;
     d->m_timestamp = QDateTime::currentMSecsSinceEpoch();
-    d->m_subject.m_node = node;
-    d->m_subjectType = Node;
+    d->m_subjectId = subjectId;
+    d->m_subjectType = observableType;
 }
 
 ChangeFlag QSceneChange::type() const
@@ -145,10 +123,10 @@ QSceneChange::ObservableType QSceneChange::observableType() const
     return d->m_subjectType;
 }
 
-QSceneChange::SubjectUnion QSceneChange::subject() const
+QNodeId QSceneChange::subjectId() const
 {
     Q_D(const QSceneChange);
-    return d->m_subject;
+    return d->m_subjectId;
 }
 
 } // Qt3D
diff --git a/src/core/qscenechange.h b/src/core/qscenechange.h
index f52871b67..3c22638d3 100644
--- a/src/core/qscenechange.h
+++ b/src/core/qscenechange.h
@@ -39,6 +39,7 @@
 
 #include <Qt3DCore/qt3dcore_global.h>
 #include <QSharedPointer>
+#include <Qt3DCore/qnodeid.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -76,27 +77,20 @@ public:
         Node
     };
 
-    union SubjectUnion {
-        QObservableInterface *m_observable;
-        QNode *m_node;
-    };
-
-    QSceneChange(ChangeFlag type, QObservableInterface *observable, Priority priority = Standard);
-    QSceneChange(ChangeFlag type, QNode *node, Priority priority = Standard);
+    QSceneChange(ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, Priority priority = Standard);
     virtual ~QSceneChange();
 
     ChangeFlag type() const;
     qint64 timestamp() const;
     QSceneChange::Priority priority() const;
     QSceneChange::ObservableType observableType() const;
-    QSceneChange::SubjectUnion subject() const;
+    QNodeId subjectId() const;
 
 protected:
     Q_DECLARE_PRIVATE(QSceneChange)
     QSceneChangePrivate *d_ptr;
     QSceneChange(QSceneChangePrivate &dd);
-    QSceneChange(QSceneChangePrivate &dd, ChangeFlag type, QObservableInterface *observable, Priority priority = Standard);
-    QSceneChange(QSceneChangePrivate &dd, ChangeFlag type, QNode *node, Priority priority = Standard);
+    QSceneChange(QSceneChangePrivate &dd, ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, Priority priority = Standard);
 
     // TODO: add timestamp from central clock and priority level
     // These can be used to resolve any conflicts between events
diff --git a/src/core/qscenechange_p.h b/src/core/qscenechange_p.h
index 40748e444..b903b1035 100644
--- a/src/core/qscenechange_p.h
+++ b/src/core/qscenechange_p.h
@@ -44,6 +44,7 @@ QT_BEGIN_NAMESPACE
 namespace Qt3D {
 
 class QSceneChange;
+class QNodeId;
 
 class QSceneChangePrivate
 {
@@ -55,7 +56,7 @@ public :
 
     QSceneChange *q_ptr;
 
-    QSceneChange::SubjectUnion m_subject;
+    QNodeId m_subjectId;
     QSceneChange::ObservableType m_subjectType;
     ChangeFlag m_type;
     QSceneChange::Priority m_priority;
diff --git a/src/core/qscenepropertychange.cpp b/src/core/qscenepropertychange.cpp
index 14144b157..9daac1bd0 100644
--- a/src/core/qscenepropertychange.cpp
+++ b/src/core/qscenepropertychange.cpp
@@ -67,8 +67,8 @@ void QScenePropertyChangePrivate::operator delete(void *ptr, size_t size)
     QScenePropertyChangePrivate::m_allocator->deallocateRawMemory(ptr, size);
 }
 
-QScenePropertyChange::QScenePropertyChange(ChangeFlag type, QObservableInterface *subject, QSceneChange::Priority priority)
-    : QSceneChange(*new QScenePropertyChangePrivate(this), type, subject, priority)
+QScenePropertyChange::QScenePropertyChange(ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, QSceneChange::Priority priority)
+    : QSceneChange(*new QScenePropertyChangePrivate(this), type, observableType, subjectId, priority)
 {
 }
 
@@ -77,8 +77,8 @@ QScenePropertyChange::QScenePropertyChange(QScenePropertyChangePrivate &dd)
 {
 }
 
-QScenePropertyChange::QScenePropertyChange(ChangeFlag type, QNode *node, QSceneChange::Priority priority)
-    : QSceneChange(*new QScenePropertyChangePrivate(this), type, node, priority)
+QScenePropertyChange::QScenePropertyChange(QScenePropertyChangePrivate &dd, ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, QSceneChange::Priority priority)
+    : QSceneChange(dd, type, observableType, subjectId, priority)
 {
 }
 
@@ -86,16 +86,6 @@ QScenePropertyChange::~QScenePropertyChange()
 {
 }
 
-QScenePropertyChange::QScenePropertyChange(QScenePropertyChangePrivate &dd, ChangeFlag type, QObservableInterface *subject, QSceneChange::Priority priority)
-    : QSceneChange(dd, type, subject, priority)
-{
-}
-
-QScenePropertyChange::QScenePropertyChange(QScenePropertyChangePrivate &dd, ChangeFlag type, QNode *node, QSceneChange::Priority priority)
-    : QSceneChange(dd, type, node, priority)
-{
-}
-
 const char *QScenePropertyChange::propertyName() const
 {
     Q_D(const QScenePropertyChange);
diff --git a/src/core/qscenepropertychange.h b/src/core/qscenepropertychange.h
index 8e1363b9c..dcedf20a5 100644
--- a/src/core/qscenepropertychange.h
+++ b/src/core/qscenepropertychange.h
@@ -48,7 +48,7 @@ class QScenePropertyChangePrivate;
 class QT3DCORESHARED_EXPORT QScenePropertyChange : public QSceneChange
 {
 public:
-    QScenePropertyChange(ChangeFlag type, QNode *node, Priority priority = Standard);
+    QScenePropertyChange(ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, Priority priority = Standard);
     virtual ~QScenePropertyChange();
 
     const char *propertyName() const;
@@ -62,10 +62,8 @@ public:
 
 protected:
     Q_DECLARE_PRIVATE(QScenePropertyChange)
-    QScenePropertyChange(ChangeFlag type, QObservableInterface *subject, Priority priority = Standard);
     QScenePropertyChange(QScenePropertyChangePrivate &dd);
-    QScenePropertyChange(QScenePropertyChangePrivate &dd, ChangeFlag type, QObservableInterface *subject, Priority priority = Standard);
-    QScenePropertyChange(QScenePropertyChangePrivate &dd, ChangeFlag type, QNode *node, Priority priority = Standard);
+    QScenePropertyChange(QScenePropertyChangePrivate &dd, ChangeFlag type, ObservableType observableType, const QNodeId &subjectId, Priority priority = Standard);
 };
 
 typedef QSharedPointer<QScenePropertyChange> QScenePropertyChangePtr;