overhaul of MotionState cleanup

Moved MotionState deletes out of PhysicsEngine.
EntityMotionStates are deleted by the PhysicsEntitySimulation.
AvatarMotionStates are deleted in the Avatar dtor.

interface/src/Application.cpp

@@ -1084,8 +1084,8 @@ Application::~Application() {
     // remove avatars from physics engine
     DependencyManager::get<AvatarManager>()->clearOtherAvatars();
     VectorOfMotionStates motionStates;
-    DependencyManager::get<AvatarManager>()->getObjectsToDelete(motionStates);
+    DependencyManager::get<AvatarManager>()->getObjectsToRemoveFromPhysics(motionStates);
-    _physicsEngine->deleteObjects(motionStates);
+    _physicsEngine->removeObjects(motionStates);
 
     DependencyManager::destroy<OffscreenUi>();
     DependencyManager::destroy<AvatarManager>();
@@ -3085,11 +3085,11 @@ void Application::update(float deltaTime) {
         PerformanceTimer perfTimer("physics");
 
         static VectorOfMotionStates motionStates;
-        _entitySimulation.getObjectsToDelete(motionStates);
+        _entitySimulation.getObjectsToRemoveFromPhysics(motionStates);
-        _physicsEngine->deleteObjects(motionStates);
+        _physicsEngine->removeObjects(motionStates);
 
         getEntities()->getTree()->withWriteLock([&] {
-            _entitySimulation.getObjectsToAdd(motionStates);
+            _entitySimulation.getObjectsToAddToPhysics(motionStates);
             _physicsEngine->addObjects(motionStates);
 
         });
@@ -3102,9 +3102,9 @@ void Application::update(float deltaTime) {
         _entitySimulation.applyActionChanges();
 
         AvatarManager* avatarManager = DependencyManager::get<AvatarManager>().data();
-        avatarManager->getObjectsToDelete(motionStates);
+        avatarManager->getObjectsToRemoveFromPhysics(motionStates);
-        _physicsEngine->deleteObjects(motionStates);
+        _physicsEngine->removeObjects(motionStates);
-        avatarManager->getObjectsToAdd(motionStates);
+        avatarManager->getObjectsToAddToPhysics(motionStates);
         _physicsEngine->addObjects(motionStates);
         avatarManager->getObjectsToChange(motionStates);
         _physicsEngine->changeObjects(motionStates);
@@ -4083,7 +4083,7 @@ bool Application::nearbyEntitiesAreReadyForPhysics() {
     });
 
     foreach (EntityItemPointer entity, entities) {
-        if (!entity->isReadyToComputeShape()) {
+        if (entity->shouldBePhysical() && !entity->isReadyToComputeShape()) {
             static QString repeatedMessage =
                 LogHandler::getInstance().addRepeatedMessageRegex("Physics disabled until entity loads: .*");
             qCDebug(interfaceapp) << "Physics disabled until entity loads: " << entity->getID() << entity->getName();

interface/src/avatar/Avatar.cpp

@@ -108,7 +108,13 @@ Avatar::Avatar(RigPointer rig) :
 }
 
 Avatar::~Avatar() {
-    assert(_motionState == nullptr);
+    for(auto attachment : _unusedAttachments) {
+        delete attachment;
+    }
+    if (_motionState) {
+        delete _motionState;
+        _motionState = nullptr;
+    }
 }
 
 const float BILLBOARD_LOD_DISTANCE = 40.0f;

interface/src/avatar/AvatarManager.cpp

@@ -165,7 +165,12 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
         avatar->setTargetScale(avatar->getUniformScale() * SHRINK_RATE);
         if (avatar->getTargetScale() <= MIN_FADE_SCALE) {
             avatar->removeFromScene(*fadingIterator, scene, pendingChanges);
-            fadingIterator = _avatarFades.erase(fadingIterator);
+            // only remove from _avatarFades if we're sure its motionState has been removed from PhysicsEngine
+            if (_motionStatesToRemoveFromPhysics.empty()) {
+                fadingIterator = _avatarFades.erase(fadingIterator);
+            } else {
+                ++fadingIterator;
+            }
         } else {
             avatar->simulate(deltaTime);
             ++fadingIterator;
@@ -193,20 +198,6 @@ AvatarSharedPointer AvatarManager::addAvatar(const QUuid& sessionUUID, const QWe
     return newAvatar;
 }
 
-// protected
-void AvatarManager::removeAvatarMotionState(AvatarSharedPointer avatar) {
-    auto rawPointer = std::static_pointer_cast<Avatar>(avatar);
-    AvatarMotionState* motionState = rawPointer->getMotionState();
-    if (motionState) {
-        // clean up physics stuff
-        motionState->clearObjectBackPointer();
-        rawPointer->setMotionState(nullptr);
-        _avatarMotionStates.remove(motionState);
-        _motionStatesToAdd.remove(motionState);
-        _motionStatesToDelete.push_back(motionState);
-    }
-}
-
 // virtual
 void AvatarManager::removeAvatar(const QUuid& sessionUUID) {
     QWriteLocker locker(&_hashLock);
@@ -220,8 +211,16 @@ void AvatarManager::removeAvatar(const QUuid& sessionUUID) {
 void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) {
     AvatarHashMap::handleRemovedAvatar(removedAvatar);
 
-    removedAvatar->die();
+    Avatar* avatar = static_cast<Avatar*>(removedAvatar.get());
-    removeAvatarMotionState(removedAvatar);
+    avatar->die();
+
+    AvatarMotionState* motionState = avatar->getMotionState();
+    if (motionState) {
+        _motionStatesThatMightUpdate.remove(motionState);
+        _motionStatesToAddToPhysics.remove(motionState);
+        _motionStatesToRemoveFromPhysics.push_back(motionState);
+    }
+
     _avatarFades.push_back(removedAvatar);
 }
 
@@ -274,22 +273,22 @@ AvatarData* AvatarManager::getAvatar(QUuid avatarID) {
 }
 
 
-void AvatarManager::getObjectsToDelete(VectorOfMotionStates& result) {
+void AvatarManager::getObjectsToRemoveFromPhysics(VectorOfMotionStates& result) {
     result.clear();
-    result.swap(_motionStatesToDelete);
+    result.swap(_motionStatesToRemoveFromPhysics);
 }
 
-void AvatarManager::getObjectsToAdd(VectorOfMotionStates& result) {
+void AvatarManager::getObjectsToAddToPhysics(VectorOfMotionStates& result) {
     result.clear();
-    for (auto motionState : _motionStatesToAdd) {
+    for (auto motionState : _motionStatesToAddToPhysics) {
         result.push_back(motionState);
     }
-    _motionStatesToAdd.clear();
+    _motionStatesToAddToPhysics.clear();
 }
 
 void AvatarManager::getObjectsToChange(VectorOfMotionStates& result) {
     result.clear();
-    for (auto state : _avatarMotionStates) {
+    for (auto state : _motionStatesThatMightUpdate) {
         if (state->_dirtyFlags > 0) {
             result.push_back(state);
         }
@@ -344,8 +343,8 @@ void AvatarManager::addAvatarToSimulation(Avatar* avatar) {
         // we don't add to the simulation now, we put it on a list to be added later
         AvatarMotionState* motionState = new AvatarMotionState(avatar, shape);
         avatar->setMotionState(motionState);
-        _motionStatesToAdd.insert(motionState);
+        _motionStatesToAddToPhysics.insert(motionState);
-        _avatarMotionStates.insert(motionState);
+        _motionStatesThatMightUpdate.insert(motionState);
     }
 }
 

interface/src/avatar/AvatarManager.h

@@ -59,8 +59,8 @@ public:
     Q_INVOKABLE AvatarData* getAvatar(QUuid avatarID);
 
 
-    void getObjectsToDelete(VectorOfMotionStates& motionStates);
+    void getObjectsToRemoveFromPhysics(VectorOfMotionStates& motionStates);
-    void getObjectsToAdd(VectorOfMotionStates& motionStates);
+    void getObjectsToAddToPhysics(VectorOfMotionStates& motionStates);
     void getObjectsToChange(VectorOfMotionStates& motionStates);
     void handleOutgoingChanges(const VectorOfMotionStates& motionStates);
     void handleCollisionEvents(const CollisionEvents& collisionEvents);
@@ -80,7 +80,6 @@ private:
     // virtual overrides
     virtual AvatarSharedPointer newSharedAvatar();
     virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
-    void removeAvatarMotionState(AvatarSharedPointer avatar);
 
     virtual void removeAvatar(const QUuid& sessionUUID);
     virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar);
@@ -93,9 +92,9 @@ private:
 
     bool _shouldShowReceiveStats = false;
 
-    SetOfAvatarMotionStates _avatarMotionStates;
+    SetOfAvatarMotionStates _motionStatesThatMightUpdate;
-    SetOfMotionStates _motionStatesToAdd;
+    SetOfMotionStates _motionStatesToAddToPhysics;
-    VectorOfMotionStates _motionStatesToDelete;
+    VectorOfMotionStates _motionStatesToRemoveFromPhysics;
 };
 
 Q_DECLARE_METATYPE(AvatarManager::LocalLight)

interface/src/avatar/AvatarMotionState.cpp

@@ -25,20 +25,17 @@ AvatarMotionState::AvatarMotionState(Avatar* avatar, btCollisionShape* shape) :
 }
 
 AvatarMotionState::~AvatarMotionState() {
+    assert(_avatar);
     _avatar = nullptr;
 }
 
 // virtual
 uint32_t AvatarMotionState::getIncomingDirtyFlags() {
-    uint32_t dirtyFlags = 0;
+    return _body ? _dirtyFlags : 0;
-    if (_body && _avatar) {
-        dirtyFlags = _dirtyFlags;
-    }
-    return dirtyFlags;
 }
 
 void AvatarMotionState::clearIncomingDirtyFlags() {
-    if (_body && _avatar) {
+    if (_body) {
         _dirtyFlags = 0;
     }
 }
@@ -50,12 +47,9 @@ MotionType AvatarMotionState::computeObjectMotionType() const {
 
 // virtual and protected
 btCollisionShape* AvatarMotionState::computeNewShape() {
-    if (_avatar) {
+    ShapeInfo shapeInfo;
-        ShapeInfo shapeInfo;
+    _avatar->computeShapeInfo(shapeInfo);
-        _avatar->computeShapeInfo(shapeInfo);
+    return getShapeManager()->getShape(shapeInfo);
-        return getShapeManager()->getShape(shapeInfo);
-    }
-    return nullptr;
 }
 
 // virtual
@@ -65,9 +59,6 @@ bool AvatarMotionState::isMoving() const {
 
 // virtual
 void AvatarMotionState::getWorldTransform(btTransform& worldTrans) const {
-    if (!_avatar) {
-        return;
-    }
     worldTrans.setOrigin(glmToBullet(getObjectPosition()));
     worldTrans.setRotation(glmToBullet(getObjectRotation()));
     if (_body) {
@@ -78,9 +69,6 @@ void AvatarMotionState::getWorldTransform(btTransform& worldTrans) const {
 
 // virtual
 void AvatarMotionState::setWorldTransform(const btTransform& worldTrans) {
-    if (!_avatar) {
-        return;
-    }
     // HACK: The PhysicsEngine does not actually move OTHER avatars -- instead it slaves their local RigidBody to the transform
     // as specified by a remote simulation.  However, to give the remote simulation time to respond to our own objects we tie
     // the other avatar's body to its true position with a simple spring. This is a HACK that will have to be improved later.
@@ -159,10 +147,3 @@ int16_t AvatarMotionState::computeCollisionGroup() {
     return COLLISION_GROUP_OTHER_AVATAR;
 }
 
-// virtual
-void AvatarMotionState::clearObjectBackPointer() {
-    ObjectMotionState::clearObjectBackPointer();
-    _avatar = nullptr;
-}
-
-

interface/src/avatar/AvatarMotionState.h

@@ -68,8 +68,7 @@ public:
 protected:
     virtual bool isReadyToComputeShape() { return true; }
     virtual btCollisionShape* computeNewShape();
-    virtual void clearObjectBackPointer();
+    Avatar* _avatar; // do NOT use smartpointer here
-    Avatar* _avatar;
     uint32_t _dirtyFlags;
 };
 

libraries/avatars/src/AvatarData.h

@@ -344,9 +344,6 @@ public:
 
     glm::vec3 getClientGlobalPosition() { return _globalPosition; }
 
-    void die() { _isDead = true; }
-    bool isDead() const { return _isDead; }
-
 public slots:
     void sendAvatarDataPacket();
     void sendIdentityPacket();
@@ -423,8 +420,6 @@ protected:
     // updates about one avatar to another.
     glm::vec3 _globalPosition;
 
-    bool _isDead { false };
-
 private:
     friend void avatarStateFromFrame(const QByteArray& frameData, AvatarData* _avatar);
     static QUrl _defaultFullAvatarModelUrl;

libraries/entities-renderer/src/RenderablePolyVoxEntityItem.h

@@ -77,7 +77,7 @@ public:
     glm::mat4 localToVoxelMatrix() const;
 
     virtual ShapeType getShapeType() const;
-    virtual bool shouldBePhysical() const { return true; }
+    virtual bool shouldBePhysical() const { return !isDead(); }
     virtual bool isReadyToComputeShape();
     virtual void computeShapeInfo(ShapeInfo& info);
 

libraries/entities/src/BoxEntityItem.h

@@ -53,7 +53,7 @@ public:
     }
     
     virtual ShapeType getShapeType() const { return SHAPE_TYPE_BOX; }
-    virtual bool shouldBePhysical() const { return true; }
+    virtual bool shouldBePhysical() const { return !isDead(); }
 
     virtual void debugDump() const;
 

libraries/entities/src/EntityItem.h

@@ -302,7 +302,7 @@ public:
 
     virtual bool contains(const glm::vec3& point) const;
 
-    virtual bool isReadyToComputeShape() { return true; }
+    virtual bool isReadyToComputeShape() { return !isDead(); }
     virtual void computeShapeInfo(ShapeInfo& info);
     virtual float getVolumeEstimate() const { return getDimensions().x * getDimensions().y * getDimensions().z; }
 

libraries/entities/src/EntitySimulation.cpp

@@ -38,16 +38,17 @@ void EntitySimulation::updateEntities() {
     sortEntitiesThatMoved();
 }
 
-void EntitySimulation::getEntitiesToDelete(VectorOfEntities& entitiesToDelete) {
+void EntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) {
     QMutexLocker lock(&_mutex);
     for (auto entity : _entitiesToDelete) {
-        // this entity is still in its tree, so we insert into the external list
+        // push this entity onto the external list
         entitiesToDelete.push_back(entity);
     }
     _entitiesToDelete.clear();
 }
 
 void EntitySimulation::removeEntityInternal(EntityItemPointer entity) {
+    // remove from all internal lists except _entitiesToDelete
     _mortalEntities.remove(entity);
     _entitiesToUpdate.remove(entity);
     _entitiesToSort.remove(entity);
@@ -59,6 +60,7 @@ void EntitySimulation::removeEntityInternal(EntityItemPointer entity) {
 void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) {
     assert(entity);
     assert(entity->isSimulated());
+    assert(entity->isDead());
     entity->clearActions(this);
     removeEntityInternal(entity);
     _entitiesToDelete.insert(entity);
@@ -89,6 +91,7 @@ void EntitySimulation::expireMortalEntities(const quint64& now) {
             quint64 expiry = entity->getExpiry();
             if (expiry < now) {
                 itemItr = _mortalEntities.erase(itemItr);
+                entity->die();
                 prepareEntityForDelete(entity);
             } else {
                 if (expiry < _nextExpiry) {
@@ -134,6 +137,7 @@ void EntitySimulation::sortEntitiesThatMoved() {
         if (success && !domainBounds.touches(newCube)) {
             qCDebug(entities) << "Entity " << entity->getEntityItemID() << " moved out of domain bounds.";
             itemItr = _entitiesToSort.erase(itemItr);
+            entity->die();
             prepareEntityForDelete(entity);
         } else {
             moveOperator.addEntityToMoveList(entity, newCube);
@@ -193,6 +197,7 @@ void EntitySimulation::changeEntity(EntityItemPointer entity) {
         AACube newCube = entity->getQueryAACube(success);
         if (success && !domainBounds.touches(newCube)) {
             qCDebug(entities) << "Entity " << entity->getEntityItemID() << " moved out of domain bounds.";
+            entity->die();
             prepareEntityForDelete(entity);
             wasRemoved = true;
         }
@@ -226,14 +231,15 @@ void EntitySimulation::clearEntities() {
     _entitiesToUpdate.clear();
     _entitiesToSort.clear();
     _simpleKinematicEntities.clear();
-    _entitiesToDelete.clear();
 
     clearEntitiesInternal();
 
-    for (auto entityItr : _allEntities) {
+    for (auto entity : _allEntities) {
-        entityItr->setSimulated(false);
+        entity->setSimulated(false);
+        entity->die();
     }
     _allEntities.clear();
+    _entitiesToDelete.clear();
 }
 
 void EntitySimulation::moveSimpleKinematics(const quint64& now) {

libraries/entities/src/EntitySimulation.h

@@ -73,7 +73,7 @@ public:
 
     EntityTreePointer getEntityTree() { return _entityTree; }
 
-    void getEntitiesToDelete(VectorOfEntities& entitiesToDelete);
+    virtual void takeEntitiesToDelete(VectorOfEntities& entitiesToDelete);
 
     /// \param entity pointer to EntityItem that needs to be put on the entitiesToDelete list and removed from others.
     virtual void prepareEntityForDelete(EntityItemPointer entity);

libraries/entities/src/EntityTree.cpp

@@ -442,6 +442,7 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator)
     const RemovedEntities& entities = theOperator.getEntities();
     foreach(const EntityToDeleteDetails& details, entities) {
         EntityItemPointer theEntity = details.entity;
+        theEntity->die();
 
         if (getIsServer()) {
             // set up the deleted entities ID
@@ -1005,7 +1006,7 @@ void EntityTree::update() {
         withWriteLock([&] {
             _simulation->updateEntities();
             VectorOfEntities pendingDeletes;
-            _simulation->getEntitiesToDelete(pendingDeletes);
+            _simulation->takeEntitiesToDelete(pendingDeletes);
 
             if (pendingDeletes.size() > 0) {
                 // translate into list of ID's

libraries/entities/src/ModelEntityItem.cpp

@@ -377,7 +377,7 @@ void ModelEntityItem::setAnimationFPS(float value) {
 
 // virtual
 bool ModelEntityItem::shouldBePhysical() const {
-    return getShapeType() != SHAPE_TYPE_NONE;
+    return !isDead() && getShapeType() != SHAPE_TYPE_NONE;
 }
 
 void ModelEntityItem::resizeJointArrays(int newSize) {

libraries/entities/src/SphereEntityItem.h

@@ -52,7 +52,7 @@ public:
     }
 
     virtual ShapeType getShapeType() const { return SHAPE_TYPE_SPHERE; }
-    virtual bool shouldBePhysical() const { return true; }
+    virtual bool shouldBePhysical() const { return !isDead(); }
     
     virtual bool supportsDetailedRayIntersection() const { return true; }
     virtual bool findDetailedRayIntersection(const glm::vec3& origin, const glm::vec3& direction,

libraries/entities/src/ZoneEntityItem.h

@@ -57,7 +57,7 @@ public:
     static bool getDrawZoneBoundaries() { return _drawZoneBoundaries; }
     static void setDrawZoneBoundaries(bool value) { _drawZoneBoundaries = value; }
     
-    virtual bool isReadyToComputeShape() { return true; }
+    virtual bool isReadyToComputeShape() { return false; }
     void updateShapeType(ShapeType type) { _shapeType = type; }
     virtual ShapeType getShapeType() const;
     

libraries/physics/src/EntityMotionState.cpp

@@ -34,7 +34,7 @@ const quint64 USECS_BETWEEN_OWNERSHIP_BIDS = USECS_PER_SECOND / 5;
 
 #ifdef WANT_DEBUG_ENTITY_TREE_LOCKS
 bool EntityMotionState::entityTreeIsLocked() const {
-    EntityTreeElementPointer element = _entity ? _entity->getElement() : nullptr;
+    EntityTreeElementPointer element = _entity->getElement();
     EntityTreePointer tree = element ? element->getTree() : nullptr;
     if (!tree) {
         return true;
@@ -48,7 +48,7 @@ bool entityTreeIsLocked() {
 #endif
 
 
-EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer entity) :
+EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItem* entity) :
     ObjectMotionState(shape),
     _entity(entity),
     _sentInactive(true),
@@ -69,14 +69,14 @@ EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer
     _loopsWithoutOwner(0)
 {
     _type = MOTIONSTATE_TYPE_ENTITY;
-    assert(_entity != nullptr);
+    assert(_entity);
     assert(entityTreeIsLocked());
     setMass(_entity->computeMass());
 }
 
 EntityMotionState::~EntityMotionState() {
-    // be sure to clear _entity before calling the destructor
+    assert(_entity);
-    assert(!_entity);
+    _entity = nullptr;
 }
 
 void EntityMotionState::updateServerPhysicsVariables(const QUuid& sessionID) {
@@ -138,11 +138,6 @@ bool EntityMotionState::handleHardAndEasyChanges(uint32_t& flags, PhysicsEngine*
     return ObjectMotionState::handleHardAndEasyChanges(flags, engine);
 }
 
-void EntityMotionState::clearObjectBackPointer() {
-    ObjectMotionState::clearObjectBackPointer();
-    _entity = nullptr;
-}
-
 MotionType EntityMotionState::computeObjectMotionType() const {
     if (!_entity) {
         return MOTION_TYPE_STATIC;
@@ -222,21 +217,15 @@ void EntityMotionState::setWorldTransform(const btTransform& worldTrans) {
 
 // virtual and protected
 bool EntityMotionState::isReadyToComputeShape() {
-    if (_entity) {
+    return _entity->isReadyToComputeShape();
-        return _entity->isReadyToComputeShape();
-    }
-    return false;
 }
 
 // virtual and protected
 btCollisionShape* EntityMotionState::computeNewShape() {
-    if (_entity) {
+    ShapeInfo shapeInfo;
-        ShapeInfo shapeInfo;
+    assert(entityTreeIsLocked());
-        assert(entityTreeIsLocked());
+    _entity->computeShapeInfo(shapeInfo);
-        _entity->computeShapeInfo(shapeInfo);
+    return getShapeManager()->getShape(shapeInfo);
-        return getShapeManager()->getShape(shapeInfo);
-    }
-    return nullptr;
 }
 
 bool EntityMotionState::isCandidateForOwnership(const QUuid& sessionID) const {
@@ -553,26 +542,17 @@ void EntityMotionState::clearIncomingDirtyFlags() {
 
 // virtual
 quint8 EntityMotionState::getSimulationPriority() const {
-    if (_entity) {
+    return _entity->getSimulationPriority();
-        return _entity->getSimulationPriority();
-    }
-    return NO_PRORITY;
 }
 
 // virtual
 QUuid EntityMotionState::getSimulatorID() const {
-    if (_entity) {
+    assert(entityTreeIsLocked());
-        assert(entityTreeIsLocked());
+    return _entity->getSimulatorID();
-        return _entity->getSimulatorID();
-    }
-    return QUuid();
 }
 
-// virtual
 void EntityMotionState::bump(quint8 priority) {
-    if (_entity) {
+    setOutgoingPriority(glm::max(VOLUNTEER_SIMULATION_PRIORITY, --priority));
-        setOutgoingPriority(glm::max(VOLUNTEER_SIMULATION_PRIORITY, --priority));
-    }
 }
 
 void EntityMotionState::resetMeasuredBodyAcceleration() {
@@ -624,18 +604,12 @@ void EntityMotionState::setMotionType(MotionType motionType) {
 
 // virtual
 QString EntityMotionState::getName() {
-    if (_entity) {
+    assert(entityTreeIsLocked());
-        assert(entityTreeIsLocked());
+    return _entity->getName();
-        return _entity->getName();
-    }
-    return "";
 }
 
 // virtual
 int16_t EntityMotionState::computeCollisionGroup() {
-    if (!_entity) {
-        return COLLISION_GROUP_STATIC;
-    }
     if (_entity->getIgnoreForCollisions()) {
         return COLLISION_GROUP_COLLISIONLESS;
     }

libraries/physics/src/EntityMotionState.h

@@ -25,7 +25,7 @@ class EntityItem;
 class EntityMotionState : public ObjectMotionState {
 public:
 
-    EntityMotionState(btCollisionShape* shape, EntityItemPointer item);
+    EntityMotionState(btCollisionShape* shape, EntityItem* item);
     virtual ~EntityMotionState();
 
     void updateServerPhysicsVariables(const QUuid& sessionID);
@@ -73,7 +73,7 @@ public:
     virtual QUuid getSimulatorID() const;
     virtual void bump(quint8 priority);
 
-    EntityItemPointer getEntity() const { return _entity; }
+    EntityItem* getEntity() const { return _entity; }
 
     void resetMeasuredBodyAcceleration();
     void measureBodyAcceleration();
@@ -94,10 +94,9 @@ protected:
 
     virtual bool isReadyToComputeShape();
     virtual btCollisionShape* computeNewShape();
-    virtual void clearObjectBackPointer();
     virtual void setMotionType(MotionType motionType);
 
-    EntityItemPointer _entity;
+    EntityItem* _entity { nullptr }; // do NOT use smartpointer here
 
     bool _sentInactive;   // true if body was inactive when we sent last update
 

libraries/physics/src/ObjectMotionState.cpp

@@ -71,9 +71,9 @@ ObjectMotionState::ObjectMotionState(btCollisionShape* shape) :
 }
 
 ObjectMotionState::~ObjectMotionState() {
-    // adebug TODO: move shape release out of PhysicsEngine and into into the ObjectMotionState dtor
     assert(!_body);
-    assert(!_shape);
+    releaseShape();
+    _type = MOTIONSTATE_TYPE_INVALID;
 }
 
 void ObjectMotionState::setBodyLinearVelocity(const glm::vec3& velocity) const {

libraries/physics/src/ObjectMotionState.h

@@ -153,9 +153,6 @@ protected:
     void setMotionType(MotionType motionType);
     void updateCCDConfiguration();
 
-    // clearObjectBackPointer() overrrides should call the base method, then actually clear the object back pointer.
-    virtual void clearObjectBackPointer() { _type = MOTIONSTATE_TYPE_INVALID; }
-
     void setRigidBody(btRigidBody* body);
 
     MotionStateType _type = MOTIONSTATE_TYPE_INVALID; // type of MotionState

libraries/physics/src/PhysicalEntitySimulation.cpp

@@ -44,6 +44,7 @@ void PhysicalEntitySimulation::updateEntitiesInternal(const quint64& now) {
 
 void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) {
     assert(entity);
+    assert(!entity->isDead());
     if (entity->shouldBePhysical()) {
         EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
         if (!motionState) {
@@ -60,8 +61,6 @@ void PhysicalEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
 
     EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
     if (motionState) {
-        motionState->clearObjectBackPointer();
-        entity->setPhysicsInfo(nullptr);
         _outgoingChanges.remove(motionState);
         _entitiesToRemoveFromPhysics.insert(entity);
     } else {
@@ -69,6 +68,23 @@ void PhysicalEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
     }
 }
 
+void PhysicalEntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) {
+    QMutexLocker lock(&_mutex);
+    for (auto entity : _entitiesToDelete) {
+        // this entity is still in its tree, so we insert into the external list
+        entitiesToDelete.push_back(entity);
+
+        // Someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo
+        // rather than do it here
+        EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
+        if (motionState) {
+            delete motionState;
+            entity->setPhysicsInfo(nullptr);
+        }
+    }
+    _entitiesToDelete.clear();
+}
+
 void PhysicalEntitySimulation::changeEntityInternal(EntityItemPointer entity) {
     // queue incoming changes: from external sources (script, EntityServer, etc) to physics engine
     assert(entity);
@@ -106,17 +122,26 @@ void PhysicalEntitySimulation::clearEntitiesInternal() {
     // first disconnect each MotionStates from its Entity
     for (auto stateItr : _physicalObjects) {
         EntityMotionState* motionState = static_cast<EntityMotionState*>(&(*stateItr));
-        EntityItemPointer entity = motionState->getEntity();
+        EntityItem* entity = motionState->getEntity();
-        if (entity) {
+        assert(entity);
-            entity->setPhysicsInfo(nullptr);
+        _entitiesToDelete.insert(EntityItemPointer(entity));
-        }
-        motionState->clearObjectBackPointer();
     }
 
-    // then delete the objects (aka MotionStates)
+    // then remove the objects from physics (aka MotionStates)
-    _physicsEngine->deleteObjects(_physicalObjects);
+    _physicsEngine->removeObjects(_physicalObjects);
 
-    // finally clear all lists (which now have only dangling pointers)
+    // delete the objects (aka MotionStates)
+    // Someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo
+    // rather than do it here
+    for (auto entity : _entitiesToDelete) {
+        EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
+        if (motionState) {
+            delete motionState;
+            entity->setPhysicsInfo(nullptr);
+        }
+    }
+
+    // finally clear all lists maintained by this class
     _physicalObjects.clear();
     _entitiesToRemoveFromPhysics.clear();
     _entitiesToAddToPhysics.clear();
@@ -128,16 +153,13 @@ void PhysicalEntitySimulation::clearEntitiesInternal() {
 void PhysicalEntitySimulation::prepareEntityForDelete(EntityItemPointer entity) {
     assert(entity);
     assert(entity->isSimulated());
+    assert(entity->isDead());
     entity->clearActions(this);
     removeEntityInternal(entity);
-
-    // the PhysicalEntitySimulation must pull the corresponding object out of the PhysicsEngine
-    // before the Entity is ready to delete so we first put them on this list
-    _entitiesToRemoveFromPhysics.insert(entity);
 }
 // end EntitySimulation overrides
 
-void PhysicalEntitySimulation::getObjectsToDelete(VectorOfMotionStates& result) {
+void PhysicalEntitySimulation::getObjectsToRemoveFromPhysics(VectorOfMotionStates& result) {
     result.clear();
     QMutexLocker lock(&_mutex);
     for (auto entity: _entitiesToRemoveFromPhysics) {
@@ -145,28 +167,30 @@ void PhysicalEntitySimulation::getObjectsToDelete(VectorOfMotionStates& result)
         if (motionState) {
             _pendingChanges.remove(motionState);
             _physicalObjects.remove(motionState);
-            motionState->clearObjectBackPointer();
             result.push_back(motionState);
         }
         _entitiesToAddToPhysics.remove(entity);
-        entity->setPhysicsInfo(nullptr);
+        if (entity->isDead()) {
-        _entitiesToDelete.insert(entity);
+            _entitiesToDelete.insert(entity);
+        }
     }
     _entitiesToRemoveFromPhysics.clear();
 }
 
-void PhysicalEntitySimulation::getObjectsToAdd(VectorOfMotionStates& result) {
+void PhysicalEntitySimulation::getObjectsToAddToPhysics(VectorOfMotionStates& result) {
     result.clear();
     QMutexLocker lock(&_mutex);
     SetOfEntities::iterator entityItr = _entitiesToAddToPhysics.begin();
     while (entityItr != _entitiesToAddToPhysics.end()) {
-        EntityItemPointer entity = *entityItr;
+        EntityItem* entity = (*entityItr).get();
         assert(!entity->getPhysicsInfo());
-        if (!entity->shouldBePhysical()) {
+        if (entity->isDead()) {
+            prepareEntityForDelete(EntityItemPointer(entity));
+        } else if (!entity->shouldBePhysical()) {
             // this entity should no longer be on the internal _entitiesToAddToPhysics
             entityItr = _entitiesToAddToPhysics.erase(entityItr);
             if (entity->isMoving()) {
-                _simpleKinematicEntities.insert(entity);
+                _simpleKinematicEntities.insert(EntityItemPointer(entity));
             }
         } else if (entity->isReadyToComputeShape()) {
             ShapeInfo shapeInfo;
@@ -212,13 +236,12 @@ void PhysicalEntitySimulation::handleOutgoingChanges(const VectorOfMotionStates&
         ObjectMotionState* state = &(*stateItr);
         if (state && state->getType() == MOTIONSTATE_TYPE_ENTITY) {
             EntityMotionState* entityState = static_cast<EntityMotionState*>(state);
-            EntityItemPointer entity = entityState->getEntity();
+            EntityItem* entity = entityState->getEntity();
-            if (entity) {
+            assert(entity);
-                if (entityState->isCandidateForOwnership(sessionID)) {
+            if (entityState->isCandidateForOwnership(sessionID)) {
-                    _outgoingChanges.insert(entityState);
+                _outgoingChanges.insert(entityState);
-                }
-                _entitiesToSort.insert(entityState->getEntity());
             }
+            _entitiesToSort.insert(EntityItemPointer(entity));
         }
     }
 

libraries/physics/src/PhysicalEntitySimulation.h

@@ -35,6 +35,8 @@ public:
     virtual void addAction(EntityActionPointer action) override;
     virtual void applyActionChanges() override;
 
+    virtual void takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) override;
+
 protected: // only called by EntitySimulation
     // overrides for EntitySimulation
     virtual void updateEntitiesInternal(const quint64& now) override;
@@ -46,8 +48,8 @@ protected: // only called by EntitySimulation
 public:
     virtual void prepareEntityForDelete(EntityItemPointer entity) override;
 
-    void getObjectsToDelete(VectorOfMotionStates& result);
+    void getObjectsToRemoveFromPhysics(VectorOfMotionStates& result);
-    void getObjectsToAdd(VectorOfMotionStates& result);
+    void getObjectsToAddToPhysics(VectorOfMotionStates& result);
     void setObjectsToChange(const VectorOfMotionStates& objectsToChange);
     void getObjectsToChange(VectorOfMotionStates& result);
 
@@ -58,7 +60,7 @@ public:
 
 private:
     SetOfEntities _entitiesToRemoveFromPhysics;
-    SetOfEntities _entitiesToAddToPhysics; // entities to be be added to PhysicsEngine (and a their EntityMotionState created)
+    SetOfEntities _entitiesToAddToPhysics;
 
     SetOfEntityMotionStates _pendingChanges; // EntityMotionStates already in PhysicsEngine that need their physics changed
     SetOfEntityMotionStates _outgoingChanges; // EntityMotionStates for which we need to send updates to entity-server

libraries/physics/src/PhysicsEngine.cpp

@@ -156,7 +156,7 @@ void PhysicsEngine::removeObjectFromDynamicsWorld(ObjectMotionState* object) {
     _dynamicsWorld->removeRigidBody(body);
 }
 
-void PhysicsEngine::deleteObjects(const VectorOfMotionStates& objects) {
+void PhysicsEngine::removeObjects(const VectorOfMotionStates& objects) {
     for (auto object : objects) {
         removeObjectFromDynamicsWorld(object);
 
@@ -165,14 +165,11 @@ void PhysicsEngine::deleteObjects(const VectorOfMotionStates& objects) {
         object->setRigidBody(nullptr);
         body->setMotionState(nullptr);
         delete body;
-        // adebug TODO: move this into ObjectMotionState dtor
-        object->releaseShape();
-        delete object;
     }
 }
 
 // Same as above, but takes a Set instead of a Vector.  Should only be called during teardown.
-void PhysicsEngine::deleteObjects(const SetOfMotionStates& objects) {
+void PhysicsEngine::removeObjects(const SetOfMotionStates& objects) {
     for (auto object : objects) {
         btRigidBody* body = object->getRigidBody();
         removeObjectFromDynamicsWorld(object);
@@ -181,9 +178,6 @@ void PhysicsEngine::deleteObjects(const SetOfMotionStates& objects) {
         object->setRigidBody(nullptr);
         body->setMotionState(nullptr);
         delete body;
-        // adebug TODO: move this into ObjectMotionState dtor
-        object->releaseShape();
-        delete object;
     }
 }
 

libraries/physics/src/PhysicsEngine.h

@@ -54,8 +54,8 @@ public:
     void setSessionUUID(const QUuid& sessionID) { _sessionID = sessionID; }
     const QUuid& getSessionID() const { return _sessionID; }
 
-    void deleteObjects(const VectorOfMotionStates& objects);
+    void removeObjects(const VectorOfMotionStates& objects);
-    void deleteObjects(const SetOfMotionStates& objects); // only called during teardown
+    void removeObjects(const SetOfMotionStates& objects); // only called during teardown
 
     void addObjects(const VectorOfMotionStates& objects);
     VectorOfMotionStates changeObjects(const VectorOfMotionStates& objects);
@@ -84,8 +84,6 @@ public:
     /// \brief call bump on any objects that touch the object corresponding to motionState
     void bump(ObjectMotionState* motionState);
 
-    void removeRigidBody(btRigidBody* body);
-
     void setCharacterController(CharacterController* character);
 
     void dumpNextStats() { _dumpNextStats = true; }

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() { }
+    virtual ~SpatiallyNestable() { assert(_isDead); }
 
     virtual const QUuid& getID() const { return _id; }
     virtual void setID(const QUuid& id) { _id = id; }
@@ -115,6 +115,9 @@ public:
     void forEachChild(std::function<void(SpatiallyNestablePointer)> actor);
     void forEachDescendant(std::function<void(SpatiallyNestablePointer)> actor);
 
+    void die() { _isDead = true; }
+    bool isDead() const { return _isDead; }
+
 protected:
     const NestableType _nestableType; // EntityItem or an AvatarData
     QUuid _id;
@@ -141,7 +144,8 @@ protected:
 private:
     mutable ReadWriteLockable _transformLock;
     Transform _transform; // this is to be combined with parent's world-transform to produce this' world-transform.
-    mutable bool _parentKnowsMe = false;
+    mutable bool _parentKnowsMe { false };
+    bool _isDead { false };
 };