From ec871e6bf7d9feb2f5e99a25320e69c720464835 Mon Sep 17 00:00:00 2001 From: fredzio Date: Sat, 14 Aug 2021 23:15:11 +0200 Subject: [PATCH] Use shared_ptr instead of weak_ptr for actors handle inside the simulation The purpose of weak_ptr is to avoid performing the simulation on deleted Actor by promoting it to a shared_ptr via a lock() call. This clutter the code with a lot of branches, whereas the overwhelmingly common case is for the call to succeed. Since the simulation is decoupled from the engine state, we can use a shared_ptr instead of a weak_ptr. This allow us to ignore (ie not handle) the rarer case where the actor is delete from the scene. This means that the simulation will run for one frame more than before for each actor, whereas the rest of the engine will be ignorant of this. --- apps/openmw/mwphysics/mtphysics.cpp | 106 ++++++++++-------------- apps/openmw/mwphysics/mtphysics.hpp | 14 ++-- apps/openmw/mwphysics/physicssystem.cpp | 18 ++-- apps/openmw/mwphysics/physicssystem.hpp | 2 +- 4 files changed, 61 insertions(+), 79 deletions(-) diff --git a/apps/openmw/mwphysics/mtphysics.cpp b/apps/openmw/mwphysics/mtphysics.cpp index e9bedcbc75..7363b9964a 100644 --- a/apps/openmw/mwphysics/mtphysics.cpp +++ b/apps/openmw/mwphysics/mtphysics.cpp @@ -207,12 +207,13 @@ namespace MWPhysics return std::make_tuple(numSteps, actualDelta); } - void PhysicsTaskScheduler::applyQueuedMovements(float & timeAccum, std::vector>&& actors, std::vector&& actorsData, osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats) + void PhysicsTaskScheduler::applyQueuedMovements(float & timeAccum, std::vector>&& actors, std::vector&& actorsData, osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats) { // This function run in the main thread. // While the mSimulationMutex is held, background physics threads can't run. std::unique_lock lock(mSimulationMutex); + assert(actors.size() == actorsData.size()); double timeStart = mTimer->tick(); @@ -221,11 +222,8 @@ namespace MWPhysics { for (size_t i = 0; i < mActors.size(); ++i) { - if (auto actor = mActors[i].lock()) - { - updateMechanics(*actor, mActorsFrameData[i]); - updateActor(*actor, mActorsFrameData[i], mAdvanceSimulation, mTimeAccum, mPhysicsDt); - } + updateMechanics(*mActors[i], mActorsFrameData[i]); + updateActor(*mActors[i], mActorsFrameData[i], mAdvanceSimulation, mTimeAccum, mPhysicsDt); } if(mAdvanceSimulation) mAsyncBudget.update(mTimer->delta_s(mAsyncStartTime, mTimeEnd), mPrevStepCount, mBudgetCursor); @@ -238,8 +236,7 @@ namespace MWPhysics // init for (size_t i = 0; i < actors.size(); ++i) { - assert(actors[i].lock()); - actorsData[i].updatePosition(*actors[i].lock(), mCollisionWorld); + actorsData[i].updatePosition(*actors[i], mCollisionWorld); } mPrevStepCount = numSteps; mRemainingSteps = numSteps; @@ -355,7 +352,7 @@ namespace MWPhysics mCollisionWorld->removeCollisionObject(collisionObject); } - void PhysicsTaskScheduler::updateSingleAabb(std::weak_ptr ptr, bool immediate) + void PhysicsTaskScheduler::updateSingleAabb(std::shared_ptr ptr, bool immediate) { if (immediate || mNumThreads == 0) { @@ -368,20 +365,15 @@ namespace MWPhysics } } - bool PhysicsTaskScheduler::getLineOfSight(const std::weak_ptr& actor1, const std::weak_ptr& actor2) + bool PhysicsTaskScheduler::getLineOfSight(const std::shared_ptr& actor1, const std::shared_ptr& actor2) { std::unique_lock lock(mLOSCacheMutex); - auto actorPtr1 = actor1.lock(); - auto actorPtr2 = actor2.lock(); - if (!actorPtr1 || !actorPtr2) - return false; - auto req = LOSRequest(actor1, actor2); auto result = std::find(mLOSCache.begin(), mLOSCache.end(), req); if (result == mLOSCache.end()) { - req.mResult = hasLineOfSight(actorPtr1.get(), actorPtr2.get()); + req.mResult = hasLineOfSight(actor1.get(), actor2.get()); mLOSCache.push_back(req); return req.mResult; } @@ -412,31 +404,28 @@ namespace MWPhysics { std::scoped_lock lock(mUpdateAabbMutex); std::for_each(mUpdateAabb.begin(), mUpdateAabb.end(), - [this](const std::weak_ptr& ptr) { updatePtrAabb(ptr); }); + [this](const std::shared_ptr& ptr) { updatePtrAabb(ptr); }); mUpdateAabb.clear(); } - void PhysicsTaskScheduler::updatePtrAabb(const std::weak_ptr& ptr) + void PhysicsTaskScheduler::updatePtrAabb(const std::shared_ptr& ptr) { - if (const auto p = ptr.lock()) + std::scoped_lock lock(mCollisionWorldMutex); + if (const auto actor = std::dynamic_pointer_cast(ptr)) { - std::scoped_lock lock(mCollisionWorldMutex); - if (const auto actor = std::dynamic_pointer_cast(p)) - { - actor->updateCollisionObjectPosition(); - mCollisionWorld->updateSingleAabb(actor->getCollisionObject()); - } - else if (const auto object = std::dynamic_pointer_cast(p)) - { - object->commitPositionChange(); - mCollisionWorld->updateSingleAabb(object->getCollisionObject()); - } - else if (const auto projectile = std::dynamic_pointer_cast(p)) - { - projectile->commitPositionChange(); - mCollisionWorld->updateSingleAabb(projectile->getCollisionObject()); - } - }; + actor->updateCollisionObjectPosition(); + mCollisionWorld->updateSingleAabb(actor->getCollisionObject()); + } + else if (const auto object = std::dynamic_pointer_cast(ptr)) + { + object->commitPositionChange(); + mCollisionWorld->updateSingleAabb(object->getCollisionObject()); + } + else if (const auto projectile = std::dynamic_pointer_cast(ptr)) + { + projectile->commitPositionChange(); + mCollisionWorld->updateSingleAabb(projectile->getCollisionObject()); + } } void PhysicsTaskScheduler::worker() @@ -452,11 +441,8 @@ namespace MWPhysics int job = 0; while (mRemainingSteps && (job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs) { - if(const auto actor = mActors[job].lock()) - { - MaybeSharedLock lockColWorld(mCollisionWorldMutex, mThreadSafeBullet); - MovementSolver::move(mActorsFrameData[job], mPhysicsDt, mCollisionWorld, *mWorldFrameData); - } + MaybeSharedLock lockColWorld(mCollisionWorldMutex, mThreadSafeBullet); + MovementSolver::move(mActorsFrameData[job], mPhysicsDt, mCollisionWorld, *mWorldFrameData); } mPostStepBarrier->wait([this] { afterPostStep(); }); @@ -465,10 +451,7 @@ namespace MWPhysics { while ((job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs) { - if(const auto actor = mActors[job].lock()) - { - handleFall(mActorsFrameData[job], mAdvanceSimulation); - } + handleFall(mActorsFrameData[job], mAdvanceSimulation); } refreshLOSCache(); @@ -481,15 +464,12 @@ namespace MWPhysics { for (size_t i = 0; i < mActors.size(); ++i) { - if(const auto actor = mActors[i].lock()) + if (mActors[i]->setPosition(mActorsFrameData[i].mPosition)) { - if (actor->setPosition(mActorsFrameData[i].mPosition)) - { - std::scoped_lock lock(mCollisionWorldMutex); - mActorsFrameData[i].mPosition = actor->getPosition(); // account for potential position change made by script - actor->updateCollisionObjectPosition(); - mCollisionWorld->updateSingleAabb(actor->getCollisionObject()); - } + std::scoped_lock lock(mCollisionWorldMutex); + mActorsFrameData[i].mPosition = mActors[i]->getPosition(); // account for potential position change made by script + mActors[i]->updateCollisionObjectPosition(); + mCollisionWorld->updateSingleAabb(mActors[i]->getCollisionObject()); } } } @@ -545,11 +525,9 @@ namespace MWPhysics for (size_t i = 0; i < mActors.size(); ++i) { - auto actor = mActors[i].lock(); - assert(actor); handleFall(mActorsFrameData[i], mAdvanceSimulation); - updateMechanics(*actor, mActorsFrameData[i]); - updateActor(*actor, mActorsFrameData[i], mAdvanceSimulation, mTimeAccum, mPhysicsDt); + updateMechanics(*mActors[i], mActorsFrameData[i]); + updateActor(*mActors[i], mActorsFrameData[i], mAdvanceSimulation, mTimeAccum, mPhysicsDt); } refreshLOSCache(); } @@ -583,6 +561,13 @@ namespace MWPhysics return (*it)->getUserPointer(); } + void PhysicsTaskScheduler::releaseSharedStates() + { + std::scoped_lock lock(mSimulationMutex, mUpdateAabbMutex); + mActors.clear(); + mUpdateAabb.clear(); + } + void PhysicsTaskScheduler::afterPreStep() { updateAabbs(); @@ -590,11 +575,8 @@ namespace MWPhysics return; for (size_t i = 0; i < mActors.size(); ++i) { - if (auto actor = mActors[i].lock()) - { - std::unique_lock lock(mCollisionWorldMutex); - MovementSolver::unstuck(mActorsFrameData[i], mCollisionWorld); - } + std::unique_lock lock(mCollisionWorldMutex); + MovementSolver::unstuck(mActorsFrameData[i], mCollisionWorld); } } diff --git a/apps/openmw/mwphysics/mtphysics.hpp b/apps/openmw/mwphysics/mtphysics.hpp index e758c557e2..e2cfccffb0 100644 --- a/apps/openmw/mwphysics/mtphysics.hpp +++ b/apps/openmw/mwphysics/mtphysics.hpp @@ -39,7 +39,7 @@ namespace MWPhysics /// @param timeAccum accumulated time from previous run to interpolate movements /// @param actorsData per actor data needed to compute new positions /// @return new position of each actor - void applyQueuedMovements(float & timeAccum, std::vector>&& actors, std::vector&& actorsData, osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats); + void applyQueuedMovements(float & timeAccum, std::vector>&& actors, std::vector&& actorsData, osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats); void resetSimulation(const ActorMap& actors); @@ -53,11 +53,13 @@ namespace MWPhysics void setCollisionFilterMask(btCollisionObject* collisionObject, int collisionFilterMask); void addCollisionObject(btCollisionObject* collisionObject, int collisionFilterGroup, int collisionFilterMask); void removeCollisionObject(btCollisionObject* collisionObject); - void updateSingleAabb(std::weak_ptr ptr, bool immediate=false); - bool getLineOfSight(const std::weak_ptr& actor1, const std::weak_ptr& actor2); + void updateSingleAabb(std::shared_ptr ptr, bool immediate=false); + bool getLineOfSight(const std::shared_ptr& actor1, const std::shared_ptr& actor2); void debugDraw(); void* getUserPointer(const btCollisionObject* object) const; + void releaseSharedStates(); // destroy all objects whose destructor can't be safely called from ~PhysicsTaskScheduler() + private: void syncComputation(); void worker(); @@ -66,7 +68,7 @@ namespace MWPhysics bool hasLineOfSight(const Actor* actor1, const Actor* actor2); void refreshLOSCache(); void updateAabbs(); - void updatePtrAabb(const std::weak_ptr& ptr); + void updatePtrAabb(const std::shared_ptr& ptr); void updateStats(osg::Timer_t frameStart, unsigned int frameNumber, osg::Stats& stats); std::tuple calculateStepConfig(float timeAccum) const; void afterPreStep(); @@ -74,7 +76,7 @@ namespace MWPhysics void afterPostSim(); std::unique_ptr mWorldFrameData; - std::vector> mActors; + std::vector> mActors; std::vector mActorsFrameData; std::unordered_set mCollisionObjects; float mDefaultPhysicsDt; @@ -83,7 +85,7 @@ namespace MWPhysics btCollisionWorld* mCollisionWorld; MWRender::DebugDrawer* mDebugDrawer; std::vector mLOSCache; - std::set, std::owner_less>> mUpdateAabb; + std::set> mUpdateAabb; // TODO: use std::experimental::flex_barrier or std::barrier once it becomes a thing std::unique_ptr mPreStepBarrier; diff --git a/apps/openmw/mwphysics/physicssystem.cpp b/apps/openmw/mwphysics/physicssystem.cpp index 846f80ec64..d881285e64 100644 --- a/apps/openmw/mwphysics/physicssystem.cpp +++ b/apps/openmw/mwphysics/physicssystem.cpp @@ -153,6 +153,7 @@ namespace MWPhysics if (mWaterCollisionObject) mTaskScheduler->removeCollisionObject(mWaterCollisionObject.get()); + mTaskScheduler->releaseSharedStates(); mHeightFields.clear(); mObjects.clear(); mActors.clear(); @@ -373,15 +374,12 @@ namespace MWPhysics bool PhysicsSystem::getLineOfSight(const MWWorld::ConstPtr &actor1, const MWWorld::ConstPtr &actor2) const { - const auto getWeakPtr = [&](const MWWorld::ConstPtr &ptr) -> std::weak_ptr - { - const auto found = mActors.find(ptr); - if (found != mActors.end()) - return { found->second }; - return {}; - }; + const auto it1 = mActors.find(actor1); + const auto it2 = mActors.find(actor2); + if (it1 == mActors.end() || it2 == mActors.end()) + return false; - return mTaskScheduler->getLineOfSight(getWeakPtr(actor1), getWeakPtr(actor2)); + return mTaskScheduler->getLineOfSight(it1->second, it2->second); } bool PhysicsSystem::isOnGround(const MWWorld::Ptr &actor) @@ -769,9 +767,9 @@ namespace MWPhysics actor->setVelocity(osg::Vec3f()); } - std::pair>, std::vector> PhysicsSystem::prepareFrameData(bool willSimulate) + std::pair>, std::vector> PhysicsSystem::prepareFrameData(bool willSimulate) { - std::pair>, std::vector> framedata; + std::pair>, std::vector> framedata; framedata.first.reserve(mActors.size()); framedata.second.reserve(mActors.size()); const MWBase::World *world = MWBase::Environment::get().getWorld(); diff --git a/apps/openmw/mwphysics/physicssystem.hpp b/apps/openmw/mwphysics/physicssystem.hpp index f1b6bc147a..b20c8f88e1 100644 --- a/apps/openmw/mwphysics/physicssystem.hpp +++ b/apps/openmw/mwphysics/physicssystem.hpp @@ -259,7 +259,7 @@ namespace MWPhysics void updateWater(); - std::pair>, std::vector> prepareFrameData(bool willSimulate); + std::pair>, std::vector> prepareFrameData(bool willSimulate); osg::ref_ptr mUnrefQueue;