Merge branch 'theemperorwantedyoutohavethisinvaliditerator' into 'master'

Prevent iterator invalidation during actor traversal

Closes #4885

See merge request OpenMW/openmw!4445

apps/openmw/mwmechanics/actor.hpp

@@ -28,7 +28,7 @@ namespace MWMechanics
     class Actor
     {
     public:
-        Actor(const MWWorld::Ptr& ptr, MWRender::Animation* animation)
+        Actor(const MWWorld::Ptr& ptr, MWRender::Animation& animation)
             : mCharacterController(ptr, animation)
             , mPositionAdjusted(ptr.getClass().getCreatureStats(ptr).getFallHeight() > 0)
         {
@@ -62,14 +62,22 @@ namespace MWMechanics
         void setPositionAdjusted(bool adjusted) { mPositionAdjusted = adjusted; }
         bool getPositionAdjusted() const { return mPositionAdjusted; }
 
+        void invalidate()
+        {
+            mInvalid = true;
+            mCharacterController.detachAnimation();
+        }
+        bool isInvalid() const { return mInvalid; }
+
     private:
         CharacterController mCharacterController;
         int mGreetingTimer{ 0 };
         float mTargetAngleRadians{ 0.f };
         GreetingState mGreetingState{ Greet_None };
-        bool mIsTurningToPlayer{ false };
         Misc::DeviatingPeriodicTimer mEngageCombat{ 1.0f, 0.25f,
             Misc::Rng::deviate(0, 0.25f, MWBase::Environment::get().getWorld()->getPrng()) };
+        bool mIsTurningToPlayer{ false };
+        bool mInvalid{ false };
         bool mPositionAdjusted;
     };
 

apps/openmw/mwmechanics/actors.cpp

@@ -122,6 +122,8 @@ namespace
     {
         for (const MWMechanics::Actor& actor : actors)
         {
+            if (actor.isInvalid())
+                continue;
             const MWWorld::Ptr& iteratedActor = actor.getPtr();
             if (iteratedActor == player || iteratedActor == actorPtr)
                 continue;
@@ -345,7 +347,7 @@ namespace MWMechanics
                     // Find something nearby.
                     for (const Actor& otherActor : actors)
                     {
-                        if (otherActor.getPtr() == ptr)
+                        if (otherActor.isInvalid() || otherActor.getPtr() == ptr)
                             continue;
 
                         updateHeadTracking(
@@ -1195,7 +1197,7 @@ namespace MWMechanics
         MWRender::Animation* anim = MWBase::Environment::get().getWorld()->getAnimation(ptr);
         if (!anim)
             return;
-        const auto it = mActors.emplace(mActors.end(), ptr, anim);
+        const auto it = mActors.emplace(mActors.end(), ptr, *anim);
         mIndex.emplace(ptr.mRef, it);
 
         if (updateImmediately)
@@ -1247,7 +1249,7 @@ namespace MWMechanics
         {
             if (!keepActive)
                 removeTemporaryEffects(iter->second->getPtr());
-            mActors.erase(iter->second);
+            iter->second->invalidate();
             mIndex.erase(iter);
         }
     }
@@ -1299,16 +1301,15 @@ namespace MWMechanics
 
     void Actors::dropActors(const MWWorld::CellStore* cellStore, const MWWorld::Ptr& ignore)
     {
-        for (auto iter = mActors.begin(); iter != mActors.end();)
+        for (Actor& actor : mActors)
         {
-            if ((iter->getPtr().isInCell() && iter->getPtr().getCell() == cellStore) && iter->getPtr() != ignore)
+            if (!actor.isInvalid() && actor.getPtr().isInCell() && actor.getPtr().getCell() == cellStore
+                && actor.getPtr() != ignore)
             {
-                removeTemporaryEffects(iter->getPtr());
+                removeTemporaryEffects(actor.getPtr());
-                mIndex.erase(iter->getPtr().mRef);
+                mIndex.erase(actor.getPtr().mRef);
-                iter = mActors.erase(iter);
+                actor.invalidate();
             }
-            else
-                ++iter;
         }
     }
 
@@ -1327,6 +1328,8 @@ namespace MWMechanics
         const MWBase::World* const world = MWBase::Environment::get().getWorld();
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             const MWWorld::Ptr& ptr = actor.getPtr();
             if (ptr == player)
                 continue; // Don't interfere with player controls.
@@ -1391,6 +1394,8 @@ namespace MWMechanics
             // Iterate through all other actors and predict collisions.
             for (const Actor& otherActor : mActors)
             {
+                if (otherActor.isInvalid())
+                    continue;
                 const MWWorld::Ptr& otherPtr = otherActor.getPtr();
                 if (otherPtr == ptr || otherPtr == currentTarget)
                     continue;
@@ -1509,6 +1514,8 @@ namespace MWMechanics
             // AI and magic effects update
             for (Actor& actor : mActors)
             {
+                if (actor.isInvalid())
+                    continue;
                 const bool isPlayer = actor.getPtr() == player;
                 CharacterController& ctrl = actor.getCharacterController();
                 MWBase::LuaManager::ActorControls* luaControls
@@ -1570,6 +1577,8 @@ namespace MWMechanics
 
                             for (const Actor& otherActor : mActors)
                             {
+                                if (otherActor.isInvalid())
+                                    continue;
                                 if (otherActor.getPtr() == actor.getPtr() || isPlayer) // player is not AI-controlled
                                     continue;
                                 engageCombat(
@@ -1627,6 +1636,8 @@ namespace MWMechanics
             CharacterController* playerCharacter = nullptr;
             for (Actor& actor : mActors)
             {
+                if (actor.isInvalid())
+                    continue;
                 const float dist = (playerPos - actor.getPtr().getRefData().getPosition().asVec3()).length();
                 const bool isPlayer = actor.getPtr() == player;
                 CreatureStats& stats = actor.getPtr().getClass().getCreatureStats(actor.getPtr());
@@ -1692,8 +1703,15 @@ namespace MWMechanics
                     luaControls->mJump = false;
             }
 
-            for (const Actor& actor : mActors)
+            for (auto it = mActors.begin(); it != mActors.end();)
             {
+                if (it->isInvalid())
+                {
+                    it = mActors.erase(it);
+                    continue;
+                }
+                const Actor& actor = *it;
+                it++;
                 const MWWorld::Class& cls = actor.getPtr().getClass();
                 CreatureStats& stats = cls.getCreatureStats(actor.getPtr());
 
@@ -1743,6 +1761,8 @@ namespace MWMechanics
     {
         for (Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             const MWWorld::Class& cls = actor.getPtr().getClass();
             CreatureStats& stats = cls.getCreatureStats(actor.getPtr());
 
@@ -1830,6 +1850,8 @@ namespace MWMechanics
     {
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             MWMechanics::ActiveSpells& spells
                 = actor.getPtr().getClass().getCreatureStats(actor.getPtr()).getActiveSpells();
             spells.purge(actor.getPtr(), casterActorId);
@@ -1849,6 +1871,8 @@ namespace MWMechanics
 
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             if (actor.getPtr().getClass().getCreatureStats(actor.getPtr()).isDead())
             {
                 adjustMagicEffects(actor.getPtr(), duration);
@@ -2046,8 +2070,11 @@ namespace MWMechanics
     void Actors::persistAnimationStates() const
     {
         for (const Actor& actor : mActors)
+        {
+            if (!actor.isInvalid())
                 actor.getCharacterController().persistAnimationState();
         }
+    }
 
     void Actors::clearAnimationQueue(const MWWorld::Ptr& ptr, bool clearScripted)
     {
@@ -2060,6 +2087,8 @@ namespace MWMechanics
     {
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             if ((actor.getPtr().getRefData().getPosition().asVec3() - position).length2() <= radius * radius)
                 out.push_back(actor.getPtr());
         }
@@ -2069,6 +2098,8 @@ namespace MWMechanics
     {
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             if ((actor.getPtr().getRefData().getPosition().asVec3() - position).length2() <= radius * radius)
                 return true;
         }
@@ -2082,6 +2113,8 @@ namespace MWMechanics
         list.push_back(actorPtr);
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             const MWWorld::Ptr& iteratedActor = actor.getPtr();
             if (iteratedActor == getPlayer())
                 continue;
@@ -2352,10 +2385,11 @@ namespace MWMechanics
         if (!MWBase::Environment::get().getMechanicsManager()->isAIActive())
             return;
 
-        for (auto it = mActors.begin(); it != mActors.end();)
+        for (const Actor& actor : mActors)
         {
-            const MWWorld::Ptr ptr = it->getPtr();
+            if (actor.isInvalid())
-            ++it;
+                continue;
+            const MWWorld::Ptr ptr = actor.getPtr();
             if (ptr == getPlayer() || !isConscious(ptr) || ptr.getClass().getCreatureStats(ptr).isParalyzed())
                 continue;
             MWMechanics::AiSequence& seq = ptr.getClass().getCreatureStats(ptr).getAiSequence();

apps/openmw/mwmechanics/character.cpp

@@ -535,7 +535,7 @@ namespace MWMechanics
 
     bool CharacterController::onOpen() const
     {
-        if (mPtr.getType() == ESM::Container::sRecordId)
+        if (mPtr.getType() == ESM::Container::sRecordId && mAnimation)
         {
             if (!mAnimation->hasAnimation("containeropen"))
                 return true;
@@ -559,7 +559,7 @@ namespace MWMechanics
     {
         if (mPtr.getType() == ESM::Container::sRecordId)
         {
-            if (!mAnimation->hasAnimation("containerclose"))
+            if (!mAnimation || !mAnimation->hasAnimation("containerclose"))
                 return;
 
             float complete, startPoint = 0.f;
@@ -886,11 +886,12 @@ namespace MWMechanics
         if (mDeathState == CharState_None && MWBase::Environment::get().getWorld()->isSwimming(mPtr))
             mDeathState = CharState_SwimDeath;
 
-        if (mDeathState == CharState_None || !mAnimation->hasAnimation(deathStateToAnimGroup(mDeathState)))
+        if (mDeathState == CharState_None
+            || (mAnimation && !mAnimation->hasAnimation(deathStateToAnimGroup(mDeathState))))
             mDeathState = chooseRandomDeathState();
 
         // Do not interrupt scripted animation by death
-        if (isScriptedAnimPlaying())
+        if (!mAnimation || isScriptedAnimPlaying())
             return;
 
         playDeath(startpoint, mDeathState);
@@ -910,13 +911,10 @@ namespace MWMechanics
         return result;
     }
 
-    CharacterController::CharacterController(const MWWorld::Ptr& ptr, MWRender::Animation* anim)
+    CharacterController::CharacterController(const MWWorld::Ptr& ptr, MWRender::Animation& anim)
         : mPtr(ptr)
-        , mAnimation(anim)
+        , mAnimation(&anim)
     {
-        if (!mAnimation)
-            return;
-
         mAnimation->setTextKeyListener(this);
 
         const MWWorld::Class& cls = mPtr.getClass();
@@ -992,17 +990,25 @@ namespace MWMechanics
     }
 
     CharacterController::~CharacterController()
+    {
+        detachAnimation();
+    }
+
+    void CharacterController::detachAnimation()
     {
         if (mAnimation)
         {
             persistAnimationState();
             mAnimation->setTextKeyListener(nullptr);
+            mAnimation = nullptr;
         }
     }
 
     void CharacterController::handleTextKey(
         std::string_view groupname, SceneUtil::TextKeyMap::ConstIterator key, const SceneUtil::TextKeyMap& map)
     {
+        if (!mAnimation)
+            return;
         std::string_view evt = key->second;
 
         MWBase::Environment::get().getLuaManager()->animationTextKey(mPtr, key->second);
@@ -1232,7 +1238,8 @@ namespace MWMechanics
 
     float CharacterController::calculateWindUp() const
     {
-        if (mCurrentWeapon.empty() || mWeaponType == ESM::Weapon::PickProbe || isRandomAttackAnimation(mCurrentWeapon))
+        if (!mAnimation || mCurrentWeapon.empty() || mWeaponType == ESM::Weapon::PickProbe
+            || isRandomAttackAnimation(mCurrentWeapon))
             return -1.f;
 
         float minAttackTime = mAnimation->getTextKeyTime(mCurrentWeapon + ": " + mAttackType + " min attack");
@@ -1950,6 +1957,8 @@ namespace MWMechanics
 
     void CharacterController::update(float duration)
     {
+        if (!mAnimation)
+            return;
         MWBase::World* world = MWBase::Environment::get().getWorld();
         MWBase::SoundManager* sndMgr = MWBase::Environment::get().getSoundManager();
         const MWWorld::Class& cls = mPtr.getClass();
@@ -2528,7 +2537,7 @@ namespace MWMechanics
             ESM::AnimationState::ScriptedAnimation anim;
             anim.mGroup = iter->mGroup;
 
-            if (iter == mAnimQueue.begin())
+            if (iter == mAnimQueue.begin() && mAnimation)
             {
                 float complete;
                 size_t loopcount;
@@ -2741,23 +2750,18 @@ namespace MWMechanics
     void CharacterController::clearAnimQueue(bool clearScriptedAnims)
     {
         // Do not interrupt scripted animations, if we want to keep them
-        if ((!isScriptedAnimPlaying() || clearScriptedAnims) && !mAnimQueue.empty())
+        if (mAnimation && (!isScriptedAnimPlaying() || clearScriptedAnims) && !mAnimQueue.empty())
             mAnimation->disable(mAnimQueue.front().mGroup);
 
         if (clearScriptedAnims)
         {
+            if (mAnimation)
                 mAnimation->setPlayScriptedOnly(false);
             mAnimQueue.clear();
             return;
         }
 
-        for (AnimationQueue::iterator it = mAnimQueue.begin(); it != mAnimQueue.end();)
+        std::erase_if(mAnimQueue, [](const AnimationQueueEntry& entry) { return !entry.mScripted; });
-        {
-            if (!it->mScripted)
-                it = mAnimQueue.erase(it);
-            else
-                ++it;
-        }
     }
 
     void CharacterController::forceStateUpdate()
@@ -2866,6 +2870,8 @@ namespace MWMechanics
 
     void CharacterController::setVisibility(float visibility) const
     {
+        if (!mAnimation)
+            return;
         // We should take actor's invisibility in account
         if (mPtr.getClass().isActor())
         {
@@ -2926,7 +2932,7 @@ namespace MWMechanics
 
     bool CharacterController::isReadyToBlock() const
     {
-        return updateCarriedLeftVisible(mWeaponType);
+        return mAnimation && updateCarriedLeftVisible(mWeaponType);
     }
 
     bool CharacterController::isKnockedDown() const
@@ -3030,6 +3036,7 @@ namespace MWMechanics
 
     void CharacterController::setActive(int active) const
     {
+        if (mAnimation)
             mAnimation->setActive(active);
     }
 
@@ -3061,6 +3068,8 @@ namespace MWMechanics
 
     float CharacterController::getAnimationMovementDirection() const
     {
+        if (!mAnimation)
+            return 0.f;
         switch (mMovementState)
         {
             case CharState_RunLeft:
@@ -3155,6 +3164,8 @@ namespace MWMechanics
 
     MWWorld::MovementDirectionFlags CharacterController::getSupportedMovementDirections() const
     {
+        if (!mAnimation)
+            return 0;
         using namespace std::string_view_literals;
         // There are fallbacks in the CharacterController::refreshMovementAnims for certain animations. Arrays below
         // represent them.

apps/openmw/mwmechanics/character.hpp

@@ -252,13 +252,21 @@ namespace MWMechanics
 
         void prepareHit();
 
+        void unpersistAnimationState();
+
+        void playBlendedAnimation(const std::string& groupname, const MWRender::AnimPriority& priority, int blendMask,
+            bool autodisable, float speedmult, std::string_view start, std::string_view stop, float startpoint,
+            uint32_t loops, bool loopfallback = false) const;
+
     public:
-        CharacterController(const MWWorld::Ptr& ptr, MWRender::Animation* anim);
+        CharacterController(const MWWorld::Ptr& ptr, MWRender::Animation& anim);
         virtual ~CharacterController();
 
         CharacterController(const CharacterController&) = delete;
         CharacterController(CharacterController&&) = delete;
 
+        void detachAnimation();
+
         const MWWorld::Ptr& getPtr() const { return mPtr; }
 
         void handleTextKey(std::string_view groupname, SceneUtil::TextKeyMap::ConstIterator key,
@@ -275,11 +283,6 @@ namespace MWMechanics
         void onClose() const;
 
         void persistAnimationState() const;
-        void unpersistAnimationState();
-
-        void playBlendedAnimation(const std::string& groupname, const MWRender::AnimPriority& priority, int blendMask,
-            bool autodisable, float speedmult, std::string_view start, std::string_view stop, float startpoint,
-            uint32_t loops, bool loopfallback = false) const;
         bool playGroup(std::string_view groupname, int mode, uint32_t count, bool scripted = false);
         bool playGroupLua(std::string_view groupname, float speed, std::string_view startKey, std::string_view stopKey,
             uint32_t loops, bool forceLoop);

apps/openmw/mwmechanics/mechanicsmanagerimp.cpp

@@ -1736,6 +1736,8 @@ namespace MWMechanics
                         .getActorId()); // Stops guard from ending combat if player is unreachable
                 for (const Actor& actor : mActors)
                 {
+                    if (actor.isInvalid())
+                        continue;
                     if (actor.getPtr().getClass().isClass(actor.getPtr(), "Guard"))
                     {
                         MWMechanics::AiSequence& aiSeq

apps/openmw/mwmechanics/objects.cpp

@@ -20,7 +20,7 @@ namespace MWMechanics
         if (anim == nullptr)
             return;
 
-        const auto it = mObjects.emplace(mObjects.end(), ptr, anim);
+        const auto it = mObjects.emplace(mObjects.end(), ptr, *anim);
         mIndex.emplace(ptr.mRef, it);
     }