Use btCollisionObject* instead of MWWorld::Ptr inside of Projectile

collision handling and castRay() to avoid calling getPtr(). It is a step forward
removing the mutex inside of PtrHolder.

Do the same for DeepestNotMeContactTestResultCallback. It is used
only for not-ranged combat for now, but do it anyway for parity with all
other callback. This way, once the PtrHolder mutex is gone one will not
have to worry about wether it is safe to use the callback in a specific
context.

To avoid use-after-free with projectile / projectile collision, defer deletion of projectile.
Since instead of storing a copy of target Ptr we have a pointer to its collision object,
we can't delete projectiles until after we finished iterating over the loops.

apps/openmw/mwphysics/actorconvexcallback.cpp

@@ -77,10 +77,8 @@ namespace MWPhysics
             auto* projectileHolder = static_cast<Projectile*>(convexResult.m_hitCollisionObject->getUserPointer());
             if (!projectileHolder->isActive())
                 return btScalar(1);
-            auto* targetHolder = static_cast<PtrHolder*>(mMe->getUserPointer());
+            if (projectileHolder->isValidTarget(mMe))
-            const MWWorld::Ptr target = targetHolder->getPtr();
+                projectileHolder->hit(mMe, convexResult.m_hitPointLocal, convexResult.m_hitNormalLocal);
-            if (projectileHolder->isValidTarget(target))
-                projectileHolder->hit(target, convexResult.m_hitPointLocal, convexResult.m_hitNormalLocal);
             return btScalar(1);
         }
 

apps/openmw/mwphysics/closestnotmerayresultcallback.cpp

@@ -7,6 +7,7 @@
 
 #include "../mwworld/class.hpp"
 
+#include "collisiontype.hpp"
 #include "ptrholder.hpp"
 
 namespace MWPhysics
@@ -19,17 +20,14 @@ namespace MWPhysics
 
     btScalar ClosestNotMeRayResultCallback::addSingleResult(btCollisionWorld::LocalRayResult& rayResult, bool normalInWorldSpace)
     {
-        if (rayResult.m_collisionObject == mMe)
+        const auto* hitObject = rayResult.m_collisionObject;
+        if (hitObject == mMe)
             return 1.f;
 
-        if (!mTargets.empty())
+        if (hitObject->getBroadphaseHandle()->m_collisionFilterGroup == CollisionType_Actor && !mTargets.empty())
         {
-            if ((std::find(mTargets.begin(), mTargets.end(), rayResult.m_collisionObject) == mTargets.end()))
+            if ((std::find(mTargets.begin(), mTargets.end(), hitObject) == mTargets.end()))
-            {
+                return 1.f;
-                auto* holder = static_cast<PtrHolder*>(rayResult.m_collisionObject->getUserPointer());
-                if (holder && !holder->getPtr().isEmpty() && holder->getPtr().getClass().isActor())
-                    return 1.f;
-            }
         }
 
         return btCollisionWorld::ClosestRayResultCallback::addSingleResult(rayResult, normalInWorldSpace);

apps/openmw/mwphysics/deepestnotmecontacttestresultcallback.cpp

@@ -6,6 +6,7 @@
 
 #include "../mwworld/class.hpp"
 
+#include "collisiontype.hpp"
 #include "ptrholder.hpp"
 
 namespace MWPhysics
@@ -23,14 +24,10 @@ namespace MWPhysics
         const btCollisionObject* collisionObject = col1Wrap->m_collisionObject;
         if (collisionObject != mMe)
         {
-            if (!mTargets.empty())
+            if (collisionObject->getBroadphaseHandle()->m_collisionFilterGroup == CollisionType_Actor && !mTargets.empty())
             {
                 if ((std::find(mTargets.begin(), mTargets.end(), collisionObject) == mTargets.end()))
-                {
+                    return 0.f;
-                    PtrHolder* holder = static_cast<PtrHolder*>(collisionObject->getUserPointer());
-                    if (holder && !holder->getPtr().isEmpty() && holder->getPtr().getClass().isActor())
-                        return 0.f;
-                }
             }
 
             btScalar distsqr = mOrigin.distance2(cp.getPositionWorldOnA());

apps/openmw/mwphysics/physicssystem.cpp

@@ -635,19 +635,7 @@ namespace MWPhysics
         if (btFrom == btTo)
             return;
 
-        const auto casterPtr = projectile->getCaster();
+        ProjectileConvexCallback resultCallback(projectile->getCasterCollisionObject(), projectile->getCollisionObject(), btFrom, btTo, projectile);
-        const auto* caster = [this,&casterPtr]() -> const btCollisionObject*
-        {
-            const Actor* actor = getActor(casterPtr);
-            if (actor)
-                return actor->getCollisionObject();
-            const Object* object = getObject(casterPtr);
-            if (object)
-                return object->getCollisionObject();
-            return nullptr;
-        }();
-
-        ProjectileConvexCallback resultCallback(caster, btFrom, btTo, projectile);
         resultCallback.m_collisionFilterMask = 0xff;
         resultCallback.m_collisionFilterGroup = CollisionType_Projectile;
 

apps/openmw/mwphysics/projectile.cpp

@@ -7,8 +7,10 @@
 
 #include "../mwworld/class.hpp"
 
+#include "actor.hpp"
 #include "collisiontype.hpp"
 #include "mtphysics.hpp"
+#include "object.hpp"
 #include "projectile.hpp"
 
 namespace MWPhysics
@@ -17,7 +19,7 @@ Projectile::Projectile(const MWWorld::Ptr& caster, const osg::Vec3f& position, f
     : mCanCrossWaterSurface(canCrossWaterSurface)
     , mCrossedWaterSurface(false)
     , mActive(true)
-    , mCaster(caster)
+    , mHitTarget(nullptr)
     , mWaterHitPosition(std::nullopt)
     , mPhysics(physicssystem)
     , mTaskScheduler(scheduler)
@@ -32,6 +34,7 @@ Projectile::Projectile(const MWWorld::Ptr& caster, const osg::Vec3f& position, f
     mCollisionObject->setUserPointer(this);
 
     setPosition(position);
+    setCaster(caster);
 
     const int collisionMask = CollisionType_World | CollisionType_HeightMap |
         CollisionType_Actor | CollisionType_Door | CollisionType_Water | CollisionType_Projectile;
@@ -77,54 +80,67 @@ bool Projectile::canTraverseWater() const
     return mCanCrossWaterSurface;
 }
 
-void Projectile::hit(const MWWorld::Ptr& target, btVector3 pos, btVector3 normal)
+void Projectile::hit(const btCollisionObject* target, btVector3 pos, btVector3 normal)
 {
-    if (!mActive.load(std::memory_order_acquire))
+    bool active = true;
+    if (!mActive.compare_exchange_strong(active, false, std::memory_order_relaxed) || !active)
         return;
-    std::scoped_lock lock(mMutex);
     mHitTarget = target;
     mHitPosition = pos;
     mHitNormal = normal;
-    mActive.store(false, std::memory_order_release);
+}
+
+MWWorld::Ptr Projectile::getTarget() const
+{
+    assert(!mActive);
+    auto* target = static_cast<PtrHolder*>(mHitTarget->getUserPointer());
+    return target ? target->getPtr() : MWWorld::Ptr();
 }
 
 MWWorld::Ptr Projectile::getCaster() const
 {
-    std::scoped_lock lock(mMutex);
     return mCaster;
 }
 
 void Projectile::setCaster(const MWWorld::Ptr& caster)
 {
-    std::scoped_lock lock(mMutex);
     mCaster = caster;
+    mCasterColObj = [this,&caster]() -> const btCollisionObject*
+    {
+        const Actor* actor = mPhysics->getActor(caster);
+        if (actor)
+            return actor->getCollisionObject();
+        const Object* object = mPhysics->getObject(caster);
+        if (object)
+            return object->getCollisionObject();
+        return nullptr;
+    }();
 }
 
 void Projectile::setValidTargets(const std::vector<MWWorld::Ptr>& targets)
 {
     std::scoped_lock lock(mMutex);
-    mValidTargets = targets;
+    mValidTargets.clear();
+    for (const auto& ptr : targets)
+    {
+        const auto* physicActor = mPhysics->getActor(ptr);
+        if (physicActor)
+            mValidTargets.push_back(physicActor->getCollisionObject());
+    }
 }
 
-bool Projectile::isValidTarget(const MWWorld::Ptr& target) const
+bool Projectile::isValidTarget(const btCollisionObject* target) const
 {
+    assert(target);
     std::scoped_lock lock(mMutex);
-    if (mCaster == target)
+    if (mCasterColObj == target)
         return false;
 
-    if (target.isEmpty() || mValidTargets.empty())
+    if (mValidTargets.empty())
         return true;
 
-    bool validTarget = false;
+    return std::any_of(mValidTargets.begin(), mValidTargets.end(),
-    for (const auto& targetActor : mValidTargets)
+            [target](const btCollisionObject* actor) { return target == actor; });
-    {
-        if (targetActor == target)
-        {
-            validTarget = true;
-            break;
-        }
-    }
-    return validTarget;
 }
 
 std::optional<btVector3> Projectile::getWaterHitPosition()

apps/openmw/mwphysics/projectile.hpp

@@ -47,21 +47,21 @@ namespace MWPhysics
             return mActive.load(std::memory_order_acquire);
         }
 
-        MWWorld::Ptr getTarget() const
+        MWWorld::Ptr getTarget() const;
-        {
-            assert(!mActive);
-            return mHitTarget;
-        }
 
         MWWorld::Ptr getCaster() const;
         void setCaster(const MWWorld::Ptr& caster);
+        const btCollisionObject* getCasterCollisionObject() const
+        {
+            return mCasterColObj;
+        }
 
         bool canTraverseWater() const;
 
-        void hit(const MWWorld::Ptr& target, btVector3 pos, btVector3 normal);
+        void hit(const btCollisionObject* target, btVector3 pos, btVector3 normal);
 
         void setValidTargets(const std::vector<MWWorld::Ptr>& targets);
-        bool isValidTarget(const MWWorld::Ptr& target) const;
+        bool isValidTarget(const btCollisionObject* target) const;
 
         std::optional<btVector3> getWaterHitPosition();
         void setWaterHitPosition(btVector3 pos);
@@ -76,13 +76,14 @@ namespace MWPhysics
         bool mCrossedWaterSurface;
         std::atomic<bool> mActive;
         MWWorld::Ptr mCaster;
-        MWWorld::Ptr mHitTarget;
+        const btCollisionObject* mCasterColObj;
+        const btCollisionObject* mHitTarget;
         std::optional<btVector3> mWaterHitPosition;
         osg::Vec3f mPosition;
         btVector3 mHitPosition;
         btVector3 mHitNormal;
 
-        std::vector<MWWorld::Ptr> mValidTargets;
+        std::vector<const btCollisionObject*> mValidTargets;
 
         mutable std::mutex mMutex;
 

apps/openmw/mwphysics/projectileconvexcallback.cpp

@@ -1,3 +1,5 @@
+#include <BulletCollision/CollisionDispatch/btCollisionObject.h>
+
 #include "../mwworld/class.hpp"
 
 #include "actor.hpp"
@@ -8,41 +10,41 @@
 
 namespace MWPhysics
 {
-    ProjectileConvexCallback::ProjectileConvexCallback(const btCollisionObject* me, const btVector3& from, const btVector3& to, Projectile* proj)
+    ProjectileConvexCallback::ProjectileConvexCallback(const btCollisionObject* caster, const btCollisionObject* me, const btVector3& from, const btVector3& to, Projectile* proj)
         : btCollisionWorld::ClosestConvexResultCallback(from, to)
-        , mMe(me), mProjectile(proj)
+        , mCaster(caster)
+        , mMe(me)
+        , mProjectile(proj)
     {
         assert(mProjectile);
     }
 
     btScalar ProjectileConvexCallback::addSingleResult(btCollisionWorld::LocalConvexResult& result, bool normalInWorldSpace)
     {
+        const auto* hitObject = result.m_hitCollisionObject;
         // don't hit the caster
-        if (result.m_hitCollisionObject == mMe)
+        if (hitObject == mCaster)
             return 1.f;
 
         // don't hit the projectile
-        if (result.m_hitCollisionObject == mProjectile->getCollisionObject())
+        if (hitObject == mMe)
             return 1.f;
 
         btCollisionWorld::ClosestConvexResultCallback::addSingleResult(result, normalInWorldSpace);
-        switch (result.m_hitCollisionObject->getBroadphaseHandle()->m_collisionFilterGroup)
+        switch (hitObject->getBroadphaseHandle()->m_collisionFilterGroup)
         {
             case CollisionType_Actor:
                 {
-                    auto* target = static_cast<Actor*>(result.m_hitCollisionObject->getUserPointer());
+                    if (!mProjectile->isValidTarget(hitObject))
-                    if (!mProjectile->isValidTarget(target->getPtr()))
                         return 1.f;
-                    mProjectile->hit(target->getPtr(), result.m_hitPointLocal, result.m_hitNormalLocal);
                     break;
                 }
             case CollisionType_Projectile:
                 {
-                    auto* target = static_cast<Projectile*>(result.m_hitCollisionObject->getUserPointer());
+                    auto* target = static_cast<Projectile*>(hitObject->getUserPointer());
-                    if (!mProjectile->isValidTarget(target->getCaster()))
+                    if (!mProjectile->isValidTarget(target->getCasterCollisionObject()))
                         return 1.f;
-                    target->hit(mProjectile->getPtr(), m_hitPointWorld, m_hitNormalWorld);
+                    target->hit(mMe, m_hitPointWorld, m_hitNormalWorld);
-                    mProjectile->hit(target->getPtr(), m_hitPointWorld, m_hitNormalWorld);
                     break;
                 }
             case CollisionType_Water:
@@ -50,17 +52,10 @@ namespace MWPhysics
                     mProjectile->setWaterHitPosition(m_hitPointWorld);
                     if (mProjectile->canTraverseWater())
                         return 1.f;
-                    mProjectile->hit(MWWorld::Ptr(), m_hitPointWorld, m_hitNormalWorld);
-                    break;
-                }
-            default:
-                {
-                    auto* target = static_cast<PtrHolder*>(result.m_hitCollisionObject->getUserPointer());
-                    auto ptr = target ? target->getPtr() : MWWorld::Ptr();
-                    mProjectile->hit(ptr, m_hitPointWorld, m_hitNormalWorld);
                     break;
                 }
         }
+        mProjectile->hit(hitObject, m_hitPointWorld, m_hitNormalWorld);
 
         return result.m_hitFraction;
     }

apps/openmw/mwphysics/projectileconvexcallback.hpp

@@ -12,11 +12,12 @@ namespace MWPhysics
     class ProjectileConvexCallback : public btCollisionWorld::ClosestConvexResultCallback
     {
     public:
-        ProjectileConvexCallback(const btCollisionObject* me, const btVector3& from, const btVector3& to, Projectile* proj);
+        ProjectileConvexCallback(const btCollisionObject* caster, const btCollisionObject* me, const btVector3& from, const btVector3& to, Projectile* proj);
 
         btScalar addSingleResult(btCollisionWorld::LocalConvexResult& result, bool normalInWorldSpace) override;
 
     private:
+        const btCollisionObject* mCaster;
         const btCollisionObject* mMe;
         Projectile* mProjectile;
     };

apps/openmw/mwworld/projectilemanager.cpp

@@ -521,7 +521,7 @@ namespace MWWorld
             }
 
             MWMechanics::projectileHit(caster, target, bow, projectileRef.getPtr(), pos, projectileState.mAttackStrength);
-            cleanupProjectile(projectileState);
+            projectileState.mToDelete = true;
         }
         for (auto& magicBoltState : mMagicBolts)
         {
@@ -550,7 +550,19 @@ namespace MWWorld
             cast.inflict(target, caster, magicBoltState.mEffects, ESM::RT_Target, false, true);
 
             MWBase::Environment::get().getWorld()->explodeSpell(pos, magicBoltState.mEffects, caster, target, ESM::RT_Target, magicBoltState.mSpellId, magicBoltState.mSourceName);
-            cleanupMagicBolt(magicBoltState);
+            magicBoltState.mToDelete = true;
+        }
+
+        for (auto& projectileState : mProjectiles)
+        {
+            if (projectileState.mToDelete)
+                cleanupProjectile(projectileState);
+        }
+
+        for (auto& magicBoltState : mMagicBolts)
+        {
+            if (magicBoltState.mToDelete)
+                cleanupMagicBolt(magicBoltState);
         }
         mProjectiles.erase(std::remove_if(mProjectiles.begin(), mProjectiles.end(), [](const State& state) { return state.mToDelete; }),
                 mProjectiles.end());