From 994eed7b83a58b60a6a40f3e550b4366e9cd10aa Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Fri, 28 Apr 2017 10:18:19 -0700
Subject: [PATCH] safer delete of AvatarMotionStates

---
 interface/src/avatar/AvatarManager.cpp | 52 ++++++++++----------------
 interface/src/avatar/AvatarManager.h   |  3 +-
 2 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp
index ec22c3403a..04ab1531ba 100644
--- a/interface/src/avatar/AvatarManager.cpp
+++ b/interface/src/avatar/AvatarManager.cpp
@@ -289,16 +289,6 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
                 avatar->removeFromScene(*avatarItr, scene, transaction);
                 scene->enqueueTransaction(transaction);
             }
-
-            // 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
@@ -312,24 +302,19 @@ AvatarSharedPointer AvatarManager::newSharedAvatar() {
     return std::make_shared<Avatar>(qApp->thread(), std::make_shared<Rig>());
 }
 
-void AvatarManager::removeAvatarFromPhysicsSimulation(Avatar* avatar) {
-    assert(avatar);
+void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) {
+    AvatarHashMap::handleRemovedAvatar(removedAvatar, removalReason);
+
+    // remove from physics
+    auto avatar = std::static_pointer_cast<Avatar>(removedAvatar);
     avatar->setPhysicsCallback(nullptr);
-    AvatarMotionStateMap::iterator itr = _motionStates.find(avatar);
+    AvatarMotionStateMap::iterator itr = _motionStates.find(avatar.get());
     if (itr != _motionStates.end()) {
         AvatarMotionState* motionState = *itr;
         _motionStatesToAddToPhysics.remove(motionState);
         _motionStatesToRemoveFromPhysics.push_back(motionState);
+        _motionStates.erase(itr);
     }
-}
-
-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);
-    removeAvatarFromPhysicsSimulation(avatar.get());
 
     if (removalReason == KillAvatarReason::TheirAvatarEnteredYourBubble) {
         emit DependencyManager::get<UsersScriptingInterface>()->enteredIgnoreRadius();
@@ -370,14 +355,8 @@ void AvatarManager::clearOtherAvatars() {
 }
 
 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();
+    assert(_motionStates.empty()); // should have called clearOtherAvatars() before getting here
+    deleteMotionStates();
 
     QReadLocker locker(&_hashLock);
     AvatarHash::iterator avatarIterator =  _avatarHash.begin();
@@ -388,9 +367,18 @@ void AvatarManager::deleteAllAvatars() {
     }
 }
 
+void AvatarManager::deleteMotionStates() {
+    // delete motionstates that were removed from physics last frame
+    for (auto state : _motionStatesToDelete) {
+        delete state;
+    }
+    _motionStatesToDelete.clear();
+}
+
 void AvatarManager::getObjectsToRemoveFromPhysics(VectorOfMotionStates& result) {
-    result.clear();
-    result.swap(_motionStatesToRemoveFromPhysics);
+    deleteMotionStates();
+    result = _motionStatesToRemoveFromPhysics;
+    _motionStatesToDelete.swap(_motionStatesToRemoveFromPhysics);
 }
 
 void AvatarManager::getObjectsToAddToPhysics(VectorOfMotionStates& result) {
diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h
index e5b2117beb..c67088a4be 100644
--- a/interface/src/avatar/AvatarManager.h
+++ b/interface/src/avatar/AvatarManager.h
@@ -92,7 +92,7 @@ private:
     void simulateAvatarFades(float deltaTime);
 
     AvatarSharedPointer newSharedAvatar() override;
-    void removeAvatarFromPhysicsSimulation(Avatar* avatar);
+    void deleteMotionStates();
     void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason) override;
 
     QVector<AvatarSharedPointer> _avatarsToFade;
@@ -100,6 +100,7 @@ private:
     using AvatarMotionStateMap = QMap<Avatar*, AvatarMotionState*>;
     AvatarMotionStateMap _motionStates;
     VectorOfMotionStates _motionStatesToRemoveFromPhysics;
+    VectorOfMotionStates _motionStatesToDelete;
     SetOfMotionStates _motionStatesToAddToPhysics;
 
     std::shared_ptr<MyAvatar> _myAvatar;