Fix Assistant Assignment Functions (#1930)

IsHealAssistantOfIndex() and IsRangedDpsAssistantOfIndex() are supposed
to iterate through the group and first return members with the
applicable role that have the assistant flag, and then iterate through
non-assistants only if there are not enough assistants for the
designated index. They are not written properly and actually completely
ignore the assistant flag.

I rely on these functions for significant roles in SSC and TK (which I
have decided I'll PR in the same way as SSC, as a long-term draft). I
have them fixed on my own fork, but it is problematic for testers if
these functions do not work.

So I've done three things here:
1. Fixed the functions to prefer members with the assistant flag.
2. Added a third parameter for ignoreDeadPlayers, like
IsAssistTankOfIndex() has. Note that the parameter is by default false
for IsAssistTankOfIndex(), meaning dead players are _not_ ignored. This
is not my preferred design choice--I think the default should be to
ignore dead players, but I have not changed the default and have made
the default the same for IsAssistHealOfIndex() and
IsAssistRangedDpsOfIndex(), since I don't know the intent of the
pre-existing boss strats that use the functions.
3. Changed the names to IsAssistHealOfIndex() and
IsAssistRangedDpsOfIndex() so they parallel IsAssistTankOfIndex(), and
made corresponding changes in the few boss strats that use the
functions.

Also, note that the functions _do _not_ exclude real players. I think
there are arguments for and against excluding real players. A fourth
parameter for this could be useful, but I've not made any change in that
regard.

src/Ai/Base/Actions/TravelAction.cpp

@@ -28,7 +28,7 @@ bool TravelAction::Execute(Event event)
     for (Unit* unit : targets)
     {
         newTarget = unit;
-        if (!newTarget)
+        if (!newTarget || !newTarget->IsInWorld() || newTarget->IsDuringRemoveFromWorld())
             continue;
 
         if (newTarget->GetMapId() != bot->GetMapId())

src/Ai/Base/Actions/UseItemAction.cpp

@@ -245,8 +245,10 @@ bool UseItemAction::UseItem(Item* item, ObjectGuid goGuid, Item* itemTarget, Uni
         {
             packet << unitTarget->GetGUID();
             targetSelected = true;
-            // If the target is bot or is an enemy, say "on self"
-            if (unitTarget == bot || (unitTarget->IsHostileTo(bot)))
+
+            if (unitTarget == bot || !unitTarget->IsInWorld() || unitTarget->IsDuringRemoveFromWorld())
+                out << " on self";
+            else if (unitTarget->IsHostileTo(bot))
                 out << " on self";
             else
                 out << " on " << unitTarget->GetName();

src/Ai/Base/Value/AttackersValue.cpp

@@ -258,6 +258,9 @@ bool PossibleAddsValue::Calculate()
 
         if (Unit* add = botAI->GetUnit(guid))
         {
+            if (!add->IsInWorld() || add->IsDuringRemoveFromWorld())
+                continue;
+
             if (!add->GetTarget() && !add->GetThreatMgr().getCurrentVictim() && add->IsHostileTo(bot))
             {
                 for (ObjectGuid const attackerGUID : attackers)

src/Ai/Base/Value/GrindTargetValue.cpp

@@ -59,26 +59,22 @@ Unit* GrindTargetValue::FindTargetForGrinding(uint32 assistCount)
     for (ObjectGuid const guid : targets)
     {
         Unit* unit = botAI->GetUnit(guid);
-
         if (!unit)
             continue;
 
+        if (!unit->IsInWorld() || unit->IsDuringRemoveFromWorld())
+            continue;
+
         auto& rep = bot->ToPlayer()->GetReputationMgr();
         if (unit->ToCreature() && !unit->ToCreature()->GetCreatureTemplate()->lootid &&
             bot->GetReactionTo(unit) >= REP_NEUTRAL)
-        {
             continue;
-        }
 
         if (!bot->IsHostileTo(unit) && unit->GetNpcFlags() != UNIT_NPC_FLAG_NONE)
-        {
             continue;
-        }
 
         if (!bot->isHonorOrXPTarget(unit))
-        {
             continue;
-        }
 
         if (abs(bot->GetPositionZ() - unit->GetPositionZ()) > INTERACTION_DISTANCE)
             continue;

src/Ai/Base/Value/NearestNpcsValue.cpp

@@ -27,7 +27,16 @@ void NearestHostileNpcsValue::FindUnits(std::list<Unit*>& targets)
     Cell::VisitObjects(bot, searcher, range);
 }
 
-bool NearestHostileNpcsValue::AcceptUnit(Unit* unit) { return unit->IsHostileTo(bot) && !unit->IsPlayer(); }
+bool NearestHostileNpcsValue::AcceptUnit(Unit* unit)
+{
+    if (!unit || !unit->IsInWorld() || unit->IsDuringRemoveFromWorld())
+        return false;
+
+    if (unit->IsPlayer())
+        return false;
+
+    return unit->IsHostileTo(bot);
+}
 
 void NearestVehiclesValue::FindUnits(std::list<Unit*>& targets)
 {

src/Ai/Base/Value/PossibleRpgTargetsValue.cpp

@@ -54,6 +54,9 @@ void PossibleRpgTargetsValue::FindUnits(std::list<Unit*>& targets)
 
 bool PossibleRpgTargetsValue::AcceptUnit(Unit* unit)
 {
+    if (!unit || !unit->IsInWorld() || unit->IsDuringRemoveFromWorld())
+        return false;
+
     if (unit->IsHostileTo(bot) || unit->IsPlayer())
         return false;
 
@@ -70,7 +73,8 @@ bool PossibleRpgTargetsValue::AcceptUnit(Unit* unit)
     }
 
     TravelTarget* travelTarget = context->GetValue<TravelTarget*>("travel target")->Get();
-    if (travelTarget->getDestination() && travelTarget->getDestination()->getEntry() == unit->GetEntry())
+    if (travelTarget && travelTarget->getDestination() &&
+        travelTarget->getDestination()->getEntry() == unit->GetEntry())
         return true;
 
     if (urand(1, 100) < 25 && unit->IsFriendlyTo(bot))
@@ -145,6 +149,9 @@ void PossibleNewRpgTargetsValue::FindUnits(std::list<Unit*>& targets)
 
 bool PossibleNewRpgTargetsValue::AcceptUnit(Unit* unit)
 {
+    if (!unit || !unit->IsInWorld() || unit->IsDuringRemoveFromWorld())
+        return false;
+
     if (unit->IsHostileTo(bot) || unit->IsPlayer())
         return false;
 

src/Ai/Raid/Icecrown/Trigger/RaidIccTriggers.cpp

@@ -99,7 +99,10 @@ bool IccGunshipCannonNearTrigger::IsActive()
 bool IccGunshipTeleportAllyTrigger::IsActive()
 {
     Unit* boss = bot->FindNearestCreature(NPC_HIGH_OVERLORD_SAURFANG, 100.0f);
-    if (!boss)
+    if (!boss || !boss->IsInWorld() || boss->IsDuringRemoveFromWorld())
+        return false;
+
+    if (!boss->IsAlive())
         return false;
 
     if (!boss->IsHostileTo(bot))
@@ -111,7 +114,10 @@ bool IccGunshipTeleportAllyTrigger::IsActive()
 bool IccGunshipTeleportHordeTrigger::IsActive()
 {
     Unit* boss = bot->FindNearestCreature(NPC_MURADIN_BRONZEBEARD, 100.0f);
-    if (!boss)
+    if (!boss || !boss->IsInWorld() || boss->IsDuringRemoveFromWorld())
+        return false;
+
+    if (!boss->IsAlive())
         return false;
 
     if (!boss->IsHostileTo(bot))

src/Ai/Raid/ObsidianSanctum/Trigger/RaidOsTriggers.cpp

@@ -78,19 +78,19 @@ bool SartharionMeleePositioningTrigger::IsActive()
 
 bool TwilightPortalEnterTrigger::IsActive()
 {
-    if (botAI->IsMainTank(bot) || botAI->IsHealAssistantOfIndex(bot, 0)) { return false; }
+    if (botAI->IsMainTank(bot) || botAI->IsAssistHealOfIndex(bot, 0)) { return false; }
 
     // In 25-man, take two healers in. Otherwise just take one
     // if (bot->GetRaidDifficulty() == RAID_DIFFICULTY_25MAN_NORMAL)
     // {
-    //     if (botAI->IsHealAssistantOfIndex(bot, 0) || botAI->IsHealAssistantOfIndex(bot, 1))
+    //     if (botAI->IsAssistHealOfIndex(bot, 0) || botAI->IsAssistHealOfIndex(bot, 1))
     //     {
     //         return false;
     //     }
     // }
     // else
     // {
-    //     if (botAI->IsHealAssistantOfIndex(bot, 0))
+    //     if (botAI->IsAssistHealOfIndex(bot, 0))
     //     {
     //         return false;
     //     }

src/Ai/Raid/Ulduar/Trigger/RaidUlduarTriggers.cpp

@@ -765,9 +765,13 @@ bool FreyaMoveToHealingSporeTrigger::IsActive()
 bool ThorimUnbalancingStrikeTrigger::IsActive()
 {
     Unit* boss = AI_VALUE2(Unit*, "find target", "thorim");
+    if (!boss || !boss->IsInWorld() || boss->IsDuringRemoveFromWorld())
+        return false;
 
-    // Check boss and it is alive
-    if (!boss || !boss->IsAlive() || !boss->IsHostileTo(bot))
+    if (!boss->IsAlive())
+        return false;
+
+    if (!boss->IsHostileTo(bot))
         return false;
 
     return bot->HasAura(SPELL_UNBALANCING_STRIKE);
@@ -804,8 +808,13 @@ bool ThorimMarkDpsTargetTrigger::IsActive()
 
         Unit* boss = AI_VALUE2(Unit*, "find target", "thorim");
 
-        // Check boss and it is alive
-        if (!boss || !boss->IsAlive() || !boss->IsHostileTo(bot))
+        if (!boss || !boss->IsInWorld() || boss->IsDuringRemoveFromWorld())
+            return false;
+
+        if (!boss->IsAlive())
+            return false;
+
+        if (!boss->IsHostileTo(bot))
             return false;
 
         if (boss->GetPositionZ() < ULDUAR_THORIM_AXIS_Z_FLOOR_THRESHOLD && (!currentSkullUnit || !currentSkullUnit->IsAlive()))
@@ -982,9 +991,13 @@ bool ThorimGauntletPositioningTrigger::IsActive()
 bool ThorimArenaPositioningTrigger::IsActive()
 {
     Unit* boss = AI_VALUE2(Unit*, "find target", "thorim");
+    if (!boss || !boss->IsInWorld() || boss->IsDuringRemoveFromWorld())
+        return false;
 
-    // Check boss and it is alive
-    if (!boss || !boss->IsAlive() || !boss->IsHostileTo(bot))
+    if (!boss->IsAlive())
+        return false;
+
+    if (!boss->IsHostileTo(bot))
         return false;
 
     if (boss->GetPositionZ() < ULDUAR_THORIM_AXIS_Z_FLOOR_THRESHOLD)
@@ -1080,9 +1093,13 @@ bool ThorimPhase2PositioningTrigger::IsActive()
         return false;
 
     Unit* boss = AI_VALUE2(Unit*, "find target", "thorim");
+    if (!boss || !boss->IsInWorld() || boss->IsDuringRemoveFromWorld())
+    return false;
 
-    // Check boss and it is alive
-    if (!boss || !boss->IsAlive() || !boss->IsHostileTo(bot))
+    if (!boss->IsAlive())
+        return false;
+
+    if (!boss->IsHostileTo(bot))
         return false;
 
     if (boss->GetPositionZ() > ULDUAR_THORIM_AXIS_Z_FLOOR_THRESHOLD)

src/Bot/PlayerbotAI.cpp

@@ -1796,35 +1796,46 @@ bool PlayerbotAI::IsCombo(Player* player)
 
 bool PlayerbotAI::IsRangedDps(Player* player, bool bySpec) { return IsRanged(player, bySpec) && IsDps(player, bySpec); }
 
-bool PlayerbotAI::IsHealAssistantOfIndex(Player* player, int index)
+bool PlayerbotAI::IsAssistHealOfIndex(Player* player, int index, bool ignoreDeadPlayers)
 {
     Group* group = player->GetGroup();
     if (!group)
-    {
         return false;
-    }
 
     int counter = 0;
 
+    // First, assistants
     for (GroupReference* ref = group->GetFirstMember(); ref; ref = ref->next())
     {
         Player* member = ref->GetSource();
-
         if (!member)
-        {
             continue;
-        }
 
-        if (IsHeal(member))  // Check if the member is a healer
+        if (ignoreDeadPlayers && !member->IsAlive())
+            continue;
+
+        if (group->IsAssistant(member->GetGUID()) && IsHeal(member))
         {
-            bool isAssistant = group->IsAssistant(member->GetGUID());
-
-            // Check if the index matches for both assistant and non-assistant healers
-            if ((isAssistant && index == counter) || (!isAssistant && index == counter))
-            {
+            if (index == counter)
                 return player == member;
-            }
+            counter++;
+        }
+    }
 
+    // If not enough assistants, get non-assistants
+    for (GroupReference* ref = group->GetFirstMember(); ref; ref = ref->next())
+    {
+        Player* member = ref->GetSource();
+        if (!member)
+            continue;
+
+        if (ignoreDeadPlayers && !member->IsAlive())
+            continue;
+
+        if (!group->IsAssistant(member->GetGUID()) && IsHeal(member))
+        {
+            if (index == counter)
+                return player == member;
             counter++;
         }
     }
@@ -1832,35 +1843,46 @@ bool PlayerbotAI::IsHealAssistantOfIndex(Player* player, int index)
     return false;
 }
 
-bool PlayerbotAI::IsRangedDpsAssistantOfIndex(Player* player, int index)
+bool PlayerbotAI::IsAssistRangedDpsOfIndex(Player* player, int index, bool ignoreDeadPlayers)
 {
     Group* group = player->GetGroup();
     if (!group)
-    {
         return false;
-    }
 
     int counter = 0;
 
+    // First, assistants
     for (GroupReference* ref = group->GetFirstMember(); ref; ref = ref->next())
     {
         Player* member = ref->GetSource();
-
         if (!member)
-        {
             continue;
-        }
 
-        if (IsRangedDps(member))  // Check if the member is a ranged DPS
+        if (ignoreDeadPlayers && !member->IsAlive())
+            continue;
+
+        if (group->IsAssistant(member->GetGUID()) && IsRangedDps(member))
         {
-            bool isAssistant = group->IsAssistant(member->GetGUID());
-
-            // Check the index for both assistant and non-assistant ranges
-            if ((isAssistant && index == counter) || (!isAssistant && index == counter))
-            {
+            if (index == counter)
                 return player == member;
-            }
+            counter++;
+        }
+    }
 
+    // If not enough assistants, get non-assistants
+    for (GroupReference* ref = group->GetFirstMember(); ref; ref = ref->next())
+    {
+        Player* member = ref->GetSource();
+        if (!member)
+            continue;
+
+        if (ignoreDeadPlayers && !member->IsAlive())
+            continue;
+
+        if (!group->IsAssistant(member->GetGUID()) && IsRangedDps(member))
+        {
+            if (index == counter)
+                return player == member;
             counter++;
         }
     }
@@ -2335,18 +2357,16 @@ bool PlayerbotAI::IsAssistTankOfIndex(Player* player, int index, bool ignoreDead
 {
     Group* group = player->GetGroup();
     if (!group)
-    {
         return false;
-    }
+
     int counter = 0;
+
+    // First, assistants
     for (GroupReference* ref = group->GetFirstMember(); ref; ref = ref->next())
     {
         Player* member = ref->GetSource();
-
         if (!member)
-        {
             continue;
-        }
 
         if (ignoreDeadPlayers && !member->IsAlive())
             continue;
@@ -2354,21 +2374,17 @@ bool PlayerbotAI::IsAssistTankOfIndex(Player* player, int index, bool ignoreDead
         if (group->IsAssistant(member->GetGUID()) && IsAssistTank(member))
         {
             if (index == counter)
-            {
                 return player == member;
-            }
             counter++;
         }
     }
-    // not enough
+
+    // If not enough assistants, get non-assistants
     for (GroupReference* ref = group->GetFirstMember(); ref; ref = ref->next())
     {
         Player* member = ref->GetSource();
-
         if (!member)
-        {
             continue;
-        }
 
         if (ignoreDeadPlayers && !member->IsAlive())
             continue;
@@ -2376,9 +2392,7 @@ bool PlayerbotAI::IsAssistTankOfIndex(Player* player, int index, bool ignoreDead
         if (!group->IsAssistant(member->GetGUID()) && IsAssistTank(member))
         {
             if (index == counter)
-            {
                 return player == member;
-            }
             counter++;
         }
     }

src/Bot/PlayerbotAI.h

@@ -429,8 +429,8 @@ public:
     static uint32 GetGroupTankNum(Player* player);
     static bool IsAssistTank(Player* player);
     static bool IsAssistTankOfIndex(Player* player, int index, bool ignoreDeadPlayers = false);
-    static bool IsHealAssistantOfIndex(Player* player, int index);
-    static bool IsRangedDpsAssistantOfIndex(Player* player, int index);
+    static bool IsAssistHealOfIndex(Player* player, int index, bool ignoreDeadPlayers = false);
+    static bool IsAssistRangedDpsOfIndex(Player* player, int index, bool ignoreDeadPlayers = false);
     bool HasAggro(Unit* unit);
     static int32 GetAssistTankIndex(Player* player);
     int32 GetGroupSlotIndex(Player* player);