server_crash_fix: OnPlayerGiveXP (#1929 )

[fef0ed4072c8_worldserver.exe_.18-12_23-30-44.txt](https://github.com/user-attachments/files/24258638/fef0ed4072c8_worldserver.exe_.18-12_23-30-44.txt) - Just added common logic and defense coding - Optimized isRandombot while at it Just for clarification i am aware the trigger of bug lies with the progression mod. Regardless it should not happen.
FixListSpellsWithCache (#1931 )
2026-01-13 00:58:33 +00:00 · 2025-12-19 23:08:18 +01:00 · 2025-12-19 23:08:01 +01:00
6 changed files with 206 additions and 144 deletions
--- a/src/Playerbots.cpp
+++ b/src/Playerbots.cpp
@@ -25,6 +25,7 @@
 #include "Metric.h"
 #include "PlayerScript.h"
 #include "PlayerbotAIConfig.h"
+#include "PlayerbotSpellCache.h"
 #include "PlayerbotWorldThreadProcessor.h"
 #include "RandomPlayerbotMgr.h"
 #include "ScriptMgr.h"
@@ -238,44 +239,39 @@ public:

    void OnPlayerGiveXP(Player* player, uint32& amount, Unit* /*victim*/, uint8 /*xpSource*/) override
    {
-        // no XP multiplier, when player is no bot.
-        if (!player || !player->GetSession()->IsBot())
+        if (sPlayerbotAIConfig->randomBotXPRate == 1.0f || !player || !player->IsInWorld())
            return;

-        // no XP gain, if master is not a bot and has xp gain disabled.
-        if (const Player* master = GET_PLAYERBOT_AI(player)->GetMaster())
+        PlayerbotAI* botAI = GET_PLAYERBOT_AI(player);
+        if (!botAI || !sRandomPlayerbotMgr->IsRandomBot(player))
+            return;
+
+        // No XP gain if master is a real player with XP gain disabled
+        if (const Player* master = botAI->GetMaster())
        {
-            if (!master->GetSession()->IsBot() && master->HasPlayerFlag(PLAYER_FLAGS_NO_XP_GAIN))
+            if (WorldSession* masterSession = master->GetSession();
+                masterSession && !masterSession->IsBot() && master->HasPlayerFlag(PLAYER_FLAGS_NO_XP_GAIN))
            {
-                amount = 0;
+                amount = 0; // disable XP multiplier
                return;
            }
        }

-        // early return
-        if (sPlayerbotAIConfig->randomBotXPRate == 1.0 || !sRandomPlayerbotMgr->IsRandomBot(player))
-            return;
-
-        // no XP multiplier, when bot is in a group with a real player.
+        // No XP multiplier if bot is in a group with at least one real player
        if (Group* group = player->GetGroup())
        {
            for (GroupReference* gref = group->GetFirstMember(); gref; gref = gref->next())
            {
-                Player* member = gref->GetSource();
-                if (!member)
+                if (Player* member = gref->GetSource())
                {
-                    continue;
-                }
-
-                if (!member->GetSession()->IsBot())
-                {
-                    return;
+                    if (!member->GetSession()->IsBot())
+                        return;
                }
            }
        }

-        // otherwise apply bot XP multiplier.
-        amount = static_cast<uint32>(std::round(static_cast<float>(amount) * sPlayerbotAIConfig->randomBotXPRate));
+        // Otherwise apply XP multiplier
+        amount = static_cast<uint32>(std::round(amount * sPlayerbotAIConfig->randomBotXPRate));
    }
 };

@@ -347,6 +343,9 @@ public:

        LOG_INFO("server.loading", ">> Loaded playerbots config in {} ms", GetMSTimeDiffToNow(oldMSTime));
        LOG_INFO("server.loading", " ");
+
+        sPlayerbotSpellCache->Initialize();
+
        LOG_INFO("server.loading", "Playerbots World Thread Processor initialized");
    }

--- a/src/RandomPlayerbotMgr.cpp
+++ b/src/RandomPlayerbotMgr.cpp
@@ -2594,17 +2594,14 @@ void RandomPlayerbotMgr::Refresh(Player* bot)

 bool RandomPlayerbotMgr::IsRandomBot(Player* bot)
 {
-    if (bot && GET_PLAYERBOT_AI(bot))
-    {
-        if (GET_PLAYERBOT_AI(bot)->IsRealPlayer())
-            return false;
-    }
-    if (bot)
-    {
-        return IsRandomBot(bot->GetGUID().GetCounter());
-    }
+    if (!bot)
+        return false;

-    return false;
+    PlayerbotAI* botAI = GET_PLAYERBOT_AI(bot);
+    if (!botAI || botAI->IsRealPlayer())
+        return false;
+
+    return IsRandomBot(bot->GetGUID().GetCounter());
 }

 bool RandomPlayerbotMgr::IsRandomBot(ObjectGuid::LowType bot)
--- a/src/database/PlayerbotSpellCache.cpp
+++ b/src/database/PlayerbotSpellCache.cpp
@@ -0,0 +1,45 @@
+#include "PlayerbotSpellCache.h"
+
+void PlayerbotSpellCache::Initialize()
+{
+    LOG_INFO("playerbots",
+        "Playerbots: ListSpellsAction caches initialized");
+    for (uint32 j = 0; j < sSkillLineAbilityStore.GetNumRows(); ++j)
+    {
+        if (SkillLineAbilityEntry const* skillLine = sSkillLineAbilityStore.LookupEntry(j))
+            skillSpells[skillLine->Spell] = skillLine;
+    }
+
+        // Fill the vendorItems cache once from the world database.
+        QueryResult results = WorldDatabase.Query("SELECT item FROM npc_vendor WHERE maxcount = 0");
+        if (results)
+        {
+            do
+            {
+                Field* fields = results->Fetch();
+                int32 entry = fields[0].Get<int32>();
+                if (entry <= 0)
+                    continue;
+
+                vendorItems.insert(static_cast<uint32>(entry));
+            }
+            while (results->NextRow());
+        }
+
+        LOG_DEBUG("playerbots",
+            "ListSpellsAction: initialized caches (skillSpells={}, vendorItems={}).",
+            skillSpells.size(), vendorItems.size());
+}
+
+SkillLineAbilityEntry const* PlayerbotSpellCache::GetSkillLine(uint32 spellId) const
+{
+    auto itr = skillSpells.find(spellId);
+    if (itr != skillSpells.end())
+        return itr->second;
+    return nullptr;
+}
+
+bool PlayerbotSpellCache::IsItemBuyable(uint32 itemId) const
+{
+    return vendorItems.find(itemId) != vendorItems.end();
+}
--- a/src/database/PlayerbotSpellCache.h
+++ b/src/database/PlayerbotSpellCache.h
@@ -0,0 +1,34 @@
+/*
+ * Copyright (C) 2016+ AzerothCore <www.azerothcore.org>, released under GNU AGPL v3 license, you may redistribute it
+ * and/or modify it under version 3 of the License, or (at your option), any later version.
+ */
+
+#ifndef _PLAYERBOT_PLAYERBOTSPELLCACHE_H
+#define _PLAYERBOT_PLAYERBOTSPELLCACHE_H
+
+#include "Playerbots.h"
+
+class PlayerbotSpellCache
+{
+public:
+    static PlayerbotSpellCache* Instance()
+    {
+        static PlayerbotSpellCache instance;
+        return &instance;
+    }
+
+    void Initialize(); // call once on startup
+
+    SkillLineAbilityEntry const* GetSkillLine(uint32 spellId) const;
+    bool IsItemBuyable(uint32 itemId) const;
+
+private:
+    PlayerbotSpellCache() = default;
+
+    std::map<uint32, SkillLineAbilityEntry const*> skillSpells;
+    std::set<uint32> vendorItems;
+};
+
+#define sPlayerbotSpellCache PlayerbotSpellCache::Instance()
+
+#endif
--- a/src/strategy/actions/ListSpellsAction.cpp
+++ b/src/strategy/actions/ListSpellsAction.cpp
@@ -7,90 +7,43 @@

 #include "Event.h"
 #include "Playerbots.h"
+#include "PlayerbotSpellCache.h"

-std::map<uint32, SkillLineAbilityEntry const*> ListSpellsAction::skillSpells;
-std::set<uint32> ListSpellsAction::vendorItems;
+using SpellListEntry = std::pair<uint32, std::string>;

-bool CompareSpells(const std::pair<uint32, std::string>& s1, const std::pair<uint32, std::string>& s2)
+// CHANGE: Simplified and cheap comparator used in MapUpdater worker thread.
+// It now avoids scanning the entire SkillLineAbilityStore for each comparison
+// and only relies on spell school and spell name to keep sorting fast and bounded.
+// lhs = the left element, rhs = the right element.
+static bool CompareSpells(SpellListEntry const& lhSpell, SpellListEntry const& rhSpell)
 {
-    SpellInfo const* si1 = sSpellMgr->GetSpellInfo(s1.first);
-    SpellInfo const* si2 = sSpellMgr->GetSpellInfo(s2.first);
-    if (!si1 || !si2)
+    SpellInfo const* lhSpellInfo = sSpellMgr->GetSpellInfo(lhSpell.first);
+    SpellInfo const* rhSpellInfo = sSpellMgr->GetSpellInfo(rhSpell.first);
+
+    if (!lhSpellInfo || !rhSpellInfo)
    {
-        LOG_ERROR("playerbots", "SpellInfo missing. {} {}", s1.first, s2.first);
-        return false;
-    }
-    uint32 p1 = si1->SchoolMask * 20000;
-    uint32 p2 = si2->SchoolMask * 20000;
-
-    uint32 skill1 = 0, skill2 = 0;
-    uint32 skillValue1 = 0, skillValue2 = 0;
-    for (uint32 j = 0; j < sSkillLineAbilityStore.GetNumRows(); ++j)
-    {
-        if (SkillLineAbilityEntry const* skillLine = sSkillLineAbilityStore.LookupEntry(j))
-        {
-            if (skillLine->Spell == s1.first)
-            {
-                skill1 = skillLine->SkillLine;
-                skillValue1 = skillLine->TrivialSkillLineRankLow;
-            }
-
-            if (skillLine->Spell == s2.first)
-            {
-                skill2 = skillLine->SkillLine;
-                skillValue2 = skillLine->TrivialSkillLineRankLow;
-            }
-        }
-
-        if (skill1 && skill2)
-            break;
+        LOG_ERROR("playerbots", "SpellInfo missing for spell {} or {}", lhSpell.first, rhSpell.first);
+        // Fallback: order by spell id to keep comparator strict and deterministic.
+        return lhSpell.first < rhSpell.first;
    }

-    p1 += skill1 * 500;
-    p2 += skill2 * 500;
+    uint32 lhsKey = lhSpellInfo->SchoolMask;
+    uint32 rhsKey = rhSpellInfo->SchoolMask;

-    p1 += skillValue1;
-    p2 += skillValue2;
-
-    if (p1 == p2)
+    if (lhsKey == rhsKey)
    {
-        return strcmp(si1->SpellName[0], si2->SpellName[0]) > 0;
-    }
+        // Defensive check: if DBC data is broken and spell names are nullptr,
+        // fall back to id ordering instead of risking a crash in std::strcmp.
+        if (!lhSpellInfo->SpellName[0] || !rhSpellInfo->SpellName[0])
+            return lhSpell.first < rhSpell.first;

-    return p1 > p2;
+        return std::strcmp(lhSpellInfo->SpellName[0], rhSpellInfo->SpellName[0]) > 0;
+    }
+    return lhsKey > rhsKey;
 }

 std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::string filter)
 {
-    if (skillSpells.empty())
-    {
-        for (uint32 j = 0; j < sSkillLineAbilityStore.GetNumRows(); ++j)
-        {
-            if (SkillLineAbilityEntry const* skillLine = sSkillLineAbilityStore.LookupEntry(j))
-                skillSpells[skillLine->Spell] = skillLine;
-        }
-    }
-
-    if (vendorItems.empty())
-    {
-        QueryResult results = WorldDatabase.Query("SELECT item FROM npc_vendor WHERE maxcount = 0");
-        if (results)
-        {
-            do
-            {
-                Field* fields = results->Fetch();
-                int32 entry = fields[0].Get<int32>();
-                if (entry <= 0)
-                    continue;
-
-                vendorItems.insert(entry);
-            } while (results->NextRow());
-        }
-    }
-
-    std::ostringstream posOut;
-    std::ostringstream negOut;
-
    uint32 skill = 0;

    std::vector<std::string> ss = split(filter, ' ');
@@ -99,13 +52,15 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
        skill = chat->parseSkill(ss[0]);
        if (skill != SKILL_NONE)
        {
-            filter = ss.size() > 1 ? filter = ss[1] : "";
+            filter = ss.size() > 1 ? ss[1] : "";
        }

-        if (ss[0] == "first" && ss[1] == "aid")
+        // Guard access to ss[1]/ss[2] to avoid out-of-bounds
+        // when the player only types "first" without "aid".
+        if (ss[0] == "first" && ss.size() > 1 && ss[1] == "aid")
        {
            skill = SKILL_FIRST_AID;
-            filter = ss.size() > 2 ? filter = ss[2] : "";
+            filter = ss.size() > 2 ? ss[2] : "";
        }
    }

@@ -115,26 +70,57 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::

    uint32 minLevel = 0;
    uint32 maxLevel = 0;
-    if (filter.find("-") != std::string::npos)
+    if (filter.find('-') != std::string::npos)
    {
        std::vector<std::string> ff = split(filter, '-');
-        minLevel = atoi(ff[0].c_str());
-        maxLevel = atoi(ff[1].c_str());
-        filter = "";
+        if (ff.size() >= 2)
+        {
+            minLevel = std::atoi(ff[0].c_str());
+            maxLevel = std::atoi(ff[1].c_str());
+            if (minLevel > maxLevel)
+                std::swap(minLevel, maxLevel);
+        }
+        filter.clear();
    }

-    bool craftableOnly = false;
-    if (filter.find("+") != std::string::npos)
+    bool canCraftNow = false;
+    if (filter.find('+') != std::string::npos)
    {
-        craftableOnly = true;
+        canCraftNow = true;
+
+        // Support "+<skill>" syntax (e.g. "spells +tailoring" or "spells tailoring+").
+        // If no explicit skill was detected yet, try to parse the filter (without '+')
+        // as a profession/skill name so that craftable-only filters still work with skills.
+        if (skill == SKILL_NONE)
+        {
+            std::string skillFilter = filter;
+
+            // Remove '+' before trying to interpret the first token as a skill name.
+            skillFilter.erase(remove(skillFilter.begin(), skillFilter.end(), '+'), skillFilter.end());
+
+            std::vector<std::string> skillTokens = split(skillFilter, ' ');
+            if (!skillTokens.empty())
+            {
+                uint32 parsedSkill = chat->parseSkill(skillTokens[0]);
+                if (parsedSkill != SKILL_NONE)
+                {
+                    skill = parsedSkill;
+
+                    // Any remaining text after the skill token becomes the "name" filter
+                    // (e.g. "spells +tailoring cloth" -> skill = tailoring, filter = "cloth").
+                    filter = skillTokens.size() > 1 ? skillTokens[1] : "";
+                }
+            }
+        }
+        // Finally remove '+' from the filter that will be used for name/range parsing.
        filter.erase(remove(filter.begin(), filter.end(), '+'), filter.end());
    }

    uint32 slot = chat->parseSlot(filter);
    if (slot != EQUIPMENT_SLOT_END)
-        filter = "";
+        filter.clear();

-    std::vector<std::pair<uint32, std::string>> spells;
+    std::vector<SpellListEntry> spells;
    for (PlayerSpellMap::iterator itr = bot->GetSpellMap().begin(); itr != bot->GetSpellMap().end(); ++itr)
    {
        if (itr->second->State == PLAYERSPELL_REMOVED || !itr->second->Active)
@@ -150,7 +136,7 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
        if (spellInfo->IsPassive())
            continue;

-        SkillLineAbilityEntry const* skillLine = skillSpells[itr->first];
+        SkillLineAbilityEntry const* skillLine = sPlayerbotSpellCache->GetSkillLine(itr->first);
        if (skill != SKILL_NONE && (!skillLine || skillLine->SkillLine != skill))
            continue;

@@ -162,7 +148,7 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
            continue;

        bool first = true;
-        int32 craftCount = -1;
+        int32 craftsPossible = -1;
        std::ostringstream materials;
        for (uint32 x = 0; x < MAX_SPELL_REAGENTS; ++x)
        {
@@ -189,12 +175,12 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::

                    FindItemByIdVisitor visitor(itemid);
                    uint32 reagentsInInventory = InventoryAction::GetItemCount(&visitor);
-                    bool buyable = (vendorItems.find(itemid) != vendorItems.end());
+                    bool buyable = sPlayerbotSpellCache->IsItemBuyable(itemid);
                    if (!buyable)
                    {
                        uint32 craftable = reagentsInInventory / reagentsRequired;
-                        if (craftCount < 0 || craftCount > craftable)
-                            craftCount = craftable;
+                        if (craftsPossible < 0 || craftsPossible > static_cast<int32>(craftable))
+                            craftsPossible = static_cast<int32>(craftable);
                    }

                    if (reagentsInInventory)
@@ -205,8 +191,8 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
            }
        }

-        if (craftCount < 0)
-            craftCount = 0;
+        if (craftsPossible < 0)
+            craftsPossible = 0;

        std::ostringstream out;
        bool filtered = false;
@@ -218,8 +204,8 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
                {
                    if (ItemTemplate const* proto = sObjectMgr->GetItemTemplate(spellInfo->Effects[i].ItemType))
                    {
-                        if (craftCount)
-                            out << "|cffffff00(x" << craftCount << ")|r ";
+                        if (craftsPossible)
+                            out << "|cffffff00(x" << craftsPossible << ")|r ";

                        out << chat->FormatItem(proto);

@@ -246,7 +232,7 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
        if (filtered)
            continue;

-        if (craftableOnly && !craftCount)
+        if (canCraftNow && !craftsPossible)
            continue;

        out << materials.str();
@@ -275,10 +261,9 @@ std::vector<std::pair<uint32, std::string>> ListSpellsAction::GetSpellList(std::
            continue;

        if (itr->first == 0)
-        {
            LOG_ERROR("playerbots", "?! {}", itr->first);
-        }
-        spells.push_back(std::pair<uint32, std::string>(itr->first, out.str()));
+
+        spells.emplace_back(itr->first, out.str());
        alreadySeenList += spellInfo->SpellName[0];
        alreadySeenList += ",";
    }
@@ -294,25 +279,28 @@ bool ListSpellsAction::Execute(Event event)

    std::string const filter = event.getParam();

-    std::vector<std::pair<uint32, std::string>> spells = GetSpellList(filter);
+    std::vector<SpellListEntry> spells = GetSpellList(filter);
+
+    if (spells.empty())
+    {
+        // CHANGE: Give early feedback when no spells match the filter.
+        botAI->TellMaster("No spells found.");
+        return true;
+    }

    botAI->TellMaster("=== Spells ===");

    std::sort(spells.begin(), spells.end(), CompareSpells);

-    uint32 count = 0;
-    for (std::vector<std::pair<uint32, std::string>>::iterator i = spells.begin(); i != spells.end(); ++i)
-    {
+    // CHANGE: Send the full spell list again so client-side addons
+    // (e.g. Multibot / Unbot) can reconstruct the
+    // complete spellbook for configuration. The heavy part that caused
+    // freezes before was the old CompareSpells implementation scanning
+    // the entire SkillLineAbility DBC on every comparison. With the new
+    // cheap comparator above, sending all lines here is safe and keeps
+    // behaviour compatible with existing addons.
+    for (std::vector<SpellListEntry>::const_iterator i = spells.begin(); i != spells.end(); ++i)
        botAI->TellMasterNoFacing(i->second);

-        // if (++count >= 50)
-        // {
-        //     std::ostringstream msg;
-        //     msg << (spells.size() - 50) << " more...";
-        //     botAI->TellMasterNoFacing(msg.str());
-        //     break;
-        // }
-    }
-
    return true;
-}
+}
--- a/src/strategy/actions/ListSpellsAction.h
+++ b/src/strategy/actions/ListSpellsAction.h
@@ -18,9 +18,8 @@ public:
    bool Execute(Event event) override;
    virtual std::vector<std::pair<uint32, std::string>> GetSpellList(std::string filter = "");

-private:
-    static std::map<uint32, SkillLineAbilityEntry const*> skillSpells;
-    static std::set<uint32> vendorItems;
+    static void InitSpellCaches();
+
 };

 #endif