add priority promotion to reduce volunteer races

also fix priority inheritance from chained collisions

libraries/entities/src/EntityItem.cpp

@@ -349,12 +349,6 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef
         return 0;
     }
 
-    // if this bitstream indicates that this node is the simulation owner, ignore any physics-related updates.
-    glm::vec3 savePosition = getPosition();
-    glm::quat saveRotation = getRotation();
-    glm::vec3 saveVelocity = _velocity;
-    glm::vec3 saveAngularVelocity = _angularVelocity;
-
     int originalLength = bytesLeftToRead;
     // TODO: figure out a way to avoid the big deep copy below.
     QByteArray originalDataBuffer((const char*)data, originalLength); // big deep copy!
@@ -550,17 +544,14 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef
             dataAt += bytes;
             bytesRead += bytes;
 
-            if (overwriteLocalData) {
+            if (_simulationOwner.set(newSimOwner)) {
-                if (_simulationOwner.set(newSimOwner)) {
+                _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID;
-                    _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID;
-                }
             }
         }
-        {
+        {   // When we own the simulation we don't accept updates to the entity's transform/velocities
+            // but since we're using macros below we have to temporarily modify overwriteLocalData.
             auto nodeList = DependencyManager::get<NodeList>();
             bool weOwnIt = _simulationOwner.matchesValidID(nodeList->getSessionUUID());
-            // When we own the simulation we don't accept updates to the entity's transform/velocities
-            // but since we're using macros below we have to temporarily modify overwriteLocalData.
             bool oldOverwrite = overwriteLocalData;
             overwriteLocalData = overwriteLocalData && !weOwnIt;
             READ_ENTITY_PROPERTY(PROP_POSITION, glm::vec3, updatePosition);
@@ -665,15 +656,8 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef
     auto nodeList = DependencyManager::get<NodeList>();
     const QUuid& myNodeID = nodeList->getSessionUUID();
     if (overwriteLocalData) {
-        if (_simulationOwner.matchesValidID(myNodeID)) {
+        if (!_simulationOwner.matchesValidID(myNodeID)) {
-            // we own the simulation, so we keep our transform+velocities and remove any related dirty flags
+            
-            // rather than accept the values in the packet
-            setPosition(savePosition);
-            setRotation(saveRotation);
-            _velocity = saveVelocity;
-            _angularVelocity = saveAngularVelocity;
-            _dirtyFlags &= ~(EntityItem::DIRTY_TRANSFORM | EntityItem::DIRTY_VELOCITIES);
-        } else {
             _lastSimulated = now;
         }
     }
@@ -988,6 +972,7 @@ bool EntityItem::setProperties(const EntityItemProperties& properties) {
     bool somethingChanged = false;
 
     // these affect TerseUpdate properties
+    SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulationOwner, setSimulationOwner);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(position, updatePosition);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(rotation, updateRotation);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(velocity, updateVelocity);
@@ -1009,7 +994,6 @@ bool EntityItem::setProperties(const EntityItemProperties& properties) {
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(collisionsWillMove, updateCollisionsWillMove);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(created, updateCreated);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(lifetime, updateLifetime);
-    SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulationOwner, setSimulationOwner);
 
     // non-simulation properties below
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(script, setScript);
@@ -1392,10 +1376,6 @@ void EntityItem::setSimulationOwner(const SimulationOwner& owner) {
     _simulationOwner.set(owner);
 }
 
-void EntityItem::promoteSimulationPriority(quint8 priority) {
-    _simulationOwner.promotePriority(priority);
-}
-
 void EntityItem::updateSimulatorID(const QUuid& value) {
     if (_simulationOwner.setID(value)) {
         _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID;
@@ -1404,7 +1384,7 @@ void EntityItem::updateSimulatorID(const QUuid& value) {
 
 void EntityItem::clearSimulationOwnership() {
     _simulationOwner.clear();
-    // don't bother setting the DIRTY_SIMULATOR_ID flag because clearSimulatorOwnership() 
+    // don't bother setting the DIRTY_SIMULATOR_ID flag because clearSimulationOwnership() 
     // is only ever called entity-server-side and the flags are only used client-side
     //_dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID;
 

libraries/entities/src/EntityItemProperties.cpp

@@ -706,7 +706,6 @@ bool EntityItemProperties::encodeEntityEditPacket(PacketType command, EntityItem
             //      PROP_PAGED_PROPERTY,
             //      PROP_CUSTOM_PROPERTIES_INCLUDED,
             
-            // adebug TODO: convert this to use APPEND_ENTITY_PROPERTY(P,V) macro?
             if (requestedProperties.getHasProperty(PROP_SIMULATION_OWNER)) {
                 LevelDetails propertyLevel = packetData->startLevel();
                 successPropertyFits = packetData->appendValue(properties._simulationOwner.toByteArray());
@@ -977,7 +976,6 @@ bool EntityItemProperties::decodeEntityEditPacket(const unsigned char* data, int
     dataAt += propertyFlags.getEncodedLength();
     processedBytes += propertyFlags.getEncodedLength();
 
-    // adebug TODO: convert this to use READ_ENTITY_PROPERTY_TO_PROPERTIES macro?
     if (propertyFlags.getHasProperty(PROP_SIMULATION_OWNER)) {
         QByteArray fromBuffer;
         int bytes = OctreePacketData::unpackDataFromBytes(dataAt, fromBuffer);

libraries/entities/src/EntityItemProperties.h

@@ -210,6 +210,7 @@ public:
 
     void clearSimulationOwner();
     void setSimulationOwner(const QUuid& id, uint8_t priority);
+    void promoteSimulationPriority(quint8 priority) { _simulationOwner.promotePriority(priority); }
 
 private:
     QUuid _id;

libraries/entities/src/EntityPropertyFlags.h

@@ -121,10 +121,10 @@ enum EntityPropertyList {
     // used by hyperlinks
     PROP_HREF,
     PROP_DESCRIPTION,
-    PROP_SIMULATION_OWNER,
     
     PROP_FACE_CAMERA,
     PROP_SCRIPT_TIMESTAMP,
+    PROP_SIMULATION_OWNER,
     
     ////////////////////////////////////////////////////////////////////////////////////////////////////
     // ATTENTION: add new properties ABOVE this line

libraries/entities/src/EntityScriptingInterface.cpp

@@ -161,9 +161,11 @@ QUuid EntityScriptingInterface::editEntity(QUuid id, EntityItemProperties proper
                         // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's
                         // simulation.
 
-                        // we re-assert our simulation ownership
+                        if (entity->getSimulationPriority() < SCRIPT_EDIT_SIMULATION_PRIORITY) {
-                        properties.setSimulationOwner(myNodeID, 
+                            // we re-assert our simulation ownership at a higher priority
-                                glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY));
+                            properties.setSimulationOwner(myNodeID, 
+                                    glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY));
+                        }
                     } else {
                         // we make a bid for simulation ownership
                         properties.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);

libraries/entities/src/EntityTree.cpp

@@ -162,7 +162,12 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI
                     // else: We assume the sender really did believe it was the simulation owner when it sent
                 } else if (submittedID == senderID) {
                     // the sender is trying to take or continue ownership
-                    if (entity->getSimulatorID().isNull() || entity->getSimulatorID() == senderID) {
+                    if (entity->getSimulatorID().isNull()) {
+                        // the sender it taking ownership
+                        properties.promoteSimulationPriority(RECRUIT_SIMULATION_PRIORITY);
+                        simulationBlocked = false;
+                    } else if (entity->getSimulatorID() == senderID) {
+                        // the sender is asserting ownership
                         simulationBlocked = false;
                     } else {
                         // the sender is trying to steal ownership from another simulator
@@ -180,6 +185,8 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI
                     // the entire update is suspect --> ignore it
                     return false;
                 }
+            } else {
+                simulationBlocked = senderID != entity->getSimulatorID();
             }
             if (simulationBlocked) {
                 // squash ownership and physics-related changes.
@@ -208,10 +215,6 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI
                     _simulation->lock();
                     _simulation->changeEntity(entity);
                     _simulation->unlock();
-                    // always promote volunteer priority
-                    if (entity->getSimulationPriority() == VOLUNTEER_SIMULATION_PRIORITY) {
-                        entity->promoteSimulationPriority(RECRUIT_SIMULATION_PRIORITY);
-                    }
                 }
             } else {
                 // normally the _simulation clears ALL updateFlags, but since there is none we do it explicitly

libraries/entities/src/SimulationOwner.cpp

@@ -67,9 +67,7 @@ bool SimulationOwner::setID(const QUuid& id) {
         _id = id;
         updateExpiry();
         if (_id.isNull()) {
-            // when id is null we clear everything
             _priority = 0;
-            _expiry = 0;
         }
         return true;
     }

libraries/octree/src/OctreePacketData.h

@@ -249,8 +249,6 @@ public:
     static int unpackDataFromBytes(const unsigned char* dataBytes, QVector<glm::vec3>& result);
     static int unpackDataFromBytes(const unsigned char* dataBytes, QByteArray& result);
 
-    QByteArray getRawData() const { return QByteArray((const char*)_uncompressed, _bytesInUse); } // adebug
-
 private:
     /// appends raw bytes, might fail if byte would cause packet to be too large
     bool append(const unsigned char* data, int length);

libraries/physics/src/EntityMotionState.cpp

@@ -118,7 +118,7 @@ void EntityMotionState::handleEasyChanges(uint32_t flags, PhysicsEngine* engine)
         }
     }
     if (flags & EntityItem::DIRTY_SIMULATOR_OWNERSHIP) {
-        // also known as "bid for ownership with SCRIPT priority"
+        // (DIRTY_SIMULATOR_OWNERSHIP really means "we should bid for ownership with SCRIPT priority")
         // we're manipulating this object directly via script, so we artificially 
         // manipulate the logic to trigger an immediate bid for ownership
         setOutgoingPriority(SCRIPT_EDIT_SIMULATION_PRIORITY);
@@ -449,6 +449,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, const Q
             // we own the simulation but the entity has stopped, so we tell the server that we're clearing simulatorID
             // but we remember that we do still own it...  and rely on the server to tell us that we don't
             properties.clearSimulationOwner();
+            _outgoingPriority = 0;
         }
         // else the ownership is not changing so we don't bother to pack it
     } else {
@@ -512,8 +513,7 @@ QUuid EntityMotionState::getSimulatorID() const {
 // virtual
 void EntityMotionState::bump(quint8 priority) {
     if (_entity) {
-        quint8 inheritedPriority = VOLUNTEER_SIMULATION_PRIORITY;
+        setOutgoingPriority(glm::max(VOLUNTEER_SIMULATION_PRIORITY, --priority));
-        setOutgoingPriority(inheritedPriority);
     }
 }