More cleanup of scripted animations

CHANGELOG.md

@@ -11,10 +11,12 @@
     Bug #4508: Can't stack enchantment buffs from different instances of the same self-cast generic magic apparel
     Bug #4610: Casting a Bound Weapon spell cancels the casting animation by equipping the weapon prematurely
     Bug #4742: Actors with wander never stop walking after Loopgroup Walkforward
+    Bug #4743: PlayGroup doesn't play non-looping animations correctly 
     Bug #4754: Stack of ammunition cannot be equipped partially
     Bug #4816: GetWeaponDrawn returns 1 before weapon is attached
     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
+    Bug #5066: Quirks with starting and stopping scripted animations
     Bug #5129: Stuttering animation on Centurion Archer
     Bug #5280: Unskinned shapes in skinned equipment are rendered in the wrong place
     Bug #5371: Keyframe animation tracks are used for any file that begins with an X
@@ -91,6 +93,7 @@
     Bug #7636: Animations bug out when switching between 1st and 3rd person, while playing a scripted animation
     Bug #7637: Actors can sometimes move while playing scripted animations
     Bug #7639: NPCs don't use hand-to-hand if their other melee skills were damaged during combat
+    Bug #7641: loopgroup loops the animation one time too many for actors
     Bug #7642: Items in repair and recharge menus aren't sorted alphabetically
     Bug #7647: NPC walk cycle bugs after greeting player
     Bug #7654: Tooltips for enchantments with invalid effects cause crashes

apps/openmw/mwmechanics/character.cpp

@@ -20,6 +20,7 @@
 #include "character.hpp"
 
 #include <array>
+#include <unordered_set>
 
 #include <components/esm/records.hpp>
 #include <components/misc/mathutil.hpp>
@@ -1189,7 +1190,7 @@ namespace MWMechanics
                 if (!animPlaying)
                 {
                     int mask = MWRender::Animation::BlendMask_Torso | MWRender::Animation::BlendMask_RightArm;
-                    mAnimation->play("idlestorm", Priority_Storm, mask, true, 1.0f, "start", "stop", 0.0f, ~0ul);
+                    mAnimation->play("idlestorm", Priority_Storm, mask, true, 1.0f, "start", "stop", 0.0f, ~0ul, true);
                 }
                 else
                 {
@@ -1246,8 +1247,47 @@ namespace MWMechanics
         }
     }
 
+    bool CharacterController::isLoopingAnimation(std::string_view group) const
+    {
+        // In Morrowind, a some animation groups are always considered looping, regardless
+        // of loop start/stop keys.
+        // To be match vanilla behavior we probably only need to check this list, but we don't
+        // want to prevent modded animations with custom group names from looping either.
+        static const std::unordered_set<std::string_view> loopingAnimations = { "walkforward", "walkback", "walkleft",
+            "walkright", "swimwalkforward", "swimwalkback", "swimwalkleft", "swimwalkright", "runforward", "runback",
+            "runleft", "runright", "swimrunforward", "swimrunback", "swimrunleft", "swimrunright", "sneakforward",
+            "sneakback", "sneakleft", "sneakright", "turnleft", "turnright", "swimturnleft", "swimturnright",
+            "spellturnleft", "spellturnright", "torch", "idle", "idle2", "idle3", "idle4", "idle5", "idle6", "idle7",
+            "idle8", "idle9", "idlesneak", "idlestorm", "idleswim", "jump", "inventoryhandtohand",
+            "inventoryweapononehand", "inventoryweapontwohand", "inventoryweapontwowide" };
+        static const std::vector<std::string_view> shortGroups = getAllWeaponTypeShortGroups();
+
+        if (mAnimation && mAnimation->getTextKeyTime(std::string(group) + ": loop start") >= 0)
+            return true;
+
+        // Most looping animations have variants for each weapon type shortgroup.
+        // Just remove the shortgroup instead of enumerating all of the possible animation groupnames.
+        // Make sure we pick the longest shortgroup so e.g. "bow" doesn't get picked over "crossbow"
+        // when the shortgroup is crossbow.
+        std::size_t suffixLength = 0;
+        for (std::string_view suffix : shortGroups)
+        {
+            if (suffix.length() > suffixLength && group.ends_with(suffix))
+            {
+                suffixLength = suffix.length();
+            }
+        }
+        group.remove_suffix(suffixLength);
+
+        return loopingAnimations.count(group) > 0;
+    }
+
     bool CharacterController::updateWeaponState()
     {
+        // If the current animation is scripted, we can't do anything here.
+        if (isScriptedAnimPlaying())
+            return false;
+
         const auto world = MWBase::Environment::get().getWorld();
         auto& prng = world->getPrng();
         MWBase::SoundManager* sndMgr = MWBase::Environment::get().getSoundManager();
@@ -1481,10 +1521,6 @@ namespace MWMechanics
                 sndMgr->stopSound3D(mPtr, wolfRun);
         }
 
-        // Combat for actors with scripted animations obviously will be buggy
-        if (isScriptedAnimPlaying())
-            return forcestateupdate;
-
         float complete = 0.f;
         bool animPlaying = false;
         ESM::WeaponType::Class weapclass = getWeaponType(mWeaponType)->mWeaponClass;
@@ -1857,33 +1893,58 @@ namespace MWMechanics
 
         if (!mAnimation->isPlaying(mAnimQueue.front().mGroup))
         {
-            // Remove the finished animation, unless it's a scripted animation that was interrupted by e.g. a rebuild of
+            // Playing animations through mwscript is weird. If an animation is
-            // the animation object.
+            // a looping animation (idle or other cyclical animations), then they
-            if (mAnimQueue.size() > 1 || !mAnimQueue.front().mScripted || mAnimQueue.front().mLoopCount == 0)
+            // will end as expected. However, if they are non-looping animations, they
+            // will stick around forever or until another animation appears in the queue.
+            bool shouldPlayOrRestart = mAnimQueue.size() > 1;
+            if (shouldPlayOrRestart || !mAnimQueue.front().mScripted
+                || (mAnimQueue.front().mLoopCount == 0 && mAnimQueue.front().mLooping))
             {
+                mAnimation->setPlayScriptedOnly(false);
                 mAnimation->disable(mAnimQueue.front().mGroup);
                 mAnimQueue.pop_front();
+                shouldPlayOrRestart = true;
             }
+            else
+                // A non-looping animation will stick around forever, so only restart if the animation
+                // actually was removed for some reason.
+                shouldPlayOrRestart = !mAnimation->getInfo(mAnimQueue.front().mGroup)
+                    && mAnimation->hasAnimation(mAnimQueue.front().mGroup);
 
-            if (!mAnimQueue.empty())
+            if (shouldPlayOrRestart)
             {
                 // Move on to the remaining items of the queue
-                bool loopfallback = mAnimQueue.front().mGroup.starts_with("idle");
+                playAnimQueue();
-                mAnimation->play(mAnimQueue.front().mGroup,
-                    mAnimQueue.front().mScripted ? Priority_Scripted : Priority_Default,
-                    MWRender::Animation::BlendMask_All, false, 1.0f, "start", "stop", 0.0f,
-                    mAnimQueue.front().mLoopCount, loopfallback);
             }
         }
         else
         {
-            mAnimQueue.front().mLoopCount = mAnimation->getCurrentLoopCount(mAnimQueue.front().mGroup);
+            float complete;
+            size_t loopcount;
+            mAnimation->getInfo(mAnimQueue.front().mGroup, &complete, nullptr, &loopcount);
+            mAnimQueue.front().mLoopCount = loopcount;
+            mAnimQueue.front().mTime = complete;
         }
 
         if (!mAnimQueue.empty())
             mAnimation->setLoopingEnabled(mAnimQueue.front().mGroup, mAnimQueue.size() <= 1);
     }
 
+    void CharacterController::playAnimQueue(bool loopStart)
+    {
+        if (!mAnimQueue.empty())
+        {
+            clearStateAnimation(mCurrentIdle);
+            mIdleState = CharState_SpecialIdle;
+            auto priority = mAnimQueue.front().mScripted ? Priority_Scripted : Priority_Default;
+            mAnimation->setPlayScriptedOnly(mAnimQueue.front().mScripted);
+            mAnimation->play(mAnimQueue.front().mGroup, priority, MWRender::Animation::BlendMask_All, false, 1.0f,
+                (loopStart ? "loop start" : "start"), "stop", mAnimQueue.front().mTime, mAnimQueue.front().mLoopCount,
+                mAnimQueue.front().mLooping);
+        }
+    }
+
     void CharacterController::update(float duration)
     {
         MWBase::World* world = MWBase::Environment::get().getWorld();
@@ -2455,10 +2516,11 @@ namespace MWMechanics
 
             if (iter == mAnimQueue.begin())
             {
-                anim.mLoopCount = mAnimation->getCurrentLoopCount(anim.mGroup);
                 float complete;
-                mAnimation->getInfo(anim.mGroup, &complete, nullptr);
+                size_t loopcount;
+                mAnimation->getInfo(anim.mGroup, &complete, nullptr, &loopcount);
                 anim.mTime = complete;
+                anim.mLoopCount = loopcount;
             }
             else
             {
@@ -2484,26 +2546,20 @@ namespace MWMechanics
                 entry.mGroup = iter->mGroup;
                 entry.mLoopCount = iter->mLoopCount;
                 entry.mScripted = true;
+                entry.mLooping = isLoopingAnimation(entry.mGroup);
+                entry.mTime = iter->mTime;
+                if (iter->mAbsolute)
+                {
+                    float start = mAnimation->getTextKeyTime(iter->mGroup + ": start");
+                    float stop = mAnimation->getTextKeyTime(iter->mGroup + ": stop");
+                    float time = std::clamp(iter->mTime, start, stop);
+                    entry.mTime = (time - start) / (stop - start);
+                }
 
                 mAnimQueue.push_back(entry);
             }
 
-            const ESM::AnimationState::ScriptedAnimation& anim = state.mScriptedAnims.front();
+            playAnimQueue();
-            float complete = anim.mTime;
-            if (anim.mAbsolute)
-            {
-                float start = mAnimation->getTextKeyTime(anim.mGroup + ": start");
-                float stop = mAnimation->getTextKeyTime(anim.mGroup + ": stop");
-                float time = std::clamp(anim.mTime, start, stop);
-                complete = (time - start) / (stop - start);
-            }
-
-            clearStateAnimation(mCurrentIdle);
-            mIdleState = CharState_SpecialIdle;
-
-            bool loopfallback = mAnimQueue.front().mGroup.starts_with("idle");
-            mAnimation->play(anim.mGroup, Priority_Scripted, MWRender::Animation::BlendMask_All, false, 1.0f, "start",
-                "stop", complete, anim.mLoopCount, loopfallback);
         }
     }
 
@@ -2516,13 +2572,14 @@ namespace MWMechanics
         if (isScriptedAnimPlaying() && !scripted)
             return true;
 
-        // If this animation is a looped animation (has a "loop start" key) that is already playing
+        bool looping = isLoopingAnimation(groupname);
+
+        // If this animation is a looped animation that is already playing
         // and has not yet reached the end of the loop, allow it to continue animating with its existing loop count
         // and remove any other animations that were queued.
         // This emulates observed behavior from the original allows the script "OutsideBanner" to animate banners
         // correctly.
-        if (!mAnimQueue.empty() && mAnimQueue.front().mGroup == groupname
+        if (!mAnimQueue.empty() && mAnimQueue.front().mGroup == groupname && looping
-            && mAnimation->getTextKeyTime(mAnimQueue.front().mGroup + ": loop start") >= 0
             && mAnimation->isPlaying(groupname))
         {
             float endOfLoop = mAnimation->getTextKeyTime(mAnimQueue.front().mGroup + ": loop stop");
@@ -2537,36 +2594,43 @@ namespace MWMechanics
             }
         }
 
-        count = std::max(count, 1);
+        // The loop count in vanilla is weird.
+        // if played with a count of 0, all objects play exactly once from start to stop.
+        // But if the count is x > 0, actors and non-actors behave differently. actors will loop
+        // exactly x times, while non-actors will loop x+1 instead.
+        if (mPtr.getClass().isActor())
+            count--;
+        count = std::max(count, 0);
 
         AnimationQueueEntry entry;
         entry.mGroup = groupname;
-        entry.mLoopCount = count - 1;
+        entry.mLoopCount = count;
+        entry.mTime = 0.f;
         entry.mScripted = scripted;
+        entry.mLooping = looping;
+
+        bool playImmediately = false;
 
         if (mode != 0 || mAnimQueue.empty() || !isAnimPlaying(mAnimQueue.front().mGroup))
         {
             clearAnimQueue(scripted);
 
-            clearStateAnimation(mCurrentIdle);
+            playImmediately = true;
-
-            mIdleState = CharState_SpecialIdle;
-            bool loopfallback = entry.mGroup.starts_with("idle");
-            mAnimation->play(groupname, scripted && groupname != "idle" ? Priority_Scripted : Priority_Default,
-                MWRender::Animation::BlendMask_All, false, 1.0f, ((mode == 2) ? "loop start" : "start"), "stop", 0.0f,
-                count - 1, loopfallback);
         }
         else
         {
             mAnimQueue.resize(1);
         }
 
-        // "PlayGroup idle" is a special case, used to remove to stop scripted animations playing
+        // "PlayGroup idle" is a special case, used to stop and remove scripted animations playing
         if (groupname == "idle")
             entry.mScripted = false;
 
         mAnimQueue.push_back(entry);
 
+        if (playImmediately)
+            playAnimQueue(mode == 2);
+
         return true;
     }
 
@@ -2577,11 +2641,10 @@ namespace MWMechanics
 
     bool CharacterController::isScriptedAnimPlaying() const
     {
+        // If the front of the anim queue is scripted, morrowind treats it as if it's
+        // still playing even if it's actually done.
         if (!mAnimQueue.empty())
-        {
+            return mAnimQueue.front().mScripted;
-            const AnimationQueueEntry& first = mAnimQueue.front();
-            return first.mScripted && isAnimPlaying(first.mGroup);
-        }
 
         return false;
     }
@@ -2611,6 +2674,7 @@ namespace MWMechanics
 
         if (clearScriptedAnims)
         {
+            mAnimation->setPlayScriptedOnly(false);
             mAnimQueue.clear();
             return;
         }
@@ -2645,6 +2709,8 @@ namespace MWMechanics
             playRandomDeath();
         }
 
+        updateAnimQueue();
+
         mAnimation->runAnimation(0.f);
     }
 

apps/openmw/mwmechanics/character.hpp

@@ -135,6 +135,8 @@ namespace MWMechanics
         {
             std::string mGroup;
             size_t mLoopCount;
+            float mTime;
+            bool mLooping;
             bool mScripted;
         };
         typedef std::deque<AnimationQueueEntry> AnimationQueue;
@@ -219,6 +221,7 @@ namespace MWMechanics
         bool isMovementAnimationControlled() const;
 
         void updateAnimQueue();
+        void playAnimQueue(bool useLoopStart = false);
 
         void updateHeadTracking(float duration);
 
@@ -245,6 +248,8 @@ namespace MWMechanics
 
         void prepareHit();
 
+        bool isLoopingAnimation(std::string_view group) const;
+
     public:
         CharacterController(const MWWorld::Ptr& ptr, MWRender::Animation* anim);
         virtual ~CharacterController();

apps/openmw/mwmechanics/weapontype.cpp

@@ -8,6 +8,8 @@
 
 #include <components/esm3/loadweap.hpp>
 
+#include <set>
+
 namespace MWMechanics
 {
     template <enum ESM::Weapon::Type>
@@ -416,4 +418,18 @@ namespace MWMechanics
 
         return &Weapon<ESM::Weapon::ShortBladeOneHand>::getValue();
     }
+
+    std::vector<std::string_view> getAllWeaponTypeShortGroups()
+    {
+        // Go via a set to eliminate duplicates.
+        std::set<std::string_view> shortGroupSet;
+        for (int type = ESM::Weapon::Type::First; type <= ESM::Weapon::Type::Last; type++)
+        {
+            std::string_view shortGroup = getWeaponType(type)->mShortGroup;
+            if (!shortGroup.empty())
+                shortGroupSet.insert(shortGroup);
+        }
+
+        return std::vector<std::string_view>(shortGroupSet.begin(), shortGroupSet.end());
+    }
 }

apps/openmw/mwmechanics/weapontype.hpp

@@ -1,6 +1,9 @@
 #ifndef GAME_MWMECHANICS_WEAPONTYPE_H
 #define GAME_MWMECHANICS_WEAPONTYPE_H
 
+#include <string_view>
+#include <vector>
+
 namespace ESM
 {
     struct WeaponType;
@@ -21,6 +24,8 @@ namespace MWMechanics
     MWWorld::ContainerStoreIterator getActiveWeapon(const MWWorld::Ptr& actor, int* weaptype);
 
     const ESM::WeaponType* getWeaponType(const int weaponType);
+
+    std::vector<std::string_view> getAllWeaponTypeShortGroups();
 }
 
 #endif

apps/openmw/mwrender/animation.cpp

@@ -529,6 +529,7 @@ namespace MWRender
         , mBodyPitchRadians(0.f)
         , mHasMagicEffects(false)
         , mAlpha(1.f)
+        , mPlayScriptedOnly(false)
     {
         for (size_t i = 0; i < sNumBlendMasks; i++)
             mAnimationTimePtr[i] = std::make_shared<AnimationTime>();
@@ -1020,7 +1021,7 @@ namespace MWRender
         return false;
     }
 
-    bool Animation::getInfo(std::string_view groupname, float* complete, float* speedmult) const
+    bool Animation::getInfo(std::string_view groupname, float* complete, float* speedmult, size_t* loopcount) const
     {
         AnimStateMap::const_iterator iter = mStates.find(groupname);
         if (iter == mStates.end())
@@ -1029,6 +1030,8 @@ namespace MWRender
                 *complete = 0.0f;
             if (speedmult)
                 *speedmult = 0.0f;
+            if (loopcount)
+                *loopcount = 0;
             return false;
         }
 
@@ -1042,6 +1045,9 @@ namespace MWRender
         }
         if (speedmult)
             *speedmult = iter->second.mSpeedMult;
+
+        if (loopcount)
+            *loopcount = iter->second.mLoopCount;
         return true;
     }
 
@@ -1054,15 +1060,6 @@ namespace MWRender
         return iter->second.getTime();
     }
 
-    size_t Animation::getCurrentLoopCount(const std::string& groupname) const
-    {
-        AnimStateMap::const_iterator iter = mStates.find(groupname);
-        if (iter == mStates.end())
-            return 0;
-
-        return iter->second.mLoopCount;
-    }
-
     void Animation::disable(std::string_view groupname)
     {
         AnimStateMap::iterator iter = mStates.find(groupname);
@@ -1141,23 +1138,12 @@ namespace MWRender
 
     osg::Vec3f Animation::runAnimation(float duration)
     {
-        // If we have scripted animations, play only them
-        bool hasScriptedAnims = false;
-        for (AnimStateMap::iterator stateiter = mStates.begin(); stateiter != mStates.end(); stateiter++)
-        {
-            if (stateiter->second.mPriority.contains(int(MWMechanics::Priority_Scripted)) && stateiter->second.mPlaying)
-            {
-                hasScriptedAnims = true;
-                break;
-            }
-        }
-
         osg::Vec3f movement(0.f, 0.f, 0.f);
         AnimStateMap::iterator stateiter = mStates.begin();
         while (stateiter != mStates.end())
         {
             AnimState& state = stateiter->second;
-            if (hasScriptedAnims && !state.mPriority.contains(int(MWMechanics::Priority_Scripted)))
+            if (mPlayScriptedOnly && !state.mPriority.contains(MWMechanics::Priority_Scripted))
             {
                 ++stateiter;
                 continue;
@@ -1263,10 +1249,6 @@ namespace MWRender
                     osg::Quat(mHeadPitchRadians, osg::Vec3f(1, 0, 0)) * osg::Quat(yaw, osg::Vec3f(0, 0, 1)));
         }
 
-        // Scripted animations should not cause movement
-        if (hasScriptedAnims)
-            return osg::Vec3f(0, 0, 0);
-
         return movement;
     }
 

apps/openmw/mwrender/animation.hpp

@@ -292,6 +292,8 @@ namespace MWRender
 
         osg::ref_ptr<SceneUtil::LightListCallback> mLightListCallback;
 
+        bool mPlayScriptedOnly;
+
         const NodeMap& getNodeMap() const;
 
         /* Sets the appropriate animations on the bone groups based on priority.
@@ -441,7 +443,8 @@ namespace MWRender
          * \param speedmult Stores the animation speed multiplier
          * \return True if the animation is active, false otherwise.
          */
-        bool getInfo(std::string_view groupname, float* complete = nullptr, float* speedmult = nullptr) const;
+        bool getInfo(std::string_view groupname, float* complete = nullptr, float* speedmult = nullptr,
+            size_t* loopcount = nullptr) const;
 
         /// Get the absolute position in the animation track of the first text key with the given group.
         float getStartTime(const std::string& groupname) const;
@@ -453,8 +456,6 @@ namespace MWRender
         /// the given group.
         float getCurrentTime(const std::string& groupname) const;
 
-        size_t getCurrentLoopCount(const std::string& groupname) const;
-
         /** Disables the specified animation group;
          * \param groupname Animation group to disable.
          */
@@ -477,6 +478,9 @@ namespace MWRender
         MWWorld::MovementDirectionFlags getSupportedMovementDirections(
             std::span<const std::string_view> prefixes) const;
 
+        bool getPlayScriptedOnly() const { return mPlayScriptedOnly; }
+        void setPlayScriptedOnly(bool playScriptedOnly) { mPlayScriptedOnly = playScriptedOnly; }
+
         virtual bool useShieldAnimations() const { return false; }
         virtual bool getWeaponsShown() const { return false; }
         virtual void showWeapons(bool showWeapon) {}

apps/openmw/mwrender/npcanimation.cpp

@@ -923,13 +923,18 @@ namespace MWRender
 
         if (mViewMode == VM_FirstPerson)
         {
-            NodeMap::iterator found = mNodeMap.find("bip01 neck");
+            // If there is no active animation, then the bip01 neck node will not be updated each frame, and the
-            if (found != mNodeMap.end())
+            // RotateController will accumulate rotations.
+            if (mStates.size() > 0)
             {
-                osg::MatrixTransform* node = found->second.get();
+                NodeMap::iterator found = mNodeMap.find("bip01 neck");
-                mFirstPersonNeckController = new RotateController(mObjectRoot.get());
+                if (found != mNodeMap.end())
-                node->addUpdateCallback(mFirstPersonNeckController);
+                {
-                mActiveControllers.emplace_back(node, mFirstPersonNeckController);
+                    osg::MatrixTransform* node = found->second.get();
+                    mFirstPersonNeckController = new RotateController(mObjectRoot.get());
+                    node->addUpdateCallback(mFirstPersonNeckController);
+                    mActiveControllers.emplace_back(node, mFirstPersonNeckController);
+                }
             }
         }
         else if (mViewMode == VM_Normal)

apps/openmw/mwscript/animationextensions.cpp

@@ -91,7 +91,7 @@ namespace MWScript
                         throw std::runtime_error("animation mode out of range");
                 }
 
-                MWBase::Environment::get().getMechanicsManager()->playAnimationGroup(ptr, group, mode, loops + 1, true);
+                MWBase::Environment::get().getMechanicsManager()->playAnimationGroup(ptr, group, mode, loops, true);
             }
         };
 

components/esm3/loadweap.hpp

@@ -24,6 +24,7 @@ namespace ESM
 
         enum Type
         {
+            First = -4,
             PickProbe = -4,
             HandToHand = -3,
             Spell = -2,
@@ -41,7 +42,8 @@ namespace ESM
             MarksmanCrossbow = 10,
             MarksmanThrown = 11,
             Arrow = 12,
-            Bolt = 13
+            Bolt = 13,
+            Last = 13
         };
 
         enum AttackType