Closed
Conversation
Phase 0-2 of total refactoring plan: - Add RefactorFlags.h for feature toggles - Create service interfaces (IConfigProvider, IBotContext, ISpellService, IChatService, IRoleService, IItemService) - Create BotServiceContainer for dependency injection - Create BotRoleService as first extracted service - Create ConfigProvider wrapping sPlayerbotAIConfig - Update PlayerbotAIAware with DI constructor - Add SetTestInstance() to PlayerbotAIConfig for testing - Add Google Test infrastructure with mocks and fixtures
Phase 3 of total refactoring plan: - Create BotSpellService implementing ISpellService - Create BotChatService implementing IChatService - Create BotItemService implementing IItemService - Create BotContext implementing IBotContext - Add MockBotServices.h with mocks for all services All services delegate to PlayerbotAI during transition period, allowing gradual migration of callers.
Phase 4 of total refactoring plan: - Create IPacketHandler interface for packet processing - Create IChatCommandHandler interface for command handling - Create PacketHandler implementing IPacketHandler - Create ChatCommandHandler implementing IChatCommandHandler Handlers delegate to PlayerbotAI during transition period.
Phase 5 of total refactoring plan: - Add BotServiceContainer member to PlayerbotAI - Initialize all services in PlayerbotAI constructor - Add GetServices() accessor for service access - Services initialized: Context, Spell, Chat, Role, Item, Config PlayerbotAI now has access to the service-based architecture. Callers can gradually migrate to use GetServices() instead of direct method calls.
Phase 6 of total refactoring plan: - Create ITravelManager interface for navigation operations - Create IRandomBotManager interface for random bot lifecycle - Create IBotRepository interface for data persistence - Create ManagerRegistry for centralized manager access - Create adapter implementations wrapping existing singletons: - TravelManagerAdapter - RandomBotManagerAdapter - BotRepositoryAdapter - Add MockManagers.h with mocks for all manager interfaces Managers can now be accessed via ManagerRegistry::Instance() or injected for testing.
Add unit test implementations for Phase 7: - ConfigProviderTest: mock configuration provider tests - RoleServiceTest: role detection mocking patterns - ManagerRegistryTest: manager registry with mock managers - HealthThresholdTest: health threshold logic patterns Update CMakeLists.txt with new test source files and include paths.
Add high-priority unit tests for core AI patterns: - TriggerLogicTest: threshold, range, cooldown, and priority patterns - ValueLogicTest: distance, percentage, threat, item, and mana efficiency - ActionLogicTest: preconditions, results, queue, timing, and movement These tests validate logic patterns without game object dependencies.
Add complete test coverage for: Engine tests: - StrategyLogicTest: activation, conflicts, requirements, state management - EngineTickTest: relevance, timing, execution queue - MultiplierTest: priority calculations, situational bonuses - StateTransitionTest: state machine, substates, events Combat tests: - TargetSelectionTest: enemy prioritization, heal target selection, AoE - SpellRotationTest: spell selection, rotation priority, GCD timing, procs - GroupCoordinationTest: tank threat, healer triage, DPS assist, roles - BuffManagementTest: self/group buffs, priority, debuff tracking - CrowdControlTest: CC target selection, break avoidance, DR tracking Movement tests: - PositioningTest: formations, combat positioning, AoE avoidance, spread/stack Resource tests: - ResourceManagementTest: potions, special items, mana conservation, food/drink Pet tests: - PetManagementTest: stances, abilities, positioning, health management Dungeon tests: - DungeonTacticsTest: boss mechanics, phases, pull management Integration tests: - ActionChainTest: trigger->action, trigger->multiplier->action, strategy stacking
Phase 1 of service migration: Move all role detection and management methods from PlayerbotAI to BotRoleService. Migrated methods (22 total): - Basic role: IsTank, IsHeal, IsDps, IsRanged, IsMelee, IsCaster, IsRangedDps, IsCombo - Tank hierarchy: IsMainTank, IsBotMainTank, IsAssistTank, IsAssistTankOfIndex - Group queries: GetGroupTankNum, GetAssistTankIndex, GetGroupSlotIndex - Index methods: GetRangedIndex, GetRangedDpsIndex, GetMeleeIndex, GetClassIndex - Assistant detection: IsHealAssistantOfIndex, IsRangedDpsAssistantOfIndex - Aggro: HasAggro Changes: - BotRoleService now has real implementations (not delegation facades) - Static methods available via BotRoleService::*Static() for direct access - Instance methods implement IRoleService interface for testability - All ~600 callers across 96 files updated to use new service - Role method declarations/implementations removed from PlayerbotAI
Phase 2 of service migration: Update all callers to use BotSpellService instead of calling PlayerbotAI spell methods directly. Updated method calls (463 total across 68 files): - CastSpell, CanCastSpell (138 calls) - HasAura, GetAura, HasAnyAuraOf (242 calls) - HasAuraToDispel, CanDispel, IsInterruptableSpellCasting - InterruptSpell, SpellInterrupted, WaitForSpellCast - CalculateGlobalCooldown, RemoveAura, RemoveShapeshift - CanCastVehicleSpell, CastVehicleSpell, IsInVehicle The service currently delegates to PlayerbotAI methods. The actual implementation migration can be done incrementally in a future refactor.
Phase 3 of service migration: Update all callers to use BotChatService instead of calling PlayerbotAI chat methods directly. Updated method calls (475 total across 112 files): - TellMaster (247 calls) - TellError (120 calls) - TellMasterNoFacing (91 calls) - PlaySound, Ping, PlayEmote, Say, Whisper (17 calls) The service currently delegates to PlayerbotAI methods.
Update 35 callers across 15 files to use service pattern: - FindPoison, FindAmmo, FindBandage, FindConsumable - FindStoneFor, FindOilFor, FindOpenableItem, FindLockedItem - GetInventoryAndEquippedItems, GetInventoryItems - HasItemInInventory, GetInventoryItemsCountWithId - ImbueItem, EnchantItem - GetEquipGearScore, GetCurrentQuestsRequiringItemId Changed from: botAI->Method() Changed to: botAI->GetServices().GetItemService().Method()
- Add AzerothCore type stubs (AcoreTypes.h, Common.h, ObjectGuid.h) - Fix CMake include paths to resolve mocks/ prefix - Add force-include for TestPrelude.h with common types - Fix MockSpellService to handle unmockable variadic HasAnyAuraOf - Update RoleServiceTest to use 2-arg role methods - Add .gitignore for build directory 331 of 344 tests now pass (96% pass rate).
- Fix mutual exclusion test: add bidirectional pvp/pve conflicts - Fix action queue test: use InProgress instead of Success as default status - Fix spell rotation priorities: balance cooldown/finisher/builder bonuses - Fix formation positioning test: correct rotated position expectations - Fix combat positioning test: place tank in front of boss correctly - Fix AoE avoidance test: adjust positions to avoid overlapping zones - Fix pet positioning test: correct expectation for pet position - Fix line-of-sight mechanic: use proper point-to-line distance algorithm All 344 tests now pass.
Fixes multiple issues introduced during the service architecture migration: - Fix corrupted #include statements in 9 files where automated refactoring broke the syntax (merged includes on single lines) - Fix PlayerbotSecurityLevel enum mismatch (enum class vs enum, uint8 vs uint32) between IChatService.h and PlayerbotSecurity.h - Fix BotState enum mismatch by adding explicit underlying type - Include interface headers in BotServiceContainer.h to resolve incomplete type errors with unique_ptr members - Add default values to IChatService methods (TellMaster, TellError, TellMasterNoFacing) to match original PlayerbotAI signatures - Fix method name typos: IsAssistTankStaticOfIndex -> IsAssistTankOfIndexStatic, IsHealStaticAssistantOfIndex -> IsHealAssistantOfIndexStatic - Add optional Player* parameter to IItemService::GetEquipGearScore - Add missing #include <vector> to IBotContext.h - Add missing Player forward declaration to IItemService.h - Add missing CastVehicleSpell(spellId, x, y, z) implementation
This fixture provides the minimum enum definitions needed for unit testing without requiring the full AzerothCore context.
Upstream commits merged: - 378254a: Fix Assistant Assignment Functions (mod-playerbots#1930) - 3d467ce: Defense checks for isHostile/unit/target (mod-playerbots#2056) - 3e21563: Create FlightMasterCache (mod-playerbots#1979) - bf456ee: Bear form for low-level cat strat (mod-playerbots#2041) - a5bd0b9: Crash fixes (mod-playerbots#2052) - 34bab48: Config comment fix (mod-playerbots#2045) Key changes: - Renamed IsHealAssistantOfIndex -> IsAssistHealOfIndex - Renamed IsRangedDpsAssistantOfIndex -> IsAssistRangedDpsOfIndex - Added ignoreDeadPlayers parameter to assistant index methods - Implemented two-pass algorithm (assistants first, then non-assistants) - Added FlightMasterCache class for flight master lookups - Added defense checks for IsInWorld/IsDuringRemoveFromWorld - Added crash fixes for UpdateAIInternal and IsBotMainTankStatic - Added bear form fallback for low-level cat druids - Fixed typo in config comment Tests: - Added RoleAssistantIndexTest with 4 new test cases - All 348 tests pass
Solve these two problems mod-playerbots#2043 mod-playerbots#1981 @Regrad is the main contributor of the code, while I was just helping to submit the pull request. Express my gratitude to him. After testing, the code is proven to be effective.
Initialize ManagerRegistry at startup in Playerbots.cpp with: - TravelManagerAdapter - RandomBotManagerAdapter - BotRepositoryAdapter Migrate ~125 call sites from direct singleton access to use sManagerRegistry.Get*() methods: - sRandomPlayerbotMgr->IsRandomBot -> sManagerRegistry.GetRandomBotManager().IsRandomBot - sRandomPlayerbotMgr->GetValue/SetValue -> sManagerRegistry.GetRandomBotManager().* - sRandomPlayerbotMgr->GetBuyMultiplier/GetSellMultiplier -> sManagerRegistry.GetRandomBotManager().* - sPlayerbotRepository->Save/Load/Reset -> sManagerRegistry.GetBotRepository().* Files migrated include action files, triggers, factory, and core bot files. Note: Some sRandomPlayerbotMgr methods not yet in IRandomBotManager interface (BattlegroundData, IsAddclassBot, etc.) still use direct access. Also adds unit tests for ManagerRegistry mock injection patterns and updates ARCHITECTURE.md to document the completed migration.
- BotItemService: Add comprehensive item management methods - BotSpellService: Add spell casting and buff management methods - BotChatService: Extend chat interface with additional helpers - Action.h: Add service accessor convenience methods - Fix various Value class includes and method signatures
- Add copy assignment operators to fix -Wdeprecated-copy warnings (NextAction, PositionInfo, CraftData, UnitPosition) - Fix member initialization order to match declaration order - Fix sign comparison warnings in loop counters - Remove pessimizing std::move() calls - Add [[maybe_unused]] to intentionally unused parameters - Update ARCHITECTURE.md with current statistics: - 333 files changed, +18,112/-2,516 lines, 28 commits - BotItemService expanded to ~780 lines - Module now compiles warning-free
Remove dead code assigning to undeclared BracketSize and TeamSize variables. Replace undeclared teamId with bot->GetTeamId() to match existing usage pattern in the same function.
Fix code style violation for multiple consecutive blank lines.
Fix cppcheck uninitvar error by initializing whispers, currentChat, hasStrategy, and hasRealPlayerMaster when _botAI is valid.
Apply fixes for 500+ coding standard violations: - Const placement: Use `Type const*` instead of `const Type*` - Reference style: Use `Type&` instead of `Type &` - Brace placement: Opening braces on new lines for control statements - Magic numbers: Replace with named constants (SPELL_, NPC_, ITEM_ prefixes) - Hungarian notation: Remove m_ prefixes from member variables - Switch statements: Add missing break before default case Also removes unused variables flagged by compiler warnings: - isRanged, MAX_HEIGHT_DIFF, ICON_NAMES in RaidIccActions.cpp - closestUpgrade in RandomItemMgr.cpp
Add safety checks for empty faces/hairs/facialHairTypes vectors in CreateRandomBot() before accessing them. When sCharSectionsStore lacks data for a race/gender combination, vector.size() - 1 underflows causing an out-of-bounds crash during random bot creation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.