Merge branch 'theemperorwantedyoutohavethisinvaliditerator' into 'master'

Prevent iterator invalidation during actor traversal

Closes #4885

See merge request OpenMW/openmw!4445

CHANGELOG.md

@@ -18,6 +18,7 @@
     Bug #4754: Stack of ammunition cannot be equipped partially
     Bug #4816: GetWeaponDrawn returns 1 before weapon is attached
     Bug #4822: Non-weapon equipment and body parts can't inherit time from parent animation
+    Bug #4885: Disable in dialogue result script causes a crash
     Bug #4898: Odd/Incorrect lighting on meshes
     Bug #5057: Weapon swing sound plays at same pitch whether it hits or misses
     Bug #5062: Root bone rotations for NPC animation don't work the same as for creature animation

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(
@@ -1196,7 +1198,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)
@@ -1248,7 +1250,7 @@ namespace MWMechanics
         {
             if (!keepActive)
                 removeTemporaryEffects(iter->second->getPtr());
-            mActors.erase(iter->second);
+            iter->second->invalidate();
             mIndex.erase(iter);
         }
     }
@@ -1300,16 +1302,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());
-                mIndex.erase(iter->getPtr().mRef);
-                iter = mActors.erase(iter);
+                removeTemporaryEffects(actor.getPtr());
+                mIndex.erase(actor.getPtr().mRef);
+                actor.invalidate();
             }
-            else
-                ++iter;
         }
     }
 
@@ -1328,6 +1329,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.
@@ -1392,6 +1395,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;
@@ -1510,6 +1515,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
@@ -1571,6 +1578,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(
@@ -1628,6 +1637,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());
@@ -1693,8 +1704,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());
 
@@ -1744,6 +1762,8 @@ namespace MWMechanics
     {
         for (Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             const MWWorld::Class& cls = actor.getPtr().getClass();
             CreatureStats& stats = cls.getCreatureStats(actor.getPtr());
 
@@ -1831,6 +1851,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);
@@ -1850,6 +1872,8 @@ namespace MWMechanics
 
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             if (actor.getPtr().getClass().getCreatureStats(actor.getPtr()).isDead())
             {
                 adjustMagicEffects(actor.getPtr(), duration);
@@ -2047,7 +2071,10 @@ namespace MWMechanics
     void Actors::persistAnimationStates() const
     {
         for (const Actor& actor : mActors)
-            actor.getCharacterController().persistAnimationState();
+        {
+            if (!actor.isInvalid())
+                actor.getCharacterController().persistAnimationState();
+        }
     }
 
     void Actors::clearAnimationQueue(const MWWorld::Ptr& ptr, bool clearScripted)
@@ -2061,6 +2088,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());
         }
@@ -2070,6 +2099,8 @@ namespace MWMechanics
     {
         for (const Actor& actor : mActors)
         {
+            if (actor.isInvalid())
+                continue;
             if ((actor.getPtr().getRefData().getPosition().asVec3() - position).length2() <= radius * radius)
                 return true;
         }
@@ -2083,6 +2114,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;
@@ -2353,10 +2386,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();
-            ++it;
+            if (actor.isInvalid())
+                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;
@@ -883,14 +883,16 @@ namespace MWMechanics
         }
 
         mDeathState = hitStateToDeathState(mHitState);
-        if (mDeathState == CharState_None && MWBase::Environment::get().getWorld()->isSwimming(mPtr))
-            mDeathState = CharState_SwimDeath;
-
-        if (mDeathState == CharState_None || !mAnimation->hasAnimation(deathStateToAnimGroup(mDeathState)))
-            mDeathState = chooseRandomDeathState();
+        if (mDeathState == CharState_None)
+        {
+            if (MWBase::Environment::get().getWorld()->isSwimming(mPtr))
+                mDeathState = CharState_SwimDeath;
+            else if (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 +912,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 +991,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 +1239,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");
@@ -1948,6 +1956,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();
@@ -2526,7 +2536,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;
@@ -2739,23 +2749,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)
         {
-            mAnimation->setPlayScriptedOnly(false);
+            if (mAnimation)
+                mAnimation->setPlayScriptedOnly(false);
             mAnimQueue.clear();
             return;
         }
 
-        for (AnimationQueue::iterator it = mAnimQueue.begin(); it != mAnimQueue.end();)
-        {
-            if (!it->mScripted)
-                it = mAnimQueue.erase(it);
-            else
-                ++it;
-        }
+        std::erase_if(mAnimQueue, [](const AnimationQueueEntry& entry) { return !entry.mScripted; });
     }
 
     void CharacterController::forceStateUpdate()
@@ -2864,6 +2869,8 @@ namespace MWMechanics
 
     void CharacterController::setVisibility(float visibility) const
     {
+        if (!mAnimation)
+            return;
         // We should take actor's invisibility in account
         if (mPtr.getClass().isActor())
         {
@@ -2924,7 +2931,7 @@ namespace MWMechanics
 
     bool CharacterController::isReadyToBlock() const
     {
-        return updateCarriedLeftVisible(mWeaponType);
+        return mAnimation && updateCarriedLeftVisible(mWeaponType);
     }
 
     bool CharacterController::isKnockedDown() const
@@ -3028,7 +3035,8 @@ namespace MWMechanics
 
     void CharacterController::setActive(int active) const
     {
-        mAnimation->setActive(active);
+        if (mAnimation)
+            mAnimation->setActive(active);
     }
 
     void CharacterController::setHeadTrackTarget(const MWWorld::ConstPtr& target)
@@ -3059,6 +3067,8 @@ namespace MWMechanics
 
     float CharacterController::getAnimationMovementDirection() const
     {
+        if (!mAnimation)
+            return 0.f;
         switch (mMovementState)
         {
             case CharState_RunLeft:
@@ -3153,6 +3163,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

@@ -251,13 +251,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,
@@ -274,11 +282,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

@@ -1718,6 +1718,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);
     }