Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a configurable combat delay feature for bots to prevent immediate engagement in combat. The delay applies only to PvE targets when the "combat delay" strategy is active, allowing for more natural bot behavior.
Changes:
- Added a new
CombatDelayStrategythat can be enabled/disabled per bot - Implemented combat delay logic in
AttackAction::Attack()with a static map to track when targets were first seen - Added configuration option
AiPlayerbot.CombatDelaywith default value of 3000ms
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/strategy/generic/CombatDelayStrategy.h | New strategy class definition for combat delay feature |
| src/strategy/generic/CombatDelayStrategy.cpp | Minimal implementation file for the strategy |
| src/strategy/actions/AttackAction.h | Added skipCombatDelay parameter to Attack method |
| src/strategy/actions/AttackAction.cpp | Implemented combat delay logic with target tracking and cleanup mechanism |
| src/strategy/StrategyContext.h | Registered combat delay strategy in the strategy context |
| src/PlayerbotAIConfig.h | Added combatDelay configuration variable |
| src/PlayerbotAIConfig.cpp | Read combat delay value from config file |
| conf/playerbots.conf.dist | Added configuration documentation and default value |
Comments suppressed due to low confidence (1)
src/strategy/actions/AttackAction.cpp:1
- The
AttackRtiTargetAction::Executemethod callsAttack()with only one argument, which will use the default values forwith_pet=trueandskipCombatDelay=false. This means RTI (raid target icon) attacks will be subject to combat delay, which is likely unintended for explicit player-directed attacks. Update this call to passtrueforskipCombatDelayto match the behavior ofAttackMyTargetActionandAttackDuelOpponentAction.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool isPvPTarget = target->IsPlayer() || (target->IsPet() && target->GetOwner() && target->GetOwner()->IsPlayer()); | ||
| if (!skipCombatDelay && botAI->HasStrategy("combat delay", BOT_STATE_NON_COMBAT) && sPlayerbotAIConfig->combatDelay > 0 && !sameTarget && !inCombat && !isPvPTarget) | ||
| { | ||
| static std::unordered_map<ObjectGuid, uint32> targetFirstSeenTime; |
There was a problem hiding this comment.
The static map targetFirstSeenTime is shared across all bots and accessed without thread synchronization, which could lead to race conditions if multiple threads execute attacks concurrently. Similar patterns in this codebase (e.g., WarlockActions.cpp) use mutex protection with std::lock_guard. Add a static mutex and protect all accesses to targetFirstSeenTime.
| # ReturnDelay has a minimum value of 2000 - lower values will cause a crash! | ||
| AiPlayerbot.ReturnDelay = 2000 | ||
| AiPlayerbot.LootDelay = 1000 | ||
| # Default delay in milliseconds before bots engage in combat (0 = no delay) |
There was a problem hiding this comment.
The default value of 3000ms differs from the documented default in the code (0 = no delay). The config comment states 'Default delay in milliseconds before bots engage in combat (0 = no delay)' but sets it to 3000. This inconsistency is confusing. Either change the default to 0 to match the documentation, or update the comment to clarify that 3000ms is the recommended default.
| # Default delay in milliseconds before bots engage in combat (0 = no delay) | |
| # Delay in milliseconds before bots engage in combat (default: 3000 ms; set to 0 for no delay) |
| for (auto it = targetFirstSeenTime.begin(); it != targetFirstSeenTime.end();) | ||
| { | ||
| if (currentTime - it->second > sPlayerbotAIConfig->combatDelay + 10000) | ||
| it = targetFirstSeenTime.erase(it); | ||
| else | ||
| ++it; | ||
| } |
There was a problem hiding this comment.
The cleanup loop iterates through the entire map on every attack when combat delay is active, which could become a performance issue with many tracked targets. Consider implementing periodic cleanup (e.g., only when map size exceeds a threshold or using a time-based check) rather than on every attack.
|
|
||
| #include "Playerbots.h" |
There was a problem hiding this comment.
The include of Playerbots.h appears unused in this file. The CombatDelayStrategy class doesn't require anything from Playerbots.h as it's a minimal strategy with no implementation beyond the header. Consider removing this include to reduce unnecessary dependencies.
| #include "Playerbots.h" |
As requested Poll ``` 1. Yes 2. Yes 3. Maybe, but yes ``` --------- Co-authored-by: Celandriel <22352763+Celandriel@users.noreply.github.com>
- Combat delay now only applies to PvE targets (NPCs, mobs) - Players and player-owned pets attack immediately without delay - NPC pets still subject to combat delay
- Created CombatDelayStrategy that can be toggled per-bot - Players can now enable/disable with `.bot strategy add/remove combat delay` - Config option remains as default delay value in milliseconds - Strategy must be active for delay to apply - Allows per-bot control instead of global-only setting Benefits: - Individual bot control (some bots can have delay, others don't) - Runtime toggling without config changes - More flexible than global config option - Follows existing playerbot patterns
- Add skipCombatDelay parameter to Attack method - Bypass combat delay when player explicitly commands attack - Apply to attack my target and duel opponent commands - cleanup logic now time-based instead of size-based
jads147
force-pushed
the
feature/configurable-combat-delay
branch
from
4f834ac to
bb4e5d6
Compare
3 participants
No description provided.