Refactor/total architecture #9
Draft
Conversation
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
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
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Pull Request
The purposes of this PR are to (1) establish a general raid helper framework for the benefit of future raid strategies and (2) make some improvements to problematic areas of the raid strategy code.
List of changes:
Added new RaidBossHelpers.cpp and RaidBossHelpers.h files in the Raid folder.
Moved reused helpers from Karazhan, Gruul, and Magtheridon strategies to the new helper files.
Modified the prior function that assigned a DPS bot to store and erase timers and trackers in associative containers--the function now includes parameters for mapId (so a bot that is not in the instance will not be assigned) and for the ability to exclude a bot (useful for excluding particular important roles, such as a Warlock tank, so they are not bogged down by these extra tasks at critical moments). I also renamed it from IsInstanceTimerManager to IsMechanicTrackerBot.
Moved all helper files in raid strategies to Util folders (was needed for ICC, MC, and Ulduar).
Renamed and reordered includes of Ulduar files in AiObjectContext.cpp to match other raid strategies.
a. This initially caused compile errors which made me realize that the existing code had several problems with missing includes and was compiling only due to the prior ordering in AiObjectContext.cpp. Therefore, I added the missing includes to Molten Core, Ulduar, and Vault of Archavon strategies.
b. Ulduar and Old Kingdom were also using the same constant name for a spell--the reordering caused a compile error here as well, which just highlighted an existing problem that was being hidden. I renamed the constant for Ulduar to fix this, but I think the better approach going forward would be to use a namespace or enum class. But that is for another time and probably another person.
Several changes with respect to Ulduar files:
a. The position constants and enums for spells and NPCs and such were in the trigger header file. I did not think that made sense so moved them to existing helper files.
b. Since the strategy does not use multipliers, I removed all files and references to multipliers in it.
c. I removed some unneeded includes. I did not do a detailed review to determine what else could be removed--I just took some out that I could tell right away were not needed.
d. I renamed the ingame strategy name from "uld" to "ulduar," which I think is clearer and is still plenty short.
Partial refactor of Gruul and Magtheridon strategies:
a. I did not due a full refactoring but made some quick changes to things I did previously that were rather stupid like repeating calculations, having useless logic like pointless IsAlive() checks for creatures already on the hostile references list, and not using the existing Position class for coordinates.
b. There were a few substantive changes, such as allowing players to pick Maulgar mage and moonkin tanks with the assistant flag, but a greater refactoring of the strategies themselves is beyond this PR.
c. I was clearing some containers used for Gruul and Magtheridon strategies; the methods are now fixed to erase only the applicable keys so that in the unlikely event that one server has multiple groups running Gruul or Magtheridon at the same time, there won't be timer or position tracker conflicts.
How to Test the Changes
I personally tested Maulgar, Gruul, and Magtheridon and confirmed that they still work as intended.
Complexity & Impact
I do not expect this PR to have any relevant changes to in-game performance, but I will defer to those more knowledgeable than I if there are concerns in this area. As I've mentioned before, you can consider me to be like a person who has taken half an intro C++ course at best.
AI Assistance
None beyond autocomplete of repetitive changes.