fix misusage of smart-pointers

interface/src/avatar/Avatar.cpp

@@ -1,4 +1,4 @@
-//
+/3/
 //  Avatar.cpp
 //  interface/src/avatar
 //
@@ -108,6 +108,7 @@ Avatar::Avatar(RigPointer rig) :
 }
 
 Avatar::~Avatar() {
+    assert(isDead()); // mark dead before calling the dtor
     for(auto attachment : _unusedAttachments) {
         delete attachment;
     }

interface/src/avatar/AvatarManager.cpp

@@ -76,6 +76,10 @@ AvatarManager::AvatarManager(QObject* parent) :
     packetReceiver.registerListener(PacketType::AvatarBillboard, this, "processAvatarBillboardPacket");
 }
 
+AvatarManager::~AvatarManager() {
+    _myAvatar->die();
+}
+
 void AvatarManager::init() {
     _myAvatar->init();
     {

interface/src/avatar/AvatarManager.h

@@ -34,6 +34,8 @@ public:
     /// Registers the script types associated with the avatar manager.
     static void registerMetaTypes(QScriptEngine* engine);
 
+    virtual ~AvatarManager();
+
     void init();
 
     MyAvatar* getMyAvatar() { return _myAvatar.get(); }

libraries/entities/src/EntityItem.cpp

@@ -85,6 +85,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID) :
 }
 
 EntityItem::~EntityItem() {
+    assert(isDead()); // mark as dead before calling dtor
     // clear out any left-over actions
     EntityTreePointer entityTree = _element ? _element->getTree() : nullptr;
     EntitySimulation* simulation = entityTree ? entityTree->getSimulation() : nullptr;

libraries/physics/src/EntityMotionState.cpp

@@ -48,9 +48,10 @@ bool entityTreeIsLocked() {
 #endif
 
 
-EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItem* entity) :
+EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer entity) :
     ObjectMotionState(shape),
-    _entity(entity),
+    _entityPtr(entity),
+    _entity(entity.get()),
     _sentInactive(true),
     _lastStep(0),
     _serverPosition(0.0f),

libraries/physics/src/EntityMotionState.h

@@ -12,11 +12,11 @@
 #ifndef hifi_EntityMotionState_h
 #define hifi_EntityMotionState_h
 
+#include <EntityTypes.h>
 #include <AACube.h>
 
 #include "ObjectMotionState.h"
 
-class EntityItem;
 
 // From the MotionState's perspective:
 //      Inside = physics simulation
@@ -25,7 +25,7 @@ class EntityItem;
 class EntityMotionState : public ObjectMotionState {
 public:
 
-    EntityMotionState(btCollisionShape* shape, EntityItem* item);
+    EntityMotionState(btCollisionShape* shape, EntityItemPointer item);
     virtual ~EntityMotionState();
 
     void updateServerPhysicsVariables(const QUuid& sessionID);
@@ -73,7 +73,7 @@ public:
     virtual QUuid getSimulatorID() const;
     virtual void bump(quint8 priority);
 
-    EntityItem* getEntity() const { return _entity; }
+    EntityItemPointer getEntity() const { return _entityPtr.lock(); }
 
     void resetMeasuredBodyAcceleration();
     void measureBodyAcceleration();
@@ -96,7 +96,15 @@ protected:
     virtual btCollisionShape* computeNewShape();
     virtual void setMotionType(MotionType motionType);
 
-    EntityItem* _entity { nullptr }; // do NOT use smartpointer here
+    // In the glorious future (when entities lib depends on physics lib) the EntityMotionState will be
+    // properly "owned" by the EntityItem and will be deleted by it in the dtor.  In pursuit of that
+    // state of affairs we can't keep a real EntityItemPointer as data member (it would produce a
+    // recursive dependency).  Instead we keep a EntityItemWeakPointer to break that dependency while
+    // still granting us the capability to generate EntityItemPointers as necessary (for external data
+    // structures that use the MotionState to get to the EntityItem).
+    EntityItemWeakPointer _entityPtr;
+    // Meanwhile we also keep a raw EntityItem* for internal stuff where the pointer is guaranteed valid.
+    EntityItem* _entity;
 
     bool _sentInactive;   // true if body was inactive when we sent last update
 

libraries/physics/src/PhysicalEntitySimulation.cpp

@@ -122,17 +122,15 @@ void PhysicalEntitySimulation::clearEntitiesInternal() {
     // first disconnect each MotionStates from its Entity
     for (auto stateItr : _physicalObjects) {
         EntityMotionState* motionState = static_cast<EntityMotionState*>(&(*stateItr));
-        EntityItem* entity = motionState->getEntity();
+        _entitiesToDelete.insert(motionState->getEntity());
-        assert(entity);
-        _entitiesToDelete.insert(EntityItemPointer(entity));
     }
 
-    // then remove the objects from physics (aka MotionStates)
+    // then remove the objects (aka MotionStates) from physics
     _physicsEngine->removeObjects(_physicalObjects);
 
-    // delete the objects (aka MotionStates)
+    // delete the MotionStates
-    // Someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo
+    // TODO: after we invert the entities/physics lib dependencies we will let EntityItem delete
-    // rather than do it here
+    // its own PhysicsInfo rather than do it here
     for (auto entity : _entitiesToDelete) {
         EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
         if (motionState) {
@@ -182,15 +180,15 @@ void PhysicalEntitySimulation::getObjectsToAddToPhysics(VectorOfMotionStates& re
     QMutexLocker lock(&_mutex);
     SetOfEntities::iterator entityItr = _entitiesToAddToPhysics.begin();
     while (entityItr != _entitiesToAddToPhysics.end()) {
-        EntityItem* entity = (*entityItr).get();
+        EntityItemPointer entity = (*entityItr);
         assert(!entity->getPhysicsInfo());
         if (entity->isDead()) {
-            prepareEntityForDelete(EntityItemPointer(entity));
+            prepareEntityForDelete(entity);
         } else if (!entity->shouldBePhysical()) {
             // this entity should no longer be on the internal _entitiesToAddToPhysics
             entityItr = _entitiesToAddToPhysics.erase(entityItr);
             if (entity->isMoving()) {
-                _simpleKinematicEntities.insert(EntityItemPointer(entity));
+                _simpleKinematicEntities.insert(entity);
             }
         } else if (entity->isReadyToComputeShape()) {
             ShapeInfo shapeInfo;
@@ -236,12 +234,12 @@ void PhysicalEntitySimulation::handleOutgoingChanges(const VectorOfMotionStates&
         ObjectMotionState* state = &(*stateItr);
         if (state && state->getType() == MOTIONSTATE_TYPE_ENTITY) {
             EntityMotionState* entityState = static_cast<EntityMotionState*>(state);
-            EntityItem* entity = entityState->getEntity();
+            EntityItemPointer entity = entityState->getEntity();
-            assert(entity);
+            assert(entity.get());
             if (entityState->isCandidateForOwnership(sessionID)) {
                 _outgoingChanges.insert(entityState);
             }
-            _entitiesToSort.insert(EntityItemPointer(entity));
+            _entitiesToSort.insert(entity);
         }
     }
 

libraries/shared/src/SpatiallyNestable.h

@@ -34,7 +34,7 @@ enum class NestableType {
 class SpatiallyNestable : public std::enable_shared_from_this<SpatiallyNestable> {
 public:
     SpatiallyNestable(NestableType nestableType, QUuid id);
-    virtual ~SpatiallyNestable() { assert(_isDead); }
+    virtual ~SpatiallyNestable() { }
 
     virtual const QUuid& getID() const { return _id; }
     virtual void setID(const QUuid& id) { _id = id; }