remove Avatar dependency on AvatarMotionState

interface/src/avatar/Avatar.cpp

@@ -149,11 +149,6 @@ Avatar::~Avatar() {
         });
     }
 
-    if (_motionState) {
-        delete _motionState;
-        _motionState = nullptr;
-    }
-
     auto geometryCache = DependencyManager::get<GeometryCache>();
     if (geometryCache) {
         geometryCache->releaseID(_nameRectGeometryID);
@@ -1202,8 +1197,8 @@ int Avatar::parseDataFromBuffer(const QByteArray& buffer) {
 
     const float MOVE_DISTANCE_THRESHOLD = 0.001f;
     _moving = glm::distance(oldPosition, getPosition()) > MOVE_DISTANCE_THRESHOLD;
-    if (_moving && _motionState) {
-        _motionState->addDirtyFlags(Simulation::DIRTY_POSITION);
+    if (_moving) {
+        addPhysicsFlags(Simulation::DIRTY_POSITION);
     }
     if (_moving || _hasNewJointData) {
         locationChanged();
@@ -1325,14 +1320,18 @@ void Avatar::getCapsule(glm::vec3& start, glm::vec3& end, float& radius) {
     radius = halfExtents.x;
 }
 
-void Avatar::setMotionState(AvatarMotionState* motionState) {
-    _motionState = motionState;
-}
-
 // virtual
 void Avatar::rebuildCollisionShape() {
-    if (_motionState) {
-        _motionState->addDirtyFlags(Simulation::DIRTY_SHAPE);
+    addPhysicsFlags(Simulation::DIRTY_SHAPE);
+}
+
+void Avatar::setPhysicsCallback(AvatarPhysicsCallback cb) {
+    _physicsCallback = cb;
+}
+
+void Avatar::addPhysicsFlags(uint32_t flags) {
+    if (_physicsCallback) {
+        _physicsCallback(flags);
     }
 }
 

interface/src/avatar/Avatar.h

@@ -11,6 +11,7 @@
 #ifndef hifi_Avatar_h
 #define hifi_Avatar_h
 
+#include <functional>
 #include <glm/glm.hpp>
 #include <glm/gtc/quaternion.hpp>
 
@@ -48,9 +49,10 @@ enum ScreenTintLayer {
     NUM_SCREEN_TINT_LAYERS
 };
 
-class AvatarMotionState;
 class Texture;
 
+using AvatarPhysicsCallback = std::function<void(uint32_t)>;
+
 class Avatar : public AvatarData {
     Q_OBJECT
 
@@ -190,7 +192,7 @@ public:
     virtual void computeShapeInfo(ShapeInfo& shapeInfo);
     void getCapsule(glm::vec3& start, glm::vec3& end, float& radius);
 
-    AvatarMotionState* getMotionState() { return _motionState; }
+    //AvatarMotionState* getMotionState() { return _motionState; }
 
     using SpatiallyNestable::setPosition;
     virtual void setPosition(const glm::vec3& position) override;
@@ -248,8 +250,12 @@ public:
     void addToScene(AvatarSharedPointer self, const render::ScenePointer& scene);
     void ensureInScene(AvatarSharedPointer self, const render::ScenePointer& scene);
     bool isInScene() const { return render::Item::isValidID(_renderItemID); }
+    bool isMoving() const { return _moving; }
 
-    void setMotionState(AvatarMotionState* motionState);
+    //void setMotionState(AvatarMotionState* motionState);
+    void setPhysicsCallback(AvatarPhysicsCallback cb);
+    void addPhysicsFlags(uint32_t flags);
+    bool isInPhysicsSimulation() const { return _physicsCallback != nullptr; }
 
 public slots:
 
@@ -366,7 +372,7 @@ private:
 
     int _voiceSphereID;
 
-    AvatarMotionState* _motionState = nullptr;
+    AvatarPhysicsCallback _physicsCallback { nullptr };
 };
 
 #endif // hifi_Avatar_h

interface/src/avatar/AvatarManager.cpp

@@ -74,6 +74,7 @@ AvatarManager::AvatarManager(QObject* parent) :
 }
 
 AvatarManager::~AvatarManager() {
+    assert(_motionStates.empty());
 }
 
 void AvatarManager::init() {
@@ -128,10 +129,9 @@ float AvatarManager::getAvatarUpdateRate(const QUuid& sessionID, const QString&
 
 float AvatarManager::getAvatarSimulationRate(const QUuid& sessionID, const QString& rateName) const {
     auto avatar = std::static_pointer_cast<Avatar>(getAvatarBySessionID(sessionID));
-    return avatar ? avatar->getSimulationRate(rateName) : 0.0f; 
+    return avatar ? avatar->getSimulationRate(rateName) : 0.0f;
 }
 
-
 void AvatarManager::updateOtherAvatars(float deltaTime) {
     // lock the hash for read to check the size
     QReadLocker lock(&_hashLock);
@@ -189,16 +189,15 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
         if (_shouldRender) {
             avatar->ensureInScene(avatar, qApp->getMain3DScene());
         }
-        if (!avatar->getMotionState()) {
+        if (!avatar->isInPhysicsSimulation()) {
             ShapeInfo shapeInfo;
             avatar->computeShapeInfo(shapeInfo);
             btCollisionShape* shape = const_cast<btCollisionShape*>(ObjectMotionState::getShapeManager()->getShape(shapeInfo));
             if (shape) {
-                // don't add to the simulation now, instead put it on a list to be added later
-                AvatarMotionState* motionState = new AvatarMotionState(avatar.get(), shape);
-                avatar->setMotionState(motionState);
+                AvatarMotionState* motionState = new AvatarMotionState(avatar, shape);
+                avatar->setPhysicsCallback([=] (uint32_t flags) { motionState->addDirtyFlags(flags); });
+                _motionStates.insert(avatar.get(), motionState);
                 _motionStatesToAddToPhysics.insert(motionState);
-                _motionStatesThatMightUpdate.insert(motionState);
             }
         }
         avatar->animateScaleChanges(deltaTime);
@@ -283,30 +282,34 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
     const float MIN_FADE_SCALE = MIN_AVATAR_SCALE;
 
     QReadLocker locker(&_hashLock);
-    QVector<AvatarSharedPointer>::iterator itr = _avatarsToFade.begin();
-    while (itr != _avatarsToFade.end()) {
-        auto avatar = std::static_pointer_cast<Avatar>(*itr);
+    QVector<AvatarSharedPointer>::iterator avatarItr = _avatarsToFade.begin();
+    while (avatarItr != _avatarsToFade.end()) {
+        auto avatar = std::static_pointer_cast<Avatar>(*avatarItr);
         avatar->setTargetScale(avatar->getUniformScale() * SHRINK_RATE);
         avatar->animateScaleChanges(deltaTime);
         if (avatar->getTargetScale() <= MIN_FADE_SCALE) {
-            // fading to zero is such a rare event we push unique transaction for each one
+            // fading to zero is such a rare event we push a unique transaction for each
             if (avatar->isInScene()) {
                 const render::ScenePointer& scene = qApp->getMain3DScene();
                 render::Transaction transaction;
-                avatar->removeFromScene(*itr, scene, transaction);
+                avatar->removeFromScene(*avatarItr, scene, transaction);
                 scene->enqueueTransaction(transaction);
             }
 
-            // only remove from _avatarsToFade if we're sure its motionState has been removed from PhysicsEngine
-            if (_motionStatesToRemoveFromPhysics.empty()) {
-                itr = _avatarsToFade.erase(itr);
-            } else {
-                ++itr;
+            // delete the motionState
+            // TODO: use SharedPointer technology to make this happen automagically
+            assert(!avatar->isInPhysicsSimulation());
+            AvatarMotionStateMap::iterator motionStateItr = _motionStates.find(avatar.get());
+            if (motionStateItr != _motionStates.end()) {
+                delete *motionStateItr;
+                _motionStates.erase(motionStateItr);
             }
+
+            avatarItr = _avatarsToFade.erase(avatarItr);
         } else {
             const bool inView = true; // HACK
             avatar->simulate(deltaTime, inView);
-            ++itr;
+            ++avatarItr;
         }
     }
 }
@@ -315,19 +318,24 @@ AvatarSharedPointer AvatarManager::newSharedAvatar() {
     return std::make_shared<Avatar>(qApp->thread(), std::make_shared<Rig>());
 }
 
+void AvatarManager::removeAvatarFromPhysicsSimulation(Avatar* avatar) {
+    assert(avatar);
+    avatar->setPhysicsCallback(nullptr);
+    AvatarMotionStateMap::iterator itr = _motionStates.find(avatar);
+    if (itr != _motionStates.end()) {
+        AvatarMotionState* motionState = *itr;
+        _motionStatesToAddToPhysics.remove(motionState);
+        _motionStatesToRemoveFromPhysics.push_back(motionState);
+    }
+}
+
 void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) {
     AvatarHashMap::handleRemovedAvatar(removedAvatar, removalReason);
 
     // removedAvatar is a shared pointer to an AvatarData but we need to get to the derived Avatar
     // class in this context so we can call methods that don't exist at the base class.
     auto avatar = std::static_pointer_cast<Avatar>(removedAvatar);
-
-    AvatarMotionState* motionState = avatar->getMotionState();
-    if (motionState) {
-        _motionStatesThatMightUpdate.remove(motionState);
-        _motionStatesToAddToPhysics.remove(motionState);
-        _motionStatesToRemoveFromPhysics.push_back(motionState);
-    }
+    removeAvatarFromPhysicsSimulation(avatar.get());
 
     if (removalReason == KillAvatarReason::TheirAvatarEnteredYourBubble) {
         emit DependencyManager::get<UsersScriptingInterface>()->enteredIgnoreRadius();
@@ -362,11 +370,21 @@ void AvatarManager::clearOtherAvatars() {
             ++avatarIterator;
         }
     }
+    assert(scene);
     scene->enqueueTransaction(transaction);
     _myAvatar->clearLookAtTargetAvatar();
 }
 
 void AvatarManager::deleteAllAvatars() {
+    // delete motionStates
+    // TODO: use shared_ptr technology to make this work automagically
+    AvatarMotionStateMap::iterator motionStateItr = _motionStates.begin();
+    while (motionStateItr != _motionStates.end()) {
+        delete *motionStateItr;
+        ++motionStateItr;
+    }
+    _motionStates.clear();
+
     QReadLocker locker(&_hashLock);
     AvatarHash::iterator avatarIterator =  _avatarHash.begin();
     while (avatarIterator != _avatarHash.end()) {
@@ -391,10 +409,12 @@ void AvatarManager::getObjectsToAddToPhysics(VectorOfMotionStates& result) {
 
 void AvatarManager::getObjectsToChange(VectorOfMotionStates& result) {
     result.clear();
-    for (auto state : _motionStatesThatMightUpdate) {
-        if (state->_dirtyFlags > 0) {
-            result.push_back(state);
+    AvatarMotionStateMap::iterator motionStateItr = _motionStates.begin();
+    while (motionStateItr != _motionStates.end()) {
+        if ((*motionStateItr)->getIncomingDirtyFlags() != 0) {
+            result.push_back(*motionStateItr);
         }
+        ++motionStateItr;
     }
 }
 

interface/src/avatar/AvatarManager.h

@@ -93,11 +93,13 @@ private:
     void simulateAvatarFades(float deltaTime);
 
     AvatarSharedPointer newSharedAvatar() override;
+    void removeAvatarFromPhysicsSimulation(Avatar* avatar);
     void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason) override;
 
     QVector<AvatarSharedPointer> _avatarsToFade;
 
-    QSet<AvatarMotionState*> _motionStatesThatMightUpdate;
+    using AvatarMotionStateMap = QMap<Avatar*, AvatarMotionState*>;
+    AvatarMotionStateMap _motionStates;
     VectorOfMotionStates _motionStatesToRemoveFromPhysics;
     SetOfMotionStates _motionStatesToAddToPhysics;
 

interface/src/avatar/AvatarMotionState.cpp

@@ -17,7 +17,7 @@
 #include "AvatarMotionState.h"
 #include "BulletUtil.h"
 
-AvatarMotionState::AvatarMotionState(Avatar* avatar, const btCollisionShape* shape) : ObjectMotionState(shape), _avatar(avatar) {
+AvatarMotionState::AvatarMotionState(AvatarSharedPointer avatar, const btCollisionShape* shape) : ObjectMotionState(shape), _avatar(avatar) {
     assert(_avatar);
     _type = MOTIONSTATE_TYPE_AVATAR;
     if (_shape) {
@@ -49,7 +49,7 @@ PhysicsMotionType AvatarMotionState::computePhysicsMotionType() const {
 // virtual and protected
 const btCollisionShape* AvatarMotionState::computeNewShape() {
     ShapeInfo shapeInfo;
-    _avatar->computeShapeInfo(shapeInfo);
+    std::static_pointer_cast<Avatar>(_avatar)->computeShapeInfo(shapeInfo);
     return getShapeManager()->getShape(shapeInfo);
 }
 
@@ -130,7 +130,7 @@ glm::vec3 AvatarMotionState::getObjectAngularVelocity() const {
 
 // virtual
 glm::vec3 AvatarMotionState::getObjectGravity() const {
-    return _avatar->getAcceleration();
+    return std::static_pointer_cast<Avatar>(_avatar)->getAcceleration();
 }
 
 // virtual

interface/src/avatar/AvatarMotionState.h

@@ -20,7 +20,7 @@ class Avatar;
 
 class AvatarMotionState : public ObjectMotionState {
 public:
-    AvatarMotionState(Avatar* avatar, const btCollisionShape* shape);
+    AvatarMotionState(AvatarSharedPointer avatar, const btCollisionShape* shape);
 
     virtual PhysicsMotionType getMotionType() const override { return _motionType; }
 
@@ -74,11 +74,7 @@ protected:
     virtual bool isReadyToComputeShape() const override { return true; }
     virtual const btCollisionShape* computeNewShape() override;
 
-    // The AvatarMotionState keeps a RAW backpointer to its Avatar because all AvatarMotionState
-    // instances are "owned" by their corresponding Avatar instance and are deleted in the Avatar dtor.
-    // In other words, it is impossible for the Avatar to be deleted out from under its MotionState.
-    // In conclusion: weak pointer shennanigans would be pure overhead.
-    Avatar* _avatar; // do NOT use smartpointer here, no need for weakpointer
+    AvatarSharedPointer _avatar;
 
     uint32_t _dirtyFlags;
 };

libraries/physics/src/PhysicsEngine.cpp

@@ -220,6 +220,7 @@ void PhysicsEngine::removeObjects(const SetOfMotionStates& objects) {
             body->setMotionState(nullptr);
             delete body;
         }
+        object->clearIncomingDirtyFlags();
     }
 }