From 9f7447ff834fdfd2256b2bec6114157ec801ba4d Mon Sep 17 00:00:00 2001 From: agatho Date: Mon, 19 Jan 2026 05:09:36 +0100 Subject: [PATCH] fix(playerbot): Fix bot spec detection, gear application, and group combat assist Three critical fixes for bot functionality: 1. BaselineRotationManager: Fix specialization detection - GetActiveTalentGroup() returns talent GROUP index (0 or 1), not spec - Changed to GetPrimarySpecialization() == ChrSpecialization::None - Bots with specs now use proper spec-based rotations instead of BASELINE 2. BotPostLoginConfigurator: Fix gear never being applied - shouldApplyGear was always false due to broken condition - Changed to always apply gear, using BotGearFactory as fallback - Added WasRecentlyConfigured() to prevent LevelManager re-leveling race - Fixed ApplyClassSpells() to properly learn specialization spells 3. GroupCombatStrategy: Fix group combat assist detection - ObjectAccessor::FindPlayer() returned NULL for some bot group members - Added FindGroupMember() helper with fallback lookups: * ObjectAccessor::FindPlayer() (in-world) * ObjectAccessor::FindConnectedPlayer() (any connected player) * BotWorldSessionMgr::GetPlayerBot() (module registry) - Bots now properly detect when group members are in combat and assist Co-Authored-By: Claude Opus 4.5 Signed-off-by: luis --- .../AI/ClassAI/BaselineRotationManager.cpp | 7 +- .../AI/Strategy/GroupCombatStrategy.cpp | 45 ++- .../Instance/BotPostLoginConfigurator.cpp | 288 ++++++++++++++---- .../Instance/BotPostLoginConfigurator.h | 19 ++ 4 files changed, 295 insertions(+), 64 deletions(-) diff --git a/src/modules/Playerbot/AI/ClassAI/BaselineRotationManager.cpp b/src/modules/Playerbot/AI/ClassAI/BaselineRotationManager.cpp index 3761aa0c6..4e7499584 100644 --- a/src/modules/Playerbot/AI/ClassAI/BaselineRotationManager.cpp +++ b/src/modules/Playerbot/AI/ClassAI/BaselineRotationManager.cpp @@ -118,9 +118,10 @@ bool BaselineRotationManager::ShouldUseBaselineRotation(Player* bot) // Also use if bot is level 10+ but hasn't chosen a specialization // (This is an edge case that should trigger auto-specialization) - // Check if bot has an active talent group (specialization) - uint8 activeGroup = bot->GetActiveTalentGroup(); - return (level >= 10 && activeGroup == 0); + // FIXED: GetActiveTalentGroup() returns talent GROUP index (0 or 1), not specialization! + // Use GetPrimarySpecialization() to correctly detect if bot has a spec assigned. + ChrSpecialization spec = bot->GetPrimarySpecialization(); + return (level >= 10 && spec == ChrSpecialization::None); } bool BaselineRotationManager::ExecuteBaselineRotation(Player* bot, ::Unit* target) diff --git a/src/modules/Playerbot/AI/Strategy/GroupCombatStrategy.cpp b/src/modules/Playerbot/AI/Strategy/GroupCombatStrategy.cpp index fa4fa537b..92c5e412a 100644 --- a/src/modules/Playerbot/AI/Strategy/GroupCombatStrategy.cpp +++ b/src/modules/Playerbot/AI/Strategy/GroupCombatStrategy.cpp @@ -23,10 +23,33 @@ #include "GridNotifiers.h" #include "CellImpl.h" #include "../../Group/GroupRoleEnums.h" // For IsPlayerHealer +#include "../../Session/BotWorldSessionMgr.h" // For GetPlayerBot fallback namespace Playerbot { +// Helper function to find a player with fallback lookups +// Uses: FindPlayer (in-world) -> FindConnectedPlayer (connected) -> GetPlayerBot (module registry) +static Player* FindGroupMember(ObjectGuid memberGuid) +{ + if (memberGuid.IsEmpty()) + return nullptr; + + // Method 1: Standard ObjectAccessor::FindPlayer (fast, in same map) + if (Player* player = ObjectAccessor::FindPlayer(memberGuid)) + return player; + + // Method 2: FindConnectedPlayer (finds any connected player, even on different maps) + if (Player* player = ObjectAccessor::FindConnectedPlayer(memberGuid)) + return player; + + // Method 3: Check our bot registry (for bots not properly registered with ObjectAccessor) + if (Player* bot = sBotWorldSessionMgr->GetPlayerBot(memberGuid)) + return bot; + + return nullptr; +} + GroupCombatStrategy::GroupCombatStrategy() : Strategy("group_combat") { @@ -129,7 +152,8 @@ void GroupCombatStrategy::UpdateBehavior(BotAI* ai, uint32 diff) Player* tankToFollow = nullptr; for (auto const& slot : group->GetMemberSlots()) { - Player* member = ObjectAccessor::FindPlayer(slot.guid); + // Use FindGroupMember() with fallback lookups + Player* member = FindGroupMember(slot.guid); if (!member || member == bot || !member->IsAlive()) continue; @@ -145,7 +169,8 @@ void GroupCombatStrategy::UpdateBehavior(BotAI* ai, uint32 diff) { for (auto const& slot : group->GetMemberSlots()) { - Player* member = ObjectAccessor::FindPlayer(slot.guid); + // Use FindGroupMember() with fallback lookups + Player* member = FindGroupMember(slot.guid); if (member && member != bot && member->IsAlive() && member->IsInCombat()) { tankToFollow = member; @@ -300,7 +325,7 @@ bool GroupCombatStrategy::IsGroupInCombat(BotAI* ai) const // CRITICAL FIX: Use GetMemberSlots() instead of GetMembers() // GetMembers()/GetSource() only works for players in the same map - // GetMemberSlots() gives us GUIDs which we can use with ObjectAccessor + // GetMemberSlots() gives us GUIDs which we can use with FindGroupMember() for (auto const& slot : group->GetMemberSlots()) { ObjectGuid memberGuid = slot.guid; @@ -312,19 +337,22 @@ bool GroupCombatStrategy::IsGroupInCombat(BotAI* ai) const continue; } - Player* member = ObjectAccessor::FindPlayer(memberGuid); + // CRITICAL FIX: Use FindGroupMember() with fallback lookups + // ObjectAccessor::FindPlayer() alone fails for bots not properly registered + Player* member = FindGroupMember(memberGuid); if (shouldLog) { if (!member) - TC_LOG_ERROR("module.playerbot.strategy", " - NULL member for GUID {}", memberGuid.ToString()); + TC_LOG_ERROR("module.playerbot.strategy", " - NULL member for GUID {} (all lookups failed)", memberGuid.ToString()); else if (member == bot) TC_LOG_ERROR("module.playerbot.strategy", " - {} (is bot - SKIPPING)", member->GetName()); else - TC_LOG_ERROR("module.playerbot.strategy", " - {} InCombat={}, HasTarget={}", + TC_LOG_ERROR("module.playerbot.strategy", " - {} InCombat={}, HasTarget={}, IsBot={}", member->GetName(), member->IsInCombat(), - member->GetSelectedUnit() != nullptr); + member->GetSelectedUnit() != nullptr, + sBotWorldSessionMgr->GetPlayerBot(memberGuid) != nullptr); } if (!member || member == bot) @@ -359,7 +387,8 @@ Unit* GroupCombatStrategy::FindGroupCombatTarget(BotAI* ai) const // 3. What group members have selected (GetSelectedUnit) for (auto const& slot : group->GetMemberSlots()) { - Player* member = ObjectAccessor::FindPlayer(slot.guid); + // Use FindGroupMember() with fallback lookups + Player* member = FindGroupMember(slot.guid); if (!member || member == bot || !member->IsInCombat()) continue; diff --git a/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.cpp b/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.cpp index b33b26e0d..dd258a9d5 100644 --- a/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.cpp +++ b/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.cpp @@ -9,11 +9,13 @@ #include "BotPostLoginConfigurator.h" #include "BotTemplateRepository.h" +#include "Equipment/BotGearFactory.h" #include "LFG/LFGBotManager.h" #include "Player.h" #include "Item.h" #include "Log.h" #include "SpellMgr.h" +#include "SpellInfo.h" #include "ObjectMgr.h" #include "DB2Stores.h" #include "World.h" @@ -61,6 +63,7 @@ void BotPostLoginConfigurator::Shutdown() { std::lock_guard lock(_configMutex); _pendingConfigs.clear(); + _recentlyConfiguredBots.clear(); } _initialized.store(false); @@ -129,6 +132,34 @@ void BotPostLoginConfigurator::RemovePendingConfiguration(ObjectGuid botGuid) } } +bool BotPostLoginConfigurator::WasRecentlyConfigured(ObjectGuid botGuid) const +{ + std::lock_guard lock(_configMutex); + bool found = _recentlyConfiguredBots.find(botGuid) != _recentlyConfiguredBots.end(); + + if (found) + { + TC_LOG_INFO("module.playerbot.configurator", + "WasRecentlyConfigured: GUID={} -> YES (in recently configured set)", + botGuid.ToString()); + } + + return found; +} + +void BotPostLoginConfigurator::ClearRecentlyConfigured(ObjectGuid botGuid) +{ + std::lock_guard lock(_configMutex); + auto erased = _recentlyConfiguredBots.erase(botGuid); + + if (erased > 0) + { + TC_LOG_INFO("module.playerbot.configurator", + "ClearRecentlyConfigured: Removed GUID={} from recently configured set (remaining: {})", + botGuid.ToString(), _recentlyConfiguredBots.size()); + } +} + // ============================================================================ // CONFIGURATION APPLICATION // ============================================================================ @@ -174,20 +205,26 @@ bool BotPostLoginConfigurator::ApplyPendingConfiguration(Player* player) bool success = true; // Get template if not cached + // NOTE: Template is OPTIONAL - warm pool bots may not have a template + // In that case, we still apply level, spec, and use BotGearFactory for equipment BotTemplate const* tmpl = config.templatePtr; if (!tmpl && config.templateId > 0) { tmpl = sBotTemplateRepository->GetTemplateById(config.templateId); + if (!tmpl) + { + TC_LOG_WARN("module.playerbot.configurator", + "Template {} not found for bot {} - will use BotGearFactory fallback", + config.templateId, player->GetName()); + } } + // If no template is available, log info and continue with fallback behavior if (!tmpl) { - TC_LOG_ERROR("module.playerbot.configurator", - "Failed to get template {} for bot {}", - config.templateId, player->GetName()); - _stats.failedConfigs.fetch_add(1); - RemovePendingConfiguration(playerGuid); - return false; + TC_LOG_INFO("module.playerbot.configurator", + "No template for bot {} (templateId={}) - using BotGearFactory for equipment", + player->GetName(), config.templateId); } // Step 1: Apply Level @@ -207,7 +244,8 @@ bool BotPostLoginConfigurator::ApplyPendingConfiguration(Player* player) } // Step 2: Apply Specialization (must be before talents) - uint32 specId = config.specId > 0 ? config.specId : tmpl->specId; + // Use config.specId if set, otherwise fallback to template spec (if available) + uint32 specId = config.specId > 0 ? config.specId : (tmpl ? tmpl->specId : 0); if (specId > 0) { TC_LOG_INFO("module.playerbot.configurator", @@ -230,8 +268,8 @@ bool BotPostLoginConfigurator::ApplyPendingConfiguration(Player* player) ApplyClassSpells(player); - // Step 4: Apply Talents - if (!tmpl->talents.talentIds.empty()) + // Step 4: Apply Talents (only if template exists with talents) + if (tmpl && !tmpl->talents.talentIds.empty()) { TC_LOG_INFO("module.playerbot.configurator", "Applying {} talents to bot {}", @@ -247,11 +285,22 @@ bool BotPostLoginConfigurator::ApplyPendingConfiguration(Player* player) } // Step 5: Apply Gear - if (config.targetGearScore > 0 || !tmpl->gearSets.empty()) + // CRITICAL FIX: Always apply gear for instance bots + // BotGearFactory will handle gear generation when template is null/empty/has no gear + // Previous condition was broken because: + // - targetGearScore=0 (templates set this to 0) + // - hasTemplateGear=false (no gear sets in database) + // - tmpl != nullptr (templates exist) + // Result: shouldApplyGear was always false, bots had no gear! + bool hasTemplateGear = tmpl && !tmpl->gearSets.empty(); + // ALWAYS apply gear - if no template gear exists, BotGearFactory will generate appropriate gear + bool shouldApplyGear = true; // Always equip gear for instance bots + + if (shouldApplyGear) { TC_LOG_INFO("module.playerbot.configurator", - "Applying gear (target GS: {}) to bot {}", - config.targetGearScore, player->GetName()); + "Applying gear to bot {} (targetGS={}, hasTemplate={}, hasTemplateGear={})", + player->GetName(), config.targetGearScore, tmpl != nullptr, hasTemplateGear); if (!ApplyGear(player, tmpl, config.targetGearScore)) { @@ -262,8 +311,8 @@ bool BotPostLoginConfigurator::ApplyPendingConfiguration(Player* player) } } - // Step 6: Apply Action Bars - if (!tmpl->actionBars.buttons.empty()) + // Step 6: Apply Action Bars (only if template exists with action bars) + if (tmpl && !tmpl->actionBars.buttons.empty()) { TC_LOG_INFO("module.playerbot.configurator", "Applying {} action buttons to bot {}", @@ -336,6 +385,19 @@ bool BotPostLoginConfigurator::ApplyPendingConfiguration(Player* player) player->GetName(), durationMs); } + // CRITICAL: Add to recently configured set BEFORE removing pending config + // This prevents the race condition where: + // 1. We remove pending config + // 2. BotWorldSessionMgr checks HasPendingConfiguration() - returns FALSE + // 3. Bot gets submitted to BotLevelManager which re-levels it + { + std::lock_guard lock(_configMutex); + _recentlyConfiguredBots.insert(playerGuid); + TC_LOG_INFO("module.playerbot.configurator", + "Added bot {} to recently configured set (size: {})", + player->GetName(), _recentlyConfiguredBots.size()); + } + // Remove pending configuration RemovePendingConfiguration(playerGuid); @@ -553,47 +615,105 @@ bool BotPostLoginConfigurator::LearnTalent(Player* player, uint32 talentId) bool BotPostLoginConfigurator::ApplyGear(Player* player, BotTemplate const* tmpl, uint32 targetGearScore) { - if (!player || !tmpl) + if (!player) return false; - // Select best gear set for target - GearSetTemplate const* gearSet = SelectGearSet(tmpl, targetGearScore); - if (!gearSet) + // First, try to use template gear set if available and has valid items + bool useTemplateGear = false; + GearSetTemplate const* gearSet = nullptr; + + if (tmpl) + { + gearSet = SelectGearSet(tmpl, targetGearScore); + if (gearSet) + { + // Check if template has any valid item IDs (non-zero) + for (auto const& slotData : gearSet->slots) + { + if (slotData.itemId != 0) + { + useTemplateGear = true; + break; + } + } + } + } + + // If template has valid items, use them + if (useTemplateGear && gearSet) { TC_LOG_INFO("module.playerbot.configurator", - "No gear set found for bot {} (target GS: {})", - player->GetName(), targetGearScore); + "Using template gear set iLvl {} (actual GS: {}) for bot {}", + gearSet->targetItemLevel, gearSet->actualGearScore, player->GetName()); + + uint32 itemsEquipped = 0; + uint32 itemsFailed = 0; + + for (uint8 slot = 0; slot < EQUIPMENT_SLOT_END; ++slot) + { + if (slot >= gearSet->slots.size()) + break; + + GearSlotTemplate const& slotData = gearSet->slots[slot]; + if (slotData.itemId == 0) + continue; + + if (EquipItem(player, slot, slotData.itemId)) + ++itemsEquipped; + else + ++itemsFailed; + } + + TC_LOG_INFO("module.playerbot.configurator", + "Applied template gear to bot {}: {} equipped, {} failed", + player->GetName(), itemsEquipped, itemsFailed); + + return itemsFailed == 0; + } + + // FALLBACK: Use BotGearFactory to generate and apply gear dynamically + // This handles cases where: + // 1. Template has no gear sets + // 2. Template gear sets have placeholder items (itemId = 0) + // 3. No template was provided + TC_LOG_INFO("module.playerbot.configurator", + "Template has no valid gear - using BotGearFactory for bot {} (level {}, class {}, spec {})", + player->GetName(), player->GetLevel(), player->GetClass(), + static_cast(player->GetPrimarySpecialization())); + + if (!sBotGearFactory->IsReady()) + { + TC_LOG_WARN("module.playerbot.configurator", + "BotGearFactory not ready - cannot generate gear for bot {}", + player->GetName()); + return false; + } + + // Determine faction + TeamId faction = player->GetTeamId(); + + // Build gear set using BotGearFactory + GearSet generatedGear = sBotGearFactory->BuildGearSet( + player->GetClass(), + static_cast(player->GetPrimarySpecialization()), + player->GetLevel(), + faction + ); + + // Apply the generated gear set + if (!sBotGearFactory->ApplyGearSet(player, generatedGear)) + { + TC_LOG_WARN("module.playerbot.configurator", + "BotGearFactory failed to apply gear to bot {}", + player->GetName()); return false; } TC_LOG_INFO("module.playerbot.configurator", - "Selected gear set iLvl {} (actual GS: {}) for bot {}", - gearSet->targetItemLevel, gearSet->actualGearScore, player->GetName()); + "BotGearFactory successfully equipped bot {} with generated gear", + player->GetName()); - uint32 itemsEquipped = 0; - uint32 itemsFailed = 0; - - // Equipment slots (0-18) - for (uint8 slot = 0; slot < EQUIPMENT_SLOT_END; ++slot) - { - if (slot >= gearSet->slots.size()) - break; - - GearSlotTemplate const& slotData = gearSet->slots[slot]; - if (slotData.itemId == 0) - continue; - - if (EquipItem(player, slot, slotData.itemId)) - ++itemsEquipped; - else - ++itemsFailed; - } - - TC_LOG_INFO("module.playerbot.configurator", - "Applied gear to bot {}: {} equipped, {} failed", - player->GetName(), itemsEquipped, itemsFailed); - - return itemsFailed == 0; + return true; } bool BotPostLoginConfigurator::EquipItem(Player* player, uint8 slot, uint32 itemId) @@ -669,16 +789,78 @@ bool BotPostLoginConfigurator::ApplyClassSpells(Player* player) player->LearnDefaultSkills(); player->UpdateSkillsForLevel(); - // Learn specialization spells if spec is set - // NOTE: SKIPPING LearnSpecializationSpells() - it calls LearnSpell() which - // sends packets via SendDirectMessage() and crashes for bots without proper session. - // The bot AI will learn necessary spells when it initializes. + // ======================================================================== + // SPECIALIZATION SPELL LEARNING + // ======================================================================== + // Use standard TrinityCore method when possible: + // - If NOT in world: LearnSpecializationSpells() won't send packets (IsInWorld check) + // - If IN world: Use AddSpell directly to avoid log spam from failed packet sends + // (Bot sessions have no socket, so SendDirectMessage logs errors but doesn't crash) + // ======================================================================== if (player->GetPrimarySpecialization() != ChrSpecialization::None) + { + if (!player->IsInWorld()) + { + // Standard TrinityCore path - safe because packets won't be sent + player->LearnSpecializationSpells(); + + TC_LOG_INFO("module.playerbot.configurator", + "ApplyClassSpells: Bot {} learned specialization spells via standard method (not in world)", + player->GetName()); + } + else + { + // Bot is already in world - use AddSpell directly to avoid log spam + uint32 specId = static_cast(player->GetPrimarySpecialization()); + uint32 spellsLearned = 0; + + if (std::vector const* specSpells = sDB2Manager.GetSpecializationSpells(specId)) + { + for (SpecializationSpellsEntry const* specSpell : *specSpells) + { + if (!specSpell) + continue; + + SpellInfo const* spellInfo = sSpellMgr->GetSpellInfo(specSpell->SpellID, DIFFICULTY_NONE); + if (!spellInfo || spellInfo->SpellLevel > player->GetLevel()) + continue; + + if (player->AddSpell(specSpell->SpellID, true, true, false, false, false, 0, false, {})) + { + ++spellsLearned; + if (specSpell->OverridesSpellID) + player->AddOverrideSpell(specSpell->OverridesSpellID, specSpell->SpellID); + } + } + } + + // Learn mastery spells + if (player->CanUseMastery()) + { + ChrSpecializationEntry const* spec = sChrSpecializationStore.LookupEntry(specId); + if (spec) + { + for (uint32 i = 0; i < MAX_MASTERY_SPELLS; ++i) + { + if (uint32 masterySpellId = spec->MasterySpellID[i]) + { + if (player->AddSpell(masterySpellId, true, true, false, false, false, 0, false, {})) + ++spellsLearned; + } + } + } + } + + TC_LOG_INFO("module.playerbot.configurator", + "ApplyClassSpells: Bot {} (spec={}) learned {} spells via AddSpell (already in world)", + player->GetName(), specId, spellsLearned); + } + } + else { TC_LOG_INFO("module.playerbot.configurator", - "ApplyClassSpells: SKIPPING LearnSpecializationSpells() for bot {} (spec={}) - crashes with SendDirectMessage", - player->GetName(), static_cast(player->GetPrimarySpecialization())); - // player->LearnSpecializationSpells(); // DISABLED - causes crash + "ApplyClassSpells: Bot {} has no specialization set - skipping spec spells", + player->GetName()); } TC_LOG_INFO("module.playerbot.configurator", diff --git a/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.h b/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.h index 3cf01b60a..affaab063 100644 --- a/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.h +++ b/src/modules/Playerbot/Lifecycle/Instance/BotPostLoginConfigurator.h @@ -14,6 +14,7 @@ #include "BotTemplateRepository.h" #include #include +#include #include #include #include @@ -117,6 +118,15 @@ public: /// Remove pending configuration (called after successful application) void RemovePendingConfiguration(ObjectGuid botGuid); + /// Check if a bot was recently JIT-configured (prevents LevelManager re-leveling) + /// This returns true for bots that have been configured but not yet processed by + /// the BotWorldSessionMgr session update loop. This prevents the race condition where + /// pending config is removed before the skip check runs. + bool WasRecentlyConfigured(ObjectGuid botGuid) const; + + /// Clear a bot from the recently configured list (called after bot is fully processed) + void ClearRecentlyConfigured(ObjectGuid botGuid); + // ======================================================================== // CONFIGURATION APPLICATION (Main thread only, called from BotSession) // ======================================================================== @@ -201,6 +211,15 @@ private: mutable std::mutex _configMutex; std::unordered_map _pendingConfigs; + // Recently configured bots - prevents BotLevelManager from re-leveling JIT bots + // This tracks bots that have been configured but not yet processed by the + // BotWorldSessionMgr session update loop. The race condition is: + // 1. ApplyPendingConfiguration() applies level, removes pending config + // 2. BotWorldSessionMgr.cpp checks HasPendingConfiguration() - returns FALSE + // 3. Bot gets submitted to BotLevelManager which re-levels it + // Solution: Track in _recentlyConfiguredBots until cleared by the session manager + std::unordered_set _recentlyConfiguredBots; + // Statistics ConfiguratorStats _stats;