Merge pull request #12185 from AndrewMeadows/physics-shouldnt-set-dirty-flags

fix race condition whereby physics may incorrectly overwrite script/network property change

libraries/entities/src/EntityItem.h

@@ -368,6 +368,7 @@ public:
     void* getPhysicsInfo() const { return _physicsInfo; }
 
     void setPhysicsInfo(void* data) { _physicsInfo = data; }
+
     EntityTreeElementPointer getElement() const { return _element; }
     EntityTreePointer getTree() const;
     virtual SpatialParentTree* getParentTree() const override;

libraries/physics/src/EntityMotionState.cpp

@@ -256,25 +256,32 @@ void EntityMotionState::setWorldTransform(const btTransform& worldTrans) {
     assert(_entity);
     assert(entityTreeIsLocked());
     measureBodyAcceleration();
-    bool positionSuccess;
+
-    _entity->setWorldPosition(bulletToGLM(worldTrans.getOrigin()) + ObjectMotionState::getWorldOffset(), positionSuccess, false);
+    // If transform or velocities are flagged as dirty it means a network or scripted change
-    if (!positionSuccess) {
+    // occured between the beginning and end of the stepSimulation() and we DON'T want to apply
-        static QString repeatedMessage =
+    // these physics simulation results.
-            LogHandler::getInstance().addRepeatedMessageRegex("EntityMotionState::setWorldTransform "
+    uint32_t flags = _entity->getDirtyFlags() & (Simulation::DIRTY_TRANSFORM | Simulation::DIRTY_VELOCITIES);
-                                                              "setPosition failed.*");
+    if (!flags) {
-        qCDebug(physics) << "EntityMotionState::setWorldTransform setPosition failed" << _entity->getID();
+        // flags are clear
+        _entity->setWorldTransform(bulletToGLM(worldTrans.getOrigin()), bulletToGLM(worldTrans.getRotation()));
+        _entity->setWorldVelocity(getBodyLinearVelocity());
+        _entity->setWorldAngularVelocity(getBodyAngularVelocity());
+        _entity->setLastSimulated(usecTimestampNow());
+    } else {
+        // only set properties NOT flagged
+        if (!(flags & Simulation::DIRTY_TRANSFORM)) {
+            _entity->setWorldTransform(bulletToGLM(worldTrans.getOrigin()), bulletToGLM(worldTrans.getRotation()));
+        }
+        if (!(flags & Simulation::DIRTY_LINEAR_VELOCITY)) {
+            _entity->setWorldVelocity(getBodyLinearVelocity());
+        }
+        if (!(flags & Simulation::DIRTY_ANGULAR_VELOCITY)) {
+            _entity->setWorldAngularVelocity(getBodyAngularVelocity());
+        }
+        if (flags != (Simulation::DIRTY_TRANSFORM | Simulation::DIRTY_VELOCITIES)) {
+            _entity->setLastSimulated(usecTimestampNow());
+        }
     }
-    bool orientationSuccess;
-    _entity->setWorldOrientation(bulletToGLM(worldTrans.getRotation()), orientationSuccess, false);
-    if (!orientationSuccess) {
-        static QString repeatedMessage =
-            LogHandler::getInstance().addRepeatedMessageRegex("EntityMotionState::setWorldTransform "
-                                                              "setOrientation failed.*");
-        qCDebug(physics) << "EntityMotionState::setWorldTransform setOrientation failed" << _entity->getID();
-    }
-    _entity->setVelocity(getBodyLinearVelocity());
-    _entity->setAngularVelocity(getBodyAngularVelocity());
-    _entity->setLastSimulated(usecTimestampNow());
 
     if (_entity->getSimulatorID().isNull()) {
         _loopsWithoutOwner++;
@@ -530,9 +537,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_
 
     if (!_body->isActive()) {
         // make sure all derivatives are zero
-        _entity->setVelocity(Vectors::ZERO);
+        zeroCleanObjectVelocities();
-        _entity->setAngularVelocity(Vectors::ZERO);
-        _entity->setAcceleration(Vectors::ZERO);
         _numInactiveUpdates++;
     } else {
         glm::vec3 gravity = _entity->getGravity();
@@ -559,9 +564,7 @@ void EntityMotionState::sendUpdate(OctreeEditPacketSender* packetSender, uint32_
             if (movingSlowly) {
                 // velocities might not be zero, but we'll fake them as such, which will hopefully help convince
                 // other simulating observers to deactivate their own copies
-                glm::vec3 zero(0.0f);
+                zeroCleanObjectVelocities();
-                _entity->setVelocity(zero);
-                _entity->setAngularVelocity(zero);
             }
         }
         _numInactiveUpdates = 0;
@@ -818,3 +821,22 @@ bool EntityMotionState::shouldBeLocallyOwned() const {
 void EntityMotionState::upgradeOutgoingPriority(uint8_t priority) {
     _outgoingPriority = glm::max<uint8_t>(_outgoingPriority, priority);
 }
+
+void EntityMotionState::zeroCleanObjectVelocities() const {
+    // If transform or velocities are flagged as dirty it means a network or scripted change
+    // occured between the beginning and end of the stepSimulation() and we DON'T want to apply
+    // these physics simulation results.
+    uint32_t flags = _entity->getDirtyFlags() & (Simulation::DIRTY_TRANSFORM | Simulation::DIRTY_VELOCITIES);
+    if (!flags) {
+        _entity->setWorldVelocity(glm::vec3(0.0f));
+        _entity->setWorldAngularVelocity(glm::vec3(0.0f));
+    } else {
+        if (!(flags & Simulation::DIRTY_LINEAR_VELOCITY)) {
+            _entity->setWorldVelocity(glm::vec3(0.0f));
+        }
+        if (!(flags & Simulation::DIRTY_ANGULAR_VELOCITY)) {
+            _entity->setWorldAngularVelocity(glm::vec3(0.0f));
+        }
+    }
+    _entity->setAcceleration(glm::vec3(0.0f));
+}

libraries/physics/src/EntityMotionState.h

@@ -87,6 +87,7 @@ public:
 protected:
     // changes _outgoingPriority only if priority is larger
     void upgradeOutgoingPriority(uint8_t priority);
+    void zeroCleanObjectVelocities() const;
 
     #ifdef WANT_DEBUG_ENTITY_TREE_LOCKS
     bool entityTreeIsLocked() const;

libraries/shared/src/GLMHelpers.h

@@ -253,6 +253,7 @@ glm::vec2 getFacingDir2D(const glm::mat4& m);
 
 inline bool isNaN(const glm::vec3& value) { return isNaN(value.x) || isNaN(value.y) || isNaN(value.z); }
 inline bool isNaN(const glm::quat& value) { return isNaN(value.w) || isNaN(value.x) || isNaN(value.y) || isNaN(value.z); }
+inline bool isNaN(const glm::mat3& value) { return isNaN(value * glm::vec3(1.0f)); }
 
 glm::mat4 orthoInverse(const glm::mat4& m);
 

libraries/shared/src/SpatiallyNestable.cpp

@@ -464,6 +464,36 @@ glm::vec3 SpatiallyNestable::localToWorldDimensions(const glm::vec3& dimensions,
     return dimensions;
 }
 
+void SpatiallyNestable::setWorldTransform(const glm::vec3& position, const glm::quat& orientation) {
+    // guard against introducing NaN into the transform
+    if (isNaN(orientation) || isNaN(position)) {
+        return;
+    }
+
+    bool changed = false;
+    bool success = true;
+    Transform parentTransform = getParentTransform(success);
+    _transformLock.withWriteLock([&] {
+        Transform myWorldTransform;
+        Transform::mult(myWorldTransform, parentTransform, _transform);
+        if (myWorldTransform.getRotation() != orientation) {
+            changed = true;
+            myWorldTransform.setRotation(orientation);
+        }
+        if (myWorldTransform.getTranslation() != position) {
+            changed = true;
+            myWorldTransform.setTranslation(position);
+        }
+        if (changed) {
+            Transform::inverseMult(_transform, parentTransform, myWorldTransform);
+            _translationChanged = usecTimestampNow();
+        }
+    });
+    if (success && changed) {
+        locationChanged(false);
+    }
+}
+
 glm::vec3 SpatiallyNestable::getWorldPosition(bool& success) const {
     return getTransform(success).getTranslation();
 }

libraries/shared/src/SpatiallyNestable.h

@@ -87,6 +87,7 @@ public:
 
     virtual Transform getParentTransform(bool& success, int depth = 0) const;
 
+    void setWorldTransform(const glm::vec3& position, const glm::quat& orientation);
     virtual glm::vec3 getWorldPosition(bool& success) const;
     virtual glm::vec3 getWorldPosition() const;
     virtual void setWorldPosition(const glm::vec3& position, bool& success, bool tellPhysics = true);