bring over code from entity-level-locking branch

assignment-client/src/AssignmentActionFactory.cpp

@@ -17,8 +17,7 @@ EntityActionPointer assignmentActionFactory(EntityActionType type, const QUuid&
 }
 
 
-EntityActionPointer AssignmentActionFactory::factory(EntitySimulation* simulation,
+EntityActionPointer AssignmentActionFactory::factory(EntityActionType type,
-                                                     EntityActionType type,
                                                      const QUuid& id,
                                                      EntityItemPointer ownerEntity,
                                                      QVariantMap arguments) {
@@ -33,9 +32,7 @@ EntityActionPointer AssignmentActionFactory::factory(EntitySimulation* simulatio
 }
 
 
-EntityActionPointer AssignmentActionFactory::factoryBA(EntitySimulation* simulation,
+EntityActionPointer AssignmentActionFactory::factoryBA(EntityItemPointer ownerEntity, QByteArray data) {
-                                                       EntityItemPointer ownerEntity,
-                                                       QByteArray data) {
     QDataStream serializedActionDataStream(data);
     EntityActionType type;
     QUuid id;

assignment-client/src/AssignmentActionFactory.h

@@ -19,14 +19,11 @@ class AssignmentActionFactory : public EntityActionFactoryInterface {
 public:
     AssignmentActionFactory() : EntityActionFactoryInterface() { }
     virtual ~AssignmentActionFactory() { }
-    virtual EntityActionPointer factory(EntitySimulation* simulation,
+    virtual EntityActionPointer factory(EntityActionType type,
-                                        EntityActionType type,
                                         const QUuid& id,
                                         EntityItemPointer ownerEntity,
                                         QVariantMap arguments);
-    virtual EntityActionPointer factoryBA(EntitySimulation* simulation,
+    virtual EntityActionPointer factoryBA(EntityItemPointer ownerEntity, QByteArray data);
-                                          EntityItemPointer ownerEntity,
-                                          QByteArray data);
 };
 
 #endif // hifi_AssignmentActionFactory_h

interface/src/InterfaceActionFactory.cpp

@@ -35,8 +35,7 @@ EntityActionPointer interfaceActionFactory(EntityActionType type, const QUuid& i
 }
 
 
-EntityActionPointer InterfaceActionFactory::factory(EntitySimulation* simulation,
+EntityActionPointer InterfaceActionFactory::factory(EntityActionType type,
-                                                    EntityActionType type,
                                                     const QUuid& id,
                                                     EntityItemPointer ownerEntity,
                                                     QVariantMap arguments) {
@@ -51,9 +50,7 @@ EntityActionPointer InterfaceActionFactory::factory(EntitySimulation* simulation
 }
 
 
-EntityActionPointer InterfaceActionFactory::factoryBA(EntitySimulation* simulation,
+EntityActionPointer InterfaceActionFactory::factoryBA(EntityItemPointer ownerEntity, QByteArray data) {
-                                                      EntityItemPointer ownerEntity,
-                                                      QByteArray data) {
     QDataStream serializedArgumentStream(data);
     EntityActionType type;
     QUuid id;

interface/src/InterfaceActionFactory.h

@@ -18,13 +18,11 @@ class InterfaceActionFactory : public EntityActionFactoryInterface {
 public:
     InterfaceActionFactory() : EntityActionFactoryInterface() { }
     virtual ~InterfaceActionFactory() { }
-    virtual EntityActionPointer factory(EntitySimulation* simulation,
+    virtual EntityActionPointer factory(EntityActionType type,
-                                        EntityActionType type,
                                         const QUuid& id,
                                         EntityItemPointer ownerEntity,
                                         QVariantMap arguments);
-    virtual EntityActionPointer factoryBA(EntitySimulation* simulation,
+    virtual EntityActionPointer factoryBA(EntityItemPointer ownerEntity,
-                                          EntityItemPointer ownerEntity,
                                           QByteArray data);
 };
 

interface/src/avatar/AvatarActionHold.cpp

@@ -85,15 +85,17 @@ void AvatarActionHold::updateActionWorker(float deltaTimeStep) {
         return;
     }
 
+    if (_positionalTarget != position || _rotationalTarget != rotation) {
+        auto ownerEntity = _ownerEntity.lock();
+        if (ownerEntity) {
+            ownerEntity->setActionDataDirty(true);
+        }
+    }
+
     _positionalTarget = position;
     _rotationalTarget = rotation;
     unlock();
 
-    auto ownerEntity = _ownerEntity.lock();
-    if (ownerEntity) {
-        ownerEntity->setActionDataDirty(true);
-    }
-
     ObjectActionSpring::updateActionWorker(deltaTimeStep);
 }
 
@@ -152,10 +154,6 @@ bool AvatarActionHold::updateArguments(QVariantMap arguments) {
     _rotationalTargetSet = true;
     _active = true;
     unlock();
-    auto ownerEntity = _ownerEntity.lock();
-    if (ownerEntity) {
-        ownerEntity->setActionDataDirty(true);
-    }
     return true;
 }
 

libraries/entities/src/EntityActionFactoryInterface.h

@@ -23,13 +23,11 @@ class EntityActionFactoryInterface : public QObject, public Dependency {
  public:
     EntityActionFactoryInterface() { }
     virtual ~EntityActionFactoryInterface() { }
-    virtual EntityActionPointer factory(EntitySimulation* simulation,
+    virtual EntityActionPointer factory(EntityActionType type,
-                                        EntityActionType type,
                                         const QUuid& id,
                                         EntityItemPointer ownerEntity,
                                         QVariantMap arguments) { assert(false); return nullptr; }
-    virtual EntityActionPointer factoryBA(EntitySimulation* simulation,
+    virtual EntityActionPointer factoryBA(EntityItemPointer ownerEntity,
-                                          EntityItemPointer ownerEntity,
                                           QByteArray data) { assert(false); return nullptr; }
 };
 

libraries/entities/src/EntityItem.cpp

@@ -1489,20 +1489,22 @@ void EntityItem::clearSimulationOwnership() {
 
 }
 
+
 bool EntityItem::addAction(EntitySimulation* simulation, EntityActionPointer action) {
+    lockForWrite();
     checkWaitingToRemove(simulation);
-    if (!checkWaitingActionData(simulation)) {
-        return false;
-    }
 
     bool result = addActionInternal(simulation, action);
     if (!result) {
         removeAction(simulation, action->getID());
     }
+
+    unlock();
     return result;
 }
 
 bool EntityItem::addActionInternal(EntitySimulation* simulation, EntityActionPointer action) {
+    assertLocked();
     assert(action);
     assert(simulation);
     auto actionOwnerEntity = action->getOwnerEntity().lock();
@@ -1523,36 +1525,37 @@ bool EntityItem::addActionInternal(EntitySimulation* simulation, EntityActionPoi
 }
 
 bool EntityItem::updateAction(EntitySimulation* simulation, const QUuid& actionID, const QVariantMap& arguments) {
+    lockForWrite();
     checkWaitingToRemove(simulation);
-    if (!checkWaitingActionData(simulation)) {
-        return false;
-    }
 
     if (!_objectActions.contains(actionID)) {
+        unlock();
         return false;
     }
     EntityActionPointer action = _objectActions[actionID];
-    bool success = action->updateArguments(arguments);
 
+    bool success = action->updateArguments(arguments);
     if (success) {
         _allActionsDataCache = serializeActions(success);
     } else {
         qDebug() << "EntityItem::updateAction failed";
     }
 
+    unlock();
     return success;
 }
 
 bool EntityItem::removeAction(EntitySimulation* simulation, const QUuid& actionID) {
+    lockForWrite();
     checkWaitingToRemove(simulation);
-    if (!checkWaitingActionData(simulation)) {
-        return false;;
-    }
 
-    return removeActionInternal(actionID);
+    bool success = removeActionInternal(actionID);
+    unlock();
+    return success;
 }
 
 bool EntityItem::removeActionInternal(const QUuid& actionID, EntitySimulation* simulation) {
+    assertWriteLocked();
     if (_objectActions.contains(actionID)) {
         if (!simulation) {
             EntityTree* entityTree = _element ? _element->getTree() : nullptr;
@@ -1575,7 +1578,7 @@ bool EntityItem::removeActionInternal(const QUuid& actionID, EntitySimulation* s
 }
 
 bool EntityItem::clearActions(EntitySimulation* simulation) {
-    _waitingActionData.clear();
+    lockForWrite();
     QHash<QUuid, EntityActionPointer>::iterator i = _objectActions.begin();
     while (i != _objectActions.end()) {
         const QUuid id = i.key();
@@ -1584,85 +1587,84 @@ bool EntityItem::clearActions(EntitySimulation* simulation) {
         action->setOwnerEntity(nullptr);
         action->removeFromSimulation(simulation);
     }
+    // empty _serializedActions means no actions for the EntityItem
     _actionsToRemove.clear();
     _allActionsDataCache.clear();
+    unlock();
     return true;
 }
 
-bool EntityItem::deserializeActions(QByteArray allActionsData, EntitySimulation* simulation) const {
+
-    bool success = true;
+void EntityItem::deserializeActions() {
-    QVector<QByteArray> serializedActions;
+    assertUnlocked();
-    if (allActionsData.size() > 0) {
+    lockForWrite();
-        QDataStream serializedActionsStream(allActionsData);
+    deserializeActionsInternal();
-        serializedActionsStream >> serializedActions;
+    unlock();
+}
+
+
+void EntityItem::deserializeActionsInternal() {
+    assertWriteLocked();
+
+    if (!_element) {
+        return;
     }
 
     // Keep track of which actions got added or updated by the new actionData
-    QSet<QUuid> updated;
 
     EntityTree* entityTree = _element ? _element->getTree() : nullptr;
-    if (!simulation) {
+    assert(entityTree);
-        simulation = entityTree ? entityTree->getSimulation() : nullptr;
+    EntitySimulation* simulation = entityTree ? entityTree->getSimulation() : nullptr;
+    assert(simulation);
+
+    QVector<QByteArray> serializedActions;
+    if (_allActionsDataCache.size() > 0) {
+        QDataStream serializedActionsStream(_allActionsDataCache);
+        serializedActionsStream >> serializedActions;
     }
 
-    if (simulation && entityTree) {
+    QSet<QUuid> updated;
-        foreach(QByteArray serializedAction, serializedActions) {
-            QDataStream serializedActionStream(serializedAction);
-            EntityActionType actionType;
-            QUuid actionID;
-            serializedActionStream >> actionType;
-            serializedActionStream >> actionID;
-            updated << actionID;
 
-            if (_objectActions.contains(actionID)) {
+    foreach(QByteArray serializedAction, serializedActions) {
-                EntityActionPointer action = _objectActions[actionID];
+        QDataStream serializedActionStream(serializedAction);
-                // TODO: make sure types match?  there isn't currently a way to
+        EntityActionType actionType;
-                // change the type of an existing action.
+        QUuid actionID;
-                action->deserialize(serializedAction);
+        serializedActionStream >> actionType;
-            } else {
+        serializedActionStream >> actionID;
-                auto actionFactory = DependencyManager::get<EntityActionFactoryInterface>();
+        updated << actionID;
-                if (simulation) {
+
-                    EntityItemPointer entity = entityTree->findEntityByEntityItemID(_id);
+        if (_objectActions.contains(actionID)) {
-                    EntityActionPointer action = actionFactory->factoryBA(simulation, entity, serializedAction);
+            EntityActionPointer action = _objectActions[actionID];
-                    if (action) {
+            // TODO: make sure types match?  there isn't currently a way to
-                        entity->addActionInternal(simulation, action);
+            // change the type of an existing action.
-                    }
+            action->deserialize(serializedAction);
-                } else {
+        } else {
-                    // we can't yet add the action.  This method will be called later.
+            auto actionFactory = DependencyManager::get<EntityActionFactoryInterface>();
-                    success = false;
+
-                }
+            // EntityItemPointer entity = entityTree->findEntityByEntityItemID(_id, false);
+            EntityItemPointer entity = shared_from_this();
+            EntityActionPointer action = actionFactory->factoryBA(entity, serializedAction);
+            if (action) {
+                entity->addActionInternal(simulation, action);
             }
         }
+    }
 
-        // remove any actions that weren't included in the new data.
+    // remove any actions that weren't included in the new data.
-        QHash<QUuid, EntityActionPointer>::const_iterator i = _objectActions.begin();
+    QHash<QUuid, EntityActionPointer>::const_iterator i = _objectActions.begin();
-        while (i != _objectActions.end()) {
+    while (i != _objectActions.end()) {
-            const QUuid id = i.key();
+        const QUuid id = i.key();
-            if (!updated.contains(id)) {
+        if (!updated.contains(id)) {
-                _actionsToRemove << id;
+            _actionsToRemove << id;
-            }
-            i++;
         }
-    } else {
+        i++;
-        // no simulation
-        success = false;
     }
 
-    return success;
+    return;
-}
-
-bool EntityItem::checkWaitingActionData(EntitySimulation* simulation) const {
-    if (_waitingActionData.size() == 0) {
-        return true;
-    }
-    bool success = deserializeActions(_waitingActionData, simulation);
-    if (success) {
-        _waitingActionData.clear();
-    }
-    return success;
 }
 
 void EntityItem::checkWaitingToRemove(EntitySimulation* simulation) {
+    assertLocked();
     foreach(QUuid actionID, _actionsToRemove) {
         removeActionInternal(actionID, simulation);
     }
@@ -1670,21 +1672,22 @@ void EntityItem::checkWaitingToRemove(EntitySimulation* simulation) {
 }
 
 void EntityItem::setActionData(QByteArray actionData) {
+    assertUnlocked();
+    lockForWrite();
+    setActionDataInternal(actionData);
+    unlock();
+}
+
+void EntityItem::setActionDataInternal(QByteArray actionData) {
+    assertWriteLocked();
     checkWaitingToRemove();
-    bool success = deserializeActions(actionData);
     _allActionsDataCache = actionData;
-    if (success) {
+    deserializeActionsInternal();
-        _waitingActionData.clear();
-    } else {
-        _waitingActionData = actionData;
-    }
 }
 
 QByteArray EntityItem::serializeActions(bool& success) const {
+    assertLocked();
     QByteArray result;
-    if (!checkWaitingActionData()) {
-        return _waitingActionData;
-    }
 
     if (_objectActions.size() == 0) {
         success = true;
@@ -1713,7 +1716,7 @@ QByteArray EntityItem::serializeActions(bool& success) const {
     return result;
 }
 
-const QByteArray EntityItem::getActionData() const {
+const QByteArray EntityItem::getActionDataInternal() const {
     if (_actionDataDirty) {
         bool success;
         QByteArray newDataCache = serializeActions(success);
@@ -1725,17 +1728,120 @@ const QByteArray EntityItem::getActionData() const {
     return _allActionsDataCache;
 }
 
+const QByteArray EntityItem::getActionData() const {
+    assertUnlocked();
+    lockForRead();
+    auto result = getActionDataInternal();
+    unlock();
+    return result;
+}
+
 QVariantMap EntityItem::getActionArguments(const QUuid& actionID) const {
     QVariantMap result;
-
+    lockForRead();
-    if (!checkWaitingActionData()) {
-        return result;
-    }
 
     if (_objectActions.contains(actionID)) {
         EntityActionPointer action = _objectActions[actionID];
         result = action->getArguments();
         result["type"] = EntityActionInterface::actionTypeToString(action->getType());
     }
+    unlock();
     return result;
 }
+
+
+
+#define ENABLE_LOCKING 1
+
+#ifdef ENABLE_LOCKING
+void EntityItem::lockForRead() const {
+    _lock.lockForRead();
+}
+
+bool EntityItem::tryLockForRead() const {
+    return _lock.tryLockForRead();
+}
+
+void EntityItem::lockForWrite() const {
+    _lock.lockForWrite();
+}
+
+bool EntityItem::tryLockForWrite() const {
+    return _lock.tryLockForWrite();
+}
+
+void EntityItem::unlock() const {
+    _lock.unlock();
+}
+
+bool EntityItem::isLocked() const {
+    bool readSuccess = tryLockForRead();
+    if (readSuccess) {
+        unlock();
+    }
+    bool writeSuccess = tryLockForWrite();
+    if (writeSuccess) {
+        unlock();
+    }
+    if (readSuccess && writeSuccess) {
+        return false;  // if we can take both kinds of lock, there was no previous lock
+    }
+    return true; // either read or write failed, so there is some lock in place.
+}
+
+
+bool EntityItem::isWriteLocked() const {
+    bool readSuccess = tryLockForRead();
+    if (readSuccess) {
+        unlock();
+        return false;
+    }
+    bool writeSuccess = tryLockForWrite();
+    if (writeSuccess) {
+        unlock();
+        return false;
+    }
+    return true; // either read or write failed, so there is some lock in place.
+}
+
+
+bool EntityItem::isUnlocked() const {
+    // this can't be sure -- this may get unlucky and hit locks from other threads.  what we're actually trying
+    // to discover is if *this* thread hasn't locked the EntityItem.  Try repeatedly to take both kinds of lock.
+    bool readSuccess = false;
+    for (int i=0; i<80; i++) {
+        readSuccess = tryLockForRead();
+        if (readSuccess) {
+            unlock();
+            break;
+        }
+        QThread::usleep(200);
+    }
+
+    bool writeSuccess = false;
+    if (readSuccess) {
+        for (int i=0; i<80; i++) {
+            writeSuccess = tryLockForWrite();
+            if (writeSuccess) {
+                unlock();
+                break;
+            }
+            QThread::usleep(300);
+        }
+    }
+
+    if (readSuccess && writeSuccess) {
+        return true;  // if we can take both kinds of lock, there was no previous lock
+    }
+    return false;
+}
+#else
+void EntityItem::lockForRead() const { }
+bool EntityItem::tryLockForRead() const { return true; }
+void EntityItem::lockForWrite() const { }
+bool EntityItem::tryLockForWrite() const { return true; }
+void EntityItem::unlock() const { }
+bool EntityItem::isLocked() const { return true; }
+bool EntityItem::isWriteLocked() const { return true; }
+bool EntityItem::isUnlocked() const { return true; }
+#endif

libraries/entities/src/EntityItem.h

@@ -68,10 +68,28 @@ const float ACTIVATION_ANGULAR_VELOCITY_DELTA = 0.03f;
 #define debugTimeOnly(T) qPrintable(QString("%1").arg(T, 16, 10))
 #define debugTreeVector(V) V << "[" << V << " in meters ]"
 
+#if DEBUG
+  #define assertLocked() assert(isLocked())
+#else
+  #define assertLocked()
+#endif
+
+#if DEBUG
+  #define assertWriteLocked() assert(isWriteLocked())
+#else
+  #define assertWriteLocked()
+#endif
+
+#if DEBUG
+  #define assertUnlocked() assert(isUnlocked())
+#else
+  #define assertUnlocked()
+#endif
+
 /// EntityItem class this is the base class for all entity types. It handles the basic properties and functionality available
 /// to all other entity types. In particular: postion, size, rotation, age, lifetime, velocity, gravity. You can not instantiate
 /// one directly, instead you must only construct one of it's derived classes with additional features.
-class EntityItem {
+class EntityItem : public std::enable_shared_from_this<EntityItem> {
     // These two classes manage lists of EntityItem pointers and must be able to cleanup pointers when an EntityItem is deleted.
     // To make the cleanup robust each EntityItem has backpointers to its manager classes (which are only ever set/cleared by 
     // the managers themselves, hence they are fiends) whose NULL status can be used to determine which managers still need to
@@ -395,10 +413,14 @@ public:
     bool hasActions() { return !_objectActions.empty(); }
     QList<QUuid> getActionIDs() { return _objectActions.keys(); }
     QVariantMap getActionArguments(const QUuid& actionID) const;
+    void deserializeActions();
     void setActionDataDirty(bool value) const { _actionDataDirty = value; }
 
 protected:
 
+    const QByteArray getActionDataInternal() const;
+    void setActionDataInternal(QByteArray actionData);
+
     static bool _sendPhysicsUpdates;
     EntityTypes::EntityType _type;
     QUuid _id;
@@ -471,19 +493,28 @@ protected:
 
     bool addActionInternal(EntitySimulation* simulation, EntityActionPointer action);
     bool removeActionInternal(const QUuid& actionID, EntitySimulation* simulation = nullptr);
-    bool deserializeActions(QByteArray allActionsData, EntitySimulation* simulation = nullptr) const;
+    void deserializeActionsInternal();
     QByteArray serializeActions(bool& success) const;
     QHash<QUuid, EntityActionPointer> _objectActions;
+
     static int _maxActionsDataSize;
     mutable QByteArray _allActionsDataCache;
     // when an entity-server starts up, EntityItem::setActionData is called before the entity-tree is
     // ready.  This means we can't find our EntityItemPointer or add the action to the simulation.  These
     // are used to keep track of and work around this situation.
-    bool checkWaitingActionData(EntitySimulation* simulation = nullptr) const;
     void checkWaitingToRemove(EntitySimulation* simulation = nullptr);
-    mutable QByteArray _waitingActionData;
     mutable QSet<QUuid> _actionsToRemove;
     mutable bool _actionDataDirty = false;
+
+    mutable QReadWriteLock _lock;
+    void lockForRead() const;
+    bool tryLockForRead() const;
+    void lockForWrite() const;
+    bool tryLockForWrite() const;
+    void unlock() const;
+    bool isLocked() const;
+    bool isWriteLocked() const;
+    bool isUnlocked() const;
 };
 
 #endif // hifi_EntityItem_h

libraries/entities/src/EntityScriptingInterface.cpp

@@ -574,7 +574,7 @@ QUuid EntityScriptingInterface::addAction(const QString& actionTypeString,
             if (actionType == ACTION_TYPE_NONE) {
                 return false;
             }
-            EntityActionPointer action = actionFactory->factory(simulation, actionType, actionID, entity, arguments);
+            EntityActionPointer action = actionFactory->factory(actionType, actionID, entity, arguments);
             if (action) {
                 entity->addAction(simulation, action);
                 auto nodeList = DependencyManager::get<NodeList>();

libraries/entities/src/EntitySimulation.cpp

@@ -146,6 +146,7 @@ void EntitySimulation::sortEntitiesThatMoved() {
 
 void EntitySimulation::addEntity(EntityItemPointer entity) {
     assert(entity);
+    entity->deserializeActions();
     if (entity->isMortal()) {
         _mortalEntities.insert(entity);
         quint64 expiry = entity->getExpiry();