Merge pull request #14766 from AndrewMeadows/grab-it

case 20701: fixes for grab

interface/src/avatar/AvatarActionHold.cpp

@@ -288,39 +288,37 @@ void AvatarActionHold::doKinematicUpdate(float deltaTimeStep) {
             glm::vec3 oneFrameVelocity = (_positionalTarget - _previousPositionalTarget) / deltaTimeStep;
 
             _measuredLinearVelocities[_measuredLinearVelocitiesIndex++] = oneFrameVelocity;
-            if (_measuredLinearVelocitiesIndex >= AvatarActionHold::velocitySmoothFrames) {
+            _measuredLinearVelocitiesIndex %= AvatarActionHold::velocitySmoothFrames;
-                _measuredLinearVelocitiesIndex = 0;
+        }
+
+        if (_kinematicSetVelocity) {
+            glm::vec3 measuredLinearVelocity = _measuredLinearVelocities[0];
+            for (int i = 1; i < AvatarActionHold::velocitySmoothFrames; i++) {
+                // there is a bit of lag between when someone releases the trigger and when the software reacts to
+                // the release.  we calculate the velocity from previous frames but we don't include several
+                // of the most recent.
+                //
+                // if _measuredLinearVelocitiesIndex is
+                //     0 -- ignore i of 3 4 5
+                //     1 -- ignore i of 4 5 0
+                //     2 -- ignore i of 5 0 1
+                //     3 -- ignore i of 0 1 2
+                //     4 -- ignore i of 1 2 3
+                //     5 -- ignore i of 2 3 4
+
+                // This code is now disabled, but I'm leaving it commented-out because I suspect it will come back.
+                // if ((i + 1) % AvatarActionHold::velocitySmoothFrames == _measuredLinearVelocitiesIndex ||
+                //     (i + 2) % AvatarActionHold::velocitySmoothFrames == _measuredLinearVelocitiesIndex ||
+                //     (i + 3) % AvatarActionHold::velocitySmoothFrames == _measuredLinearVelocitiesIndex) {
+                //     continue;
+                // }
+
+                measuredLinearVelocity += _measuredLinearVelocities[i];
             }
-        }
+            measuredLinearVelocity /= (float)(AvatarActionHold::velocitySmoothFrames
-
-        glm::vec3 measuredLinearVelocity;
-        for (int i = 0; i < AvatarActionHold::velocitySmoothFrames; i++) {
-            // there is a bit of lag between when someone releases the trigger and when the software reacts to
-            // the release.  we calculate the velocity from previous frames but we don't include several
-            // of the most recent.
-            //
-            // if _measuredLinearVelocitiesIndex is
-            //     0 -- ignore i of 3 4 5
-            //     1 -- ignore i of 4 5 0
-            //     2 -- ignore i of 5 0 1
-            //     3 -- ignore i of 0 1 2
-            //     4 -- ignore i of 1 2 3
-            //     5 -- ignore i of 2 3 4
-
-            // This code is now disabled, but I'm leaving it commented-out because I suspect it will come back.
-            // if ((i + 1) % AvatarActionHold::velocitySmoothFrames == _measuredLinearVelocitiesIndex ||
-            //     (i + 2) % AvatarActionHold::velocitySmoothFrames == _measuredLinearVelocitiesIndex ||
-            //     (i + 3) % AvatarActionHold::velocitySmoothFrames == _measuredLinearVelocitiesIndex) {
-            //     continue;
-            // }
-
-            measuredLinearVelocity += _measuredLinearVelocities[i];
-        }
-        measuredLinearVelocity /= (float)(AvatarActionHold::velocitySmoothFrames
                                           // - 3  // 3 because of the 3 we skipped, above
                                           );
 
-        if (_kinematicSetVelocity) {
             rigidBody->setLinearVelocity(glmToBullet(measuredLinearVelocity));
             rigidBody->setAngularVelocity(glmToBullet(_angularVelocityTarget));
         }

interface/src/avatar/AvatarManager.cpp

@@ -450,6 +450,7 @@ void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar
         _spaceProxiesToDelete.push_back(avatar->getSpaceIndex());
     }
     AvatarHashMap::handleRemovedAvatar(avatar, removalReason);
+    avatar->tearDownGrabs();
 
     avatar->die();
     queuePhysicsChange(avatar);

interface/src/avatar/MyAvatar.cpp

@@ -866,7 +866,7 @@ void MyAvatar::simulate(float deltaTime, bool inView) {
     // and all of its joints, now update our attachements.
     Avatar::simulateAttachments(deltaTime);
     relayJointDataToChildren();
-    if (updateGrabs()) {
+    if (applyGrabChanges()) {
         _cauterizationNeedsUpdate = true;
     }
 
@@ -5373,7 +5373,7 @@ void MyAvatar::releaseGrab(const QUuid& grabID) {
 
     _avatarGrabsLock.withWriteLock([&] {
         if (_avatarGrabData.remove(grabID)) {
-            _deletedAvatarGrabs.insert(grabID);
+            _grabsToDelete.push_back(grabID);
             tellHandler = true;
         }
     });

interface/src/avatar/OtherAvatar.cpp

@@ -217,7 +217,7 @@ void OtherAvatar::simulate(float deltaTime, bool inView) {
 
     {
         PROFILE_RANGE(simulation, "grabs");
-        updateGrabs();
+        applyGrabChanges();
     }
 
     updateFadingStatus();

libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp

@@ -324,88 +324,79 @@ void Avatar::removeAvatarEntitiesFromTree() {
     }
 }
 
-bool Avatar::updateGrabs() {
+bool Avatar::applyGrabChanges() {
+    if (!_avatarGrabDataChanged && _grabsToChange.empty() && _grabsToDelete.empty()) {
+        // early exit for most common case: nothing to do
+        return false;
+    }
+
     bool grabAddedOrRemoved = false;
-    // update the Grabs according to any changes in _avatarGrabData
     _avatarGrabsLock.withWriteLock([&] {
         if (_avatarGrabDataChanged) {
+            // collect changes in _avatarGrabData
             foreach (auto grabID, _avatarGrabData.keys()) {
-                AvatarGrabMap::iterator grabItr = _avatarGrabs.find(grabID);
+                MapOfGrabs::iterator itr = _avatarGrabs.find(grabID);
-                if (grabItr == _avatarGrabs.end()) {
+                if (itr == _avatarGrabs.end()) {
                     GrabPointer grab = std::make_shared<Grab>();
                     grab->fromByteArray(_avatarGrabData.value(grabID));
                     _avatarGrabs[grabID] = grab;
-                    _changedAvatarGrabs.insert(grabID);
+                    _grabsToChange.insert(grabID);
                 } else {
-                    GrabPointer grab = grabItr.value();
+                    bool changed = itr->second->fromByteArray(_avatarGrabData.value(grabID));
-                    bool changed = grab->fromByteArray(_avatarGrabData.value(grabID));
                     if (changed) {
-                        _changedAvatarGrabs.insert(grabID);
+                        _grabsToChange.insert(grabID);
                     }
                 }
             }
             _avatarGrabDataChanged = false;
         }
 
-        auto treeRenderer = DependencyManager::get<EntityTreeRenderer>();
+        // delete _avatarGrabs
-        auto entityTree = treeRenderer ? treeRenderer->getTree() : nullptr;
+        VectorOfIDs undeleted;
-        EntityEditPacketSender* packetSender = treeRenderer ? treeRenderer->getPacketSender() : nullptr;
+        for (const auto& id : _grabsToDelete) {
-        auto sessionID = DependencyManager::get<NodeList>()->getSessionUUID();
+            MapOfGrabs::iterator itr = _avatarGrabs.find(id);
-
+            if (itr == _avatarGrabs.end()) {
-        QMutableSetIterator<QUuid> delItr(_deletedAvatarGrabs);
-        while (delItr.hasNext()) {
-            QUuid grabID = delItr.next();
-            GrabPointer grab = _avatarGrabs[grabID];
-            if (!grab) {
-                delItr.remove();
                 continue;
             }
 
             bool success;
+            const GrabPointer& grab = itr->second;
             SpatiallyNestablePointer target = SpatiallyNestable::findByID(grab->getTargetID(), success);
-
-            // only clear this entry from the _deletedAvatarGrabs if we found the entity.
             if (success && target) {
-                bool iShouldTellServer = target->getEditSenderID() == sessionID;
-
-                EntityItemPointer entity = std::dynamic_pointer_cast<EntityItem>(target);
-                if (entity && entity->isAvatarEntity() && (entity->getOwningAvatarID() == sessionID ||
-                                                           entity->getOwningAvatarID() == AVATAR_SELF_ID)) {
-                    // this is our own avatar-entity, so we always tell the server about the release
-                    iShouldTellServer = true;
-                }
-
                 target->removeGrab(grab);
-                delItr.remove();
+                _avatarGrabs.erase(itr);
-                // in case this is the last grab on an entity, we need to shrink the queryAACube and tell the server
-                // about the final position.
-                if (entityTree) {
-                    bool force = true;
-                    entityTree->withWriteLock([&] {
-                        entityTree->updateEntityQueryAACube(target, packetSender, force, iShouldTellServer);
-                    });
-                }
                 grabAddedOrRemoved = true;
+            } else {
+                undeleted.push_back(id);
             }
-            _avatarGrabs.remove(grabID);
-            _changedAvatarGrabs.remove(grabID);
         }
+        _grabsToDelete = std::move(undeleted);
 
-        QMutableSetIterator<QUuid> changeItr(_changedAvatarGrabs);
+        // change _avatarGrabs and add Actions to target
-        while (changeItr.hasNext()) {
+        SetOfIDs unchanged;
-            QUuid grabID = changeItr.next();
+        for (const auto& id : _grabsToChange) {
-            GrabPointer& grab = _avatarGrabs[grabID];
+            MapOfGrabs::iterator itr = _avatarGrabs.find(id);
+            if (itr == _avatarGrabs.end()) {
+                continue;
+            }
 
             bool success;
+            const GrabPointer& grab = itr->second;
             SpatiallyNestablePointer target = SpatiallyNestable::findByID(grab->getTargetID(), success);
-
             if (success && target) {
                 target->addGrab(grab);
-                // only clear this entry from the _changedAvatarGrabs if we found the entity.
+                if (isMyAvatar()) {
-                changeItr.remove();
+                    EntityItemPointer entity = std::dynamic_pointer_cast<EntityItem>(target);
+                    if (entity) {
+                        entity->upgradeScriptSimulationPriority(PERSONAL_SIMULATION_PRIORITY);
+                    }
+                }
                 grabAddedOrRemoved = true;
+            } else {
+                unchanged.insert(id);
             }
         }
+        _grabsToChange = std::move(unchanged);
     });
     return grabAddedOrRemoved;
 }
@@ -413,8 +404,8 @@ bool Avatar::updateGrabs() {
 void Avatar::accumulateGrabPositions(std::map<QUuid, GrabLocationAccumulator>& grabAccumulators) {
     // relay avatar's joint position to grabbed target in a way that allows for averaging
     _avatarGrabsLock.withReadLock([&] {
-        foreach (auto grabID, _avatarGrabs.keys()) {
+        for (const auto& entry : _avatarGrabs) {
-            const GrabPointer& grab = _avatarGrabs.value(grabID);
+            const GrabPointer& grab = entry.second;
 
             if (!grab || !grab->getActionID().isNull()) {
                 continue; // the accumulated value isn't used, in this case.
@@ -432,6 +423,20 @@ void Avatar::accumulateGrabPositions(std::map<QUuid, GrabLocationAccumulator>& g
     });
 }
 
+void Avatar::tearDownGrabs() {
+    _avatarGrabsLock.withWriteLock([&] {
+        for (const auto& entry : _avatarGrabs) {
+            _grabsToDelete.push_back(entry.first);
+        }
+        _grabsToChange.clear();
+    });
+    applyGrabChanges();
+    if (!_grabsToDelete.empty()) {
+        // some grabs failed to delete, which is a possible "leak", so we log about it
+        qWarning() << "Failed to tearDown" << _grabsToDelete.size() << "grabs for Avatar" << getID();
+    }
+}
+
 void Avatar::relayJointDataToChildren() {
     forEachChild([&](SpatiallyNestablePointer child) {
         if (child->getNestableType() == NestableType::Entity) {
@@ -1929,3 +1934,12 @@ scriptable::ScriptableModelBase Avatar::getScriptableModel() {
     }
     return result;
 }
+
+void Avatar::clearAvatarGrabData(const QUuid& id) {
+    AvatarData::clearAvatarGrabData(id);
+    _avatarGrabsLock.withWriteLock([&] {
+        if (_avatarGrabs.find(id) != _avatarGrabs.end()) {
+            _grabsToDelete.push_back(id);
+        }
+    });
+}

libraries/avatars-renderer/src/avatars-renderer/Avatar.h

@@ -14,6 +14,9 @@
 #include <functional>
 #include <glm/glm.hpp>
 #include <glm/gtc/quaternion.hpp>
+#include <map>
+#include <set>
+#include <vector>
 
 #include <QtCore/QUuid>
 
@@ -23,10 +26,12 @@
 #include <graphics-scripting/Forward.h>
 #include <GLMHelpers.h>
 
+#include <Grab.h>
+#include <ThreadSafeValueCache.h>
+
 #include "Head.h"
 #include "SkeletonModel.h"
 #include "Rig.h"
-#include <ThreadSafeValueCache.h>
 
 #include "MetaModelPayload.h"
 
@@ -436,6 +441,8 @@ public:
 
     void accumulateGrabPositions(std::map<QUuid, GrabLocationAccumulator>& grabAccumulators);
 
+    void tearDownGrabs();
+
 signals:
     void targetScaleChanged(float targetScale);
 
@@ -538,7 +545,7 @@ protected:
 
     // protected methods...
     bool isLookingAtMe(AvatarSharedPointer avatar) const;
-    bool updateGrabs();
+    bool applyGrabChanges();
     void relayJointDataToChildren();
 
     void fade(render::Transaction& transaction, render::Transition::Type type);
@@ -625,8 +632,15 @@ protected:
 
     static void metaBlendshapeOperator(render::ItemID renderItemID, int blendshapeNumber, const QVector<BlendshapeOffset>& blendshapeOffsets,
                                        const QVector<int>& blendedMeshSizes, const render::ItemIDs& subItemIDs);
+    void clearAvatarGrabData(const QUuid& grabID) override;
 
-    AvatarGrabMap _avatarGrabs;
+    using SetOfIDs = std::set<QUuid>;
+    using VectorOfIDs = std::vector<QUuid>;
+    using MapOfGrabs = std::map<QUuid, GrabPointer>;
+
+    MapOfGrabs _avatarGrabs;
+    SetOfIDs _grabsToChange; // updated grab IDs -- changes needed to entities or physics
+    VectorOfIDs _grabsToDelete; // deleted grab IDs -- changes needed to entities or physics
 };
 
 #endif // hifi_Avatar_h

libraries/avatars/src/AvatarData.cpp

@@ -3010,7 +3010,6 @@ void AvatarData::clearAvatarGrabData(const QUuid& grabID) {
     _avatarGrabsLock.withWriteLock([&] {
         if (_avatarGrabData.remove(grabID)) {
             _avatarGrabDataChanged = true;
-            _deletedAvatarGrabs.insert(grabID);
         }
     });
 }

libraries/avatars/src/AvatarData.h

@@ -54,7 +54,6 @@
 #include "AvatarTraits.h"
 #include "HeadData.h"
 #include "PathUtils.h"
-#include "Grab.h"
 
 #include <graphics/Material.h>
 
@@ -67,8 +66,6 @@ using PackedAvatarEntityMap = QMap<QUuid, QByteArray>; // similar to AvatarEntit
 using AvatarEntityIDs = QSet<QUuid>;
 
 using AvatarGrabDataMap = QMap<QUuid, QByteArray>;
-using AvatarGrabIDs = QSet<QUuid>;
-using AvatarGrabMap = QMap<QUuid, GrabPointer>;
 
 using AvatarDataSequenceNumber = uint16_t;
 
@@ -1475,8 +1472,6 @@ protected:
     mutable ReadWriteLockable _avatarGrabsLock;
     AvatarGrabDataMap _avatarGrabData;
     bool _avatarGrabDataChanged { false }; // by network
-    AvatarGrabIDs _changedAvatarGrabs; // updated grab IDs -- changes needed to entities or physics
-    AvatarGrabIDs _deletedAvatarGrabs; // deleted grab IDs -- changes needed to entities or physics
 
     // used to transform any sensor into world space, including the _hmdSensorMat, or hand controllers.
     ThreadSafeValueCache<glm::mat4> _sensorToWorldMatrixCache { glm::mat4() };
@@ -1539,7 +1534,7 @@ protected:
     }
 
     bool updateAvatarGrabData(const QUuid& grabID, const QByteArray& grabData);
-    void clearAvatarGrabData(const QUuid& grabID);
+    virtual void clearAvatarGrabData(const QUuid& grabID);
 
 private:
     friend void avatarStateFromFrame(const QByteArray& frameData, AvatarData* _avatar);

libraries/entities/src/EntityItem.cpp

@@ -3425,7 +3425,19 @@ void EntityItem::addGrab(GrabPointer grab) {
     enableNoBootstrap();
     SpatiallyNestable::addGrab(grab);
 
-    if (getDynamic() && getParentID().isNull()) {
+    if (!getParentID().isNull()) {
+        return;
+    }
+
+    int jointIndex = grab->getParentJointIndex();
+    bool isFarGrab = jointIndex == FARGRAB_RIGHTHAND_INDEX
+        || jointIndex == FARGRAB_LEFTHAND_INDEX
+        || jointIndex == FARGRAB_MOUSE_INDEX;
+
+    // GRAB HACK: FarGrab doesn't work on non-dynamic things yet, but we really really want NearGrab
+    // (aka Hold) to work for such ojects, hence we filter the useAction case like so:
+    bool useAction = getDynamic() || (_physicsInfo && !isFarGrab);
+    if (useAction) {
         EntityTreePointer entityTree = getTree();
         assert(entityTree);
         EntitySimulationPointer simulation = entityTree ? entityTree->getSimulation() : nullptr;
@@ -3436,13 +3448,11 @@ void EntityItem::addGrab(GrabPointer grab) {
 
         EntityDynamicType dynamicType;
         QVariantMap arguments;
-        int grabParentJointIndex =grab->getParentJointIndex();
+        if (isFarGrab) {
-        if (grabParentJointIndex == FARGRAB_RIGHTHAND_INDEX || grabParentJointIndex == FARGRAB_LEFTHAND_INDEX ||
-            grabParentJointIndex == FARGRAB_MOUSE_INDEX) {
             // add a far-grab action
             dynamicType = DYNAMIC_TYPE_FAR_GRAB;
             arguments["otherID"] = grab->getOwnerID();
-            arguments["otherJointIndex"] = grabParentJointIndex;
+            arguments["otherJointIndex"] = jointIndex;
             arguments["targetPosition"] = vec3ToQMap(grab->getPositionalOffset());
             arguments["targetRotation"] = quatToQMap(grab->getRotationalOffset());
             arguments["linearTimeScale"] = 0.05;
@@ -3463,11 +3473,23 @@ void EntityItem::addGrab(GrabPointer grab) {
         grab->setActionID(actionID);
         _grabActions[actionID] = action;
         simulation->addDynamic(action);
+        markDirtyFlags(Simulation::DIRTY_MOTION_TYPE);
+        simulation->changeEntity(getThisPointer());
     }
 }
 
 void EntityItem::removeGrab(GrabPointer grab) {
+    int oldNumGrabs = _grabs.size();
     SpatiallyNestable::removeGrab(grab);
+    if (!getDynamic() && _grabs.size() != oldNumGrabs) {
+        // GRAB HACK: the expected behavior is for non-dynamic grabbed things to NOT be throwable
+        // so we slam the velocities to zero here whenever the number of grabs change.
+        // NOTE: if there is still another grab in effect this shouldn't interfere with the object's motion
+        // because that grab will slam the position+velocities next frame.
+        setLocalVelocity(glm::vec3(0.0f));
+        setAngularVelocity(glm::vec3(0.0f));
+    }
+    markDirtyFlags(Simulation::DIRTY_MOTION_TYPE);
 
     QUuid actionID = grab->getActionID();
     if (!actionID.isNull()) {

libraries/entities/src/SimulationOwner.h

@@ -96,11 +96,11 @@ const uint8_t RECRUIT_SIMULATION_PRIORITY = VOLUNTEER_SIMULATION_PRIORITY + 1;
 // When poking objects with scripts an observer will bid at SCRIPT_EDIT priority.
 const uint8_t SCRIPT_GRAB_SIMULATION_PRIORITY = 128;
 const uint8_t SCRIPT_POKE_SIMULATION_PRIORITY = SCRIPT_GRAB_SIMULATION_PRIORITY - 1;
-const uint8_t AVATAR_ENTITY_SIMULATION_PRIORITY = 255;
 
 // PERSONAL priority (needs a better name) is the level at which a simulation observer owns its own avatar
 // which really just means: things that collide with it will be bid at a priority level one lower
 const uint8_t PERSONAL_SIMULATION_PRIORITY = SCRIPT_GRAB_SIMULATION_PRIORITY;
+const uint8_t AVATAR_ENTITY_SIMULATION_PRIORITY = PERSONAL_SIMULATION_PRIORITY;
 
 
 class SimulationOwner {

libraries/physics/src/EntityMotionState.cpp

@@ -81,7 +81,7 @@ EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer
     setShape(shape);
 
     if (_entity->isAvatarEntity() && _entity->getOwningAvatarID() != Physics::getSessionUUID()) {
-        // avatar entities entities are always thus, so we cache this fact in _ownershipState
+        // avatar entities are always thus, so we cache this fact in _ownershipState
         _ownershipState = EntityMotionState::OwnershipState::Unownable;
     }
 
@@ -211,6 +211,7 @@ PhysicsMotionType EntityMotionState::computePhysicsMotionType() const {
     }
     if (_entity->isMovingRelativeToParent() ||
         _entity->hasActions() ||
+        _entity->hasGrabs() ||
         _entity->hasAncestorOfType(NestableType::Avatar)) {
         return MOTION_TYPE_KINEMATIC;
     }
@@ -235,11 +236,19 @@ void EntityMotionState::getWorldTransform(btTransform& worldTrans) const {
     assert(entityTreeIsLocked());
     if (_motionType == MOTION_TYPE_KINEMATIC) {
         BT_PROFILE("kinematicIntegration");
+        uint32_t thisStep = ObjectMotionState::getWorldSimulationStep();
+        if (hasInternalKinematicChanges()) {
+            // ACTION_CAN_CONTROL_KINEMATIC_OBJECT_HACK: This kinematic body was moved by an Action
+            // and doesn't require transform update because the body is authoritative and its transform
+            // has already been copied out --> do no kinematic integration.
+            clearInternalKinematicChanges();
+            _lastKinematicStep = thisStep;
+            return;
+        }
         // This is physical kinematic motion which steps strictly by the subframe count
         // of the physics simulation and uses full gravity for acceleration.
         _entity->setAcceleration(_entity->getGravity());
 
-        uint32_t thisStep = ObjectMotionState::getWorldSimulationStep();
         float dt = (thisStep - _lastKinematicStep) * PHYSICS_ENGINE_FIXED_SUBSTEP;
         _lastKinematicStep = thisStep;
         _entity->stepKinematicMotion(dt);
@@ -767,6 +776,7 @@ bool EntityMotionState::shouldSendBid() const {
     // NOTE: this method is only ever called when the entity's simulation is NOT locally owned
     return _body->isActive()
         && (_region == workload::Region::R1)
+        && _ownershipState != EntityMotionState::OwnershipState::Unownable
         && glm::max(glm::max(VOLUNTEER_SIMULATION_PRIORITY, _bumpedPriority), _entity->getScriptSimulationPriority()) >= _entity->getSimulationPriority()
         && !_entity->getLocked();
 }

libraries/physics/src/EntityMotionState.h

@@ -88,7 +88,6 @@ public:
     virtual void computeCollisionGroupAndMask(int32_t& group, int32_t& mask) const override;
 
     bool shouldSendBid() const;
-    uint8_t computeFinalBidPriority() const;
 
     bool isLocallyOwned() const override;
     bool isLocallyOwnedOrShouldBe() const override; // aka shouldEmitCollisionEvents()
@@ -100,6 +99,7 @@ public:
     void saveKinematicState(btScalar timeStep) override;
 
 protected:
+    uint8_t computeFinalBidPriority() const;
     void updateSendVelocities();
     uint64_t getNextBidExpiry() const { return _nextBidExpiry; }
     void initForBid();

libraries/physics/src/ObjectMotionState.h

@@ -160,8 +160,9 @@ public:
 
     bool hasInternalKinematicChanges() const { return _hasInternalKinematicChanges; }
 
-    void dirtyInternalKinematicChanges() { _hasInternalKinematicChanges = true; }
+    // these methods are declared const so they can be called inside other const methods
-    void clearInternalKinematicChanges() { _hasInternalKinematicChanges = false; }
+    void dirtyInternalKinematicChanges() const { _hasInternalKinematicChanges = true; }
+    void clearInternalKinematicChanges() const { _hasInternalKinematicChanges = false; }
 
     virtual bool isLocallyOwned() const { return false; }
     virtual bool isLocallyOwnedOrShouldBe() const { return false; } // aka shouldEmitCollisionEvents()
@@ -185,8 +186,11 @@ protected:
     btRigidBody* _body { nullptr };
     float _density { 1.0f };
 
+    // ACTION_CAN_CONTROL_KINEMATIC_OBJECT_HACK: These date members allow an Action
+    // to operate on a kinematic object without screwing up our default kinematic integration
+    // which is done in the MotionState::getWorldTransform().
     mutable uint32_t _lastKinematicStep;
-    bool _hasInternalKinematicChanges { false };
+    mutable bool _hasInternalKinematicChanges { false };
 };
 
 using SetOfMotionStates = QSet<ObjectMotionState*>;

libraries/physics/src/ThreadSafeDynamicsWorld.cpp

@@ -100,9 +100,11 @@ void ThreadSafeDynamicsWorld::synchronizeMotionState(btRigidBody* body) {
     if (body->isKinematicObject()) {
         ObjectMotionState* objectMotionState = static_cast<ObjectMotionState*>(body->getMotionState());
         if (objectMotionState->hasInternalKinematicChanges()) {
-            // this is a special case where the kinematic motion has been updated by an Action
+            // ACTION_CAN_CONTROL_KINEMATIC_OBJECT_HACK:
-            // so we supply the body's current transform to the MotionState
+            // This is a special case where the kinematic motion has been updated by an Action
-            objectMotionState->clearInternalKinematicChanges();
+            // so we supply the body's current transform to the MotionState,
+            // but we DON'T clear the internalKinematicChanges bit here because
+            // objectMotionState.getWorldTransform() will use and clear it later
             body->getMotionState()->setWorldTransform(body->getWorldTransform());
         }
         return;