fix(monster): prevent spiral movement loop when overlaps creature#184
fix(monster): prevent spiral movement loop when overlaps creature#184
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughgoToFollowCreature() gains a short “simple step” branch that may perform a single-distance step and calls startAutoWalk(direction); both the simple-step and the path-update flows invoke a new Monster::onFollowCreatureComplete() hook to reorder or remove the follow-target from targetList. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/monster.cpp (1)
550-564:⚠️ Potential issue | 🔴 CriticalConfirm: asymmetric targetList handling for summons causes target loss.
The concern in the original review is valid.
selectTarget()requires the target to already exist intargetList(it returnsfalseat line 612 if the target is not found viafind_if). WhenonFollowCreatureComplete()erases a summon's target, there is no re-add mechanism:
onThinkTarget()skips summons entirely (if (!isSummon())at line 870), so summons never callsearchTarget()to repopulate targets.- A summon's only attempt to add targets is at line 731:
selectTarget(master->getAttackedCreature()), which fails silently if the target was erased and is no longer intargetList.This creates the asymmetry: non-summons use
iter_swapto deprioritize but retain targets in the list, while summons erase entirely with no recovery path. A summon can permanently lose a target if it is erased during pursuit and the master's attack target remains the same.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monster.cpp` around lines 550 - 564, The current onFollowCreatureComplete() erases a summon’s target from targetList which prevents selectTarget() from finding/re-using it (and onThinkTarget() never calls searchTarget() for summons), causing permanent target loss; change the summon branch in Monster::onFollowCreatureComplete() to preserve the target ordering instead of erasing (e.g., use std::iter_swap(it, targetList.end()-1) like the non-summon branch, with proper handling when targetList has size 1) so summons retain the target entry, or alternatively push the target to the back rather than erase it; update any edge-case handling so selectTarget(), onThinkTarget(), and searchTarget() can continue to operate on the preserved entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/monster.cpp`:
- Line 530: The line declaring the variable Direction dir = DIRECTION_NONE; has
two extra tabs; fix the clang-format error by removing the extra indentation so
the declaration aligns with the surrounding function body indentation (match
other local variable declarations in the same function), e.g., make the start of
"Direction dir = DIRECTION_NONE;" align with neighboring statements in the
function.
---
Outside diff comments:
In `@src/monster.cpp`:
- Around line 550-564: The current onFollowCreatureComplete() erases a summon’s
target from targetList which prevents selectTarget() from finding/re-using it
(and onThinkTarget() never calls searchTarget() for summons), causing permanent
target loss; change the summon branch in Monster::onFollowCreatureComplete() to
preserve the target ordering instead of erasing (e.g., use std::iter_swap(it,
targetList.end()-1) like the non-summon branch, with proper handling when
targetList has size 1) so summons retain the target entry, or alternatively push
the target to the back rather than erase it; update any edge-case handling so
selectTarget(), onThinkTarget(), and searchTarget() can continue to operate on
the preserved entry.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/monster.cpp (2)
532-532:isFleeing()is redundant in thesimpleStepcondition.
getPathSearchParams(called just above at line 528) setsfpp.maxTargetDist = Map::maxViewportXfor non-summon fleeing monsters (line 1978), which is always> 1. SoisFleeing() || fpp.maxTargetDist > 1always reduces tofpp.maxTargetDist > 1— theisFleeing()arm can never fire independently.♻️ Suggested simplification
- const auto simpleStep = !isSummon() && (isFleeing() || fpp.maxTargetDist > 1); + const auto simpleStep = !isSummon() && fpp.maxTargetDist > 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monster.cpp` at line 532, The condition for simpleStep is redundant because getPathSearchParams sets fpp.maxTargetDist = Map::maxViewportX (>1) for non-summon fleeing monsters, so remove the unnecessary isFleeing() check: change the expression that defines simpleStep (in the block using getPathSearchParams) to rely solely on !isSummon() && (fpp.maxTargetDist > 1) and ensure no other logic depends on isFleeing() being evaluated there; update the variable definition that currently references isFleeing() and fpp.maxTargetDist accordingly.
546-560:onFollowCreatureCompleteshould be declaredprivateinmonster.h.The method is only called from
goToFollowCreature()within the same class and exposing it as public unnecessarily widens the API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monster.cpp` around lines 546 - 560, The method onFollowCreatureComplete is currently exposed publicly but is only invoked from within the class (goToFollowCreature), so update the Monster class declaration in monster.h to move the onFollowCreatureComplete() declaration into the private section (or add a private: label and place it there), ensuring the signature matches the implementation in src/monster.cpp and no other callers rely on it being public; recompile to verify access changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/monster.cpp`:
- Line 530: The previous extra indentation and naming issue has been resolved;
ensure the local variable is declared as "auto direction = DIRECTION_NONE;" with
a single-tab indentation level (not "Direction dir" or extra tabs) where the
monster direction is initialized so the symbol direction is consistently used
throughout the surrounding functions (e.g., any uses of direction in movement
logic or checks should reference this renamed variable).
---
Nitpick comments:
In `@src/monster.cpp`:
- Line 532: The condition for simpleStep is redundant because
getPathSearchParams sets fpp.maxTargetDist = Map::maxViewportX (>1) for
non-summon fleeing monsters, so remove the unnecessary isFleeing() check: change
the expression that defines simpleStep (in the block using getPathSearchParams)
to rely solely on !isSummon() && (fpp.maxTargetDist > 1) and ensure no other
logic depends on isFleeing() being evaluated there; update the variable
definition that currently references isFleeing() and fpp.maxTargetDist
accordingly.
- Around line 546-560: The method onFollowCreatureComplete is currently exposed
publicly but is only invoked from within the class (goToFollowCreature), so
update the Monster class declaration in monster.h to move the
onFollowCreatureComplete() declaration into the private section (or add a
private: label and place it there), ensuring the signature matches the
implementation in src/monster.cpp and no other callers rely on it being public;
recompile to verify access changes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/monster.cpp (1)
532-535:getDistanceStepcan returntruewithdirectionstillDIRECTION_NONEin rare diagonal dead-end cases.In
getDistanceStep, several diagonal-escape branches (e.g. lines 1367–1373, 1415–1421, etc.) setdirectiononly if two compoundcanWalkToconditions hold, then unconditionallyreturn true— so if none of those conditions match,directionremainsDIRECTION_NONE.With
auto direction = DIRECTION_NONE, callingstartAutoWalk(direction)then setslistWalkDir = {DIRECTION_NONE}and queues a walk event that produces a wasted tick (creature doesn't actually move). Recovery happens on the next think cycle, so the impact is a brief hesitation rather than a hard break. This is a pre-existing quirk ingetDistanceStep, but it is newly exercised by this code path.Consider guarding:
🛡️ Proposed guard
- if (getDistanceStep(followCreature->getPosition(), direction, isFleeing())) { + if (getDistanceStep(followCreature->getPosition(), direction, isFleeing()) && direction != DIRECTION_NONE) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monster.cpp` around lines 532 - 535, The call site assumes getDistanceStep sets a valid direction but getDistanceStep can return true while leaving direction as DIRECTION_NONE; update the follow logic to guard against that: after calling getDistanceStep(followCreature->getPosition(), direction, isFleeing()) only set hasFollowPath = true and call startAutoWalk(direction) when direction != DIRECTION_NONE (keep the existing variables: direction, hasFollowPath, startAutoWalk, followCreature), otherwise do not enqueue a DIRECTION_NONE walk and let the next think cycle retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/monster.cpp`:
- Around line 532-535: The call site assumes getDistanceStep sets a valid
direction but getDistanceStep can return true while leaving direction as
DIRECTION_NONE; update the follow logic to guard against that: after calling
getDistanceStep(followCreature->getPosition(), direction, isFleeing()) only set
hasFollowPath = true and call startAutoWalk(direction) when direction !=
DIRECTION_NONE (keep the existing variables: direction, hasFollowPath,
startAutoWalk, followCreature), otherwise do not enqueue a DIRECTION_NONE walk
and let the next think cycle retry.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/monster.cpp (1)
553-556:⚠️ Potential issue | 🟡 MinorVerify intentionality of hard-erasing follow target from
targetListfor summons on path failure.This
erasebranch is new behavior introduced by this PR. When a summon fails A* pathing (!hasFollowPath), its follow target is permanently removed fromtargetList, whereas non-summons (theelse ifbranch) merely rotate it to the back of the list. SinceselectTarget()requires the creature to be intargetList, erasing it prevents immediate re-selection via the normal targeting flow.While a re-population path exists—
onCreatureFound()can re-add the target viaaddTarget()if it re-enters visibility—this requires the creature to be discovered again and pass theisOpponent()check. For summons ordered to attack an enemy that becomes temporarily unreachable (e.g., obstacle between summon and target), this erase behavior is stricter than non-summons and could cause unnecessary target abandonment if re-discovery is delayed or the target doesn't re-enter the summon's awareness radius.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monster.cpp` around lines 553 - 556, The current branch removes a summon’s follow target from targetList on failed pathing (targetList.erase(it)), which prevents selectTarget() from reselecting it; change this to match the non-summon behavior by moving the entry to the back (use std::iter_swap(it, targetList.end() - 1)) instead of erasing, so hasFollowPath failure for isSummon() doesn’t permanently drop the target; update the block handling isSummon() and ensure related flows (selectTarget(), onCreatureFound(), addTarget(), hasFollowPath) keep consistent semantics.
🧹 Nitpick comments (1)
src/monster.cpp (1)
530-531:simpleStepcondition simplifies to!isSummon()in practice.
fpp.maxTargetDistis set tomType->info.targetDistanceingetPathSearchParams(minimum 1 for any configured monster), makingfpp.maxTargetDist >= 1a tautology for virtually all monster types. TheisFleeing() ||clause is equally redundant since fleeing monsters havefpp.maxTargetDist = Map::maxViewportX. The condition could be simplified to!isSummon(), or tightened with a more explicit predicate (e.g.,distance == 0) if the intent is specifically to handle the overlap case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/monster.cpp` around lines 530 - 531, The current simpleStep check is effectively always true when not a summon; replace the tautological condition by either (A) simplifying it to const auto simpleStep = !isSummon(); if the original intent was just to exclude summons, or (B) tightening it to explicitly detect the overlap case, e.g. const auto simpleStep = !isSummon() && distance == 0; if you meant to only handle zero-distance overlap; update the line that defines simpleStep (and adjust any dependent logic) accordingly—refer to simpleStep, isSummon(), isFleeing(), fpp.maxTargetDist, and getPathSearchParams/mType->info.targetDistance when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/monster.cpp`:
- Around line 533-537: After getDistanceStep(...) returns true, check the
direction value and avoid calling startAutoWalk when it is DIRECTION_NONE: if
getDistanceStep(followCreature->getPosition(), direction, isFleeing()) returns
true but direction == DIRECTION_NONE, call onFollowCreatureComplete() and return
without invoking startAutoWalk or queuing a walk; otherwise proceed with
hasFollowPath = true and startAutoWalk(direction). This prevents DIRECTION_NONE
from being pushed into listWalkDir and later passed to getNextPosition.
---
Outside diff comments:
In `@src/monster.cpp`:
- Around line 553-556: The current branch removes a summon’s follow target from
targetList on failed pathing (targetList.erase(it)), which prevents
selectTarget() from reselecting it; change this to match the non-summon behavior
by moving the entry to the back (use std::iter_swap(it, targetList.end() - 1))
instead of erasing, so hasFollowPath failure for isSummon() doesn’t permanently
drop the target; update the block handling isSummon() and ensure related flows
(selectTarget(), onCreatureFound(), addTarget(), hasFollowPath) keep consistent
semantics.
---
Nitpick comments:
In `@src/monster.cpp`:
- Around line 530-531: The current simpleStep check is effectively always true
when not a summon; replace the tautological condition by either (A) simplifying
it to const auto simpleStep = !isSummon(); if the original intent was just to
exclude summons, or (B) tightening it to explicitly detect the overlap case,
e.g. const auto simpleStep = !isSummon() && distance == 0; if you meant to only
handle zero-distance overlap; update the line that defines simpleStep (and
adjust any dependent logic) accordingly—refer to simpleStep, isSummon(),
isFleeing(), fpp.maxTargetDist, and
getPathSearchParams/mType->info.targetDistance when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/monster.cpp`:
- Around line 530-545: The follow logic fails to mark a successful follow when
getDistanceStep(...) returns true but leaves direction == DIRECTION_NONE
(monster already at targetDistance); update the simpleStep branch in the follow
routine so hasFollowPath is set to true whenever getDistanceStep(...) returns
true (regardless of direction) before calling onFollowCreatureComplete(),
keeping the existing startAutoWalk(direction) call only when direction !=
DIRECTION_NONE; adjust the block around getDistanceStep, DIRECTION_NONE,
hasFollowPath, startAutoWalk and onFollowCreatureComplete to ensure being
exactly at targetDistance is treated as a completed follow.
| const auto simpleStep = !isSummon() && (isFleeing() || fpp.maxTargetDist >= 1); | ||
| if (simpleStep) { | ||
| auto direction = DIRECTION_NONE; | ||
| if (getDistanceStep(followCreature->getPosition(), direction, isFleeing())) { | ||
| if (direction != DIRECTION_NONE) { | ||
| hasFollowPath = true; | ||
| startAutoWalk(direction); | ||
| } | ||
| } | ||
|
|
||
| if (dir != DIRECTION_NONE) { | ||
| listWalkDir.clear(); | ||
| listWalkDir.push_back(dir); | ||
|
|
||
| hasFollowPath = true; | ||
| startAutoWalk(); | ||
| onFollowCreatureComplete(); | ||
| return; | ||
| } | ||
| } else { | ||
| updateFollowCreaturePath(fpp); | ||
| } | ||
|
|
||
| updateFollowCreaturePath(fpp); | ||
| onFollowCreatureComplete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how hasFollowPath is used in context
echo "=== hasFollowPath assignments in src/monster.cpp ==="
rg -n 'hasFollowPath\s*=' src/monster.cpp -B1 -A1
echo ""
echo "=== hasFollowPath usage in onFollowCreatureComplete ==="
rg -n 'onFollowCreatureComplete' src/monster.cpp -A15 | head -30
echo ""
echo "=== onThink context around searchTarget ==="
rg -n 'searchTarget' src/monster.cpp -B3 -A1
echo ""
echo "=== getDistanceStep implementation ==="
rg -n 'getDistanceStep' src/monster.cpp -A20 | head -40Repository: atlas-kit/atlas
Length of output: 3219
🏁 Script executed:
#!/bin/bash
echo "=== All hasFollowPath references in codebase ==="
rg -n 'hasFollowPath' src/ -B1 -A1
echo ""
echo "=== Check if hasFollowPath is a member variable ==="
rg -n 'bool hasFollowPath' src/ -B1 -A1
echo ""
echo "=== Check initialization in constructor ==="
rg -n 'Monster::Monster' src/monster.cpp -A30 | head -50Repository: atlas-kit/atlas
Length of output: 3810
hasFollowPath is not set when the monster reaches exactly targetDistance.
When getDistanceStep() returns true with direction remaining DIRECTION_NONE (line 1307–1309: the monster is at mType->info.targetDistance), the code skips the hasFollowPath = true assignment and proceeds to onFollowCreatureComplete(). This leaves hasFollowPath in its previous state.
Downstream consequences:
onFollowCreatureComplete()(line 557) swaps the target to the end of the list whenhasFollowPathis false, demoting an already-reached target.onThink()(line 741–742) callssearchTarget()when!hasFollowPath, triggering unnecessary target switches despite the monster being at the desired distance.getNextStep()(line 1144–1151) treats!hasFollowPathas needing random stepping instead of dance-step logic.- Summon spawning (line 939) is prevented when
hasFollowPathis false.
Since being at targetDistance is a successful follow state, set hasFollowPath = true unconditionally when getDistanceStep() returns true:
Suggested fix
if (getDistanceStep(followCreature->getPosition(), direction, isFleeing())) {
+ hasFollowPath = true;
if (direction != DIRECTION_NONE) {
- hasFollowPath = true;
startAutoWalk(direction);
}
onFollowCreatureComplete();
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/monster.cpp` around lines 530 - 545, The follow logic fails to mark a
successful follow when getDistanceStep(...) returns true but leaves direction ==
DIRECTION_NONE (monster already at targetDistance); update the simpleStep branch
in the follow routine so hasFollowPath is set to true whenever
getDistanceStep(...) returns true (regardless of direction) before calling
onFollowCreatureComplete(), keeping the existing startAutoWalk(direction) call
only when direction != DIRECTION_NONE; adjust the block around getDistanceStep,
DIRECTION_NONE, hasFollowPath, startAutoWalk and onFollowCreatureComplete to
ensure being exactly at targetDistance is treated as a completed follow.
2 participants
Pull Request Prelude
Changes Proposed
Fixes a movement bug where monsters would enter a permanent "spiral loop" and lose their smooth walking ability when a player stood directly on top of them.
Previously, the nested logic in goToFollowCreature would get stuck in a state where it only performed jittery, step-by-step movements. Now, the logic to use a simplified simpleStep check and ensuring a clean fallback to the A* pathfinder (updateFollowCreaturePath), the creature can now correctly recalculate its route out of an overlap. This restores natural movement flow and prevents the "step-by-step" movement error.
Introduced in otland#4811
Issue address otland#4865
Summary by CodeRabbit