Core/Movement: Fix movement generator priority sorting mismatch between MovementGeneratorComparator and MotionMaster::DirectAdd

* Also changed generators container from multiset to set because uniqueness was already ensured by deactivation logic in MotionMaster::DirectAdd

Author: Shauren

src/server/game/AI/SmartScripts/SmartAI.cpp

@@ -807,7 +807,7 @@ void SmartAI::SetCombatMove(bool on, bool stopMoving)
         {
             if (!me->HasReactState(REACT_PASSIVE) && me->GetVictim() && !me->GetMotionMaster()->HasMovementGenerator([](MovementGenerator const* movement) -> bool
                 {
-                    return movement->GetMovementGeneratorType() == CHASE_MOTION_TYPE && movement->Mode == MOTION_MODE_DEFAULT && movement->Priority == MOTION_PRIORITY_NORMAL;
+                    return movement->GetMovementGeneratorType() == CHASE_MOTION_TYPE && movement->Priority == MOTION_PRIORITY_NORMAL;
                 }))
             {
                 SetRun(_run);
@@ -816,7 +816,7 @@ void SmartAI::SetCombatMove(bool on, bool stopMoving)
         }
         else if (MovementGenerator* movement = me->GetMotionMaster()->GetMovementGenerator([](MovementGenerator const* a) -> bool
             {
-                return a->GetMovementGeneratorType() == CHASE_MOTION_TYPE && a->Mode == MOTION_MODE_DEFAULT && a->Priority == MOTION_PRIORITY_NORMAL;
+                return a->GetMovementGeneratorType() == CHASE_MOTION_TYPE && a->Priority == MOTION_PRIORITY_NORMAL;
             }))
         {
             me->GetMotionMaster()->Remove(movement);

src/server/game/Movement/MotionMaster.cpp

@@ -72,12 +72,7 @@ void MovementGeneratorDeleter::operator()(MovementGenerator* a)
 
 bool MovementGeneratorComparator::operator()(MovementGenerator const* a, MovementGenerator const* b) const
 {
-    if (a->Mode > b->Mode)
-        return true;
-    else if (a->Mode == b->Mode)
-        return a->Priority > b->Priority;
-
-    return false;
+    return a->Priority > b->Priority;
 }
 
 MovementGeneratorInformation::MovementGeneratorInformation(MovementGeneratorType type, ObjectGuid targetGUID, std::string const& targetName) : Type(type), TargetGUID(targetGUID), TargetName(targetName) { }
@@ -88,8 +83,8 @@ MotionMaster::~MotionMaster()
 {
     _delayedActions.clear();
 
-    for (auto itr = _generators.begin(); itr != _generators.end(); itr = _generators.erase(itr))
-        MovementGeneratorPointerDeleter(*itr);
+    for (MovementGenerator* movement : _generators)
+        MovementGeneratorPointerDeleter(movement);
 }
 
 void MotionMaster::Initialize()
@@ -248,13 +243,12 @@ MovementGenerator* MotionMaster::GetMovementGenerator(std::function<bool(Movemen
                 movement = _defaultGenerator.get();
             break;
         case MOTION_SLOT_ACTIVE:
-            if (!_generators.empty())
-            {
-                auto itr = std::find_if(_generators.begin(), _generators.end(), std::ref(filter));
-                if (itr != _generators.end())
-                    movement = *itr;
-            }
+        {
+            auto itr = std::ranges::find_if(_generators, std::ref(filter));
+            if (itr != _generators.end())
+                movement = *itr;
             break;
+        }
         default:
             break;
     }
@@ -271,15 +265,10 @@ bool MotionMaster::HasMovementGenerator(std::function<bool(MovementGenerator con
     switch (slot)
     {
         case MOTION_SLOT_DEFAULT:
-            if (_defaultGenerator && filter(_defaultGenerator.get()))
-                value = true;
+            value = _defaultGenerator && filter(_defaultGenerator.get());
             break;
         case MOTION_SLOT_ACTIVE:
-            if (!_generators.empty())
-            {
-                auto itr = std::find_if(_generators.begin(), _generators.end(), std::ref(filter));
-                value = itr != _generators.end();
-            }
+            value = std::ranges::any_of(_generators, std::ref(filter));
             break;
         default:
             break;
@@ -346,7 +335,8 @@ void MotionMaster::Add(MovementGenerator* movement, MovementSlot slot/* = MOTION
 
     if (movement->HasFlag(MOVEMENTGENERATOR_FLAG_IMMEDIATE) && movement->HasFlag(MOVEMENTGENERATOR_FLAG_INITIALIZATION_PENDING))
     {
-        if (!movement->Initialize(_owner))
+        bool wouldBecomeTop = _generators.lower_bound(movement) == _generators.begin();
+        if (!wouldBecomeTop || !movement->Initialize(_owner))
         {
             delete movement;
             return;
@@ -390,29 +380,27 @@ void MotionMaster::Remove(MovementGenerator* movement, MovementSlot slot/* = MOT
                 DirectClearDefault();
             break;
         case MOTION_SLOT_ACTIVE:
-            if (!_generators.empty())
-            {
-                auto bounds = _generators.equal_range(movement);
-                auto itr = std::find(bounds.first, bounds.second, movement);
-                if (itr != _generators.end())
-                    Remove(itr, GetCurrentMovementGenerator() == *itr, false);
-            }
+        {
+            auto itr = _generators.find(movement);
+            if (itr != _generators.end())
+                Remove(itr, GetCurrentMovementGenerator() == *itr, false);
             break;
+        }
         default:
             break;
     }
 }
 
-void MotionMaster::Remove(MovementGeneratorType type, MovementSlot slot/* = MOTION_SLOT_ACTIVE*/, MovementGeneratorMode mode/* = MOTION_MODE_DEFAULT*/)
+void MotionMaster::Remove(MovementGeneratorType type, MovementSlot slot/* = MOTION_SLOT_ACTIVE*/)
 {
     if (IsInvalidMovementGeneratorType(type) || IsInvalidMovementSlot(slot))
         return;
 
     if (HasFlag(MOTIONMASTER_FLAG_DELAYED))
     {
-        DelayedActionDefine action = [this, type, slot, mode]()
+        DelayedActionDefine action = [this, type, slot]()
         {
-            Remove(type, slot, mode);
+            Remove(type, slot);
         };
         _delayedActions.emplace_back(std::move(action), MOTIONMASTER_DELAYED_REMOVE_TYPE);
         return;
@@ -428,16 +416,14 @@ void MotionMaster::Remove(MovementGeneratorType type, MovementSlot slot/* = MOTI
                 DirectClearDefault();
             break;
         case MOTION_SLOT_ACTIVE:
-            if (!_generators.empty())
+            do
             {
-                auto itr = std::find_if(_generators.begin(), _generators.end(), [type, mode](MovementGenerator const* a) -> bool
-                {
-                    return a->GetMovementGeneratorType() == type && a->Mode == mode;
-                });
-
-                if (itr != _generators.end())
-                    Remove(itr, GetCurrentMovementGenerator() == *itr, false);
-            }
+                auto itr = std::ranges::find(_generators, type,
+                    [](MovementGenerator const* a) { return a->GetMovementGeneratorType(); });
+                if (itr == _generators.end())
+                    break;
+                Remove(itr, GetCurrentMovementGenerator() == *itr, false);
+            } while (true);
             break;
         default:
             break;
@@ -491,28 +477,6 @@ void MotionMaster::Clear(MovementSlot slot)
     }
 }
 
-void MotionMaster::Clear(MovementGeneratorMode mode)
-{
-    if (HasFlag(MOTIONMASTER_FLAG_DELAYED))
-    {
-        DelayedActionDefine action = [this, mode]()
-        {
-            Clear(mode);
-        };
-        _delayedActions.emplace_back(std::move(action), MOTIONMASTER_DELAYED_CLEAR_MODE);
-        return;
-    }
-
-    if (Empty())
-        return;
-
-    std::function<bool(MovementGenerator*)> criteria = [mode](MovementGenerator* a) -> bool
-    {
-        return a->Mode == mode;
-    };
-    DirectClear(criteria);
-}
-
 void MotionMaster::Clear(MovementGeneratorPriority priority)
 {
     if (HasFlag(MOTIONMASTER_FLAG_DELAYED))
@@ -1106,17 +1070,18 @@ void MotionMaster::ResolveDelayedActions()
     }
 }
 
-void MotionMaster::Remove(MotionMasterContainer::iterator iterator, bool active, bool movementInform)
+void MotionMaster::Remove(MotionMasterContainer::iterator& iterator, bool active, bool movementInform)
 {
     MovementGenerator* pointer = *iterator;
-    _generators.erase(iterator);
+    iterator = _generators.erase(iterator);
     Delete(pointer, active, movementInform);
 }
 
 void MotionMaster::Pop(bool active, bool movementInform)
 {
-    if (!_generators.empty())
-        Remove(_generators.begin(), active, movementInform);
+    auto itr = _generators.begin();
+    if (itr != _generators.end())
+        Remove(itr, active, movementInform);
 }
 
 void MotionMaster::DirectInitialize()
@@ -1150,14 +1115,11 @@ void MotionMaster::DirectClearDefault()
 
 void MotionMaster::DirectClear(std::function<bool(MovementGenerator*)> const& filter)
 {
-    if (_generators.empty())
-        return;
-
-    MovementGenerator const* top = GetCurrentMovementGenerator();
     for (auto itr = _generators.begin(); itr != _generators.end();)
     {
         if (filter(*itr))
         {
+            MovementGenerator const* top = GetCurrentMovementGenerator(); // erase may change top, get fresh value on every removal
             MovementGenerator* movement = *itr;
             itr = _generators.erase(itr);
             Delete(movement, movement == top, false);
@@ -1180,39 +1142,23 @@ void MotionMaster::DirectAdd(MovementGenerator* movement, MovementSlot slot/* =
                 AddFlag(MOTIONMASTER_FLAG_STATIC_INITIALIZATION_PENDING);
             break;
         case MOTION_SLOT_ACTIVE:
+        {
+            auto where = _generators.lower_bound(movement);
             if (!_generators.empty())
             {
-                if (movement->Priority >= (*_generators.begin())->Priority)
-                {
-                    auto itr = _generators.begin();
-                    MovementGenerator* currentTopMovement = *itr;
-                    if (movement->Priority == currentTopMovement->Priority)
-                    {
-                        if (movement->Mode > currentTopMovement->Mode)
-                            currentTopMovement->Deactivate(_owner);
-                        else if (movement->Mode == currentTopMovement->Mode)
-                            Remove(itr, true, false);
-                    }
-                    else
-                        currentTopMovement->Deactivate(_owner);
-                }
-                else
-                {
-                    auto itr = std::ranges::find_if(_generators, [movement](MovementGenerator const* a) -> bool
-                    {
-                        return a->Priority == movement->Priority && a->Mode == movement->Mode;
-                    });
-
-                    if (itr != _generators.end())
-                        Remove(itr, false, false);
-                }
+                bool replacesExisting = where != _generators.end() && !_generators.key_comp()(movement, *where);
+                auto top = _generators.begin();
+                if (replacesExisting)
+                    Remove(where, where == top, false);
+                else if (where == top)
+                    (*top)->Deactivate(_owner);
             }
             else
                 _defaultGenerator->Deactivate(_owner);
 
             if (!movement->HasFlag(MOVEMENTGENERATOR_FLAG_IMMEDIATE))
             {
-                _generators.emplace(movement);
+                _generators.emplace_hint(where, movement);
                 AddBaseUnitState(movement);
             }
             else
@@ -1221,6 +1167,7 @@ void MotionMaster::DirectAdd(MovementGenerator* movement, MovementSlot slot/* =
                 delete movement;
             }
             break;
+        }
         default:
             break;
     }

src/server/game/Movement/MotionMaster.h

@@ -135,15 +135,12 @@ class TC_GAME_API MotionMaster
         void Remove(MovementGenerator* movement, MovementSlot slot = MOTION_SLOT_ACTIVE);
         // Removes first found movement
         // NOTE: MOTION_SLOT_DEFAULT will be autofilled with IDLE_MOTION_TYPE
-        void Remove(MovementGeneratorType type, MovementSlot slot = MOTION_SLOT_ACTIVE, MovementGeneratorMode mode = MOTION_MODE_DEFAULT);
+        void Remove(MovementGeneratorType type, MovementSlot slot = MOTION_SLOT_ACTIVE);
         // NOTE: MOTION_SLOT_DEFAULT wont be affected
         void Clear();
         // Removes all movements for the given MovementSlot
         // NOTE: MOTION_SLOT_DEFAULT will be autofilled with IDLE_MOTION_TYPE
         void Clear(MovementSlot slot);
-        // Removes all movements with the given MovementGeneratorMode
-        // NOTE: MOTION_SLOT_DEFAULT wont be affected
-        void Clear(MovementGeneratorMode mode);
         // Removes all movements with the given MovementGeneratorPriority
         // NOTE: MOTION_SLOT_DEFAULT wont be affected
         void Clear(MovementGeneratorPriority priority);
@@ -196,15 +193,15 @@ class TC_GAME_API MotionMaster
         void LaunchMoveSpline(std::function<void(Movement::MoveSplineInit& init)>&& initializer, uint32 id = 0, MovementGeneratorPriority priority = MOTION_PRIORITY_NORMAL, MovementGeneratorType type = EFFECT_MOTION_TYPE);
     private:
         typedef std::unique_ptr<MovementGenerator, MovementGeneratorDeleter> MovementGeneratorPointer;
-        typedef std::multiset<MovementGenerator*, MovementGeneratorComparator> MotionMasterContainer;
+        typedef std::set<MovementGenerator*, MovementGeneratorComparator> MotionMasterContainer;
         typedef std::unordered_multimap<uint32, MovementGenerator const*> MotionMasterUnitStatesContainer;
 
         void AddFlag(uint8 const flag) { _flags |= flag; }
         bool HasFlag(uint8 const flag) const { return (_flags & flag) != 0; }
         void RemoveFlag(uint8 const flag) { _flags &= ~flag; }
 
         void ResolveDelayedActions();
-        void Remove(MotionMasterContainer::iterator iterator, bool active, bool movementInform);
+        void Remove(MotionMasterContainer::iterator& iterator, bool active, bool movementInform);
         void Pop(bool active, bool movementInform);
         void DirectInitialize();
         void DirectClear();

src/server/game/Movement/MovementDefines.h

@@ -62,17 +62,11 @@ constexpr bool CanStopMovementForSpellCasting(MovementGeneratorType type)
     return true;
 }
 
-enum MovementGeneratorMode : uint8
-{
-    MOTION_MODE_DEFAULT = 0,
-    MOTION_MODE_OVERRIDE
-};
-
 enum MovementGeneratorPriority : uint8
 {
-    MOTION_PRIORITY_NONE = 0,
-    MOTION_PRIORITY_NORMAL,
-    MOTION_PRIORITY_HIGHEST
+    MOTION_PRIORITY_NONE    = 0,
+    MOTION_PRIORITY_NORMAL  = 64,
+    MOTION_PRIORITY_HIGHEST = 128
 };
 
 enum MovementSlot : uint8

src/server/game/Movement/MovementGenerator.cpp

@@ -21,20 +21,16 @@
 #include "MovementDefines.h"
 #include "PathGenerator.h"
 #include "RandomMovementGenerator.h"
+#include "StringFormat.h"
 #include "UnitAI.h"
 #include "WaypointMovementGenerator.h"
 
 MovementGenerator::~MovementGenerator() { }
 
 std::string MovementGenerator::GetDebugInfo() const
 {
-    std::stringstream sstr;
-    sstr << std::boolalpha
-        << "Mode: " << std::to_string(Mode)
-        << " Priority: " << std::to_string(Priority)
-        << " Flags: " << Flags
-        << " BaseUniteState: " << BaseUnitState;
-    return sstr.str();
+    return Trinity::StringFormat("Priority: {} Flags: {} BaseUniteState: {}",
+        Priority, Flags, BaseUnitState);
 }
 
 IdleMovementFactory::IdleMovementFactory() : MovementGeneratorCreator(IDLE_MOTION_TYPE) { }

src/server/game/Movement/MovementGenerator.h

@@ -48,7 +48,7 @@ enum MovementGeneratorFlags : uint16
 class TC_GAME_API MovementGenerator
 {
     public:
-        MovementGenerator() : Mode(0), Priority(0), Flags(MOVEMENTGENERATOR_FLAG_NONE), BaseUnitState(0) { }
+        MovementGenerator() : Priority(0), Flags(MOVEMENTGENERATOR_FLAG_NONE), BaseUnitState(0) { }
         virtual ~MovementGenerator();
 
         // on top first update
@@ -77,7 +77,6 @@ class TC_GAME_API MovementGenerator
 
         virtual std::string GetDebugInfo() const;
 
-        uint8 Mode;
         uint8 Priority;
         uint16 Flags;
         uint32 BaseUnitState;

src/server/game/Movement/MovementGenerators/ChaseMovementGenerator.cpp

@@ -68,7 +68,6 @@ static void DoMovementInform(Unit* owner, Unit* target)
 ChaseMovementGenerator::ChaseMovementGenerator(Unit *target, Optional<ChaseRange> range, Optional<ChaseAngle> angle) : AbstractFollower(ASSERT_NOTNULL(target)), _range(range),
     _angle(angle), _rangeCheckTimer(RANGE_CHECK_INTERVAL)
 {
-    Mode = MOTION_MODE_DEFAULT;
     Priority = MOTION_PRIORITY_NORMAL;
     Flags = MOVEMENTGENERATOR_FLAG_INITIALIZATION_PENDING;
     BaseUnitState = UNIT_STATE_CHASE;

src/server/game/Movement/MovementGenerators/ConfusedMovementGenerator.cpp

@@ -27,7 +27,6 @@
 template<class T>
 ConfusedMovementGenerator<T>::ConfusedMovementGenerator() : _timer(0), _x(0.f), _y(0.f), _z(0.f)
 {
-    this->Mode = MOTION_MODE_DEFAULT;
     this->Priority = MOTION_PRIORITY_HIGHEST;
     this->Flags = MOVEMENTGENERATOR_FLAG_INITIALIZATION_PENDING;
     this->BaseUnitState = UNIT_STATE_CONFUSED;

src/server/game/Movement/MovementGenerators/FleeingMovementGenerator.cpp

@@ -32,7 +32,6 @@
 template<class T>
 FleeingMovementGenerator<T>::FleeingMovementGenerator(ObjectGuid fleeTargetGUID) : _fleeTargetGUID(fleeTargetGUID), _timer(0)
 {
-    this->Mode = MOTION_MODE_DEFAULT;
     this->Priority = MOTION_PRIORITY_HIGHEST;
     this->Flags = MOVEMENTGENERATOR_FLAG_INITIALIZATION_PENDING;
     this->BaseUnitState = UNIT_STATE_FLEEING;