Refactor transport ship shore finding logic #2684
Conversation
Replace Manhattan-based shoreline sampling and per-candidate path checks with a single water-expansion search (multi-source, multi-target) that finds the nearest reachable shoreline by true boat distance.
|
|
WalkthroughAdded a water-aware Dijkstra-like search and MinHeap to TransportShipUtils, plus helpers to identify and traverse water tiles. Replaced multiple MiniAStar/BFS water path calculations with the new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
🧹 Nitpick comments (4)
src/core/game/TransportShipUtils.ts (4)
4-52: MinHeap implementation looks solid.The binary min-heap is correctly implemented with standard bubble operations. The bitwise shift for parent index calculation is efficient and idiomatic.
Optional: Consider extracting to a shared utility if other pathfinding code could benefit
If Dijkstra-style searches are used elsewhere in the codebase, extracting MinHeap to a shared data structures file could reduce duplication. For now, keeping it local is fine.
65-144: Dijkstra-based water pathfinding is correctly implemented.The algorithm correctly performs multi-source (water tiles adjacent to target shore) to multi-target (water tiles adjacent to player shores) expansion. The early termination optimization (lines 98-101) and stale entry filtering (lines 118-119) are standard Dijkstra optimizations.
Minor items:
- Line 98 comment typo: "If the like destination water" should be "If the destination water tiles are" or just "If destination water"
- Lines 85-86, 87-88: Odd formatting with
continueon a separate line - this will be fixed by Prettier- Performance consideration: The default
maxStepsof 250,000 provides safety but could take time on complex maps. The caller-specified values of 3,000,000 in some call sites are quite high. Consider documenting the expected performance characteristics or validating with large maps.Quick typo fix
- // If the like destination water is already next to the player's shore, return immediately + // If destination water is already next to the player's shore, return immediately
196-202: Water-based spawn detection correctly replaces previous logic.The integration of
closestShoreFromPlayerByWaterwith lake filtering is appropriate for determining valid lake transport spawn points.The
maxSteps: 3_000_000is quite high. If lake pathfinding on large maps becomes slow, consider tuning this value or adding early-exit optimizations.
283-288: Shore deployment source correctly uses water pathfinding.The integration is correct and the validation checks (line 287) are appropriate.
Line 283 comment clarity: "IMPORTANT CHANGE:One exact water search replaces MiniAStar + sampling" has a formatting issue (missing space after colon) and could be clearer. Suggest:
- // IMPORTANT CHANGE:One exact water search replaces MiniAStar + sampling. + // Uses single water-expansion search instead of MiniAStar + sampling
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/TransportShipUtils.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/game/TransportShipUtils.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/game/TransportShipUtils.ts
🪛 GitHub Actions: 🧪 CI
src/core/game/TransportShipUtils.ts
[warning] 1-1: Code style issues found in this file. Run 'prettier --write' to fix.
🪛 GitHub Check: 🔍 ESLint
src/core/game/TransportShipUtils.ts
[failure] 57-57:
Prefer using nullish coalescing operator (??) instead of a logical or (||), as it is a safer operator.
🔇 Additional comments (5)
src/core/game/TransportShipUtils.ts (5)
61-63: LGTM!Clean helper function with clear intent.
214-216: LGTM on spawn point determination.Correctly uses the new water-based search to find reachable spawn shores.
231-238: Correct bidirectional water pathfinding integration.Both source and destination shore determination now correctly use water-based reachability rather than Manhattan distance.
310-336: LGTM on refactoring to use filtered border tiles.The variable naming is clearer with
borderShoreTiles. Note that this function still uses Manhattan distance rather than water-based pathfinding, which appears intentional since it's generating candidates for testing rather than performing final pathfinding.
54-59: Remove unnecessaryas anycast—all methods are already on the GameMap interface.The fallback pattern checks for methods (
isWater,isOcean,isLake) that are all defined on theGameMapinterface. Theas anycast can be removed. Simplify to:function isWaterTile(gm: GameMap, t: TileRef): boolean { return gm.isWater(t); }This uses clean, idiomatic TypeScript without bypassing the type system.
Note: The
||operator on line 57 is correct—it returns true if either condition is true. The??operator would not work here.⛔ Skipped due to learnings
Learnt from: scamiv Repo: openfrontio/OpenFrontIO PR: 2607 File: src/core/execution/PlayerExecution.ts:271-295 Timestamp: 2025-12-13T14:58:29.645Z Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Updated the code in line with the checks requested by code rabbit
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TransportShipUtils.ts (1)
334-345: Fix broken extremum tracking logic.The
else-ifchain is incorrect. If a tile has both the minimum X coordinate AND minimum Y coordinate, onlyminXgets set, andminYremains null. Each extremum check should be independent.🔎 Fix with independent checks
if (cell.x < minX) { minX = cell.x; extremumTiles.minX = tile; - } else if (cell.y < minY) { + } + if (cell.y < minY) { minY = cell.y; extremumTiles.minY = tile; - } else if (cell.x > maxX) { + } + if (cell.x > maxX) { maxX = cell.x; extremumTiles.maxX = tile; - } else if (cell.y > maxY) { + } + if (cell.y > maxY) { + maxY = cell.y; extremumTiles.maxY = tile; }
🧹 Nitpick comments (2)
src/core/game/TransportShipUtils.ts (2)
4-52: MinHeap implementation looks correct.The min-heap operations (push, pop, bubbleUp, bubbleDown) follow standard algorithms and handle edge cases properly.
Consider using a well-known priority queue library instead of a custom implementation to reduce maintenance burden and potential bugs in the future.
54-62: Avoidanycast; use proper type guards.The cast to
anybypasses TypeScript's type safety. Consider using type guards or extending the GameMap interface to includeisWaterif it's expected to exist.🔎 Improved type-safe approach
function isWaterTile(gm: GameMap, t: TileRef): boolean { - const anyGM = gm as any; - if (typeof anyGM.isWater === "function") return anyGM.isWater(t); - if (typeof anyGM.isOcean === "function") { - if (anyGM.isOcean(t)) return true; + const gmWithWater = gm as GameMap & { isWater?: (t: TileRef) => boolean }; + if (gmWithWater.isWater) return gmWithWater.isWater(t); + if (gm.isOcean(t)) return true; - return gm.isLake(t); - } return gm.isLake(t); }Note: From the relevant code snippets,
GameMapalready hasisWater,isOcean, andisLakemethods, so the defensive checks may be unnecessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/TransportShipUtils.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/game/TransportShipUtils.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/game/TransportShipUtils.ts
🧬 Code graph analysis (1)
src/core/game/TransportShipUtils.ts (3)
src/core/game/GameMap.ts (2)
GameMap(6-51)TileRef(3-3)src/core/game/Game.ts (1)
Player(530-667)src/core/worker/WorkerClient.ts (1)
transportShipSpawn(228-256)
🔇 Additional comments (3)
src/core/game/TransportShipUtils.ts (3)
68-147: Verify maxSteps default doesn't cause performance issues.The default
maxStepsof 250,000 is quite large. On maps with extensive water networks, this could cause noticeable delays or even timeouts in the pathfinding loop.Consider testing this function on the largest expected maps to ensure the default limit provides adequate safety without degrading user experience. If timeouts occur, consider lowering the default or making it configurable based on map size.
236-246: LGTM: Water-based pathfinding correctly integrated.The refactor properly uses the new water-aware search for both source and destination shores, with appropriate fallback to
closestShoreTNfor terra nullis.
291-298: Water-based search correctly replaces sampling approach.The unified water search is cleaner than the previous MiniAStar + sampling logic. The validation at line 297 correctly ensures the result is a valid shore owned by the player.
Note:
maxSteps: 500_000is still quite high; consider reducing if performance testing reveals issues.
| const spawn = closestShoreFromPlayerByWater(game, player, dst, { | ||
| waterFilter: (t) => game.isLake(t), | ||
| maxSteps: 3_000_000, | ||
| }); | ||
|
|
||
| for (const t of sorted) { | ||
| if (game.owner(t) === player) { | ||
| return transportShipSpawn(game, player, t); | ||
| } | ||
| } | ||
| return false; | ||
| if (spawn === null) return false; | ||
| return transportShipSpawn(game, player, spawn); |
There was a problem hiding this comment.
Excessive maxSteps value may cause performance degradation.
The maxSteps: 3_000_000 is extremely large and could cause significant delays or lock up the game on maps with large lake networks. This is 12× the default value of 250,000.
Consider reducing this to a more reasonable value (e.g., 500,000 or less) or making it proportional to map size. If long searches are truly necessary, consider adding progress indicators or moving this to a web worker to avoid blocking the main thread.
🤖 Prompt for AI Agents
In src/core/game/TransportShipUtils.ts around lines 201 to 207 the explicit
maxSteps value is set to an excessively large 3_000_000 which can freeze or
severely slow the game on large lake networks; change this to a safer default
(e.g., 500_000 or lower) or compute it proportionally from map size (for example
derive maxSteps from total tile count or map dimensions) and use that computed
value here; if long searches are required, move the search off the main thread
(web worker) or make it incremental with progress indicators to avoid blocking;
update the call to closestShoreFromPlayerByWater to use the new
bounded/proportional maxSteps.
| const spawn = closestShoreFromPlayerByWater(game, player, targetTile, { | ||
| maxSteps: 3_000_000, | ||
| }); |
There was a problem hiding this comment.
Excessive maxSteps value here as well.
Same concern as in canBuildTransportShip: the maxSteps: 3_000_000 could cause performance issues on large maps.
Reduce to a more reasonable value or make it configurable based on map characteristics.
| maxY: null, | ||
| }; | ||
|
|
||
| maxY = 1; |
There was a problem hiding this comment.
Remove dead code: maxY = 1; assignment.
This assignment appears to be leftover debug code or a copy-paste error. The value is never read before being potentially overwritten at line 343-344.
🔎 Remove dead code
- maxY = 1;
const borderShoreTiles = Array.from(player.borderTiles()).filter((t) =>
gm.isShore(t),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| maxY = 1; | |
| const borderShoreTiles = Array.from(player.borderTiles()).filter((t) => | |
| gm.isShore(t), | |
| ); |
🤖 Prompt for AI Agents
In src/core/game/TransportShipUtils.ts around line 320, there is a dead
assignment "maxY = 1;" that is never read before being potentially overwritten
later (lines ~343-344); remove that line entirely and run tests/lint to ensure
no other code relies on that temporary value and that variable
initialization/usage remains correct where maxY is actually set.
3 participants
Description:
Replace Manhattan-based shoreline sampling and per-candidate path checks with a single water-expansion search (multi-source, multi-target) that finds the nearest reachable shoreline by true boat distance.
Describe the PR.
The current boat pathfinding struggles with large disparities between straightline distance and real travel distance. This is because the heuristics based approach didn't punish land tiles enough.
The way I solved for this was by having a fill approach to path finding. Now, the target pixel will select an adjacent water node and check to see if it's a valid expansion. It will continue doing this until it eventually reaches the first shoreline pixel for the player. It then calls the AStar to determine what the ideal route would be.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
gravityswitch