From 4872a565c93d0d11082f4fb7944d93c389ee049d Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Tue, 12 May 2015 21:46:52 -0700
Subject: [PATCH] bid for simulation ownership

---
 libraries/entities/src/EntityItem.cpp         |  10 +-
 libraries/entities/src/EntityItem.h           |   2 +
 .../entities/src/EntityScriptingInterface.cpp |  27 ++--
 libraries/entities/src/EntitySimulation.h     |   3 +-
 libraries/entities/src/EntityTree.cpp         |  63 ++++++--
 libraries/entities/src/EntityTree.h           |   7 +-
 libraries/physics/src/EntityMotionState.cpp   | 150 ++++++++++--------
 libraries/physics/src/EntityMotionState.h     |  15 +-
 .../physics/src/PhysicalEntitySimulation.cpp  |  18 ++-
 libraries/physics/src/PhysicsEngine.cpp       |   2 +
 tests/octree/src/ModelTests.cpp               |   4 +-
 11 files changed, 185 insertions(+), 116 deletions(-)

diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp
index ed78a03083..36b78d9a41 100644
--- a/libraries/entities/src/EntityItem.cpp
+++ b/libraries/entities/src/EntityItem.cpp
@@ -565,7 +565,7 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef
         READ_ENTITY_PROPERTY_STRING(PROP_USER_DATA, setUserData);
 
         if (args.bitstreamVersion >= VERSION_ENTITIES_HAVE_ACCELERATION) {
-            READ_ENTITY_PROPERTY_UUID(PROP_SIMULATOR_ID, setSimulatorID);
+            READ_ENTITY_PROPERTY_UUID(PROP_SIMULATOR_ID, updateSimulatorID);
         }
 
         if (args.bitstreamVersion >= VERSION_ENTITIES_HAS_MARKETPLACE_ID) {
@@ -940,7 +940,7 @@ bool EntityItem::setProperties(const EntityItemProperties& properties) {
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(collisionsWillMove, updateCollisionsWillMove);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(locked, setLocked);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(userData, setUserData);
-    SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulatorID, setSimulatorID);
+    SET_ENTITY_PROPERTY_FROM_PROPERTIES(simulatorID, updateSimulatorID);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(marketplaceID, setMarketplaceID);
     SET_ENTITY_PROPERTY_FROM_PROPERTIES(name, setName);
 
@@ -1270,8 +1270,14 @@ void EntityItem::updateLifetime(float value) {
 }
 
 void EntityItem::setSimulatorID(const QUuid& value) {
+    _simulatorID = value;
+    _simulatorIDChangedTime = usecTimestampNow();
+}
+
+void EntityItem::updateSimulatorID(const QUuid& value) {
     if (_simulatorID != value) {
         _simulatorID = value;
         _simulatorIDChangedTime = usecTimestampNow();
+        _dirtyFlags |= EntityItem::DIRTY_SIMULATOR_ID;
     }
 }
diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h
index d9096bf429..4ae2178f14 100644
--- a/libraries/entities/src/EntityItem.h
+++ b/libraries/entities/src/EntityItem.h
@@ -82,6 +82,7 @@ public:
         DIRTY_UPDATEABLE = 0x0200,
         DIRTY_MATERIAL = 0x00400,
         DIRTY_PHYSICS_ACTIVATION = 0x0800, // we want to activate the object
+        DIRTY_SIMULATOR_ID = 0x1000,
         DIRTY_TRANSFORM = DIRTY_POSITION | DIRTY_ROTATION,
         DIRTY_VELOCITIES = DIRTY_LINEAR_VELOCITY | DIRTY_ANGULAR_VELOCITY
     };
@@ -284,6 +285,7 @@ public:
 
     QUuid getSimulatorID() const { return _simulatorID; }
     void setSimulatorID(const QUuid& value);
+    void updateSimulatorID(const QUuid& value);
     quint64 getSimulatorIDChangedTime() const { return _simulatorIDChangedTime; }
     
     const QString& getMarketplaceID() const { return _marketplaceID; }
diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp
index 46ca70aa43..62e61dc039 100644
--- a/libraries/entities/src/EntityScriptingInterface.cpp
+++ b/libraries/entities/src/EntityScriptingInterface.cpp
@@ -61,13 +61,12 @@ void EntityScriptingInterface::setEntityTree(EntityTree* modelTree) {
     }
 }
 
-
-
-void setSimId(EntityItemProperties& propertiesWithSimID, EntityItem* entity) {
+void bidForSimulationOwnership(EntityItemProperties& properties) {
+    // We make a bid for simulation ownership by declaring our sessionID as simulation owner 
+    // in the outgoing properties.  The EntityServer may accept the bid or might not.
     auto nodeList = DependencyManager::get<NodeList>();
     const QUuid myNodeID = nodeList->getSessionUUID();
-    propertiesWithSimID.setSimulatorID(myNodeID);
-    entity->setSimulatorID(myNodeID);
+    properties.setSimulatorID(myNodeID);
 }
 
 
@@ -89,7 +88,7 @@ EntityItemID EntityScriptingInterface::addEntity(const EntityItemProperties& pro
         entity->setLastBroadcast(usecTimestampNow());
         if (entity) {
             // This Node is creating a new object.  If it's in motion, set this Node as the simulator.
-            setSimId(propertiesWithSimID, entity);
+            bidForSimulationOwnership(propertiesWithSimID);
         } else {
             qCDebug(entities) << "script failed to add new Entity to local Octree";
             success = false;
@@ -163,29 +162,31 @@ EntityItemID EntityScriptingInterface::editEntity(EntityItemID entityID, const E
         }
     }
 
-    EntityItemProperties propertiesWithSimID = properties;
-
     // If we have a local entity tree set, then also update it. We can do this even if we don't know
     // the actual id, because we can edit out local entities just with creatorTokenID
     if (_entityTree) {
         _entityTree->lockForWrite();
-        _entityTree->updateEntity(entityID, propertiesWithSimID, canAdjustLocks());
+        _entityTree->updateEntity(entityID, properties);
         _entityTree->unlock();
     }
 
     // if at this point, we know the id, send the update to the entity server
     if (entityID.isKnownID) {
         // make sure the properties has a type, so that the encode can know which properties to include
-        if (propertiesWithSimID.getType() == EntityTypes::Unknown) {
+        if (properties.getType() == EntityTypes::Unknown) {
             EntityItem* entity = _entityTree->findEntityByEntityItemID(entityID);
             if (entity) {
+                // we need to change the outgoing properties, so we make a copy, modify, and send.
+                EntityItemProperties modifiedProperties = properties;
                 entity->setLastBroadcast(usecTimestampNow());
-                propertiesWithSimID.setType(entity->getType());
-                setSimId(propertiesWithSimID, entity);
+                modifiedProperties.setType(entity->getType());
+                bidForSimulationOwnership(modifiedProperties);
+                queueEntityMessage(PacketTypeEntityAddOrEdit, entityID, modifiedProperties);
+                return entityID;
             }
         }
 
-        queueEntityMessage(PacketTypeEntityAddOrEdit, entityID, propertiesWithSimID);
+        queueEntityMessage(PacketTypeEntityAddOrEdit, entityID, properties);
     }
     
     return entityID;
diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h
index a73afe9fd7..f5a100eba0 100644
--- a/libraries/entities/src/EntitySimulation.h
+++ b/libraries/entities/src/EntitySimulation.h
@@ -37,7 +37,8 @@ const int DIRTY_SIMULATION_FLAGS =
         EntityItem::DIRTY_SHAPE |
         EntityItem::DIRTY_LIFETIME |
         EntityItem::DIRTY_UPDATEABLE |
-        EntityItem::DIRTY_MATERIAL;
+        EntityItem::DIRTY_MATERIAL |
+        EntityItem::DIRTY_SIMULATOR_ID;
 
 class EntitySimulation : public QObject {
 Q_OBJECT
diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp
index 5aea021e7a..306fbeb413 100644
--- a/libraries/entities/src/EntityTree.cpp
+++ b/libraries/entities/src/EntityTree.cpp
@@ -86,7 +86,7 @@ void EntityTree::postAddEntity(EntityItem* entity) {
     emit addingEntity(entity->getEntityItemID());
 }
 
-bool EntityTree::updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, bool allowLockChange) {
+bool EntityTree::updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, const SharedNodePointer& senderNode) {
     EntityTreeElement* containingElement = getContainingElement(entityID);
     if (!containingElement) {
         qCDebug(entities) << "UNEXPECTED!!!!  EntityTree::updateEntity() entityID doesn't exist!!! entityID=" << entityID;
@@ -99,22 +99,34 @@ bool EntityTree::updateEntity(const EntityItemID& entityID, const EntityItemProp
         return false;
     }
 
-    return updateEntityWithElement(existingEntity, properties, containingElement, allowLockChange);
+    return updateEntityWithElement(existingEntity, properties, containingElement, senderNode);
 }
 
-bool EntityTree::updateEntity(EntityItem* entity, const EntityItemProperties& properties, bool allowLockChange) {
+bool EntityTree::updateEntity(EntityItem* entity, const EntityItemProperties& properties, const SharedNodePointer& senderNode) {
     EntityTreeElement* containingElement = getContainingElement(entity->getEntityItemID());
     if (!containingElement) {
         qCDebug(entities) << "UNEXPECTED!!!!  EntityTree::updateEntity() entity-->element lookup failed!!! entityID=" 
             << entity->getEntityItemID();
         return false;
     }
-    return updateEntityWithElement(entity, properties, containingElement, allowLockChange);
+    return updateEntityWithElement(entity, properties, containingElement, senderNode);
 }
 
 bool EntityTree::updateEntityWithElement(EntityItem* entity, const EntityItemProperties& origProperties, 
-                                         EntityTreeElement* containingElement, bool allowLockChange) {
+                                         EntityTreeElement* containingElement, const SharedNodePointer& senderNode) {
     EntityItemProperties properties = origProperties;
+
+    bool allowLockChange;
+    QUuid senderID;
+    if (senderNode.isNull()) {
+        auto nodeList = DependencyManager::get<NodeList>();
+        allowLockChange = nodeList->getThisNodeCanAdjustLocks();
+        senderID = nodeList->getSessionUUID();
+    } else {
+        allowLockChange = senderNode->getCanAdjustLocks();
+        senderID = senderNode->getUUID();
+    }
+
     if (!allowLockChange && (entity->getLocked() != properties.getLocked())) {
         qCDebug(entities) << "Refusing disallowed lock adjustment.";
         return false;
@@ -134,22 +146,41 @@ bool EntityTree::updateEntityWithElement(EntityItem* entity, const EntityItemPro
             }
         }
     } else {
-        if (properties.simulatorIDChanged() &&
-            !entity->getSimulatorID().isNull() &&
-            properties.getSimulatorID() != entity->getSimulatorID()) {
-            // A Node is trying to take ownership of the simulation of this entity from another Node.  Only allow this
-            // if ownership hasn't recently changed.
-            if (usecTimestampNow() - entity->getSimulatorIDChangedTime() < SIMULATOR_CHANGE_LOCKOUT_PERIOD) {
-                qCDebug(entities) << "simulator_change_lockout_period:"
-                                  << entity->getSimulatorID() << "to" << properties.getSimulatorID();
+        if (getIsServer()) {
+            bool simulationBlocked = !entity->getSimulatorID().isNull();
+            if (properties.simulatorIDChanged()) {
+                QUuid submittedID = properties.getSimulatorID();
+                // a legit interface will only submit their own ID or NULL:
+                if (submittedID.isNull()) {
+                    if (entity->getSimulatorID() == senderID) {
+                        // We only allow the simulation owner to clear their own simulationID's.
+                        simulationBlocked = false;
+                    }
+                    // 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) {
+                        simulationBlocked = false;
+                    } else {
+                        // the sender is trying to steal ownership from another simulator
+                        // so we apply the ownership change filter
+                        if (usecTimestampNow() - entity->getSimulatorIDChangedTime() > SIMULATOR_CHANGE_LOCKOUT_PERIOD) {
+                            simulationBlocked = false;
+                        }
+                    }
+                } else {
+                    // the entire update is suspect --> ignore it
+                    return false;
+                }
+            }
+            if (simulationBlocked) {
                 // squash the physics-related changes.
                 properties.setSimulatorIDChanged(false);
                 properties.setPositionChanged(false);
                 properties.setRotationChanged(false);
-            } else {
-                qCDebug(entities) << "allowing simulatorID change";
             }
         }
+        // else client accepts what the server says
 
         QString entityScriptBefore = entity->getScript();
         uint32_t preFlags = entity->getDirtyFlags();
@@ -664,7 +695,7 @@ int EntityTree::processEditPacketData(PacketType packetType, const unsigned char
                             qCDebug(entities) << "User [" << senderNode->getUUID() << "] editing entity. ID:" << entityItemID;
                             qCDebug(entities) << "   properties:" << properties;
                         }
-                        updateEntity(entityItemID, properties, senderNode->getCanAdjustLocks());
+                        updateEntity(entityItemID, properties, senderNode);
                         existingEntity->markAsChangedOnServer();
                     } else {
                         qCDebug(entities) << "User attempted to edit an unknown entity. ID:" << entityItemID;
diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h
index f99160f4ed..c0cc81b3af 100644
--- a/libraries/entities/src/EntityTree.h
+++ b/libraries/entities/src/EntityTree.h
@@ -88,10 +88,10 @@ public:
     EntityItem* addEntity(const EntityItemID& entityID, const EntityItemProperties& properties);
 
     // use this method if you only know the entityID
-    bool updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, bool allowLockChange);
+    bool updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, const SharedNodePointer& senderNode = SharedNodePointer(nullptr));
 
     // use this method if you have a pointer to the entity (avoid an extra entity lookup)
-    bool updateEntity(EntityItem* entity, const EntityItemProperties& properties, bool allowLockChange);
+    bool updateEntity(EntityItem* entity, const EntityItemProperties& properties, const SharedNodePointer& senderNode = SharedNodePointer(nullptr));
 
     void deleteEntity(const EntityItemID& entityID, bool force = false, bool ignoreWarnings = false);
     void deleteEntities(QSet<EntityItemID> entityIDs, bool force = false, bool ignoreWarnings = false);
@@ -178,7 +178,8 @@ private:
 
     void processRemovedEntities(const DeleteEntityOperator& theOperator);
     bool updateEntityWithElement(EntityItem* entity, const EntityItemProperties& properties, 
-                                 EntityTreeElement* containingElement, bool allowLockChange);
+                                 EntityTreeElement* containingElement,
+                                 const SharedNodePointer& senderNode = SharedNodePointer(nullptr));
     static bool findNearPointOperation(OctreeElement* element, void* extraData);
     static bool findInSphereOperation(OctreeElement* element, void* extraData);
     static bool findInCubeOperation(OctreeElement* element, void* extraData);
diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp
index e5d8c9dc81..4ac66678e7 100644
--- a/libraries/physics/src/EntityMotionState.cpp
+++ b/libraries/physics/src/EntityMotionState.cpp
@@ -35,8 +35,9 @@ EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItem* entity
     _serverGravity(0.0f),
     _serverAcceleration(0.0f),
     _accelerationNearlyGravityCount(0),
-    _shouldClaimSimulationOwnership(false),
-    _movingStepsWithoutSimulationOwner(0)
+    _touchesOurSimulation(false),
+    _framesSinceSimulatorBid(0),
+    _movingFramesWithoutSimulationOwner(0)
 {
     _type = MOTION_STATE_TYPE_ENTITY;
     assert(entity != nullptr);
@@ -60,6 +61,15 @@ void EntityMotionState::updateServerPhysicsVariables(uint32_t flags) {
     if (flags & EntityItem::DIRTY_ANGULAR_VELOCITY) {
         _serverAngularVelocity = _entity->getAngularVelocity();
     }
+    if (flags & EntityItem::DIRTY_SIMULATOR_ID) {
+        auto nodeList = DependencyManager::get<NodeList>();
+        const QUuid& sessionID = nodeList->getSessionUUID();
+        if (_entity->getSimulatorID() != sessionID) {
+            _touchesOurSimulation = false;
+            _movingFramesWithoutSimulationOwner = 0;
+            _framesSinceSimulatorBid = 0;
+        }
+    }
 }
 
 // virtual
@@ -143,19 +153,16 @@ void EntityMotionState::setWorldTransform(const btTransform& worldTrans) {
 
     _entity->setLastSimulated(usecTimestampNow());
 
-    // if (_entity->getSimulatorID().isNull() && isMoving()) {
-    if (_entity->getSimulatorID().isNull() && isMovingVsServer()) {
-        // if object is moving and has no owner, attempt to claim simulation ownership.
-        _movingStepsWithoutSimulationOwner++;
+    if (_entity->getSimulatorID().isNull()) {
+        _movingFramesWithoutSimulationOwner++;
+
+        const uint32_t ownershipClaimDelay = 50; // TODO -- how to pick this?  based on meters from our characterController?
+        if (_movingFramesWithoutSimulationOwner > ownershipClaimDelay) {
+            //qDebug() << "Warning -- claiming something I saw moving." << getName();
+            _touchesOurSimulation = true;
+        }
     } else {
-        _movingStepsWithoutSimulationOwner = 0;
-    }
-
-    uint32_t ownershipClaimDelay = 50; // TODO -- how to pick this?  based on meters from our characterController?
-
-    if (_movingStepsWithoutSimulationOwner > ownershipClaimDelay) {
-        //qDebug() << "Warning -- claiming something I saw moving." << getName();
-        setShouldClaimSimulationOwnership(true);
+        _movingFramesWithoutSimulationOwner = 0;
     }
 
     #ifdef WANT_DEBUG
@@ -177,8 +184,12 @@ void EntityMotionState::computeObjectShapeInfo(ShapeInfo& shapeInfo) {
 // we alwasy resend packets for objects that have stopped moving up to some max limit.
 const int MAX_NUM_NON_MOVING_UPDATES = 5;
 
-bool EntityMotionState::doesNotNeedToSendUpdate() const { 
-    return !_body || (_body->isActive() && _numNonMovingUpdates > MAX_NUM_NON_MOVING_UPDATES);
+bool EntityMotionState::doesNotNeedToSendUpdate(const QUuid& sessionID) const { 
+    if (!_body || !_entity) {
+        return true;
+    }
+
+    return (sessionID != _entity->getSimulatorID() && !_touchesOurSimulation);
 }
 
 bool EntityMotionState::remoteSimulationOutOfSync(uint32_t simulationStep) {
@@ -191,6 +202,7 @@ bool EntityMotionState::remoteSimulationOutOfSync(uint32_t simulationStep) {
         _serverVelocity = bulletToGLM(_body->getLinearVelocity());
         _serverAngularVelocity = bulletToGLM(_body->getAngularVelocity());
         _lastStep = simulationStep;
+        _sentMoving = false;
         return false;
     }
     
@@ -202,35 +214,37 @@ bool EntityMotionState::remoteSimulationOutOfSync(uint32_t simulationStep) {
 
     int numSteps = simulationStep - _lastStep;
     float dt = (float)(numSteps) * PHYSICS_ENGINE_FIXED_SUBSTEP;
-    _lastStep = simulationStep;
-    bool isActive = _body->isActive();
 
     const float NON_MOVING_UPDATE_PERIOD = 1.0f;
-    if (_sentMoving) {
-        if (!isActive) {
-            // object has gone inactive but our last send was moving --> send non-moving update immediately
-            return true;
-        }
-    } else if (dt > NON_MOVING_UPDATE_PERIOD && _numNonMovingUpdates < MAX_NUM_NON_MOVING_UPDATES) {
-        // RELIABLE_SEND_HACK: since we're not yet using a reliable method for non-moving update packets 
-        // we repeat these at the the MAX period above, and only send a limited number of them.
+    if (!_sentMoving) {
+        // we resend non-moving update every NON_MOVING_UPDATE_PERIOD
+        // until it is removed from the outgoing updates 
+        // (which happens when we don't own the simulation and it isn't touching our simulation)
+        return (dt > NON_MOVING_UPDATE_PERIOD);
+    }
+
+    bool isActive = _body->isActive();
+    if (!isActive) {
+        // object has gone inactive but our last send was moving --> send non-moving update immediately
         return true;
     }
 
+    _lastStep = simulationStep;
+    if (glm::length2(_serverVelocity) > 0.0f) {
+        _serverVelocity += _serverAcceleration * dt;
+        _serverVelocity *= powf(1.0f - _body->getLinearDamping(), dt);
+        _serverPosition += dt * _serverVelocity;
+    }
+
     // Else we measure the error between current and extrapolated transform (according to expected behavior 
     // of remote EntitySimulation) and return true if the error is significant.
 
     // NOTE: math is done in the simulation-frame, which is NOT necessarily the same as the world-frame 
     // due to _worldOffset.
+    // TODO: compensate for _worldOffset offset here
 
     // compute position error
-    if (glm::length2(_serverVelocity) > 0.0f) {
-        _serverVelocity += _serverAcceleration * dt;
-        _serverVelocity *= powf(1.0f - _body->getLinearDamping(), dt);
-        _serverPosition += dt * _serverVelocity;
-    }
 
-    // TODO: compensate for simulation offset here
     btTransform worldTrans = _body->getWorldTransform();
     glm::vec3 position = bulletToGLM(worldTrans.getOrigin());
     
@@ -283,31 +297,36 @@ bool EntityMotionState::remoteSimulationOutOfSync(uint32_t simulationStep) {
     return (fabsf(glm::dot(actualRotation, _serverRotation)) < MIN_ROTATION_DOT);
 }
 
-bool EntityMotionState::shouldSendUpdate(uint32_t simulationFrame) {
-    if (!_entity || !remoteSimulationOutOfSync(simulationFrame)) {
+bool EntityMotionState::shouldSendUpdate(uint32_t simulationFrame, const QUuid& sessionID) {
+    // NOTE: we expect _entity and _body to be valid in this context, since shouldSendUpdate() is only called 
+    // after doesNotNeedToSendUpdate() returns false and that call should return 'true' if _entity or _body are NULL. 
+    assert(_entity);
+    assert(_body);
+
+    if (!remoteSimulationOutOfSync(simulationFrame)) {
         return false;
     }
 
-    if (getShouldClaimSimulationOwnership()) {
+    if (_entity->getSimulatorID() == sessionID) {
+        // we own the simulation
         return true;
     }
 
-    auto nodeList = DependencyManager::get<NodeList>();
-    const QUuid& myNodeID = nodeList->getSessionUUID();
-    const QUuid& simulatorID = _entity->getSimulatorID();
-
-    if (simulatorID != myNodeID) {
-        // some other Node owns the simulating of this, so don't broadcast the results of local simulation.
-        return false;
+    const uint32_t FRAMES_BETWEEN_OWNERSHIP_CLAIMS = 30;
+    if (_touchesOurSimulation) {
+        ++_framesSinceSimulatorBid;
+        if (_framesSinceSimulatorBid > FRAMES_BETWEEN_OWNERSHIP_CLAIMS) {
+            // we don't own the simulation, but it's time to bid for it
+            return true;
+        }
     }
 
-    return true;
+    return false;
 }
 
-void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_t step) {
-    if (!_entity || !_entity->isKnownID()) {
-        return; // never update entities that are unknown
-    }
+void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, const QUuid& sessionID, uint32_t step) {
+    assert(_entity);
+    assert(_entity->isKnownID());
 
     bool active = _body->isActive();
     if (!active) {
@@ -350,11 +369,11 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_
     _serverAcceleration = _entity->getAcceleration();
     _serverAngularVelocity = _entity->getAngularVelocity();
 
-    _sentMoving = _serverVelocity != glm::vec3(0.0f) || _serverAngularVelocity != glm::vec3(0.0f);
+    _sentMoving = _serverVelocity != glm::vec3(0.0f) || _serverAngularVelocity != _serverVelocity || _serverAcceleration != _serverVelocity;
 
     EntityItemProperties properties = _entity->getProperties();
 
-    // explicitly set the properties that changed
+    // explicitly set the properties that changed so that they will be packed
     properties.setPosition(_serverPosition);
     properties.setRotation(_serverRotation);
     properties.setVelocity(_serverVelocity);
@@ -386,23 +405,20 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_
         properties.setLastEdited(_entity->getLastEdited());
     }
 
-    auto nodeList = DependencyManager::get<NodeList>();
-    QUuid myNodeID = nodeList->getSessionUUID();
-    QUuid simulatorID = _entity->getSimulatorID();
-
-    if (getShouldClaimSimulationOwnership()) {
-        // we think we should own it, so we tell the server that we do, 
-        // but we don't remember that we own it...
-        // instead we expect the sever to tell us later whose ownership it has accepted
-        properties.setSimulatorID(myNodeID);
-        setShouldClaimSimulationOwnership(false);
-    } else if (simulatorID == myNodeID 
-            && !_sentMoving 
-            && _numNonMovingUpdates == MAX_NUM_NON_MOVING_UPDATES) {
-        // we own it, the entity has stopped, and we're sending the last non-moving update
-        // --> give up ownership
-        _entity->setSimulatorID(QUuid());
-        properties.setSimulatorID(QUuid());
+    if (sessionID == _entity->getSimulatorID()) {
+        // we think we own the simulation
+        if (!_sentMoving) {
+            // 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.setSimulatorID(QUuid());
+            _touchesOurSimulation = false;
+        } else {
+            // explicitly set the property's simulatorID so that it is flagged as changed and will be packed
+            properties.setSimulatorID(sessionID);
+        }
+    } else {
+        // we don't own the simulation for this entity yet, but we're sending a bid for it
+        properties.setSimulatorID(sessionID);
     }
 
     if (EntityItem::getSendPhysicsUpdates()) {
@@ -451,7 +467,7 @@ QUuid EntityMotionState::getSimulatorID() const {
 
 // virtual
 void EntityMotionState::bump() {
-    setShouldClaimSimulationOwnership(true);
+    _touchesOurSimulation = true;
 }
 
 void EntityMotionState::resetMeasuredBodyAcceleration() {
diff --git a/libraries/physics/src/EntityMotionState.h b/libraries/physics/src/EntityMotionState.h
index 6028662aa0..afb7d257a6 100644
--- a/libraries/physics/src/EntityMotionState.h
+++ b/libraries/physics/src/EntityMotionState.h
@@ -46,13 +46,11 @@ public:
 
     virtual void computeObjectShapeInfo(ShapeInfo& shapeInfo);
 
-    bool doesNotNeedToSendUpdate() const;
+    // TODO: Andrew to rename doesNotNeedToSendUpdate()
+    bool doesNotNeedToSendUpdate(const QUuid& sessionID) const;
     bool remoteSimulationOutOfSync(uint32_t simulationStep);
-    bool shouldSendUpdate(uint32_t simulationFrame);
-    void sendUpdate(OctreeEditPacketSender* packetSender, uint32_t step);
-
-    void setShouldClaimSimulationOwnership(bool value) { _shouldClaimSimulationOwnership = value; }
-    bool getShouldClaimSimulationOwnership() { return _shouldClaimSimulationOwnership; }
+    bool shouldSendUpdate(uint32_t simulationFrame, const QUuid& sessionID);
+    void sendUpdate(OctreeEditPacketSender* packetSender, const QUuid& sessionID, uint32_t step);
 
     virtual uint32_t getAndClearIncomingDirtyFlags() const;
 
@@ -109,8 +107,9 @@ protected:
     glm::vec3 _measuredAcceleration;
 
     quint8 _accelerationNearlyGravityCount;
-    bool _shouldClaimSimulationOwnership;
-    quint32 _movingStepsWithoutSimulationOwner;
+    bool _touchesOurSimulation;
+    uint32_t _framesSinceOwnershipBid;
+    uint32_t _movingFramesWithoutSimulationOwner;
 };
 
 #endif // hifi_EntityMotionState_h
diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp
index ed7b986800..f1be0c2145 100644
--- a/libraries/physics/src/PhysicalEntitySimulation.cpp
+++ b/libraries/physics/src/PhysicalEntitySimulation.cpp
@@ -195,12 +195,22 @@ void PhysicalEntitySimulation::handleOutgoingChanges(VectorOfMotionStates& motio
             EntityMotionState* entityState = static_cast<EntityMotionState*>(state);
             EntityItem* entity = entityState->getEntity();
             if (entity) {
-                _outgoingChanges.insert(entityState);
+                if (entity->isKnownID()) {
+                    _outgoingChanges.insert(entityState);
+                }
                 _entitiesToSort.insert(entityState->getEntity());
             }
         }
     }
 
+    auto nodeList = DependencyManager::get<NodeList>();
+    const QUuid& sessionID = nodeList->getSessionUUID();
+    if (sessionID.isNull()) {
+        // no updates to send
+        _outgoingChanges.clear();
+        return;
+    }
+
     // send outgoing packets
     uint32_t numSubsteps = _physicsEngine->getNumSubsteps();
     if (_lastStepSendPackets != numSubsteps) {
@@ -209,10 +219,10 @@ void PhysicalEntitySimulation::handleOutgoingChanges(VectorOfMotionStates& motio
         QSet<EntityMotionState*>::iterator stateItr = _outgoingChanges.begin();
         while (stateItr != _outgoingChanges.end()) {
             EntityMotionState* state = *stateItr;
-            if (state->doesNotNeedToSendUpdate()) {
+            if (state->doesNotNeedToSendUpdate(sessionID)) {
                 stateItr = _outgoingChanges.erase(stateItr);
-            } else if (state->shouldSendUpdate(numSubsteps)) {
-                state->sendUpdate(_entityPacketSender, numSubsteps);
+            } else if (state->shouldSendUpdate(numSubsteps, sessionID)) {
+                state->sendUpdate(_entityPacketSender, sessionID, numSubsteps);
                 ++stateItr;
             } else {
                 ++stateItr;
diff --git a/libraries/physics/src/PhysicsEngine.cpp b/libraries/physics/src/PhysicsEngine.cpp
index ea74a87286..bfd0b6cb28 100644
--- a/libraries/physics/src/PhysicsEngine.cpp
+++ b/libraries/physics/src/PhysicsEngine.cpp
@@ -147,6 +147,7 @@ void PhysicsEngine::deleteObjects(VectorOfMotionStates& objects) {
         // NOTE: setRigidBody() modifies body->m_userPointer so we should clear the MotionState's body BEFORE deleting it.
         btRigidBody* body = object->getRigidBody();
         object->setRigidBody(nullptr);
+        body->setMotionState(nullptr);
         delete body;
         object->releaseShape();
         delete object;
@@ -161,6 +162,7 @@ void PhysicsEngine::deleteObjects(SetOfMotionStates& objects) {
     
         // NOTE: setRigidBody() modifies body->m_userPointer so we should clear the MotionState's body BEFORE deleting it.
         object->setRigidBody(nullptr);
+        body->setMotionState(nullptr);
         delete body;
         object->releaseShape();
         delete object;
diff --git a/tests/octree/src/ModelTests.cpp b/tests/octree/src/ModelTests.cpp
index e4309100af..2e2b873115 100644
--- a/tests/octree/src/ModelTests.cpp
+++ b/tests/octree/src/ModelTests.cpp
@@ -103,7 +103,7 @@ void EntityTests::entityTreeTests(bool verbose) {
 
         properties.setPosition(newPosition);
 
-        tree.updateEntity(entityID, properties, true);
+        tree.updateEntity(entityID, properties);
         
         float targetRadius = oneMeter * 2.0f;
         const EntityItem* foundEntityByRadius = tree.findClosestEntity(positionNearOrigin, targetRadius);
@@ -143,7 +143,7 @@ void EntityTests::entityTreeTests(bool verbose) {
 
         properties.setPosition(newPosition);
 
-        tree.updateEntity(entityID, properties, true);
+        tree.updateEntity(entityID, properties);
         
         float targetRadius = oneMeter * 2.0f;
         const EntityItem* foundEntityByRadius = tree.findClosestEntity(positionAtCenter, targetRadius);