From 1bdcf8f0cb00b868b271ac5f2abba9e56862ea25 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 21:26:10 -0800 Subject: [PATCH 01/12] MyAvatar bids for ownerhsip on grab --- libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index 17d10cdf49..ffeb6ff545 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -401,6 +401,12 @@ bool Avatar::updateGrabs() { if (success && target) { target->addGrab(grab); + if (isMyAvatar()) { + EntityItemPointer entity = std::dynamic_pointer_cast(target); + if (entity) { + entity->upgradeScriptSimulationPriority(PERSONAL_SIMULATION_PRIORITY); + } + } // only clear this entry from the _changedAvatarGrabs if we found the entity. changeItr.remove(); grabAddedOrRemoved = true; From de68f6ca499926bdf7102196c44d669fe005b291 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 21:26:57 -0800 Subject: [PATCH 02/12] use Hold action use Hold action for any non-dynamic Entity with Body in phys simulation also set DIRTY_MOTION_TYPE bit, add to changed list on add/remove grab --- libraries/entities/src/EntityItem.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 8b6595d8c0..87f398c80c 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -3360,7 +3360,7 @@ void EntityItem::addGrab(GrabPointer grab) { enableNoBootstrap(); SpatiallyNestable::addGrab(grab); - if (getDynamic() && getParentID().isNull()) { + if (_physicsInfo && getParentID().isNull()) { EntityTreePointer entityTree = getTree(); assert(entityTree); EntitySimulationPointer simulation = entityTree ? entityTree->getSimulation() : nullptr; @@ -3398,11 +3398,14 @@ 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) { SpatiallyNestable::removeGrab(grab); + markDirtyFlags(Simulation::DIRTY_MOTION_TYPE); QUuid actionID = grab->getActionID(); if (!actionID.isNull()) { From 965d0ccf6034a3052b0a719a0e4e97d93759d1cc Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 21:29:49 -0800 Subject: [PATCH 03/12] reduced priority for AvatarEntities --- libraries/entities/src/SimulationOwner.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/entities/src/SimulationOwner.h b/libraries/entities/src/SimulationOwner.h index bd444d28dd..f574234354 100644 --- a/libraries/entities/src/SimulationOwner.h +++ b/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 { From d948168727a64f12c1b8a96488bab5c4c2aefe23 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 21:30:15 -0800 Subject: [PATCH 04/12] computeMotionType() returns KINEMATIC for things with grab --- libraries/physics/src/EntityMotionState.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index 9814e358c3..b417aa2448 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -211,6 +211,7 @@ PhysicsMotionType EntityMotionState::computePhysicsMotionType() const { } if (_entity->isMovingRelativeToParent() || _entity->hasActions() || + _entity->hasGrabs() || _entity->hasAncestorOfType(NestableType::Avatar)) { return MOTION_TYPE_KINEMATIC; } @@ -767,6 +768,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(); } From 160bef8f23542e09325b8a6d4f02e8c9021244ee Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 23:18:52 -0800 Subject: [PATCH 05/12] cleanup and minor optimizations --- interface/src/avatar/AvatarActionHold.cpp | 56 +++++++++++------------ 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/interface/src/avatar/AvatarActionHold.cpp b/interface/src/avatar/AvatarActionHold.cpp index dd99907b8e..5fb9c9a0ee 100644 --- a/interface/src/avatar/AvatarActionHold.cpp +++ b/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 = 0; + _measuredLinearVelocitiesIndex %= AvatarActionHold::velocitySmoothFrames; + } + + 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]; } - } - - 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 + measuredLinearVelocity /= (float)(AvatarActionHold::velocitySmoothFrames // - 3 // 3 because of the 3 we skipped, above ); - if (_kinematicSetVelocity) { rigidBody->setLinearVelocity(glmToBullet(measuredLinearVelocity)); rigidBody->setAngularVelocity(glmToBullet(_angularVelocityTarget)); } From fd457a9c0e4840d1c03b541bfa4bb568b9a7d907 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 23:19:20 -0800 Subject: [PATCH 06/12] remove cruft and fix a resource leak --- .../src/avatars-renderer/Avatar.cpp | 31 ++++++------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index ffeb6ff545..c5f5725594 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -325,6 +325,11 @@ void Avatar::removeAvatarEntitiesFromTree() { } bool Avatar::updateGrabs() { + if (!_avatarGrabDataChanged && _changedAvatarGrabs.empty() && _deletedAvatarGrabs.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([&] { @@ -347,11 +352,6 @@ bool Avatar::updateGrabs() { _avatarGrabDataChanged = false; } - auto treeRenderer = DependencyManager::get(); - auto entityTree = treeRenderer ? treeRenderer->getTree() : nullptr; - EntityEditPacketSender* packetSender = treeRenderer ? treeRenderer->getPacketSender() : nullptr; - auto sessionID = DependencyManager::get()->getSessionUUID(); - QMutableSetIterator delItr(_deletedAvatarGrabs); while (delItr.hasNext()) { QUuid grabID = delItr.next(); @@ -366,25 +366,8 @@ bool Avatar::updateGrabs() { // 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(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(); - // 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; } _avatarGrabs.remove(grabID); @@ -395,6 +378,10 @@ bool Avatar::updateGrabs() { while (changeItr.hasNext()) { QUuid grabID = changeItr.next(); GrabPointer& grab = _avatarGrabs[grabID]; + if (!grab) { + changeItr.remove(); + continue; + } bool success; SpatiallyNestablePointer target = SpatiallyNestable::findByID(grab->getTargetID(), success); From b73187aed9fcc6739987cb479e2b892b63a95fe2 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 23:19:33 -0800 Subject: [PATCH 07/12] fix typo in comment --- libraries/physics/src/EntityMotionState.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index b417aa2448..9d51c9c3e6 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/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; } From aff1244e09c2e1ef04e41878ab4439912cde7d9c Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Jan 2019 23:19:48 -0800 Subject: [PATCH 08/12] make method protected because we can --- libraries/physics/src/EntityMotionState.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/physics/src/EntityMotionState.h b/libraries/physics/src/EntityMotionState.h index 653e3f4252..ddf384dc77 100644 --- a/libraries/physics/src/EntityMotionState.h +++ b/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(); From ee3fde9df1ee9b3432c1d6f7f5b2a879a1f9603e Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 24 Jan 2019 14:39:00 -0800 Subject: [PATCH 09/12] unravel spaghetti, prefer std over Qt containers --- interface/src/avatar/MyAvatar.cpp | 2 +- .../src/avatars-renderer/Avatar.cpp | 61 +++++++++++-------- .../src/avatars-renderer/Avatar.h | 16 ++++- libraries/avatars/src/AvatarData.cpp | 1 - libraries/avatars/src/AvatarData.h | 7 +-- 5 files changed, 50 insertions(+), 37 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 84d6689e92..2ccc540105 100755 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -5310,7 +5310,7 @@ void MyAvatar::releaseGrab(const QUuid& grabID) { _avatarGrabsLock.withWriteLock([&] { if (_avatarGrabData.remove(grabID)) { - _deletedAvatarGrabs.insert(grabID); + _deletedAvatarGrabs.push_back(grabID); tellHandler = true; } }); diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index c5f5725594..d27a3026af 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -331,19 +331,18 @@ bool Avatar::updateGrabs() { } 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); - if (grabItr == _avatarGrabs.end()) { + MapOfGrabs::iterator itr = _avatarGrabs.find(grabID); + if (itr == _avatarGrabs.end()) { GrabPointer grab = std::make_shared(); grab->fromByteArray(_avatarGrabData.value(grabID)); _avatarGrabs[grabID] = grab; _changedAvatarGrabs.insert(grabID); } else { - GrabPointer grab = grabItr.value(); - bool changed = grab->fromByteArray(_avatarGrabData.value(grabID)); + bool changed = itr->second->fromByteArray(_avatarGrabData.value(grabID)); if (changed) { _changedAvatarGrabs.insert(grabID); } @@ -352,40 +351,38 @@ bool Avatar::updateGrabs() { _avatarGrabDataChanged = false; } - QMutableSetIterator delItr(_deletedAvatarGrabs); - while (delItr.hasNext()) { - QUuid grabID = delItr.next(); - GrabPointer grab = _avatarGrabs[grabID]; - if (!grab) { - delItr.remove(); + // delete _avatarGrabs + VectorOfIDs undeleted; + for (const auto& id : _deletedAvatarGrabs) { + 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); - - // only clear this entry from the _deletedAvatarGrabs if we found the entity. if (success && target) { target->removeGrab(grab); - delItr.remove(); + _avatarGrabs.erase(itr); grabAddedOrRemoved = true; + } else { + undeleted.push_back(id); } - _avatarGrabs.remove(grabID); - _changedAvatarGrabs.remove(grabID); } + _deletedAvatarGrabs = std::move(undeleted); - QMutableSetIterator changeItr(_changedAvatarGrabs); - while (changeItr.hasNext()) { - QUuid grabID = changeItr.next(); - GrabPointer& grab = _avatarGrabs[grabID]; - if (!grab) { - changeItr.remove(); + // change _avatarGrabs and add Actions to target + SetOfIDs unchanged; + for (const auto& id : _changedAvatarGrabs) { + 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); if (isMyAvatar()) { @@ -394,11 +391,12 @@ bool Avatar::updateGrabs() { entity->upgradeScriptSimulationPriority(PERSONAL_SIMULATION_PRIORITY); } } - // only clear this entry from the _changedAvatarGrabs if we found the entity. - changeItr.remove(); grabAddedOrRemoved = true; + } else { + unchanged.insert(id); } } + _changedAvatarGrabs = std::move(unchanged); }); return grabAddedOrRemoved; } @@ -406,8 +404,8 @@ bool Avatar::updateGrabs() { void Avatar::accumulateGrabPositions(std::map& grabAccumulators) { // relay avatar's joint position to grabbed target in a way that allows for averaging _avatarGrabsLock.withReadLock([&] { - foreach (auto grabID, _avatarGrabs.keys()) { - const GrabPointer& grab = _avatarGrabs.value(grabID); + for (const auto& entry : _avatarGrabs) { + const GrabPointer& grab = entry.second; if (!grab || !grab->getActionID().isNull()) { continue; // the accumulated value isn't used, in this case. @@ -1922,3 +1920,12 @@ scriptable::ScriptableModelBase Avatar::getScriptableModel() { } return result; } + +void Avatar::clearAvatarGrabData(const QUuid& id) { + AvatarData::clearAvatarGrabData(id); + _avatarGrabsLock.withWriteLock([&] { + if (_avatarGrabs.find(id) == _avatarGrabs.end()) { + _deletedAvatarGrabs.push_back(id); + } + }); +} diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.h b/libraries/avatars-renderer/src/avatars-renderer/Avatar.h index d5431ad2d2..75ae697603 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.h +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.h @@ -14,6 +14,9 @@ #include #include #include +#include +#include +#include #include @@ -23,10 +26,12 @@ #include #include +#include +#include + #include "Head.h" #include "SkeletonModel.h" #include "Rig.h" -#include #include "MetaModelPayload.h" @@ -625,8 +630,15 @@ protected: static void metaBlendshapeOperator(render::ItemID renderItemID, int blendshapeNumber, const QVector& blendshapeOffsets, const QVector& blendedMeshSizes, const render::ItemIDs& subItemIDs); + void clearAvatarGrabData(const QUuid& grabID) override; - AvatarGrabMap _avatarGrabs; + using SetOfIDs = std::set; + using VectorOfIDs = std::vector; + using MapOfGrabs = std::map; + + MapOfGrabs _avatarGrabs; + SetOfIDs _changedAvatarGrabs; // updated grab IDs -- changes needed to entities or physics + VectorOfIDs _deletedAvatarGrabs; // deleted grab IDs -- changes needed to entities or physics }; #endif // hifi_Avatar_h diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index ba3845e8e7..f4981d7347 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -2997,7 +2997,6 @@ void AvatarData::clearAvatarGrabData(const QUuid& grabID) { _avatarGrabsLock.withWriteLock([&] { if (_avatarGrabData.remove(grabID)) { _avatarGrabDataChanged = true; - _deletedAvatarGrabs.insert(grabID); } }); } diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 2889e5ffa3..9aabf9cb9d 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -54,7 +54,6 @@ #include "AvatarTraits.h" #include "HeadData.h" #include "PathUtils.h" -#include "Grab.h" #include @@ -67,8 +66,6 @@ using PackedAvatarEntityMap = QMap; // similar to AvatarEntit using AvatarEntityIDs = QSet; using AvatarGrabDataMap = QMap; -using AvatarGrabIDs = QSet; -using AvatarGrabMap = QMap; using AvatarDataSequenceNumber = uint16_t; @@ -1473,8 +1470,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 _sensorToWorldMatrixCache { glm::mat4() }; @@ -1537,7 +1532,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); From 09fe9735fac987c6656699089b41190db6aebb27 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 24 Jan 2019 14:57:54 -0800 Subject: [PATCH 10/12] plug grab leak, more correct names --- interface/src/avatar/AvatarManager.cpp | 1 + interface/src/avatar/MyAvatar.cpp | 4 +-- interface/src/avatar/OtherAvatar.cpp | 2 +- .../src/avatars-renderer/Avatar.cpp | 32 +++++++++++++------ .../src/avatars-renderer/Avatar.h | 8 +++-- 5 files changed, 32 insertions(+), 15 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index b9c7dc729d..2a83724f9e 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -452,6 +452,7 @@ void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar _spaceProxiesToDelete.push_back(avatar->getSpaceIndex()); } AvatarHashMap::handleRemovedAvatar(avatar, removalReason); + avatar->tearDownGrabs(); avatar->die(); queuePhysicsChange(avatar); diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 2ccc540105..7bb8d38ffa 100755 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -817,7 +817,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; } @@ -5310,7 +5310,7 @@ void MyAvatar::releaseGrab(const QUuid& grabID) { _avatarGrabsLock.withWriteLock([&] { if (_avatarGrabData.remove(grabID)) { - _deletedAvatarGrabs.push_back(grabID); + _grabsToDelete.push_back(grabID); tellHandler = true; } }); diff --git a/interface/src/avatar/OtherAvatar.cpp b/interface/src/avatar/OtherAvatar.cpp index 1f4fdc5e52..803e4be610 100644 --- a/interface/src/avatar/OtherAvatar.cpp +++ b/interface/src/avatar/OtherAvatar.cpp @@ -225,7 +225,7 @@ void OtherAvatar::simulate(float deltaTime, bool inView) { { PROFILE_RANGE(simulation, "grabs"); - updateGrabs(); + applyGrabChanges(); } updateFadingStatus(); diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index d27a3026af..7cdc82d7e5 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -324,8 +324,8 @@ void Avatar::removeAvatarEntitiesFromTree() { } } -bool Avatar::updateGrabs() { - if (!_avatarGrabDataChanged && _changedAvatarGrabs.empty() && _deletedAvatarGrabs.empty()) { +bool Avatar::applyGrabChanges() { + if (!_avatarGrabDataChanged && _grabsToChange.empty() && _grabsToDelete.empty()) { // early exit for most common case: nothing to do return false; } @@ -340,11 +340,11 @@ bool Avatar::updateGrabs() { GrabPointer grab = std::make_shared(); grab->fromByteArray(_avatarGrabData.value(grabID)); _avatarGrabs[grabID] = grab; - _changedAvatarGrabs.insert(grabID); + _grabsToChange.insert(grabID); } else { bool changed = itr->second->fromByteArray(_avatarGrabData.value(grabID)); if (changed) { - _changedAvatarGrabs.insert(grabID); + _grabsToChange.insert(grabID); } } } @@ -353,7 +353,7 @@ bool Avatar::updateGrabs() { // delete _avatarGrabs VectorOfIDs undeleted; - for (const auto& id : _deletedAvatarGrabs) { + for (const auto& id : _grabsToDelete) { MapOfGrabs::iterator itr = _avatarGrabs.find(id); if (itr == _avatarGrabs.end()) { continue; @@ -370,11 +370,11 @@ bool Avatar::updateGrabs() { undeleted.push_back(id); } } - _deletedAvatarGrabs = std::move(undeleted); + _grabsToDelete = std::move(undeleted); // change _avatarGrabs and add Actions to target SetOfIDs unchanged; - for (const auto& id : _changedAvatarGrabs) { + for (const auto& id : _grabsToChange) { MapOfGrabs::iterator itr = _avatarGrabs.find(id); if (itr == _avatarGrabs.end()) { continue; @@ -396,7 +396,7 @@ bool Avatar::updateGrabs() { unchanged.insert(id); } } - _changedAvatarGrabs = std::move(unchanged); + _grabsToChange = std::move(unchanged); }); return grabAddedOrRemoved; } @@ -423,6 +423,20 @@ void Avatar::accumulateGrabPositions(std::map& 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) { @@ -1925,7 +1939,7 @@ void Avatar::clearAvatarGrabData(const QUuid& id) { AvatarData::clearAvatarGrabData(id); _avatarGrabsLock.withWriteLock([&] { if (_avatarGrabs.find(id) == _avatarGrabs.end()) { - _deletedAvatarGrabs.push_back(id); + _grabsToDelete.push_back(id); } }); } diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.h b/libraries/avatars-renderer/src/avatars-renderer/Avatar.h index 75ae697603..1c3768e937 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.h +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.h @@ -441,6 +441,8 @@ public: void accumulateGrabPositions(std::map& grabAccumulators); + void tearDownGrabs(); + signals: void targetScaleChanged(float targetScale); @@ -543,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); @@ -637,8 +639,8 @@ protected: using MapOfGrabs = std::map; MapOfGrabs _avatarGrabs; - SetOfIDs _changedAvatarGrabs; // updated grab IDs -- changes needed to entities or physics - VectorOfIDs _deletedAvatarGrabs; // deleted grab IDs -- changes needed to entities or physics + 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 From c9ef439a7fe471e306cab077d052dcbd04224813 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 25 Jan 2019 08:46:24 -0800 Subject: [PATCH 11/12] fix more grab bugs --- libraries/entities/src/EntityItem.cpp | 29 +++++++++++++++---- libraries/physics/src/EntityMotionState.cpp | 10 ++++++- libraries/physics/src/ObjectMotionState.h | 10 +++++-- .../physics/src/ThreadSafeDynamicsWorld.cpp | 8 +++-- 4 files changed, 45 insertions(+), 12 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 87f398c80c..8ded2b2342 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -3360,7 +3360,19 @@ void EntityItem::addGrab(GrabPointer grab) { enableNoBootstrap(); SpatiallyNestable::addGrab(grab); - if (_physicsInfo && 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; @@ -3371,13 +3383,11 @@ void EntityItem::addGrab(GrabPointer grab) { EntityDynamicType dynamicType; QVariantMap arguments; - int grabParentJointIndex =grab->getParentJointIndex(); - if (grabParentJointIndex == FARGRAB_RIGHTHAND_INDEX || grabParentJointIndex == FARGRAB_LEFTHAND_INDEX || - grabParentJointIndex == FARGRAB_MOUSE_INDEX) { + if (isFarGrab) { // 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; @@ -3404,7 +3414,16 @@ void EntityItem::addGrab(GrabPointer grab) { } 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(); diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index 9d51c9c3e6..a82931064a 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -236,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); diff --git a/libraries/physics/src/ObjectMotionState.h b/libraries/physics/src/ObjectMotionState.h index 74173c3f47..e5a61834e4 100644 --- a/libraries/physics/src/ObjectMotionState.h +++ b/libraries/physics/src/ObjectMotionState.h @@ -160,8 +160,9 @@ public: bool hasInternalKinematicChanges() const { return _hasInternalKinematicChanges; } - void dirtyInternalKinematicChanges() { _hasInternalKinematicChanges = true; } - void clearInternalKinematicChanges() { _hasInternalKinematicChanges = false; } + // these methods are declared const so they can be called inside other const methods + 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; diff --git a/libraries/physics/src/ThreadSafeDynamicsWorld.cpp b/libraries/physics/src/ThreadSafeDynamicsWorld.cpp index 17a52f7cd9..4094097741 100644 --- a/libraries/physics/src/ThreadSafeDynamicsWorld.cpp +++ b/libraries/physics/src/ThreadSafeDynamicsWorld.cpp @@ -100,9 +100,11 @@ void ThreadSafeDynamicsWorld::synchronizeMotionState(btRigidBody* body) { if (body->isKinematicObject()) { ObjectMotionState* objectMotionState = static_cast(body->getMotionState()); if (objectMotionState->hasInternalKinematicChanges()) { - // this is a special case where the kinematic motion has been updated by an Action - // so we supply the body's current transform to the MotionState - objectMotionState->clearInternalKinematicChanges(); + // ACTION_CAN_CONTROL_KINEMATIC_OBJECT_HACK: + // This is a special case where the kinematic motion has been updated by an Action + // 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; From f2b9f088b48dfa26c6907ceb53d0890c0bdc4eed Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 28 Jan 2019 08:58:57 -0800 Subject: [PATCH 12/12] fix bad comparison causing undeleted grabs --- libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index 7cdc82d7e5..87d189696a 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -1938,7 +1938,7 @@ scriptable::ScriptableModelBase Avatar::getScriptableModel() { void Avatar::clearAvatarGrabData(const QUuid& id) { AvatarData::clearAvatarGrabData(id); _avatarGrabsLock.withWriteLock([&] { - if (_avatarGrabs.find(id) == _avatarGrabs.end()) { + if (_avatarGrabs.find(id) != _avatarGrabs.end()) { _grabsToDelete.push_back(id); } });