more lock fixing

interface/src/avatar/AvatarActionHold.cpp

@@ -86,13 +86,9 @@ bool AvatarActionHold::updateArguments(QVariantMap arguments) {
     QString hand;
     QUuid holderID;
     bool needUpdate = false;
-    bool updateArgumentsSuceeded = true;
 
+    bool somethingChanged = ObjectAction::updateArguments(arguments);
     withReadLock([&]{
-        if (!ObjectAction::updateArguments(arguments)) {
-            updateArgumentsSuceeded = false;
-            return;
-        }
         bool ok = true;
         relativePosition = EntityActionInterface::extractVec3Argument("hold", arguments, "relativePosition", ok, false);
         if (!ok) {
@@ -121,7 +117,8 @@ bool AvatarActionHold::updateArguments(QVariantMap arguments) {
         auto myAvatar = DependencyManager::get<AvatarManager>()->getMyAvatar();
         holderID = myAvatar->getSessionUUID();
 
-        if (relativePosition != _relativePosition
+        if (somethingChanged ||
+            relativePosition != _relativePosition
             || relativeRotation != _relativeRotation
             || timeScale != _linearTimeScale
             || hand != _hand
@@ -130,10 +127,6 @@ bool AvatarActionHold::updateArguments(QVariantMap arguments) {
         }
     });
 
-    if (!updateArgumentsSuceeded) {
-        return false;
-    }
-
     if (needUpdate) {
         withWriteLock([&] {
             _relativePosition = relativePosition;
@@ -221,7 +214,7 @@ void AvatarActionHold::deserialize(QByteArray serializedArguments) {
 
         qDebug() << "deserialize hold: " << _holderID
                  << _relativePosition.x << _relativePosition.y << _relativePosition.z
-                 << _hand;
+                 << _hand << _expires;
 
         _active = true;
     });

libraries/entities/src/EntityItem.cpp

@@ -1579,6 +1579,9 @@ bool EntityItem::removeActionInternal(const QUuid& actionID, EntitySimulation* s
         }
 
         EntityActionPointer action = _objectActions[actionID];
+
+        qDebug() << "EntityItem::removeActionInternal clearing _ownerEntity";
+
         action->setOwnerEntity(nullptr);
         _objectActions.remove(actionID);
 
@@ -1601,6 +1604,7 @@ bool EntityItem::clearActions(EntitySimulation* simulation) {
             const QUuid id = i.key();
             EntityActionPointer action = _objectActions[id];
             i = _objectActions.erase(i);
+            qDebug() << "EntityItem::clearActions clearing _ownerEntity";
             action->setOwnerEntity(nullptr);
             action->removeFromSimulation(simulation);
         }
@@ -1644,6 +1648,8 @@ void EntityItem::deserializeActionsInternal() {
     // Keep track of which actions got added or updated by the new actionData
     QSet<QUuid> updated;
 
+    qDebug() << "EntityItem::deserializeActionsInternal" << serializedActions.size();
+
     foreach(QByteArray serializedAction, serializedActions) {
         QDataStream serializedActionStream(serializedAction);
         EntityActionType actionType;
@@ -1722,8 +1728,11 @@ void EntityItem::setActionData(QByteArray actionData) {
 void EntityItem::setActionDataInternal(QByteArray actionData) {
     assertWriteLocked();
     if (_allActionsDataCache != actionData) {
+        qDebug() << "EntityItem::setActionDataInternal yes";
         _allActionsDataCache = actionData;
         deserializeActionsInternal();
+    } else {
+        qDebug() << "EntityItem::setActionDataInternal no";
     }
     checkWaitingToRemove();
 }

libraries/physics/src/ObjectAction.cpp

@@ -63,35 +63,48 @@ void ObjectAction::updateAction(btCollisionWorld* collisionWorld, btScalar delta
 }
 
 bool ObjectAction::updateArguments(QVariantMap arguments) {
-    bool lifetimeSet = true;
+    bool somethingChanged = false;
-    float lifetime = EntityActionInterface::extractFloatArgument("action", arguments, "lifetime", lifetimeSet, false);
-    if (lifetimeSet) {
-        quint64 now = usecTimestampNow();
-        _expires = now + (quint64)(lifetime * USECS_PER_SECOND);
-    } else {
-        _expires = 0;
-    }
 
-    bool tagSet = true;
+    withWriteLock([&]{
-    QString tag = EntityActionInterface::extractStringArgument("action", arguments, "tag", tagSet, false);
+        quint64 previousExpires = _expires;
-    if (tagSet) {
+        QString previousTag = _tag;
-        _tag = tag;
-    } else {
-        tag = "";
-    }
 
-    return true;
+        bool lifetimeSet = true;
+        float lifetime = EntityActionInterface::extractFloatArgument("action", arguments, "lifetime", lifetimeSet, false);
+        if (lifetimeSet) {
+            quint64 now = usecTimestampNow();
+            _expires = now + (quint64)(lifetime * USECS_PER_SECOND);
+        } else {
+            _expires = 0;
+        }
+
+        bool tagSet = true;
+        QString tag = EntityActionInterface::extractStringArgument("action", arguments, "tag", tagSet, false);
+        if (tagSet) {
+            _tag = tag;
+        } else {
+            tag = "";
+        }
+
+        if (previousExpires != _expires || previousTag != _tag) {
+            somethingChanged = true;
+        }
+    });
+
+    return somethingChanged;
 }
 
 QVariantMap ObjectAction::getArguments() {
     QVariantMap arguments;
-    if (_expires == 0) {
+    withReadLock([&]{
-        arguments["lifetime"] = 0.0f;
+        if (_expires == 0) {
-    } else {
+            arguments["lifetime"] = 0.0f;
-        quint64 now = usecTimestampNow();
+        } else {
-        arguments["lifetime"] = (float)(_expires - now) / (float)USECS_PER_SECOND;
+            quint64 now = usecTimestampNow();
-    }
+            arguments["lifetime"] = (float)(_expires - now) / (float)USECS_PER_SECOND;
-    arguments["tag"] = _tag;
+        }
+        arguments["tag"] = _tag;
+    });
     return arguments;
 }
 
@@ -100,20 +113,30 @@ void ObjectAction::debugDraw(btIDebugDraw* debugDrawer) {
 }
 
 void ObjectAction::removeFromSimulation(EntitySimulation* simulation) const {
-    simulation->removeAction(_id);
+    QUuid myID;
+    withReadLock([&]{
+        myID = _id;
+    });
+    simulation->removeAction(myID);
 }
 
 btRigidBody* ObjectAction::getRigidBody() {
-    auto ownerEntity = _ownerEntity.lock();
+    ObjectMotionState* motionState = nullptr;
-    if (!ownerEntity) {
+    withReadLock([&]{
-        return nullptr;
+        auto ownerEntity = _ownerEntity.lock();
+        if (!ownerEntity) {
+            return;
+        }
+        void* physicsInfo = ownerEntity->getPhysicsInfo();
+        if (!physicsInfo) {
+            return;
+        }
+        motionState = static_cast<ObjectMotionState*>(physicsInfo);
+    });
+    if (motionState) {
+        return motionState->getRigidBody();
     }
-    void* physicsInfo = ownerEntity->getPhysicsInfo();
+    return nullptr;
-    if (!physicsInfo) {
-        return nullptr;
-    }
-    ObjectMotionState* motionState = static_cast<ObjectMotionState*>(physicsInfo);
-    return motionState->getRigidBody();
 }
 
 glm::vec3 ObjectAction::getPosition() {

libraries/physics/src/ObjectActionOffset.cpp

@@ -80,35 +80,46 @@ void ObjectActionOffset::updateActionWorker(btScalar deltaTimeStep) {
 
 
 bool ObjectActionOffset::updateArguments(QVariantMap arguments) {
-    if (!ObjectAction::updateArguments(arguments)) {
+    glm::vec3 pointToOffsetFrom;
-        return false;
+    float linearTimeScale;
-    }
+    float linearDistance;
-    bool ok = true;
-    glm::vec3 pointToOffsetFrom =
-        EntityActionInterface::extractVec3Argument("offset action", arguments, "pointToOffsetFrom", ok, true);
-    if (!ok) {
-        pointToOffsetFrom = _pointToOffsetFrom;
-    }
 
-    ok = true;
+    bool needUpdate = false;
-    float linearTimeScale =
+    bool somethingChanged = ObjectAction::updateArguments(arguments);
-        EntityActionInterface::extractFloatArgument("offset action", arguments, "linearTimeScale", ok, false);
-    if (!ok) {
-        linearTimeScale = _linearTimeScale;
-    }
 
-    ok = true;
+    withReadLock([&]{
-    float linearDistance =
+        bool ok = true;
-        EntityActionInterface::extractFloatArgument("offset action", arguments, "linearDistance", ok, false);
+        pointToOffsetFrom =
-    if (!ok) {
+            EntityActionInterface::extractVec3Argument("offset action", arguments, "pointToOffsetFrom", ok, true);
-        linearDistance = _linearDistance;
+        if (!ok) {
-    }
+            pointToOffsetFrom = _pointToOffsetFrom;
+        }
 
-    // only change stuff if something actually changed
+        ok = true;
-    if (_pointToOffsetFrom != pointToOffsetFrom
+        linearTimeScale =
-            || _linearTimeScale != linearTimeScale
+            EntityActionInterface::extractFloatArgument("offset action", arguments, "linearTimeScale", ok, false);
-            || _linearDistance != linearDistance) {
+        if (!ok) {
+            linearTimeScale = _linearTimeScale;
+        }
 
+        ok = true;
+        linearDistance =
+            EntityActionInterface::extractFloatArgument("offset action", arguments, "linearDistance", ok, false);
+        if (!ok) {
+            linearDistance = _linearDistance;
+        }
+
+        // only change stuff if something actually changed
+        if (somethingChanged ||
+            _pointToOffsetFrom != pointToOffsetFrom ||
+            _linearTimeScale != linearTimeScale ||
+            _linearDistance != linearDistance) {
+            needUpdate = true;
+        }
+    });
+
+
+    if (needUpdate) {
         withWriteLock([&] {
             _pointToOffsetFrom = pointToOffsetFrom;
             _linearTimeScale = linearTimeScale;
@@ -118,6 +129,7 @@ bool ObjectActionOffset::updateArguments(QVariantMap arguments) {
             activateBody();
         });
     }
+
     return true;
 }
 

libraries/physics/src/ObjectActionSpring.cpp

@@ -107,43 +107,52 @@ void ObjectActionSpring::updateActionWorker(btScalar deltaTimeStep) {
 
 const float MIN_TIMESCALE = 0.1f;
 
+
 bool ObjectActionSpring::updateArguments(QVariantMap arguments) {
-    if (!ObjectAction::updateArguments(arguments)) {
+    glm::vec3 positionalTarget;
-        return false;
+    float linearTimeScale;
-    }
+    glm::quat rotationalTarget;
-    // targets are required, spring-constants are optional
+    float angularTimeScale;
-    bool ok = true;
-    glm::vec3 positionalTarget =
-        EntityActionInterface::extractVec3Argument("spring action", arguments, "targetPosition", ok, false);
-    if (!ok) {
-        positionalTarget = _positionalTarget;
-    }
-    ok = true;
-    float linearTimeScale =
-        EntityActionInterface::extractFloatArgument("spring action", arguments, "linearTimeScale", ok, false);
-    if (!ok || linearTimeScale <= 0.0f) {
-        linearTimeScale = _linearTimeScale;
-    }
 
-    ok = true;
+    bool needUpdate = false;
-    glm::quat rotationalTarget =
+    bool somethingChanged = ObjectAction::updateArguments(arguments);
-        EntityActionInterface::extractQuatArgument("spring action", arguments, "targetRotation", ok, false);
+    withReadLock([&]{
-    if (!ok) {
+        // targets are required, spring-constants are optional
-        rotationalTarget = _rotationalTarget;
+        bool ok = true;
-    }
+        positionalTarget = EntityActionInterface::extractVec3Argument("spring action", arguments, "targetPosition", ok, false);
+        if (!ok) {
+            positionalTarget = _positionalTarget;
+        }
+        ok = true;
+        linearTimeScale = EntityActionInterface::extractFloatArgument("spring action", arguments, "linearTimeScale", ok, false);
+        if (!ok || linearTimeScale <= 0.0f) {
+            linearTimeScale = _linearTimeScale;
+        }
 
-    ok = true;
+        ok = true;
-    float angularTimeScale =
+        rotationalTarget = EntityActionInterface::extractQuatArgument("spring action", arguments, "targetRotation", ok, false);
-        EntityActionInterface::extractFloatArgument("spring action", arguments, "angularTimeScale", ok, false);
+        if (!ok) {
-    if (!ok) {
+            rotationalTarget = _rotationalTarget;
-        angularTimeScale = _angularTimeScale;
+        }
-    }
 
-    if (positionalTarget != _positionalTarget
+        ok = true;
-            || linearTimeScale != _linearTimeScale
+        angularTimeScale =
-            || rotationalTarget != _rotationalTarget
+            EntityActionInterface::extractFloatArgument("spring action", arguments, "angularTimeScale", ok, false);
-            || angularTimeScale != _angularTimeScale) {
+        if (!ok) {
-        // something changed
+            angularTimeScale = _angularTimeScale;
+        }
+
+        if (somethingChanged ||
+            positionalTarget != _positionalTarget ||
+            linearTimeScale != _linearTimeScale ||
+            rotationalTarget != _rotationalTarget ||
+            angularTimeScale != _angularTimeScale) {
+            // something changed
+            needUpdate = true;
+        }
+    });
+
+    if (needUpdate) {
         withWriteLock([&] {
             _positionalTarget = positionalTarget;
             _linearTimeScale = glm::max(MIN_TIMESCALE, glm::abs(linearTimeScale));
@@ -151,8 +160,15 @@ bool ObjectActionSpring::updateArguments(QVariantMap arguments) {
             _angularTimeScale = glm::max(MIN_TIMESCALE, glm::abs(angularTimeScale));
             _active = true;
             activateBody();
+
+            auto ownerEntity = _ownerEntity.lock();
+            if (ownerEntity) {
+                ownerEntity->setActionDataDirty(true);
+            }
+
         });
     }
+
     return true;
 }