remove unneeded lock around action-data in entity item. Actions now use a weak pointer to keep track of owner entity

assignment-client/src/AssignmentAction.h

@@ -27,7 +27,7 @@ public:
     const QUuid& getID() const { return _id; }
     virtual EntityActionType getType() { return _type; }
     virtual void removeFromSimulation(EntitySimulation* simulation) const;
-    virtual const EntityItemPointer& getOwnerEntity() const { return _ownerEntity; }
+    virtual const EntityItemWeakPointer getOwnerEntity() const { return _ownerEntity; }
     virtual void setOwnerEntity(const EntityItemPointer ownerEntity) { _ownerEntity = ownerEntity; }
     virtual bool updateArguments(QVariantMap arguments) { assert(false); return false; }
 
@@ -50,7 +50,7 @@ protected:
     virtual void setAngularVelocity(glm::vec3 angularVelocity) { assert(false); }
 
     bool _active;
-    EntityItemPointer _ownerEntity;
+    EntityItemWeakPointer _ownerEntity;
 };
 
 #endif // hifi_AssignmentAction_h

libraries/entities/src/EntityActionInterface.h

@@ -35,7 +35,7 @@ public:
     virtual EntityActionType getType() { assert(false); return ACTION_TYPE_NONE; }
 
     virtual void removeFromSimulation(EntitySimulation* simulation) const = 0;
-    virtual const EntityItemPointer& getOwnerEntity() const = 0;
+    virtual const EntityItemWeakPointer getOwnerEntity() const = 0;
     virtual void setOwnerEntity(const EntityItemPointer ownerEntity) = 0;
     virtual bool updateArguments(QVariantMap arguments) = 0;
 

libraries/entities/src/EntityItem.cpp

@@ -1371,13 +1371,13 @@ void EntityItem::updateSimulatorID(const QUuid& value) {
 
 bool EntityItem::addAction(EntitySimulation* simulation, EntityActionPointer action) {
     assert(action);
+    auto actionOwnerEntity = action->getOwnerEntity().lock();
+    assert(actionOwnerEntity);
+    assert(actionOwnerEntity.get() == this);
+
     const QUuid& actionID = action->getID();
-    _objectActionsLock.lockForWrite();
     assert(!_objectActions.contains(actionID) || _objectActions[actionID] == action);
     _objectActions[actionID] = action;
-    _objectActionsLock.unlock();
-
-    assert(action->getOwnerEntity().get() == this);
 
     simulation->addAction(action);
     bool success = serializeActionData();
@@ -1388,13 +1388,10 @@ bool EntityItem::addAction(EntitySimulation* simulation, EntityActionPointer act
 }
 
 bool EntityItem::updateAction(EntitySimulation* simulation, const QUuid& actionID, const QVariantMap& arguments) {
-    _objectActionsLock.lockForRead();
     if (!_objectActions.contains(actionID)) {
-        _objectActionsLock.unlock();
         return false;
     }
     EntityActionPointer action = _objectActions[actionID];
-    _objectActionsLock.unlock();
     bool success = action->updateArguments(arguments);
     if (success) {
         success = serializeActionData();
@@ -1403,21 +1400,17 @@ bool EntityItem::updateAction(EntitySimulation* simulation, const QUuid& actionI
 }
 
 bool EntityItem::removeAction(EntitySimulation* simulation, const QUuid& actionID) {
-    _objectActionsLock.lockForWrite();
     if (_objectActions.contains(actionID)) {
         EntityActionPointer action = _objectActions[actionID];
         _objectActions.remove(actionID);
-        _objectActionsLock.unlock();
         action->setOwnerEntity(nullptr);
         action->removeFromSimulation(simulation);
         return serializeActionData();
     }
-    _objectActionsLock.unlock();
     return false;
 }
 
 bool EntityItem::clearActions(EntitySimulation* simulation) {
-    _objectActionsLock.lockForWrite();
     QHash<QUuid, EntityActionPointer>::iterator i = _objectActions.begin();
     while (i != _objectActions.end()) {
         const QUuid id = i.key();
@@ -1427,7 +1420,6 @@ bool EntityItem::clearActions(EntitySimulation* simulation) {
         action->removeFromSimulation(simulation);
     }
     _actionData = QByteArray();
-    _objectActionsLock.unlock();
     return true;
 }
 
@@ -1456,14 +1448,11 @@ void EntityItem::setActionData(QByteArray actionData) {
         dsForAction >> actionID;
         updated << actionID;
 
-        _objectActionsLock.lockForRead();
         if (_objectActions.contains(actionID)) {
             EntityActionPointer action = _objectActions[actionID];
-            _objectActionsLock.unlock();
             // XXX make sure types match?
             action->deserialize(serializedAction);
         } else {
-            _objectActionsLock.unlock();
             auto actionFactory = DependencyManager::get<EntityActionFactoryInterface>();
 
             EntityTree* entityTree = _element ? _element->getTree() : nullptr;
@@ -1480,7 +1469,6 @@ void EntityItem::setActionData(QByteArray actionData) {
     EntityTree* entityTree = _element ? _element->getTree() : nullptr;
     EntitySimulation* simulation = entityTree ? entityTree->getSimulation() : nullptr;
     if (simulation) {
-        _objectActionsLock.lockForWrite();
         QHash<QUuid, EntityActionPointer>::iterator i = _objectActions.begin();
         while (i != _objectActions.end()) {
             const QUuid id = i.key();
@@ -1493,18 +1481,13 @@ void EntityItem::setActionData(QByteArray actionData) {
             action->setOwnerEntity(nullptr);
             action->removeFromSimulation(simulation);
         }
-        _objectActionsLock.unlock();
     }
 }
 
 
 bool EntityItem::serializeActionData() {
-    _objectActionsLock.lockForRead();
     if (_objectActions.size() == 0) {
-        _objectActionsLock.unlock();
-        _objectActionsLock.lockForWrite();
         _actionData = QByteArray();
-        _objectActionsLock.unlock();
         return true;
     }
 
@@ -1517,7 +1500,6 @@ bool EntityItem::serializeActionData() {
         serializedActions << bytesForAction;
         i++;
     }
-    _objectActionsLock.unlock();
 
     QByteArray result;
     QDataStream ds(&result, QIODevice::WriteOnly);

libraries/entities/src/EntityItem.h

@@ -461,7 +461,6 @@ protected:
     bool _simulated; // set by EntitySimulation
 
     bool serializeActionData();
-    QReadWriteLock _objectActionsLock;
     QHash<QUuid, EntityActionPointer> _objectActions;
     static int _maxActionDataSize;
     QByteArray _actionData;

libraries/entities/src/EntityTypes.h

@@ -21,6 +21,7 @@
 
 class EntityItem;
 typedef std::shared_ptr<EntityItem> EntityItemPointer;
+typedef std::weak_ptr<EntityItem> EntityItemWeakPointer;
 
 inline uint qHash(const EntityItemPointer& a, uint seed) {
     return qHash(a.get(), seed);

libraries/physics/src/ObjectAction.cpp

@@ -27,10 +27,6 @@ void ObjectAction::updateAction(btCollisionWorld* collisionWorld, btScalar delta
     if (!_active) {
         return;
     }
-    if (!_ownerEntity) {
-        qDebug() << "ObjectAction::updateAction no owner entity";
-        return;
-    }
     updateActionWorker(deltaTimeStep);
 }
 
@@ -42,10 +38,11 @@ void ObjectAction::removeFromSimulation(EntitySimulation* simulation) const {
 }
 
 btRigidBody* ObjectAction::getRigidBody() {
-    if (!_ownerEntity) {
+    auto ownerEntity = _ownerEntity.lock();
+    if (!ownerEntity) {
         return nullptr;
     }
-    void* physicsInfo = _ownerEntity->getPhysicsInfo();
+    void* physicsInfo = ownerEntity->getPhysicsInfo();
     if (!physicsInfo) {
         return nullptr;
     }

libraries/physics/src/ObjectAction.h

@@ -32,7 +32,7 @@ public:
     const QUuid& getID() const { return _id; }
     virtual EntityActionType getType() { assert(false); return ACTION_TYPE_NONE; }
     virtual void removeFromSimulation(EntitySimulation* simulation) const;
-    virtual const EntityItemPointer& getOwnerEntity() const { return _ownerEntity; }
+    virtual const EntityItemWeakPointer getOwnerEntity() const { return _ownerEntity; }
     virtual void setOwnerEntity(const EntityItemPointer ownerEntity) { _ownerEntity = ownerEntity; }
     virtual bool updateArguments(QVariantMap arguments) { return false; }
 
@@ -68,7 +68,7 @@ protected:
     void unlock() { _lock.unlock(); }
 
     bool _active;
-    EntityItemPointer _ownerEntity;
+    EntityItemWeakPointer _ownerEntity;
 };
 
 #endif // hifi_ObjectAction_h

libraries/physics/src/ObjectActionOffset.cpp

@@ -32,7 +32,12 @@ void ObjectActionOffset::updateActionWorker(btScalar deltaTimeStep) {
         return;
     }
 
-    void* physicsInfo = _ownerEntity->getPhysicsInfo();
+    auto ownerEntity = _ownerEntity.lock();
+    if (!ownerEntity) {
+        return;
+    }
+
+    void* physicsInfo = ownerEntity->getPhysicsInfo();
     if (!physicsInfo) {
         unlock();
         return;

libraries/physics/src/ObjectActionSpring.cpp

@@ -33,7 +33,12 @@ void ObjectActionSpring::updateActionWorker(btScalar deltaTimeStep) {
         return;
     }
 
-    void* physicsInfo = _ownerEntity->getPhysicsInfo();
+    auto ownerEntity = _ownerEntity.lock();
+    if (!ownerEntity) {
+        return;
+    }
+
+    void* physicsInfo = ownerEntity->getPhysicsInfo();
     if (!physicsInfo) {
         unlock();
         return;