Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new optional “cautious” bot strategy that attempts to prevent bots from pathing into nearby hostile aggro ranges, with an optional config flag to enable it by default for bots.
Changes:
- Introduces
CautiousStrategyand registers it inStrategyContextso it can be enabled as a named strategy. - Adds
AiPlayerbot.Cautiousconfig support (cautiousDefault) and applies the strategy by default viaAiFactorywhen enabled. - Extends movement logic to detect “aggro positions” and block/limit movement when the cautious strategy is active.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/Bots/playerbot/strategy/generic/CautiousStrategy.h |
Defines the new cautious strategy flag. |
src/modules/Bots/playerbot/strategy/actions/PositionAction.cpp |
Warns on positioning near hostiles (when cautious is active) and routes manual moves through the new MoveTo signature. |
src/modules/Bots/playerbot/strategy/actions/MovementActions.h |
Updates MoveTo signature and exposes aggro-check helpers. |
src/modules/Bots/playerbot/strategy/actions/MovementActions.cpp |
Implements aggro-zone checks and applies them to movement/follow behavior. |
src/modules/Bots/playerbot/strategy/StrategyContext.h |
Registers cautious strategy in the strategy factory. |
src/modules/Bots/playerbot/aiplayerbot.conf.dist.in |
Documents the new AiPlayerbot.Cautious config option. |
src/modules/Bots/playerbot/PlayerbotAIConfig.h |
Adds cautiousDefault config field. |
src/modules/Bots/playerbot/PlayerbotAIConfig.cpp |
Initializes/loads AiPlayerbot.Cautious. |
src/modules/Bots/playerbot/PlayerbotAI.h |
Adds a convenience overload for HasStrategy(name) using currentState. |
src/modules/Bots/playerbot/AiFactory.cpp |
Auto-applies cautious to combat/non-combat engines when configured. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return false; | ||
|
|
There was a problem hiding this comment.
In cautious mode this hard-blocks movement when the destination would enter a creature aggro radius. Since many callers use the mapId-based MoveTo directly (or via MoveNear), this can cause actions to fail with no fallback and the bot to stop rather than “move as close as possible” to the requested point. Consider clamping (x,y) to the nearest aggro-free point (similar to the Unit* overload) instead of returning false outright, or providing a safe-move variant that callers can opt into.
| return false; | |
| { | |
| // In cautious mode, try to clamp to the nearest aggro-free point instead of failing outright. | |
| float botX = bot->GetPositionX(); | |
| float botY = bot->GetPositionY(); | |
| float dirX = x - botX; | |
| float dirY = y - botY; | |
| float lenSq = dirX * dirX + dirY * dirY; | |
| if (lenSq <= 0.0f) | |
| { | |
| // Already at the target or direction is undefined; no safe adjustment possible. | |
| return false; | |
| } | |
| float len = sqrt(lenSq); | |
| dirX /= len; | |
| dirY /= len; | |
| const float step = 1.0f; | |
| const float maxBackoff = 10.0f; | |
| bool foundSafe = false; | |
| for (float backoff = step; backoff <= maxBackoff; backoff += step) | |
| { | |
| float candidateX = x - dirX * backoff; | |
| float candidateY = y - dirY * backoff; | |
| float candidateZ = z; | |
| bot->UpdateGroundPositionZ(candidateX, candidateY, candidateZ); | |
| if (!IsMovingAllowed(mapId, candidateX, candidateY, candidateZ)) | |
| continue; | |
| if (IsAggroPosition(candidateX, candidateY)) | |
| continue; | |
| x = candidateX; | |
| y = candidateY; | |
| z = candidateZ; | |
| foundSafe = true; | |
| break; | |
| } | |
| if (!foundSafe) | |
| return false; | |
| } |
| return false; | ||
|
|
There was a problem hiding this comment.
Follow() currently aborts entirely if the computed follow point is in an aggro zone. This contradicts the stated goal of letting the bot approach “as close as possible” without pulling, and can leave cautious bots unable to follow their group in dungeons (no partial movement toward a safe point). Consider reducing the follow distance to the maximum aggro-free distance toward (followX, followY), or fall back to a MoveTo() toward a clamped safe point instead of returning false.
| return false; | |
| { | |
| // Try to move as close as possible toward the follow point without entering aggro | |
| float botX = bot->GetPositionX(); | |
| float botY = bot->GetPositionY(); | |
| // If our current position is already in an aggro zone, we cannot safely move closer | |
| if (IsAggroPosition(botX, botY)) | |
| return false; | |
| float safeX = botX; | |
| float safeY = botY; | |
| bool foundSafe = false; | |
| // Binary search along the segment from the bot toward the desired follow point | |
| float low = 0.0f; | |
| float high = 1.0f; | |
| for (int i = 0; i < 10; ++i) | |
| { | |
| float mid = (low + high) * 0.5f; | |
| float testX = botX + (followX - botX) * mid; | |
| float testY = botY + (followY - botY) * mid; | |
| if (!IsAggroPosition(testX, testY)) | |
| { | |
| foundSafe = true; | |
| safeX = testX; | |
| safeY = testY; | |
| low = mid; | |
| } | |
| else | |
| { | |
| high = mid; | |
| } | |
| } | |
| // If we did not find any safe point beyond our current position, abort as before | |
| const float minMoveDistSq = 0.25f; // do not move if adjustment is negligible (< 0.5 units) | |
| float dx = safeX - botX; | |
| float dy = safeY - botY; | |
| if (!foundSafe || (dx * dx + dy * dy) < minMoveDistSq) | |
| return false; | |
| // Move directly to the furthest safe point toward the follow position | |
| if (!MoveTo(bot->GetMapId(), safeX, safeY, bot->GetPositionZ())) | |
| return false; | |
| AI_VALUE(LastMovement&, "last movement").Set(target); | |
| return true; | |
| } |
There was a problem hiding this comment.
This situation implies the player being followed is charging in combat, which would cause the follower to also enter combat, allowing them to enter aggro range.
| list<ObjectGuid> targets = AI_VALUE(list<ObjectGuid>, "possible targets"); | ||
| for (list<ObjectGuid>::iterator i = targets.begin(); i != targets.end(); ++i) | ||
| { |
There was a problem hiding this comment.
CalculateAggroFreeDistance() iterates over the full "possible targets" list every time an aggro check is needed (e.g., Follow() calls IsAggroPosition each tick). This introduces an O(N) scan in a hot path and also copies the list into a local variable. Consider narrowing candidates (e.g., only hostile creatures within maxDist + aggroRange, or using a nearer/hostile-only value), and/or avoiding the list copy if the value API allows returning a const reference.
There was a problem hiding this comment.
I have no idea what it's talking about. The purpose of this code is to DISCOVER hostiles, and its iterating on a CACHED list, so it's not like it's doing a full map scan. Besides, if you are concerned about the hit (on a CACHED list again), then don't enable Cautious. I also can't help but notice that it had no better suggestion for magically scanning pre-known "hostiles".
There was a problem hiding this comment.
Ah, on second thought, perhaps what it is suggesting is caching hostile mobs... hmmm...
|
|
||
| if (tEntry < safeDist) | ||
| { | ||
| safeDist = std::max(0.0f, tEntry - 2.0f); |
There was a problem hiding this comment.
The 2.0f safety buffer is a new magic number. Please replace it with a named constant (or config-derived value) and add a brief rationale so it’s clear why 2 yards is the right margin and can be tuned if needed.
There was a problem hiding this comment.
True, but c'mon.
| # Bot can flee for enemy | ||
| #AiPlayerbot.FleeingEnabled = 1 | ||
|
|
||
| # Bot avoids pulling aggro during movement by default |
There was a problem hiding this comment.
The comment says bots avoid pulling aggro "by default", but the shown setting is 0 and the code default is false (disabled unless enabled). Please reword this to reflect that it’s optional/disabled by default, or change the documented default value accordingly to avoid confusing operators.
| # Bot avoids pulling aggro during movement by default | |
| # If enabled, bot avoids pulling aggro during movement (disabled by default) |
There was a problem hiding this comment.
Ugh, it's playing a semantic game here. IF you enable Cautious in the configuration file, then all bots will get the cautious strategy BY DEFAULT (when they are loaded). This would make it different than other strategies that, if enabled, you must still manually add to desired Bots. Could this be clearer? I suppose, but geesh.
Adds an optional strategy called "Cautious" that can be auto-applied to bots from a new configuration option, OR applied manually as desired.
The purpose of the strategy is to prevent bots from wiping their groups in dungeons with careless behavior that aggros new mobs. This behavior was rampant. The change makes the Bot "Cautious", checking every move to ensure it won't create new aggro where it did not already exist, without impacting the ability to be directed into combat by a player, or entering combat itself.
It does allow a bot to get as close as possible to new aggro, however, which is often enough to get into range for what it must do in a fight.
This change is