Closed
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request establishes a boss helper framework primarily for Ulduar raid encounters, while also refactoring existing raid helper code across multiple raid instances to consolidate common functionality and improve maintainability.
Changes:
- Created a centralized
RaidBossHelpershelper module with common functions (marking targets, setting RTI targets, finding units, etc.) that were previously duplicated across Magtheridon, Karazhan, and Gruul's Lair - Refactored raid helper code to use
Positionstruct instead of customLocationstructs, and made constantsconstexprwhere appropriate - Renamed Ulduar strategy from "uld" to "ulduar" for better clarity
- Fixed a bug where
SPELL_SHADOW_CRASHshould have beenSPELL_VEZAX_SHADOW_CRASH - Removed unnecessary
IsAlive()checks in Magtheridon code whereGetChanneler()already validates this - Removed unused
RaidUlduarMultipliersfiles
Reviewed changes
Copilot reviewed 40 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Bot/PlayerbotAI.cpp | Updated Ulduar strategy name from "uld" to "ulduar" |
| src/Bot/Engine/AiObjectContext.cpp | Moved Ulduar includes to proper subdirectory location |
| src/Ai/Raid/RaidBossHelpers.h/cpp | New centralized helper functions extracted from raid-specific helpers |
| src/Ai/Raid/Ulduar/Util/RaidUlduarBossHelper.h/cpp | Consolidated Ulduar constants and helper class definitions |
| src/Ai/Raid/Ulduar/Util/RaidUlduarScripts.h | New header for Ulduar script includes |
| src/Ai/Raid/Ulduar/Trigger/RaidUlduarTriggers.h/cpp | Moved constants to BossHelper, fixed spell ID bug |
| src/Ai/Raid/Ulduar/Strategy/RaidUlduarStrategy.h/cpp | Renamed strategy and removed unused multipliers |
| src/Ai/Raid/Ulduar/Multiplier/* | Removed unused multiplier files |
| src/Ai/Raid/Ulduar/Action/RaidUlduarActions.cpp | Removed unused includes |
| src/Ai/Raid/RaidStrategyContext.h | Updated strategy name mapping from "uld" to "ulduar" |
| src/Ai/Raid/VaultOfArchavon/* | Added includes for new BossAura helpers |
| src/Ai/Raid/MoltenCore/Util/RaidMcHelpers.h | New helper file with Molten Core constants |
| src/Ai/Raid/MoltenCore/* | Added includes for new BossAura helpers |
| src/Ai/Raid/Magtheridon/Util/RaidMagtheridonHelpers.h/cpp | Refactored to use Position, removed duplicated functions, extracted GetChanneler logic |
| src/Ai/Raid/Magtheridon/Trigger/RaidMagtheridonTriggers.cpp | Removed redundant IsAlive checks, simplified logic |
| src/Ai/Raid/Magtheridon/Multiplier/RaidMagtheridonMultipliers.cpp | Updated to use RaidBossHelpers |
| src/Ai/Raid/Magtheridon/Action/RaidMagtheridonActions.h/cpp | Refactored to use Position and RaidBossHelpers |
| src/Ai/Raid/Karazhan/Util/RaidKarazhanHelpers.h/cpp | Extracted common functions to RaidBossHelpers, made constants constexpr |
| src/Ai/Raid/Karazhan/Trigger/RaidKarazhanTriggers.cpp | Updated to use IsMechanicTrackerBot from RaidBossHelpers |
| src/Ai/Raid/Karazhan/Multiplier/RaidKarazhanMultipliers.cpp | Added include for RaidBossHelpers |
| src/Ai/Raid/Karazhan/Action/RaidKarazhanActions.cpp | Updated to use IsMechanicTrackerBot from RaidBossHelpers |
| src/Ai/Raid/Icecrown/Util/RaidIccScripts.h | New header for ICC script includes |
| src/Ai/Raid/GruulsLair/Util/RaidGruulsLairHelpers.h/cpp | Refactored to use Position, removed duplicated functions, improved tank selection logic |
| src/Ai/Raid/GruulsLair/Trigger/RaidGruulsLairTriggers.h/cpp | Renamed triggers, removed redundant IsAlive checks, updated to use explicit parameter |
| src/Ai/Raid/GruulsLair/Strategy/RaidGruulsLairStrategy.cpp | Updated trigger and action names |
| src/Ai/Raid/GruulsLair/RaidGruulsLairTriggerContext.h | Updated trigger creator names |
| src/Ai/Raid/GruulsLair/RaidGruulsLairActionContext.h | Updated action creator names |
| src/Ai/Raid/GruulsLair/Multiplier/RaidGruulsLairMultipliers.cpp | Refactored to use CastReachTargetSpellAction |
| src/Ai/Raid/GruulsLair/Action/RaidGruulsLairActions.h/cpp | Renamed action classes, refactored to use Position and RaidBossHelpers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Unit* olm = AI_VALUE2(Unit*, "find target", "olm the summoner"); | ||
|
|
||
| return botAI->IsAssistTankOfIndex(bot, 0) && olm && olm->IsAlive(); | ||
| return botAI->IsAssistTankOfIndex(bot, 0, false) && olm; |
There was a problem hiding this comment.
The third parameter false is explicitly passed to IsAssistTankOfIndex in the triggers (lines 20, 27, 54, 55), but lines 448-449 in RaidGruulsLairActions.cpp call the same function without this parameter, relying on the default value instead. For consistency, either both should explicitly pass false or both should rely on the default.
| #include "Playerbots.h" | ||
| #include "ScriptedCreature.h" | ||
|
|
||
| const uint32 ULDUAR_MAP_ID = 603; |
There was a problem hiding this comment.
ULDUAR_MAP_ID should be declared as constexpr for consistency with other map ID constants in the codebase (e.g., MAGTHERIDON_MAP_ID at line 36 in RaidMagtheridonHelpers.h, KARAZHAN_MAP_ID at line 77 in RaidKarazhanHelpers.h, and GRUULS_LAIR_MAP_ID at line 29 in RaidGruulsLairHelpers.h).
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
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
Describe what this change does and why it is needed...
Design Philosophy
We prioritize stability, performance, and predictability over behavioral realism.
Complex player-mimicking logic is intentionally limited due to its negative impact on scalability, maintainability, and
long-term robustness.
Excessive processing overhead can lead to server hiccups, increased CPU usage, and degraded performance for all
participants. Because every action and
decision tree is executed per bot and per trigger, even small increases in logic complexity can scale poorly and
negatively affect both players and
world (random) bots. Bots are not expected to behave perfectly, and perfect simulation of human decision-making is not a
project goal. Increased behavioral
realism often introduces disproportionate cost, reduced predictability, and significantly higher maintenance overhead.
Every additional branch of logic increases long-term responsibility. All decision paths must be tested, validated, and
maintained continuously as the system evolves.
If advanced or AI-intensive behavior is introduced, the default configuration must remain the lightweight decision
model. More complex behavior should only be
available as an explicit opt-in option, clearly documented as having a measurable performance cost.
Principles:
Stability before intelligence
A stable system is always preferred over a smarter one.
Performance is a shared resource
Any increase in bot cost affects all players and all bots.
Simple logic scales better than smart logic
Predictable behavior under load is more valuable than perfect decisions.
Complexity must justify itself
If a feature cannot clearly explain its cost, it should not exist.
Defaults must be cheap
Expensive behavior must always be optional and clearly communicated.
Bots should look reasonable, not perfect
The goal is believable behavior, not human simulation.
Before submitting, confirm that this change aligns with those principles.
Feature Evaluation
Please answer the following:
How to Test the Changes
Complexity & Impact
Does this change add new decision branches?
Does this change increase per-bot or per-tick processing?
Could this logic scale poorly under load?
Defaults & Configuration
If this introduces more advanced or AI-heavy logic:
AI Assistance
If yes, please specify:
AI assistance is allowed, but all submitted code must be fully understood, reviewed, and owned by the contributor.
Any AI-influenced changes must be verified against existing CORE and PB logic. We expect contributors to be honest
about what they do and do not understand.
Final Checklist
Notes for Reviewers
Anything that significantly improves realism at the cost of stability or performance should be carefully discussed
before merging.