Refactor of EquipActions (#1994)

#PR Description 

The root cause of issue #1987 was the AI Value item usage becoming a
very expensive call when bots gained professions accidentally.

My original approach was to eliminate it entirely, but after inputs and
testing I decided to introduce a more focused Ai value "Item upgrade"
that only checks equipment and ammo inheriting directly from item usage,
so the logic is unified between them.

Upgrades are now only assessed when receiving an item that can be
equipped.

Additionally, I noticed that winning loot rolls did not trigger the
upgrade action, so I added a new package handler for that.


Performance needs to be re-evaluated, but I expect a reduction in calls
and in the cost of each call.

I tested with bots and selfbot in deadmines and ahadowfang keep.

---------

Co-authored-by: bashermens <31279994+hermensbas@users.noreply.github.com>

conf/playerbots.conf.dist

@@ -2182,4 +2182,4 @@ AiPlayerbot.SummonAtInnkeepersEnabled = 1
 # 30% more damage, 40% damage reduction (tank bots), increased all resistances, reduced threat for non tank bots, increased threat for tank bots.
 # Buffs will be applied on PP, Sindragosa and Lich King
 
-AiPlayerbot.EnableICCBuffs = 1
+AiPlayerbot.EnableICCBuffs = 1

src/Ai/Base/Actions/EquipAction.cpp

@@ -328,7 +328,43 @@ void EquipAction::EquipItem(Item* item)
     botAI->TellMaster(out);
 }
 
-bool EquipUpgradesAction::Execute(Event event)
+ItemIds EquipAction::SelectInventoryItemsToEquip()
+{
+    CollectItemsVisitor visitor;
+    IterateItems(&visitor, ITERATE_ITEMS_IN_BAGS);
+
+    ItemIds items;
+    for (auto i = visitor.items.begin(); i != visitor.items.end(); ++i)
+    {
+        Item* item = *i;
+        if (!item)
+            continue;
+
+        ItemTemplate const* itemTemplate = item->GetTemplate();
+        if (!itemTemplate)
+            continue;
+
+        //TODO Expand to Glyphs and Gems, that can be placed in equipment
+        //Pre-filter non-equipable items
+        if (itemTemplate->InventoryType == INVTYPE_NON_EQUIP)
+            continue;
+
+        int32 randomProperty = item->GetItemRandomPropertyId();
+        uint32 itemId = item->GetTemplate()->ItemId;
+        std::string itemUsageParam;
+        if (randomProperty != 0)
+            itemUsageParam = std::to_string(itemId) + "," + std::to_string(randomProperty);
+        else
+            itemUsageParam = std::to_string(itemId);
+
+        ItemUsage usage = AI_VALUE2(ItemUsage, "item upgrade", itemUsageParam);
+        if (usage == ITEM_USAGE_EQUIP || usage == ITEM_USAGE_REPLACE || usage == ITEM_USAGE_BAD_EQUIP)
+            items.insert(itemId);
+    }
+    return items;
+}
+
+bool EquipUpgradesTriggeredAction::Execute(Event event)
 {
     if (!sPlayerbotAIConfig.autoEquipUpgradeLoot && !sRandomPlayerbotMgr.IsRandomBot(bot))
         return false;
@@ -361,72 +397,18 @@ bool EquipUpgradesAction::Execute(Event event)
         p >> itemId;
 
         ItemTemplate const* item = sObjectMgr->GetItemTemplate(itemId);
-        if (item->Class == ITEM_CLASS_TRADE_GOODS && item->SubClass == ITEM_SUBCLASS_MEAT)
+        if (item->InventoryType == INVTYPE_NON_EQUIP)
             return false;
     }
 
-    CollectItemsVisitor visitor;
+    ItemIds items = SelectInventoryItemsToEquip();
-    IterateItems(&visitor, ITERATE_ITEMS_IN_BAGS);
-
-    ItemIds items;
-    for (auto i = visitor.items.begin(); i != visitor.items.end(); ++i)
-    {
-        Item* item = *i;
-        if (!item)
-            break;
-        int32 randomProperty = item->GetItemRandomPropertyId();
-        uint32 itemId = item->GetTemplate()->ItemId;
-        std::string itemUsageParam;
-        if (randomProperty != 0)
-        {
-            itemUsageParam = std::to_string(itemId) + "," + std::to_string(randomProperty);
-        }
-        else
-        {
-            itemUsageParam = std::to_string(itemId);
-        }
-        ItemUsage usage = AI_VALUE2(ItemUsage, "item usage", itemUsageParam);
-
-        if (usage == ITEM_USAGE_EQUIP || usage == ITEM_USAGE_REPLACE || usage == ITEM_USAGE_BAD_EQUIP)
-        {
-            items.insert(itemId);
-        }
-    }
-
     EquipItems(items);
     return true;
 }
 
 bool EquipUpgradeAction::Execute(Event event)
 {
-    CollectItemsVisitor visitor;
+    ItemIds items = SelectInventoryItemsToEquip();
-    IterateItems(&visitor, ITERATE_ITEMS_IN_BAGS);
-
-    ItemIds items;
-    for (auto i = visitor.items.begin(); i != visitor.items.end(); ++i)
-    {
-        Item* item = *i;
-        if (!item)
-            break;
-        int32 randomProperty = item->GetItemRandomPropertyId();
-        uint32 itemId = item->GetTemplate()->ItemId;
-        std::string itemUsageParam;
-        if (randomProperty != 0)
-        {
-            itemUsageParam = std::to_string(itemId) + "," + std::to_string(randomProperty);
-        }
-        else
-        {
-            itemUsageParam = std::to_string(itemId);
-        }
-        ItemUsage usage = AI_VALUE2(ItemUsage, "item usage", itemUsageParam);
-
-        if (usage == ITEM_USAGE_EQUIP || usage == ITEM_USAGE_REPLACE || usage == ITEM_USAGE_BAD_EQUIP)
-        {
-            items.insert(itemId);
-        }
-    }
-
     EquipItems(items);
     return true;
 }

src/Ai/Base/Actions/EquipAction.h

@@ -8,6 +8,7 @@
 
 #include "ChatHelper.h"
 #include "InventoryAction.h"
+#include "Item.h"
 
 class FindItemVisitor;
 class Item;
@@ -20,6 +21,7 @@ public:
 
     bool Execute(Event event) override;
     void EquipItems(ItemIds ids);
+    ItemIds SelectInventoryItemsToEquip();
 
 private:
     void EquipItem(FindItemVisitor* visitor);
@@ -27,10 +29,10 @@ private:
     void EquipItem(Item* item);
 };
 
-class EquipUpgradesAction : public EquipAction
+class EquipUpgradesTriggeredAction : public EquipAction
 {
 public:
-    EquipUpgradesAction(PlayerbotAI* botAI, std::string const name = "equip upgrades") : EquipAction(botAI, name) {}
+    explicit EquipUpgradesTriggeredAction(PlayerbotAI* botAI, std::string const name = "equip upgrades") : EquipAction(botAI, name) {}
 
     bool Execute(Event event) override;
 };
@@ -38,7 +40,7 @@ public:
 class EquipUpgradeAction : public EquipAction
 {
 public:
-    EquipUpgradeAction(PlayerbotAI* botAI, std::string const name = "equip upgrade") : EquipAction(botAI, name) {}
+    explicit EquipUpgradeAction(PlayerbotAI* botAI, std::string const name = "equip upgrade") : EquipAction(botAI, name) {}
 
     bool Execute(Event event) override;
 };

src/Ai/Base/ChatActionContext.h

@@ -120,7 +120,7 @@ public:
         creators["use"] = &ChatActionContext::use;
         creators["item count"] = &ChatActionContext::item_count;
         creators["equip"] = &ChatActionContext::equip;
-        creators["equip upgrades"] = &ChatActionContext::equip_upgrades;
+        creators["equip upgrades"] = &ChatActionContext::equip_upgrade;
         creators["unequip"] = &ChatActionContext::unequip;
         creators["sell"] = &ChatActionContext::sell;
         creators["buy"] = &ChatActionContext::buy;
@@ -258,7 +258,6 @@ private:
     static Action* talents(PlayerbotAI* botAI) { return new ChangeTalentsAction(botAI); }
 
     static Action* equip(PlayerbotAI* botAI) { return new EquipAction(botAI); }
-    static Action* equip_upgrades(PlayerbotAI* botAI) { return new EquipUpgradesAction(botAI); }
     static Action* unequip(PlayerbotAI* botAI) { return new UnequipAction(botAI); }
     static Action* sell(PlayerbotAI* botAI) { return new SellAction(botAI); }
     static Action* buy(PlayerbotAI* botAI) { return new BuyAction(botAI); }

src/Ai/Base/Strategy/WorldPacketHandlerStrategy.cpp

@@ -42,6 +42,7 @@ void WorldPacketHandlerStrategy::InitTriggers(std::vector<TriggerNode*>& trigger
                                                                                 NextAction("query item usage", relevance),
                                                                                 NextAction("equip upgrades", relevance) }));
     triggers.push_back(new TriggerNode("item push result", { NextAction("quest item push result", relevance) }));
+    triggers.push_back(new TriggerNode("loot roll won", { NextAction("equip upgrades", relevance) }));
     triggers.push_back(new TriggerNode("ready check finished", { NextAction("finish ready check", relevance) }));
     // triggers.push_back(new TriggerNode("often", { NextAction("security check", relevance), NextAction("check mail", relevance) }));
     triggers.push_back(new TriggerNode("guild invite", { NextAction("guild accept", relevance) }));

src/Ai/Base/Value/ItemUsageValue.cpp

@@ -19,19 +19,9 @@
 
 ItemUsage ItemUsageValue::Calculate()
 {
-    uint32 itemId = 0;
+    ParsedItemUsage const parsed = GetItemIdFromQualifier();
-    uint32 randomPropertyId = 0;
+    uint32 itemId = parsed.itemId;
-    size_t pos = qualifier.find(",");
+    uint32 randomPropertyId = parsed.randomPropertyId;
-    if (pos != std::string::npos)
-    {
-        itemId = atoi(qualifier.substr(0, pos).c_str());
-        randomPropertyId = atoi(qualifier.substr(pos + 1).c_str());
-    }
-    else
-    {
-        itemId = atoi(qualifier.c_str());
-    }
-
     if (!itemId)
         return ITEM_USAGE_NONE;
 
@@ -142,96 +132,30 @@ ItemUsage ItemUsageValue::Calculate()
 
     // If the loot is from an item in the bot’s bags, ignore syncQuestWithPlayer
     if (isLootFromItem && botNeedsItemForQuest)
-    {
         return ITEM_USAGE_QUEST;
-    }
 
     // If the bot is NOT acting alone and the master needs this quest item, defer to the master
     if (!isSelfBot && masterNeedsItemForQuest)
-    {
         return ITEM_USAGE_NONE;
-    }
 
     // If the bot itself needs the item for a quest, allow looting
     if (botNeedsItemForQuest)
-    {
         return ITEM_USAGE_QUEST;
-    }
 
     if (proto->Class == ITEM_CLASS_PROJECTILE && bot->CanUseItem(proto) == EQUIP_ERR_OK)
     {
-        if (bot->getClass() == CLASS_HUNTER || bot->getClass() == CLASS_ROGUE || bot->getClass() == CLASS_WARRIOR)
+        ItemUsage ammoUsage = QueryItemUsageForAmmo(proto);
-        {
+        if (ammoUsage != ITEM_USAGE_NONE)
-            Item* rangedWeapon = bot->GetItemByPos(INVENTORY_SLOT_BAG_0, EQUIPMENT_SLOT_RANGED);
+            return ammoUsage;
-            uint32 requiredSubClass = 0;
-
-            if (rangedWeapon)
-            {
-                switch (rangedWeapon->GetTemplate()->SubClass)
-                {
-                    case ITEM_SUBCLASS_WEAPON_GUN:
-                        requiredSubClass = ITEM_SUBCLASS_BULLET;
-                        break;
-                    case ITEM_SUBCLASS_WEAPON_BOW:
-                    case ITEM_SUBCLASS_WEAPON_CROSSBOW:
-                        requiredSubClass = ITEM_SUBCLASS_ARROW;
-                        break;
-                }
-            }
-
-            // Ensure the item is the correct ammo type for the equipped ranged weapon
-            if (proto->SubClass == requiredSubClass)
-            {
-                float ammoCount = BetterStacks(proto, "ammo");
-                float requiredAmmo = (bot->getClass() == CLASS_HUNTER) ? 8 : 2; // Hunters get 8 stacks, others 2
-                uint32 currentAmmoId = bot->GetUInt32Value(PLAYER_AMMO_ID);
-
-                // Check if the bot has an ammo type assigned
-                if (currentAmmoId == 0)
-                {
-                    return ITEM_USAGE_EQUIP;  // Equip the ammo if no ammo
-                }
-                // Compare new ammo vs current equipped ammo
-                ItemTemplate const* currentAmmoProto = sObjectMgr->GetItemTemplate(currentAmmoId);
-                if (currentAmmoProto)
-                {
-                    uint32 currentAmmoDPS = (currentAmmoProto->Damage[0].DamageMin + currentAmmoProto->Damage[0].DamageMax) * 1000 / 2;
-                    uint32 newAmmoDPS = (proto->Damage[0].DamageMin + proto->Damage[0].DamageMax) * 1000 / 2;
-
-                    if (newAmmoDPS > currentAmmoDPS) // New ammo meets upgrade condition
-                    {
-                        return ITEM_USAGE_EQUIP;
-                    }
-                    if (newAmmoDPS < currentAmmoDPS) // New ammo is worse
-                    {
-                        return ITEM_USAGE_NONE;
-                    }
-                }
-                // Ensure we have enough ammo in the inventory
-                if (ammoCount < requiredAmmo)
-                {
-                    ammoCount += CurrentStacks(proto);
-
-                    if (ammoCount < requiredAmmo)  // Buy ammo to reach the proper supply
-                        return ITEM_USAGE_AMMO;
-                    else if (ammoCount < requiredAmmo + 1)
-                        return ITEM_USAGE_KEEP;  // Keep the ammo if we don't have too much.
-                }
-            }
-        }
     }
-
     // Need to add something like free bagspace or item value.
     if (proto->SellPrice > 0)
     {
         if (proto->Quality >= ITEM_QUALITY_NORMAL && !isSoulbound)
-        {
             return ITEM_USAGE_AH;
-        }
+
         else
-        {
             return ITEM_USAGE_VENDOR;
-        }
     }
 
     return ITEM_USAGE_NONE;
@@ -480,6 +404,80 @@ ItemUsage ItemUsageValue::QueryItemUsageForEquip(ItemTemplate const* itemProto,
     return ITEM_USAGE_NONE;
 }
 
+ItemUsage ItemUsageValue::QueryItemUsageForAmmo(ItemTemplate const* proto)
+{
+    if (bot->getClass() != CLASS_HUNTER || bot->getClass() != CLASS_ROGUE || bot->getClass() != CLASS_WARRIOR)
+        return ITEM_USAGE_NONE;
+
+    Item* rangedWeapon = bot->GetItemByPos(INVENTORY_SLOT_BAG_0, EQUIPMENT_SLOT_RANGED);
+    uint32 requiredSubClass = 0;
+
+    if (rangedWeapon)
+    {
+        switch (rangedWeapon->GetTemplate()->SubClass)
+        {
+            case ITEM_SUBCLASS_WEAPON_GUN:
+                requiredSubClass = ITEM_SUBCLASS_BULLET;
+                break;
+            case ITEM_SUBCLASS_WEAPON_BOW:
+            case ITEM_SUBCLASS_WEAPON_CROSSBOW:
+                requiredSubClass = ITEM_SUBCLASS_ARROW;
+                break;
+        }
+    }
+
+    // Ensure the item is the correct ammo type for the equipped ranged weapon
+    if (proto->SubClass == requiredSubClass)
+    {
+        float ammoCount = BetterStacks(proto, "ammo");
+        float requiredAmmo = (bot->getClass() == CLASS_HUNTER) ? 8 : 2; // Hunters get 8 stacks, others 2
+        uint32 currentAmmoId = bot->GetUInt32Value(PLAYER_AMMO_ID);
+
+        // Check if the bot has an ammo type assigned
+        if (currentAmmoId == 0)
+            return ITEM_USAGE_EQUIP;  // Equip the ammo if no ammo
+        // Compare new ammo vs current equipped ammo
+        ItemTemplate const* currentAmmoProto = sObjectMgr->GetItemTemplate(currentAmmoId);
+        if (currentAmmoProto)
+        {
+            uint32 currentAmmoDPS = (currentAmmoProto->Damage[0].DamageMin + currentAmmoProto->Damage[0].DamageMax) * 1000 / 2;
+            uint32 newAmmoDPS = (proto->Damage[0].DamageMin + proto->Damage[0].DamageMax) * 1000 / 2;
+
+            if (newAmmoDPS > currentAmmoDPS) // New ammo meets upgrade condition
+                return ITEM_USAGE_EQUIP;
+
+            if (newAmmoDPS < currentAmmoDPS) // New ammo is worse
+                return ITEM_USAGE_NONE;
+        }
+        // Ensure we have enough ammo in the inventory
+        if (ammoCount < requiredAmmo)
+        {
+            ammoCount += CurrentStacks(proto);
+
+            if (ammoCount < requiredAmmo)  // Buy ammo to reach the proper supply
+                return ITEM_USAGE_AMMO;
+            else if (ammoCount < requiredAmmo + 1)
+                return ITEM_USAGE_KEEP;  // Keep the ammo if we don't have too much.
+        }
+    }
+    return ITEM_USAGE_NONE;
+}
+
+ParsedItemUsage ItemUsageValue::GetItemIdFromQualifier()
+{
+    ParsedItemUsage parsed;
+
+    size_t const pos = qualifier.find(",");
+    if (pos != std::string::npos)
+    {
+        parsed.itemId = atoi(qualifier.substr(0, pos).c_str());
+        parsed.randomPropertyId = atoi(qualifier.substr(pos + 1).c_str());
+        return parsed;
+    }
+    else
+        parsed.itemId = atoi(qualifier.c_str());
+    return parsed;
+}
 // Return smaltest bag size equipped
 uint32 ItemUsageValue::GetSmallestBagSize()
 {
@@ -913,3 +911,25 @@ std::string const ItemUsageValue::GetConsumableType(ItemTemplate const* proto, b
 
     return "";
 }
+
+ItemUsage ItemUpgradeValue::Calculate()
+{
+    ParsedItemUsage parsed = GetItemIdFromQualifier();
+    uint32 itemId = parsed.itemId;
+    uint32 randomPropertyId = parsed.randomPropertyId;
+    if (!itemId)
+        return ITEM_USAGE_NONE;
+
+    ItemTemplate const* proto = sObjectMgr->GetItemTemplate(itemId);
+    if (!proto)
+        return ITEM_USAGE_NONE;
+
+    ItemUsage equip = QueryItemUsageForEquip(proto, randomPropertyId);
+    if (equip != ITEM_USAGE_NONE)
+        return equip;
+
+    if (proto->Class == ITEM_CLASS_PROJECTILE && bot->CanUseItem(proto) == EQUIP_ERR_OK)
+        return QueryItemUsageForAmmo(proto);
+
+    return ITEM_USAGE_NONE;
+}

src/Ai/Base/Value/ItemUsageValue.h

@@ -14,7 +14,11 @@ class Player;
 class PlayerbotAI;
 
 struct ItemTemplate;
-
+struct ParsedItemUsage
+{
+    uint32 itemId = 0;
+    int32 randomPropertyId = 0;
+};
 enum ItemUsage : uint32
 {
     ITEM_USAGE_NONE = 0,
@@ -42,8 +46,12 @@ public:
 
     ItemUsage Calculate() override;
 
-private:
+protected:
     ItemUsage QueryItemUsageForEquip(ItemTemplate const* proto, int32 randomPropertyId = 0);
+    ItemUsage QueryItemUsageForAmmo(ItemTemplate const* proto);
+    ParsedItemUsage GetItemIdFromQualifier();
+
+private:
     uint32 GetSmallestBagSize();
     bool IsItemUsefulForQuest(Player* player, ItemTemplate const* proto);
     bool IsItemNeededForSkill(ItemTemplate const* proto);
@@ -61,4 +69,14 @@ public:
     static std::string const GetConsumableType(ItemTemplate const* proto, bool hasMana);
 };
 
+class ItemUpgradeValue : public ItemUsageValue
+{
+public:
+    ItemUpgradeValue(PlayerbotAI* botAI, std::string const name = "item upgrade") : ItemUsageValue(botAI, name)
+    {
+    }
+
+    ItemUsage Calculate() override;
+};
+
 #endif

src/Ai/Base/ValueContext.h

@@ -216,6 +216,7 @@ public:
         creators["formation"] = &ValueContext::formation;
         creators["stance"] = &ValueContext::stance;
         creators["item usage"] = &ValueContext::item_usage;
+        creators["item upgrade"] = &ValueContext::item_upgrade;
         creators["speed"] = &ValueContext::speed;
         creators["last said"] = &ValueContext::last_said;
         creators["last emote"] = &ValueContext::last_emote;
@@ -341,6 +342,7 @@ private:
     static UntypedValue* already_seen_players(PlayerbotAI* botAI) { return new AlreadySeenPlayersValue(botAI); }
     static UntypedValue* new_player_nearby(PlayerbotAI* botAI) { return new NewPlayerNearbyValue(botAI); }
     static UntypedValue* item_usage(PlayerbotAI* botAI) { return new ItemUsageValue(botAI); }
+    static UntypedValue* item_upgrade(PlayerbotAI* botAI) { return new ItemUpgradeValue(botAI); }
     static UntypedValue* formation(PlayerbotAI* botAI) { return new FormationValue(botAI); }
     static UntypedValue* stance(PlayerbotAI* botAI) { return new StanceValue(botAI); }
     static UntypedValue* mana_save_level(PlayerbotAI* botAI) { return new ManaSaveLevelValue(botAI); }

src/Ai/Base/WorldPacketTriggerContext.h

@@ -46,6 +46,7 @@ public:
         creators["questgiver quest details"] = &WorldPacketTriggerContext::questgiver_quest_details;
 
         creators["item push result"] = &WorldPacketTriggerContext::item_push_result;
+        creators["loot roll won"] = &WorldPacketTriggerContext::loot_roll_won;
         creators["party command"] = &WorldPacketTriggerContext::party_command;
         creators["taxi done"] = &WorldPacketTriggerContext::taxi_done;
         creators["cast failed"] = &WorldPacketTriggerContext::cast_failed;
@@ -92,6 +93,7 @@ private:
     static Trigger* taxi_done(PlayerbotAI* botAI) { return new WorldPacketTrigger(botAI, "taxi done"); }
     static Trigger* party_command(PlayerbotAI* botAI) { return new WorldPacketTrigger(botAI, "party command"); }
     static Trigger* item_push_result(PlayerbotAI* botAI) { return new WorldPacketTrigger(botAI, "item push result"); }
+    static Trigger* loot_roll_won(PlayerbotAI* botAI) { return new WorldPacketTrigger(botAI, "loot roll won"); }
 
     // quest
     static Trigger* quest_update_add_kill(PlayerbotAI* ai) { return new WorldPacketTrigger(ai, "quest update add kill"); }

src/Bot/PlayerbotAI.cpp

@@ -185,6 +185,7 @@ PlayerbotAI::PlayerbotAI(Player* bot)
     botOutgoingPacketHandlers.AddHandler(SMSG_TRADE_STATUS_EXTENDED, "trade status extended");
     botOutgoingPacketHandlers.AddHandler(SMSG_LOOT_RESPONSE, "loot response");
     botOutgoingPacketHandlers.AddHandler(SMSG_ITEM_PUSH_RESULT, "item push result");
+    botOutgoingPacketHandlers.AddHandler(SMSG_LOOT_ROLL_WON, "loot roll won");
     botOutgoingPacketHandlers.AddHandler(SMSG_PARTY_COMMAND_RESULT, "party command");
     botOutgoingPacketHandlers.AddHandler(SMSG_LEVELUP_INFO, "levelup");
     botOutgoingPacketHandlers.AddHandler(SMSG_LOG_XPGAIN, "xpgain");