Fix bot party follow across multi-floor maps

[chuanhd]

What was wrong

Some maps, such as Dungeon and Lost Tower, contain several disconnected floors inside the same runtime map. When the party leader moved to another floor, a bot follower still considered the leader to be on the same map. It kept trying an impossible walking route and never considered using a warp gate to regroup.

There was a related problem when tracking whether the leader had settled after changing maps: map numbers are not unique enough to distinguish every runtime map instance.

What changed

The follow logic now treats these as two separate cases:

• When the leader is on another map, the bot keeps the existing settle delay and follows through the normal legal warp flow.
• When the leader is on the same map, the bot tries to walk first. If no path exists, it looks for a legal gate whose landing area can reach the leader.

To choose that gate, the bot runs one flood fill from the leader across the walk map. This tells us which gate landing points are reachable and how far they are from the leader without running a separate A* search for every point of every gate. Among the usable gates, the bot chooses the one with the shortest worst-case landing distance.

Gate placement is random and WarpToAsync does not retry a bad landing point. For that reason, a gate is accepted only when every point that random placement may choose is walkable and can reach the leader. This is intentionally conservative: it avoids making follow behavior succeed or fail by chance. The gate containing the bot's current position is also skipped, so the bot cannot repeatedly warp back to the floor where it is already standing.

If no suitable gate is found, the bot waits 20 seconds before doing another gate search. This backoff is separate from the normal warp cooldown: a failed search should not prevent unrelated warp behavior. The failed follow attempt also leaves the tick available for normal local hunting instead of making the bot idle.

Leader-settle and follow-warp timing now use TimeProvider, which makes the cooldown behavior deterministic in server-side tests.

Compatibility notes

• Warp lookup prefers an exact map-definition id when the warp list contains one. If it does not, it falls back to the previous same-map-number behavior so runtime or instanced map definitions still work.
• The identity tier is selected before checking level requirements. This prevents an inaccessible exact entry from falling through to an easier entry belonging to another same-number map definition.
• Map access still uses the class-adjusted move requirement and the character's plain level, just like player warps. The reset-aware combat level is not used to bypass access requirements.
• Exit-gate placement is now shared through ExitGateExtensions, including support for single-coordinate gates and validation of reversed bounds. For normal multi-coordinate gates it preserves the existing upper-exclusive random-placement behavior.
• That upper-exclusive rule is specific to placement. Gate containment and terrain painting elsewhere still use inclusive upper bounds; this PR documents the difference rather than changing those unrelated behaviors.

Tests

The tests exercise the logic directly without a client or UI. They cover:

• leader transitions between runtime maps sharing the same map number;
• walking before warping, and falling back to hunting when regrouping is impossible;
• selecting a reachable gate instead of a closer gate on the wrong region;
• random landing-area safety and avoiding the bot's current gate;
• failed-search backoff and retry after the cooldown;
• exact-definition lookup, same-number fallback, and level-requirement isolation;
• exit-gate placement bounds, including single-point and invalid gates;
• actual map-change events, so a test cannot pass merely because the bot walked.

dotnet test tests/MUnique.OpenMU.Tests/MUnique.OpenMU.Tests.csproj -p:ci=true --no-restore
# Passed: 857

dotnet test tests/MUnique.OpenMU.Tests/MUnique.OpenMU.Tests.csproj -p:ci=true --no-restore --filter 'FullyQualifiedName~ExitGateExtensionsTest'
# Passed: 4

[sven-n]

Review

I read through src/GameLogic/Bots/BotNavigator.cs and the new test. The direction is good — both problems described (map-number collisions in the settle tracker, and followers stuck on a disconnected region of the same map) are real, and the fix is small and localized. A few things worth addressing before merge.

Correctness / behaviour

1. TryGetNearestLegalWarp can pick the warp the bot is already standing on. In the same-map branch the nearest legal warp is chosen purely by GateDistance(gate, leader.Position). Nothing checks that the chosen gate is on a different region than the bot, or that it actually gets the bot closer. If the leader sits on an isolated pocket whose closest legal warp entry happens to be the bot's own floor gate, the bot warps to where it already is, clears _travelPath, and repeats every 20s — the exact "re-issuing an impossible path every tick" symptom, just at cooldown pace. Worth guarding, e.g. skip a candidate whose gate is reachable from the bot's current position (or simply closer to the bot than to the leader).

2. GateDistance is Manhattan distance on the gate's centre point. On maps whose floors are disconnected regions, coordinate proximity does not imply region membership — two gates that lead to different floors can be spatially adjacent. It's a reasonable heuristic, but please say so in the comment rather than implying it finds the leader's floor; and it makes point 1 more likely, not less.

3. Dead-ending the tick when no legal warp exists. Both the cross-map branch and the new same-map branch return true (tick consumed) even when nothing happened — no warp, no walk. The bot then neither follows nor hunts until the stuck watchdog fires. In the new branch specifically, returning false when TravelTowardAsync failed and no warp is available would let the bot hunt where it stands instead of idling. This is partly pre-existing, but the new path adds another way to reach it.

4. _hasDestination is left set to an unreachable destination. The branch sets _destination/_hasDestination = true before attempting travel; if travel fails and the warp is on cooldown, that stale unreachable destination survives the tick. The success path clears it, the cooldown path doesn't.

Consistency

5. Two clocks in one class. HasLeaderSettled now uses _timeProvider, while _lastWarpUtc, _nextShoppingCheckUtc, _resetDueAtUtc and the rest still use DateTime.UtcNow directly. That's fine as an incremental step, but it means the new warp behaviour can't be tested deterministically (test 2 relies on _lastWarpUtc starting at MinValue). Either route the cooldown checks through _timeProvider too, or note in the ctor doc that it currently only covers the settle timer.

6. TryGetLegalWarp semantics changed silently. It went from w.Gate?.Map?.Number == mapDefinition.Number to IsSameMapDefinition, which — when both ids are non-empty — requires id equality. Anywhere a runtime GameMap.Definition is not the same persisted entity as the one referenced from Configuration.WarpList (e.g. instanced maps), warps that previously matched by number will now stop matching, and the bot will conclude it "cannot legally follow" and kick itself out of the party. Please confirm both sides always come from the same config graph, and consider mentioning this tightening in the PR description since it affects TryGetLegalWarp's existing callers, not just the new one.

7. GetLegalWarps materialises with .ToList() on every call, including the per-tick TryGetLegalWarp path that only needs the first match. Returning the lazy IEnumerable keeps the old short-circuit.

Tests

8. LeaderSettleTrackingDistinguishesMapsWithSameNumberAsync doesn't test what its name says. _leaderMapId is compared against GameMap.Id, which is a fresh Guid.NewGuid() per GameMap instance (GameMap.cs:43) — not the definition id. The assertion therefore holds for any two distinct GameMap instances; the differing Number/Discriminator on the definitions is incidental. It does still catch the old number-based bug, so it's a valid regression test, but the name and the doc comment overstate it. Either rename to reflect map-instance identity, or add a case with two instances of the same definition to pin down the intended semantics.

9. Test 2 asserts the outcome only via position. Is.InRange(200, 201) would also pass if the bot had walked there. Asserting that _travelPath was cleared, or that the position changed in a single tick (walking couldn't cover that distance), would make the "warped, not walked" claim explicit.

Nits

• The HasLeaderSettled doc says "Tracks the leader's map instance per bot" — good, that now matches the implementation; the internal visibility bump on it and TryFollowLeaderAsync is test-only, which is fine given InternalsVisibleTo("MUnique.OpenMU.Tests"), but an // internal for testing marker helps future readers.
• IsSameMapDefinition's Guid.Empty fallback compares Number + Discriminator; a one-line comment on why the empty-id case exists (non-persisted definitions in tests?) would help.

Nothing here is a blocker for the core idea — items 1, 3 and 6 are the ones I'd want resolved before merge.

---

Generated by Claude Code

[sven-n]

Re-review (head 70219d2, 8 new commits)

Thanks for the follow-ups — most of my previous points are properly addressed:

• Self-warp loop — solved, and solved better than I suggested. FindBestReachableLegalWarpAsync requiring a walkable route from the gate's landing area to the leader implicitly excludes gates in the bot's own region (if such a path existed, TravelTowardAsync wouldn't have failed), and the landingPoints.Contains(this._player.Position) guard covers the degenerate case.
• Tick dead-ends — solved. TryRegroupWithLeaderOnSameMapAsync now returns false when nothing happened, and the split into TryHandleCrossMapLeaderFollowAsync / TryRegroupWithLeaderOnSameMapAsync with the integration into EvaluateFollowerHuntingAsync reads much better than the old single method.
• TryGetLegalWarp tightening — solved, and the tiered exact-id-then-same-number lookup with the comment about the identity tier being selected before the level filter is exactly the right subtlety to call out.
• Mixed clocks — solved; _lastWarpUtc and WarpCooldown now go through _timeProvider.
• Test assertions — much stronger. MapChangeRecordingPlugIn pins "warped, not walked", and the cooldown-then-after-cooldown sequence is a good addition.
• Distance-only gate ranking is gone in favour of real path lengths, so the Manhattan-heuristic objection no longer applies (GateDistance survives only as a tie-break).

Remaining items, roughly in order of how much they matter.

1. The failure path has no backoff (blocking, in my view)

_lastWarpUtc is only updated when a warp is actually performed. When FindBestReachableLegalWarpAsync returns null — no legal gate, or none whose landing area reaches the leader — the cooldown is never armed, so the entire scan re-runs on every single tick for as long as the leader stays unreachable. That's the "re-issuing an impossible path every tick" symptom the PR set out to remove, moved from one A* to N of them. Arming the cooldown on a failed search too (or a separate _lastFollowWarpSearchUtc) would fix it.

2. Cost of the search

Per invocation the search does, for every legal warp candidate, one full A* per landing cell of its gate, each acquiring the shared TravelPathFinderPool semaphore — on top of the failed leader-directed A* that got us here. For a 5×5 gate with three candidates that's 75 whole-map searches for one follower on one tick. With a party of followers on a Lost Tower-style map, and combined with item 1, this can plausibly starve the pathfinder pool for every other bot on the server.

Some options, cheapest first:

• Pre-filter candidates by GateDistance and only path-check the best few.
• Path once from a representative walkable landing cell, and use the walk map / a flood fill for the rest (region membership is what you actually need — the worst-case length is only used for ranking).
• Cache the result per (map, leader region) for the duration of the cooldown.

3. "Every landing cell must be walkable and reachable" may reject real gates

GetWorstLandingPathLengthAsync returns null if any cell in the gate rectangle is non-walkable or has no route. Given that PlaceAtGateAsync doesn't validate walkability, being conservative is defensible — but a single blocked tile inside an otherwise fine multi-cell gate rectangle disqualifies the gate outright, and real ExitGate rectangles in the config do include blocked tiles. Consider "at least one walkable cell, and every walkable cell reaches the target" instead. Either way, please state the chosen rule in the doc comment — right now "complete landing area can reach him" reads as an invariant rather than a policy.

This is also untested: every gate in BotLeaderFollowMapIdentityTest is built by CreateGate(map, x, y) with X2 = x + 1, which under the new exclusive-bound rule is a single landing cell. The worst-case-over-cells logic, the partial-blockage rejection, and the ranking between candidates of differing path length are all unexercised.

4. The exclusive X2/Y2 contract contradicts the rest of the codebase

ExitGateExtensionsTest.PossibleLandingPointsUseExclusiveUpperBounds now asserts exclusive upper bounds as the intended contract, but everywhere else gates are treated as inclusive:

• GameMapTerrain.cs:122 — for (int x = gate.X1; x <= gate.X2; x++) (safezone marking)
• PointExtensions.cs:24 — point.X <= rectangle.X2
• PlayerActions/WarpGateAction.cs:71 — currentPosition.X <= gate.X2 + inaccuracy

So the safezone is painted over [X1, X2] while placement only ever lands in [X1, X2). The exclusivity is pre-existing in Rand.NextInt(gate.X1, gate.X2), and I'm not asking you to change placement behaviour in this PR — but codifying it in a new named contract (GetPossibleLandingPoints) and a test titled "matching the random placement logic" makes the divergence permanent and harder to spot later. At minimum, please note in the XML doc that this is the placement convention and that gate containment elsewhere is inclusive; ideally raise it as a separate issue.

Relatedly, two more call sites still duplicate the raw expression and would benefit from the new helper: Bots/BotGenerator.cs:408 and Resets/ResetCharacterAction.cs:205.

5. Smaller things

• TryHandleCrossMapLeaderFollowAsync no longer uses its cancellationToken (all travel moved to the same-map method). Drop the parameter, or keep it and add a cancellationToken.ThrowIfCancellationRequested() — an unused documented parameter invites a warning and confuses the next reader.
• EvaluateFollowerHuntingAsync discards the bool from TryRegroupWithLeaderOnSameMapAsync. If the result only exists for the tests, say so; otherwise the caller should probably act on it.
• TryRegroupWithLeaderOnSameMapAsync clears _hasDestination up front and leaves it cleared on the "walk failed, warp on cooldown" path. That's a deliberate-looking side effect on a method that reports "nothing happened" — worth a word in the comment.
• The comment removed from the pathfinder block ("the OperationCanceledException is expected and handled in SafeEvaluateAsync") was useful context; now that FindPath also takes the token and can throw from inside the try, it's more relevant than before, not less.
• LeaderSettleTrackingDistinguishesMapsWithSameNumberAsync still tests GameMap instance identity rather than same-number/different-definition (_leaderMapId compares GameMap.Id, a per-instance Guid.NewGuid()). Still a valid regression test for the old number-based bug; the name and doc just claim more than it checks.

CI: Codacy is green, the Azure MUnique.OpenMU build was still queued when I looked — worth confirming it goes green before merge.

Items 1 and 2 are the ones I'd want resolved; 3 and 4 are judgement calls I'd like to see stated explicitly rather than necessarily changed.

---

Generated by Claude Code