HPA* Pathfinding #2815
HPA* Pathfinding #2815
Conversation
WalkthroughAdds a NavMesh-based pathfinding subsystem (gateway graph, FastA*/BFS, NavMesh + adapters) with a MiniA* fallback; integrates NavMesh into Game behind a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Game
participant Config
participant NavMesh
participant GatewayGraph
participant FastAStar
Game->>Config: disableNavMesh()
alt NavMesh enabled
Game->>NavMesh: new NavMesh(game, options)
NavMesh->>GatewayGraph: build graph (GatewayGraphBuilder)
GatewayGraph->>FastAStar: provide adapters
FastAStar-->>GatewayGraph: routes/edges
NavMesh->>NavMesh: create planners & cache
Game->>Game: store _navMesh
else NavMesh disabled
Game->>Game: _navMesh = null
end
sequenceDiagram
autonumber
participant Ship
participant PathFinderLegacy as MiniPathFinder
participant PathFinderNew as PathFinder (PathFinders.Water)
participant Game
Ship->>PathFinderLegacy: nextTile(from,to)
PathFinderLegacy-->>Ship: { type: Pending/NextTile/Completed/PathNotFound, node? }
Ship->>Ship: handle legacy cases
Ship->>PathFinderNew: next(from,to)
PathFinderNew-->>Ship: { status: PENDING/NEXT/COMPLETE/NOT_FOUND, node? }
Ship->>Ship: handle new PathStatus (move / complete / deactivate)
Ship->>Game: update position / delete unit if NOT_FOUND
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
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. 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: 7
🤖 Fix all issues with AI agents
In @src/core/game/GameImpl.ts:
- Around line 110-113: Wrap the NavMesh construction/initialization in a
try-catch to prevent constructor crashes: when !_config.disableNavMesh() create
the NavMesh instance but call its initialize() inside a try block, and on any
thrown error log the failure (including the error details) and set this._navMesh
= null so code relying on PathFinders.Water can fall back to legacy pathfinding;
ensure you reference NavMesh, initialize(), this._navMesh and
_config.disableNavMesh() in the fix and keep the Game constructor robust by not
rethrowing the error.
In @src/core/pathfinding/navmesh/FastAStarAdapter.ts:
- Around line 19-28: Add a comment inside the heuristic method explaining the
fallback to returning 0 when this.graph.getGateway(node) or
this.graph.getGateway(goal) returns undefined: state that returning 0 disables
the heuristic and effectively falls back to Dijkstra, note this is intentional
for invalid or non-gateway node IDs, and briefly document scenarios when
graph.getGateway(...) may return undefined (e.g., out-of-range IDs, removed
nodes, or non-navigable nodes) so future maintainers understand the trade-off;
place the comment near the existing getGateway checks in heuristic and reference
the methods heuristic and graph.getGateway for clarity.
In @src/core/pathfinding/navmesh/NavMesh.ts:
- Around line 54-55: The code uses Math.max with a single argument when
computing gatewayCount: replace "Math.max(this.graph.getAllGateways().length)"
with the raw length value by directly assigning gatewayCount =
this.graph.getAllGateways().length so that gatewayCount correctly reflects the
number of gateways; then instantiate this.gatewayAStar = new
FastAStar(gatewayCount) as before (update the expression in the NavMesh
constructor or where gatewayCount is defined).
In @tests/pathfinding/playground/api/pathfinding.ts:
- Around line 25-37: The pfMiniCache stores MiniAStarAdapter instances tied to a
specific game, but clearCache() currently only clears the map cache and leaves
pfMiniCache stale; update clearCache() to invoke clearAdapterCaches() (which
clears pfMiniCache) so that adapters are reset whenever maps are cleared,
preventing getPfMiniAdapter/mapName from returning adapters bound to an old game
instance.
In @tests/pathfinding/playground/public/styles.css:
- Around line 442-449: There are two conflicting @keyframes named "spin" (used
for the refresh icon and the loading spinner), so rename one of them (e.g.,
"spin-refresh" or "spin-loader") and update any corresponding animation
references (CSS classes or properties that use animation: spin ...) to the new
name; ensure the unique symbol you change matches where it's applied (e.g., the
refresh icon rule and the loading-spinner rule) so the intended animation isn't
overridden.
In @tests/pathfinding/playground/README.md:
- Around line 9-15: The fenced code block in
tests/pathfinding/playground/README.md is missing a language identifier; update
the opening fence to specify "bash" (i.e., change the triple backtick line that
begins the block to ```bash) so the snippet is highlighted correctly; ensure the
closing backticks remain unchanged and that no other content is modified.
In @tests/pathfinding/playground/server.ts:
- Around line 87-104: The try/catch around the
app.get("/api/maps/:name/thumbnail") handler does not catch asynchronous
sendFile errors; modify the handler to call res.sendFile(thumbnailPath, (err) =>
{ ... }) and handle the error inside that callback (log the error with
req.params.name, and send an appropriate status and JSON body using err.status
|| 404 and err.message). Keep or use a small try/catch only for synchronous path
construction, but ensure asynchronous errors from res.sendFile are handled in
the callback (or forwarded to next(err)) referencing sendFile, thumbnailPath,
req and res.
🧹 Nitpick comments (24)
tests/pathfinding/playground/README.md (1)
23-25: Consider maintainability implications of generated code.The disclosure states the code is "mostly generated by a robot and therefore probably unmaintainable by humans." While this is acceptable for a one-off playground, it could create technical debt if this tool needs future updates or bug fixes. Consider adding documentation about the generation process or key entry points for anyone who needs to maintain this later.
src/core/pathfinding/adapters/MiniAStarAdapter.ts (1)
45-45: Document or make maxSteps configurable.The
maxSteps = 100_000limit is hardcoded without explanation. This prevents infinite loops but could fail for legitimate long-distance paths on large maps. Consider:
- Making it configurable via options
- Adding a comment explaining the limit choice
- Logging a warning when the limit is reached
src/core/pathfinding/navmesh/FastBFS.ts (3)
19-35: Consider validating start node before search.The search begins by marking the start node as visited without checking if it's a valid node via
isValidNode(). If the start node is invalid (e.g., water when searching for land), the search will still add it to the queue and visit it.💡 Proposed validation
search<T>( width: number, height: number, start: number, maxDistance: number, isValidNode: FastBFSAdapter<T>["isValidNode"], visitor: FastBFSAdapter<T>["visitor"], ): T | null { + // Validate start node + if (!isValidNode(start)) { + return null; + } + const stamp = this.nextStamp(); const lastRowStart = (height - 1) * width;
45-58: Visitor return semantics use three different types.The visitor callback uses three different return types (
T | null | undefined) with distinct meanings:
- Returns
T: Target found- Returns
null: Reject node, skip neighbors- Returns
undefined: Continue exploringWhile this works, using a tagged union would be clearer and more type-safe:
type VisitorResult<T> = | { action: 'found'; value: T } | { action: 'reject' } | { action: 'continue' };This eliminates ambiguity and makes the intent explicit at call sites.
107-117: Stamp overflow handling is expensive but correct.The overflow check and
visitedStamp.fill(0)at lines 110-114 handles the extremely rare case of stamp wrapping after ~4 billion searches. While correct, this will cause a performance spike when it occurs. Consider logging when this happens to track if it's ever reached in practice.📊 Add telemetry
if (this.stamp === 0) { // Overflow - reset (extremely rare) + console.warn('FastBFS stamp overflow - resetting visited tracking'); this.visitedStamp.fill(0); this.stamp = 1; }tests/pathfinding/README.md (1)
1-152: Consider markdown formatting fixes.The documentation is clear and comprehensive. Consider these optional improvements for consistency:
- Add language specifiers to fenced code blocks at lines 14 and 71 (use
textorplaintext)- Wrap the bare URL at line 152 in angle brackets:
<http://localhost:5555>📝 Formatting improvements
-``` +```text tests/pathfinding/-``` +```text ================================================================================-Then open http://localhost:5555 in your browser. +Then open <http://localhost:5555> in your browser.tests/pathfinding/benchmark/scenarios/default.ts (1)
3-30: Consider using Record<> for clearer type intent.The index signature
{ [k: string]: [number, number] }works fine, butRecord<string, [number, number]>is slightly more idiomatic in TypeScript and expresses the intent more clearly.♻️ Optional type refinement
-export const PORTS: { [k: string]: [number, number] } = { +export const PORTS: Record<string, [number, number]> = { Miami: [1014, 694], Boston: [1120, 498],tests/pathfinding/benchmark/generate.ts (2)
73-79: Shuffle uses biased sort comparator.
array.sort(() => Math.random() - 0.5)does not produce uniform distribution. For benchmark data this is likely acceptable, but for truly random sampling consider Fisher-Yates shuffle.🔧 Optional: Use Fisher-Yates for unbiased shuffle
- const shuffled = shorelinePorts.sort(() => Math.random() - 0.5); - for (let i = 0; i < numPortsToSelect; i++) { - selectedPorts.push(shuffled[i]); - } + // Fisher-Yates shuffle (in-place) + for (let i = shorelinePorts.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)); + [shorelinePorts[i], shorelinePorts[j]] = [shorelinePorts[j], shorelinePorts[i]]; + } + for (let i = 0; i < numPortsToSelect; i++) { + selectedPorts.push(shorelinePorts[i]); + }
267-285: Move interface definitions before usage.The
Port,Route, andTemplateParamsinterfaces are defined after they're used ingenerateScenarioForMap. TypeScript hoisting handles this, but placing interfaces at the top (after imports) improves readability.tests/pathfinding/playground/public/styles.css (1)
127-130: Add visible focus indicator for keyboard accessibility.The select focus state removes the outline but only adds a border color change. Consider adding a more visible focus indicator for keyboard users.
.welcome-selector select:focus { outline: none; border-color: #0066cc; + box-shadow: 0 0 0 2px rgba(0, 102, 204, 0.3); }tests/pathfinding/playground/server.ts (2)
246-253: Use proper Express types for error middleware.The
nextparameter usesanytype. ImportNextFunctionfrom Express for proper typing.-import express, { Request, Response } from "express"; +import express, { NextFunction, Request, Response } from "express"; // ... -app.use((err: Error, req: Request, res: Response, next: any) => { +app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
118-166: Consider extracting shared validation logic.The coordinate validation in
/api/pathfindand/api/pathfind-pfminiis identical. A small helper function would reduce duplication.🔧 Extract validation helper
function validatePathfindRequest(body: any): { error: string; message: string } | null { const { map, from, to } = body; if (!map || !from || !to) { return { error: "Invalid request", message: "Missing required fields: map, from, to" }; } if (!Array.isArray(from) || from.length !== 2 || !Array.isArray(to) || to.length !== 2) { return { error: "Invalid coordinates", message: "from and to must be [x, y] coordinate arrays" }; } return null; }Also applies to: 179-226
src/core/pathfinding/navmesh/WaterComponents.ts (3)
125-131: Consider usingset()for typed array copy.The manual loop works, but typed arrays have a built-in
set()method that may be faster for bulk copies.♻️ Suggested improvement
private upgradeToUint16Array(ids: Uint8Array): Uint16Array { const newIds = new Uint16Array(this.numTiles); - for (let i = 0; i < this.numTiles; i++) { - newIds[i] = ids[i]; - } + newIds.set(ids); return newIds; }
196-199: Nullish coalescing on array index is redundant here.
this.componentIds[tile]returnsundefinedonly whentileis out of bounds. If that happens, returning0hides a bug. Consider throwing or logging for invalid tiles.♻️ Suggested improvement
getComponentId(tile: TileRef): number { if (!this.componentIds) return 0; - return this.componentIds[tile] ?? 0; + return this.componentIds[tile]; }
89-119: Direct terrain access is an intentional optimization with documented performance trade-off.The
as anycast is necessary for a 30–50% performance gain (reducing initialization from 15ms to 10ms for large maps). There is already a fallback path: the constructor'saccessTerrainDirectlyparameter defaults totruebut allows callers to use the slower, type-safeisWater()method instead.If terrain access becomes a common pattern in the codebase, consider adding a typed getter method to
GameMap(e.g.,rawTerrain()) to avoid repeatedas anycasts. For now, the existing fallback mechanism is sufficient.tests/pathfinding/playground/api/pathfinding.ts (1)
14-14: Consider stronger typing instead ofany.Several places use
anytype which loses type safety:
- Line 14:
timings: any- Line 31:
game: any- Line 44:
game: any- Line 82:
timings: anyFor a test utility this is acceptable, but consider using the actual
Gametype from imports.Also applies to: 31-31, 44-44, 82-82
src/core/pathfinding/navmesh/FastAStarAdapter.ts (2)
13-17: Linear search in getCost may be slow for high-degree nodes.
getCostiterates through all edges withfind(). If nodes have many neighbors, this becomes O(n) per neighbor lookup during A* expansion. Consider using a Map for O(1) edge lookup if this becomes a bottleneck.♻️ Alternative using pre-computed edge map
// In GatewayGraphAdapter, consider caching edges as Map<number, Map<number, number>> // where outer key is 'from' node, inner key is 'to' node, value is cost getCost(from: number, to: number): number { return this.edgeMap.get(from)?.get(to) ?? 1; }
101-103: Uniform cost ignores diagonal movement.
getCostreturns 1 for all moves, but diagonal moves cover ~1.41x the distance of cardinal moves. If path quality matters, consider returningMath.SQRT2for diagonal neighbors.tests/pathfinding/utils.ts (2)
167-168: Global console.debug mutation affects all tests.Setting
console.debug = () => {}mutates a global object which persists across tests. Consider using a spy/mock that can be restored, or scoping this to specific tests.♻️ Suggested approach using saved reference
+const originalDebug = console.debug; + export async function setupFromPath( mapDirectory: string, mapName: string, gameConfig: Partial<GameConfig> = {}, humans: PlayerInfo[] = [], ): Promise<Game> { // Suppress console.debug for tests console.debug = () => {}; + // Consider: restore with originalDebug after setup if needed
109-121: Division by zero is safe here but consider edge case.When
executionsis 0, this returnsInfinity. While unlikely to be called with 0, a guard would make the function more defensive.♻️ Suggested guard
export function measureExecutionTime( adapter: PathFinder, route: BenchmarkRoute, executions: number = 1, ): number | null { + if (executions <= 0) return null; + const { time } = measureTime(() => {tests/pathfinding/playground/api/maps.ts (3)
87-93: Manifest warning may be noisy.If most map folders lack a
manifest.json, this warning will appear for every map on every call. Consider removing the warning or making it conditional on a debug flag, since the fallback toformatMapNameis the expected behavior.♻️ Suggested change
} catch (e) { // If manifest doesn't exist or doesn't have name, use formatted folder name - console.warn( - `Could not read manifest for ${name}:`, - e instanceof Error ? e.message : e, - ); + // Silently fall back to formatted folder name }
108-129: Missing error handling for map loading failures.If
setupFromPathornavMesh.initialize()throws, the error propagates without cleanup. The function should either handle errors or document that callers must handle them.♻️ Suggested improvement
export async function loadMap(mapName: string): Promise<MapCache> { // Check cache first if (cache.has(mapName)) { return cache.get(mapName)!; } const mapsDir = getMapsDirectory(); - // Use the existing setupFromPath utility to load the map - const game = await setupFromPath(mapsDir, mapName); + let game: Game; + try { + game = await setupFromPath(mapsDir, mapName); + } catch (e) { + throw new Error(`Failed to load map "${mapName}": ${e instanceof Error ? e.message : e}`); + } // Initialize NavMesh const navMesh = new NavMesh(game, { cachePaths: config.cachePaths }); navMesh.initialize();
147-149: Fragile access to private NavMesh internals.Using
(navMesh as any).graphbypasses type safety. IfNavMeshchanges its internal structure, this code breaks without compile-time warning. Consider adding a public method likegetDebugGraph()toNavMeshfor this purpose.src/core/pathfinding/navmesh/GatewayGraph.ts (1)
508-509: Bidirectional edges pushed to sector.edges.Both
edge1andedge2are pushed regardless of whether they replace existing edges. This meanssector.edgesmay contain obsolete entries when edges are updated. Sincesector.edgesis only used for debug counting (line 198), this is harmless but could be misleading.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
map-generator/assets/test_maps/world/image.pngis excluded by!**/*.pngtests/testdata/maps/world/map.binis excluded by!**/*.bintests/testdata/maps/world/map16x.binis excluded by!**/*.bintests/testdata/maps/world/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (39)
map-generator/assets/test_maps/world/info.jsonmap-generator/main.gosrc/core/Schemas.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/pathfinding/PathFinder.tssrc/core/pathfinding/PathFinding.tssrc/core/pathfinding/adapters/MiniAStarAdapter.tssrc/core/pathfinding/adapters/NavMeshAdapter.tssrc/core/pathfinding/navmesh/FastAStar.tssrc/core/pathfinding/navmesh/FastAStarAdapter.tssrc/core/pathfinding/navmesh/FastBFS.tssrc/core/pathfinding/navmesh/GatewayGraph.tssrc/core/pathfinding/navmesh/NavMesh.tssrc/core/pathfinding/navmesh/WaterComponents.tstests/core/executions/TradeShipExecution.test.tstests/core/pathfinding/PathFinder.test.tstests/core/pathfinding/utils.tstests/pathfinding/README.mdtests/pathfinding/benchmark/generate.tstests/pathfinding/benchmark/run.tstests/pathfinding/benchmark/scenarios/default.tstests/pathfinding/benchmark/scenarios/synthetic/giantworldmap.tstests/pathfinding/playground/README.mdtests/pathfinding/playground/api/maps.tstests/pathfinding/playground/api/pathfinding.tstests/pathfinding/playground/public/client.jstests/pathfinding/playground/public/index.htmltests/pathfinding/playground/public/styles.csstests/pathfinding/playground/server.tstests/pathfinding/utils.tstests/perf/AstarPerf.tstests/testdata/maps/world/manifest.jsontests/testdata/maps/world/thumbnail.webp
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/configuration/Config.ts
📚 Learning: 2025-05-22T23:43:54.789Z
Learnt from: jrouillard
Repo: openfrontio/OpenFrontIO PR: 848
File: src/core/execution/TradeShipExecution.ts:110-134
Timestamp: 2025-05-22T23:43:54.789Z
Learning: In the OpenFrontIO project, MiniAStar.compute() only returns PathFindResultType.Pending, PathFindResultType.Completed, or PathFindResultType.PathNotFound - it doesn't return PathFindResultType.NextTile. When PathFindResultType.Completed is returned, the full path can be retrieved via reconstructPath().
Applied to files:
src/core/pathfinding/adapters/MiniAStarAdapter.tssrc/core/pathfinding/PathFinding.tssrc/core/pathfinding/PathFinder.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/execution/TradeShipExecution.ts
📚 Learning: 2025-08-19T11:00:55.422Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1864
File: resources/maps/arabianpeninsula/manifest.json:13-170
Timestamp: 2025-08-19T11:00:55.422Z
Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.
Applied to files:
tests/testdata/maps/world/manifest.jsonmap-generator/assets/test_maps/world/info.json
📚 Learning: 2025-08-10T14:37:35.777Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1770
File: resources/maps/world/manifest.json:279-355
Timestamp: 2025-08-10T14:37:35.777Z
Learning: The OpenFrontIO World Map includes both real-world nations and fictional/creative entities like "Polar Bears" as playable nations. These special nations may use non-two-letter flag codes (e.g., "polar_bears" instead of a two-letter code), and this is intentional design. The corresponding flag assets are stored in resources/flags/ with matching filenames.
Applied to files:
tests/testdata/maps/world/manifest.jsonmap-generator/assets/test_maps/world/info.json
📚 Learning: 2025-05-22T23:48:51.113Z
Learnt from: jrouillard
Repo: openfrontio/OpenFrontIO PR: 848
File: src/core/execution/TradeShipExecution.ts:125-127
Timestamp: 2025-05-22T23:48:51.113Z
Learning: In TradeShipExecution, the path length is already checked in the computeNewPath() function. If the path is empty, it returns PathFindResultType.PathNotFound instead of PathFindResultType.Completed, making additional empty path checks unnecessary when handling the Completed case.
Applied to files:
tests/core/executions/TradeShipExecution.test.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/execution/TradeShipExecution.ts
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.
Applied to files:
tests/pathfinding/benchmark/scenarios/default.tstests/pathfinding/benchmark/scenarios/synthetic/giantworldmap.tsmap-generator/assets/test_maps/world/info.json
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-11-29T22:22:37.178Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Applied to files:
src/core/pathfinding/navmesh/NavMesh.tssrc/core/pathfinding/navmesh/FastAStarAdapter.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
tests/pathfinding/playground/public/index.html
📚 Learning: 2025-12-29T23:33:17.920Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:17.920Z
Learning: In Tailwind CSS v4, update blur-related utilities in HTML templates. The mappings are: backdrop-blur-sm (v3) -> backdrop-blur-xs (v4); backdrop-blur (bare) -> backdrop-blur-sm; blur-sm -> blur-xs. Apply these changes to all HTML files (e.g., index.html and other templates) to reflect the v4 naming. Consider updating a project-wide search/replace or using a codemod to ensure consistency across the codebase.
Applied to files:
tests/pathfinding/playground/public/index.html
📚 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/execution/TransportShipExecution.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/core/game/GameImpl.ts
🧬 Code graph analysis (19)
tests/core/pathfinding/utils.ts (2)
src/core/game/Game.ts (1)
Game(686-786)src/core/game/GameMap.ts (1)
GameMapImpl(58-369)
src/core/pathfinding/navmesh/WaterComponents.ts (1)
src/core/game/GameMap.ts (2)
GameMap(6-56)TileRef(3-3)
src/core/pathfinding/navmesh/FastAStar.ts (1)
src/core/game/GameImpl.ts (1)
neighbors(903-905)
src/core/pathfinding/adapters/MiniAStarAdapter.ts (3)
src/core/pathfinding/PathFinder.ts (3)
PathFinder(19-22)MiniAStarOptions(24-28)PathResult(13-17)src/core/pathfinding/PathFinding.ts (1)
MiniPathFinder(117-217)src/core/game/GameMap.ts (1)
TileRef(3-3)
tests/pathfinding/benchmark/scenarios/default.ts (1)
tests/pathfinding/benchmark/scenarios/synthetic/giantworldmap.ts (3)
MAP_NAME(1-1)PORTS(3-204)ROUTES(206-1207)
src/core/game/Game.ts (1)
src/core/pathfinding/navmesh/NavMesh.ts (1)
NavMesh(27-819)
src/core/pathfinding/adapters/NavMeshAdapter.ts (5)
src/core/pathfinding/PathFinder.ts (2)
PathFinder(19-22)PathResult(13-17)src/core/pathfinding/navmesh/NavMesh.ts (1)
NavMesh(27-819)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/Game.ts (1)
Game(686-786)src/core/game/GameImpl.ts (1)
navMesh(967-969)
tests/pathfinding/benchmark/scenarios/synthetic/giantworldmap.ts (1)
tests/pathfinding/benchmark/scenarios/default.ts (2)
PORTS(3-30)ROUTES(32-55)
tests/pathfinding/benchmark/run.ts (1)
tests/pathfinding/utils.ts (8)
getScenario(59-92)getAdapter(45-57)printHeader(154-159)printRow(141-148)BenchmarkResult(30-34)measurePathLength(94-100)calculateStats(123-139)measureExecutionTime(109-121)
src/core/pathfinding/PathFinding.ts (4)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/pathfinding/AStar.ts (1)
AStar(1-4)src/core/game/Game.ts (1)
Game(686-786)src/core/pathfinding/MiniAStar.ts (1)
MiniAStar(41-101)
src/core/pathfinding/navmesh/NavMesh.ts (5)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/pathfinding/navmesh/GatewayGraph.ts (3)
GatewayGraph(40-81)GatewayGraphBuilder(83-587)Gateway(6-12)src/core/pathfinding/navmesh/FastAStar.ts (1)
FastAStar(88-202)src/core/pathfinding/navmesh/FastAStarAdapter.ts (2)
GatewayGraphAdapter(5-29)BoundedGameMapAdapter(31-114)src/core/pathfinding/adapters/NavMeshAdapter.ts (1)
next(20-73)
tests/pathfinding/playground/public/client.js (2)
src/core/game/GameImpl.ts (5)
x(852-854)y(855-857)isWater(915-917)map(206-208)navMesh(967-969)src/core/execution/TradeShipExecution.ts (1)
isActive(177-179)
tests/perf/AstarPerf.ts (1)
src/core/pathfinding/PathFinding.ts (1)
MiniPathFinder(117-217)
src/core/execution/TransportShipExecution.ts (1)
src/core/pathfinding/PathFinder.ts (1)
PathFinders(30-43)
src/core/game/GameImpl.ts (1)
src/core/pathfinding/navmesh/NavMesh.ts (1)
NavMesh(27-819)
src/core/execution/WarshipExecution.ts (1)
src/core/pathfinding/PathFinder.ts (1)
PathFinders(30-43)
tests/pathfinding/utils.ts (6)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/pathfinding/PathFinder.ts (2)
PathFinder(19-22)PathFinders(30-43)src/core/game/GameImpl.ts (3)
width(861-863)config(345-347)createGame(47-56)src/core/Schemas.ts (1)
GameConfig(90-90)src/core/game/TerrainMapLoader.ts (2)
MapManifest(19-25)genTerrainFromBin(74-90)tests/util/TestConfig.ts (1)
TestConfig(12-88)
tests/pathfinding/playground/api/pathfinding.ts (4)
src/core/pathfinding/adapters/MiniAStarAdapter.ts (1)
MiniAStarAdapter(15-66)src/core/game/GameMap.ts (1)
TileRef(3-3)tests/pathfinding/playground/api/maps.ts (1)
loadMap(108-129)src/core/game/GameImpl.ts (3)
miniMap(209-211)x(852-854)y(855-857)
src/core/execution/TradeShipExecution.ts (1)
src/core/pathfinding/PathFinder.ts (1)
PathFinders(30-43)
🪛 markdownlint-cli2 (0.18.1)
tests/pathfinding/playground/README.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
tests/pathfinding/README.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
152-152: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
🔇 Additional comments (88)
map-generator/assets/test_maps/world/info.json (1)
1-310: LGTM!The world map data structure is consistent and follows established patterns. The use of non-standard flag codes like "polar_bears" is intentional design per project conventions. Nation names are correctly included without requiring translation keys.
src/core/pathfinding/PathFinding.ts (1)
117-142: Clean refactoring to remove factory injection.The change from constructor-injected
newAStarcallback to a privatecreateAStar()method simplifies the class while maintaining the same functionality. The direct instantiation ofMiniAStarmakes the code easier to follow.src/core/pathfinding/adapters/MiniAStarAdapter.ts (1)
42-65: Add destination to path when pathfinding completes.When
next()returnsCOMPLETEat line 50, the method returns the path without verifying the destination tile is included. SinceCOMPLETEmeans pathfinding finished (with the unit close enough to the destination), the final destination should be in the returned path.for (let i = 0; i < maxSteps; i++) { const result = this.next(current, to); if (result.status === PathStatus.COMPLETE) { + if (current !== to) { + path.push(to); + } return path; } if (result.status === PathStatus.NOT_FOUND) { return null; } if (result.status === PathStatus.NEXT) { current = result.node; path.push(current); } }⛔ Skipped due to learnings
Learnt from: jrouillard Repo: openfrontio/OpenFrontIO PR: 848 File: src/core/execution/TradeShipExecution.ts:110-134 Timestamp: 2025-05-22T23:43:54.789Z Learning: In the OpenFrontIO project, MiniAStar.compute() only returns PathFindResultType.Pending, PathFindResultType.Completed, or PathFindResultType.PathNotFound - it doesn't return PathFindResultType.NextTile. When PathFindResultType.Completed is returned, the full path can be retrieved via reconstructPath().Learnt from: jrouillard Repo: openfrontio/OpenFrontIO PR: 848 File: src/core/execution/TradeShipExecution.ts:125-127 Timestamp: 2025-05-22T23:48:51.113Z Learning: In TradeShipExecution, the path length is already checked in the computeNewPath() function. If the path is empty, it returns PathFindResultType.PathNotFound instead of PathFindResultType.Completed, making additional empty path checks unnecessary when handling the Completed case.src/core/configuration/Config.ts (1)
82-82: LGTM! Clean interface extension.The method signature follows the existing pattern and integrates well with other configuration flags.
src/core/game/Game.ts (1)
2-2: LGTM! Clean API extension.The import and method signature properly expose NavMesh as an optional feature. The nullable return type correctly reflects that NavMesh may not be initialized depending on configuration.
Also applies to: 785-785
src/core/Schemas.ts (1)
179-179: LGTM! Proper schema extension.The optional boolean field follows Zod v4 conventions and integrates cleanly with the existing GameConfigSchema. The naming is consistent with similar flags like
disableNations.src/core/configuration/DefaultConfig.ts (1)
314-316: LGTM! Clean implementation.The method correctly returns the config value with a sensible default (
false= NavMesh enabled). The implementation follows the established pattern for boolean configuration flags.map-generator/main.go (1)
71-71: No action needed. The "world" map appearing twice in the registry (line 62 for production, line 71 withIsTest: true) is intentional design. The code comment at lines 18-20 documents that "each entry contains the folder name and a flag indicating if it's a test map." The processing function correctly handles this by passing bothmapItem.NameandmapItem.IsTesttoprocessMap()(line 229), treating the combination as a unique identifier. This pattern allows the same map to be processed for different contexts (test and production).src/core/game/GameImpl.ts (3)
3-3: Clean import.
90-90: Proper nullable field declaration.
967-969: Clean accessor implementation.tests/core/executions/TradeShipExecution.test.ts (3)
3-3: Import updated correctly for new API.
87-87: Mock correctly reflects new PathResult shape.
118-118: Completion scenario mock is correct.tests/testdata/maps/world/manifest.json (1)
1-325: Valid test data structure.The manifest structure is correct with proper map dimensions and scaled variants. The duplicate flag codes (Russia/Siberia, Antarctica regions) and special flag code (polar_bears) align with existing World Map design patterns from the learnings.
src/core/execution/TransportShipExecution.ts (4)
14-14: Import updated for new pathfinding API.
72-72: Clean initialization with automatic fallback.The
PathFinders.Water(mg)factory correctly falls back to legacy pathfinding when NavMesh is unavailable, ensuring backward compatibility without additional conditional logic.
226-226: Pathfinding call updated to new API.
227-283: Switch statement correctly updated to PathStatus enum.All pathfinding states are handled with logic unchanged from the original implementation. The mechanical refactor from PathFindResultType to PathStatus maintains the same behavior.
tests/pathfinding/playground/public/index.html (1)
1-315: Well-structured playground UI.The HTML is clean, semantic, and properly organized. Good separation of concerns with external CSS and JS references.
tests/pathfinding/benchmark/scenarios/default.ts (1)
32-55: Good type safety for routes.The
Array<[keyof typeof PORTS, keyof typeof PORTS]>type ensures route endpoints are valid port names at compile time. This prevents typos and invalid references.tests/core/pathfinding/utils.ts (4)
30-40: Good input validation.Clear error message helps debug malformed test maps.
50-54: Verify that uniform Plains terrain is sufficient for tests.Setting only
LAND_BITresults in magnitude 0, making all land tiles Plains (perGameMap.terrainType()). If pathfinding tests need varied terrain types (Highland, Mountain) to test movement costs, consider allowing magnitude specification in the input format.
65-100: Minimap aggregation logic is sound.The "water if any tile in 2x2 block is water" approach is conservative and appropriate for pathfinding. Edge case handling for non-even dimensions is correct.
109-134: Config enables NavMesh by default for tests.Setting
disableNavMesh: falseensures test utilities exercise the new NavMesh pathfinding. This is appropriate for validating the new feature.tests/perf/AstarPerf.ts (1)
4-31: Clean API migration.The change from
PathFinder.Mini(...)tonew MiniPathFinder(...).nextTile(...)is straightforward and makes the API more explicit. The benchmarks remain consistent.src/core/pathfinding/adapters/NavMeshAdapter.ts (4)
12-18: Constructor properly validates NavMesh availability.Throwing an error if
navMeshis unavailable is appropriate defensive programming. The adapter cannot function without it.
20-39: Input validation and edge cases handled well.Type checks, validity checks, and early returns for trivial cases (same position, within distance threshold) are all appropriate.
41-63: Path caching and deviation handling are sound.Resetting the cache when destination changes and recomputing when the unit deviates from the expected path are both important for handling dynamic environments.
79-98: Path caching logic is correct.The try-catch handles potential errors from
navMesh.findPath(). Skipping the first tile (lines 93-95) when it matchesfromis correct since we want the next tile, not the current position.src/core/execution/TradeShipExecution.ts (3)
11-11: Clean import update for new PathFinder API.Good consolidation of pathfinding imports from the new module.
28-31: Factory method simplifies pathfinder creation.Using
PathFinders.Water(mg)is cleaner than the old approach. The factory handles NavMesh availability internally (falls back to legacy if unavailable per the relevant code snippet fromPathFinder.ts).
104-129: Well-structured migration to new PathStatus API.The switch statement covers all
PathStatuscases cleanly:
PENDING: re-renders unit at current position (same as before).NEXT: updates safety, moves, increments counter.COMPLETE: finishes execution.NOT_FOUND: cleans up gracefully.One small note: Line 113 and 116 access
result.nodeafter confirmingresult.status === PathStatus.NEXT. TypeScript should narrow the type, but if type inference struggles, consider a type guard or destructuring after the status check.Based on learnings, path length validation happens upstream, so this logic is sound.
tests/pathfinding/benchmark/generate.ts (2)
1-29: Clear script structure with good documentation.Usage examples and constants are well-defined. The shebang allows direct execution with node.
177-259: Main function handles CLI arguments well.Good separation of
--allmode vs single map mode with proper error messages and exit codes.tests/core/pathfinding/PathFinder.test.ts (4)
1-34: Well-organized test setup with parameterized adapters.Using
describe.eachto run identical tests against bothMiniAStarAdapterandNavMeshAdapterensures consistent behavior. The sharedworldGameinstance inbeforeAllavoids repeated expensive setup.
159-172: Type assertion needed for discriminated union.The cast
(result as { node: TileRef }).nodeon lines 167-168 and 202-206 is required because TypeScript doesn't narrowresultinside the loop after checkingresult.status. Consider extracting to a helper or using a type guard if this pattern repeats elsewhere.
318-332: "Known bugs" tests only use MiniAStarAdapter.These tests hardcode
MiniAStarAdapterinstead of using the parameterizedcreatefunction. If this is intentional (documenting MiniAStar-specific quirks), add a comment explaining why. Otherwise, usecreate(game)for consistency.
214-242: Good error handling tests with console spy.Suppressing console.error output during null-input tests keeps test output clean while verifying behavior. The spy is properly restored after each test.
tests/pathfinding/playground/public/styles.css (1)
1-12: Solid CSS foundation for playground UI.Universal reset with
box-sizing: border-boxand dark theme setup looks good for developer tooling.tests/pathfinding/playground/server.ts (1)
1-36: Clean Express server setup.Good use of compression middleware and appropriate JSON body limit for large path data.
tests/pathfinding/benchmark/run.ts (3)
1-43: Well-structured benchmark runner with clear defaults.Good separation of imports, constants, and type definitions. The
RunOptionsinterface is simple and clear.
45-180: Thorough benchmark execution with three metrics.The
runScenariofunction cleanly measures:
- Initialization time (includes NavMesh setup for default adapter)
- Path distance (total tiles)
- Pathfinding time (averaged over iterations)
Silent mode output is well-formatted for batch processing.
207-282: CLI argument handling covers all use cases.Good handling of
--synthetic,--all, and--silentflags with appropriate error messages when required arguments are missing.tests/pathfinding/benchmark/scenarios/synthetic/giantworldmap.ts (2)
1-10: Auto-generated benchmark scenario data.This file follows the same structure as
default.tswith proper TypeScript types. TheMAP_NAMEvalue "giantworldmap" is correct per project conventions. Based on learnings, this key name is used consistently across all language files.
206-1207: Large route dataset for comprehensive benchmarking.The routes array provides good coverage with connections between sequential ports plus additional cross-connections. This pattern exercises the pathfinding system well.
src/core/execution/WarshipExecution.ts (4)
11-11: LGTM!Clean import update aligns with the new PathFinder abstraction. The typed union
PathStatusand factoryPathFindersare the right pattern here.
29-29: LGTM!Good use of
PathFinders.Water(mg)which handles NavMesh availability automatically with fallback to legacy pathfinder when needed.
179-199: LGTM!The switch on
PathStatuscovers all cases correctly. The API migration fromnextTile()tonext()is complete and the result handling matches the new typed union pattern.
211-230: LGTM!Patrol logic correctly uses the new
PathStatusunion. All four status cases are handled properly.src/core/pathfinding/navmesh/WaterComponents.ts (1)
139-190: LGTM!The scan-line flood-fill is well-implemented. Processing horizontal spans improves cache locality, and the queue-based approach avoids stack overflow on large maps.
src/core/pathfinding/PathFinder.ts (3)
6-17: LGTM!Good use of discriminated union for
PathResult. This is cleaner than class hierarchies and gives exhaustive type checking in switch statements.
19-22: LGTM!Clean interface with two focused methods. The optional
distparameter innext()allows early termination when close enough to target.
30-42: LGTM!The factory pattern with automatic fallback to legacy pathfinder when navmesh is unavailable is a good approach. Keeps consumer code simple.
src/core/pathfinding/navmesh/FastAStar.ts (4)
40-76: LGTM!MinHeap implementation is correct. Bubble-up and bubble-down operations properly maintain the heap invariant. Using typed arrays avoids object allocation overhead.
119-152: Edge case: start equals goal returns empty path.When
start === goal, the search immediately returnsreconstructPath(start, goal)which produces[start](a single-element path). This is correct behavior, but worth noting.However, if the goal check happens before any neighbors are processed, this is efficient. The code looks good.
106-117: LGTM!Good handling of stamp overflow. The wrap-around from
2^32 - 1back to0is extremely rare (~4 billion searches) but the reset logic is correct.
184-201: LGTM!Path reconstruction with safety check for broken
cameFromchain is defensive. The early return of empty array prevents infinite loops if data is corrupted.tests/pathfinding/playground/api/pathfinding.ts (1)
53-111: LGTM!The
computePathfunction properly validates inputs, handles debug data extraction, and returns a well-structured result. Good separation between NavMesh path computation and debug info extraction.src/core/pathfinding/navmesh/FastAStarAdapter.ts (2)
58-75: Clamping logic for out-of-bounds tiles is subtle.The special handling for start/goal tiles that are outside bounds (lines 63-74) clamps their coordinates. This means the actual tile position in the path may differ from the logical node position. Ensure downstream code handles this correctly.
84-99: LGTM!Neighbor discovery correctly filters to water tiles and validates bounds. Clean implementation.
tests/pathfinding/utils.ts (3)
45-57: LGTM!Clean adapter factory. The
legacyanddefaultnames are clear, and throwing on unknown adapter prevents silent failures.
59-92: LGTM!The scenario loading and game setup with conditional NavMesh initialization is well-structured. Clean separation of concerns.
161-221: LGTM!The
setupFromPathfunction is comprehensive. Good file existence checks, proper manifest loading, and flexible config spreading.tests/pathfinding/playground/api/maps.ts (5)
1-17: Clean imports and type definitions.The interfaces
MapInfoandMapCacheare simple and well-defined. Good use of typed imports.
19-33: Simple config pattern works well for playground.The spread-based
setConfigallows partial updates cleanly.
35-43: LGTM!Standard ESM pattern for resolving directory paths relative to the module.
45-63: Nice readable string transformation chain.The step-by-step comments make each regex purpose clear.
219-224: LGTM!Simple and does what it says.
src/core/pathfinding/navmesh/NavMesh.ts (7)
136-172: Good early-exit optimization for short paths.Trying a local bounded search before gateway routing makes sense for nearby points. The fallback to gateway graph on failure is correct.
196-241: Clean handling of gateway edge cases.The same-gateway case (lines 216-241) correctly uses multi-sector search instead of gateway routing. Good fallback design.
356-366: Path caching mutates shared graph edges.Storing
segmentPathdirectly onedge.pathandreverseEdge.pathmutates objects owned byGatewayGraph. This works but creates tight coupling. If the graph is shared or reused, cached paths from one query could affect another.For playground/test code this is fine, but worth documenting if used in production.
433-494: Correct BFS-based gateway search.Finding the nearest gateway by water path distance (not straight-line) is the right approach for water navigation.
509-592: Clean bounded search implementation.The separation between single-sector and multi-sector search with appropriately sized buffers is a good optimization.
594-671: Path upscaling with interpolation.The 2x scaling followed by interpolation creates smooth transitions. The start/end fixup ensures exact endpoints.
762-818: Efficient path smoothing with adaptive step size.The step-based look-ahead (
path.length / 20) balances quality vs. speed for long paths. The final-tile check ensures endpoints aren't missed.tests/pathfinding/playground/public/client.js (6)
1-131: Well-structured canvas layering.Using separate canvases for static (map, overlay) and dynamic (interactive) content is good for render performance.
252-265: Throttle implementation is simple but effective.The skip-based throttle ensures max 60fps updates during drag. Final position is guaranteed by mouseup handler (line 426).
666-771: Map switching with proper state cleanup.Good pattern: clear paths, resize canvases, render layers, then restore URL state if requested.
782-877: Clean separation between NavMesh and PF.Mini requests.Each request preserves the other algorithm's result, allowing comparison. Error handling with user feedback is good.
1069-1133: Fast tile rendering with ImageData.Using
createImageDataand direct pixel writes is much faster than individualfillRectcalls. Good optimization for large maps.
1222-1330: Screen-space rendering for crisp interactive elements.Drawing paths and points at screen coordinates (after transform) keeps them sharp at all zoom levels.
src/core/pathfinding/navmesh/GatewayGraph.ts (6)
6-38: Clean data types for gateway graph.Simple interfaces with clear purpose. The
componentIdonGatewayenables the connectivity optimization.
40-81: Immutable graph holder with clean API.Using
readonlyparameters and simple lookups keeps this class focused. Good separation from the builder.
83-116: Builder with mutable state producing immutable result.The
tileToGatewaymap ensures O(1) duplicate detection. Clean builder pattern.
118-255: Well-instrumented build process.The phased approach (water components → gateways → edges) with timing breakdowns makes debugging easy.
347-427: Gateway placement at run midpoints.Grouping contiguous water tiles and placing gateways at midpoints is the standard HPA* approach. Good implementation.
528-586: Efficient multi-target BFS.Single BFS finding all reachable gateways is better than separate searches. The early exit when all targets found is a nice optimization.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/core/pathfinding/PathFinder.test.ts:
- Line 215: The comment in PathFinder.test.ts contains a typo: change "conosle"
to "console" in the comment mentioning MiniAStar (e.g., the line "// MiniAStar
logs conosle error when nulls passed, muted in test") so it reads "// MiniAStar
logs console error when nulls passed, muted in test"; locate the comment near
the PathFinder tests referencing MiniAStar and correct the spelling.
- Line 165: The test uses a non-existent Vitest matcher `toBeOneOf` on
`expect(result.status)`; replace this with a standard matcher by asserting that
an array of allowed statuses contains the actual value (e.g., use
`expect([PathStatus.NEXT, PathStatus.COMPLETE]).toContain(result.status)`),
keeping references to `result.status` and the `PathStatus` enum in the
`PathFinder.test.ts` test.
🧹 Nitpick comments (2)
tests/core/pathfinding/PathFinder.test.ts (2)
167-167: Consider using type guards for better type safety.The type assertions
(result as { node: TileRef })bypass TypeScript's type system. IfPathResultis a discriminated union based onstatus, consider using type guards or checking the property existence explicitly.♻️ Example with property checking
- current = (result as { node: TileRef }).node; + if ('node' in result) { + current = result.node; + }Or define a type guard:
function hasNode(result: PathResult): result is { status: PathStatus.NEXT; node: TileRef } { return result.status === PathStatus.NEXT && 'node' in result; } // Usage: if (hasNode(result)) { current = result.node; }Also applies to: 202-202, 205-205
318-332: Verify the intent of testing known bugs.These tests explicitly validate buggy behavior (paths crossing land barriers). The commit message indicates this is intentional to prevent accidental fixes, but this approach prevents you from verifying when bugs are actually resolved.
Consider alternative approaches:
- Use
test.failing()to mark these as expected failures when the bug is fixed- Document bugs separately and use feature flags to toggle the behavior
- Create separate regression tests that verify the correct behavior for when bugs are fixed
This design choice should align with your team's testing philosophy. Do you want me to show examples of using
test.failing()or feature-flag-based testing?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/core/pathfinding/PathFinder.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/pathfinding/benchmark/run.ts:
- Around line 226-229: The readdirSync(syntheticScenariosDir) call can throw a
cryptic ENOENT if syntheticScenariosDir is missing; update the code around the
scenarioFiles assignment to first check that syntheticScenariosDir exists (e.g.,
using existsSync or statSync) or wrap readdirSync in a try/catch, and if the
directory is missing throw or log a clear, descriptive error (e.g., "synthetic
scenarios directory not found: <path>") before exiting; keep the behavior of an
empty directory returning zero scenarios intact and reference the scenarioFiles
variable and syntheticScenariosDir when implementing the check.
🧹 Nitpick comments (4)
tests/pathfinding/benchmark/run.ts (1)
115-134: Consider null check instead of non-null assertion on line 127.
measureExecutionTimereturnsnumber | nullper its type signature, but the assertionexecTime!assumes it never returns null. While the current implementation always returns a number, the assertion could mask future bugs if the implementation changes.♻️ Safer null handling
const execTime = measureExecutionTime( adapter, route, options.iterations ?? DEFAULT_ITERATIONS, ); result.executionTime = execTime; if (!silent) { - printRow([route.name, `${execTime!.toFixed(2)}ms`], [40, 12]); + printRow([route.name, execTime !== null ? `${execTime.toFixed(2)}ms` : "N/A"], [40, 12]); }tests/pathfinding/utils.ts (3)
177-178: Globalconsole.debugsuppression has side effects.Overwriting
console.debugglobally affects all code that runs aftersetupFromPath. This is fine for isolated benchmarks but could hide useful debug output if the function is called in other contexts.Consider scoping this to a test setup hook or making it optional via a parameter.
♻️ Optional suppression via parameter
export async function setupFromPath( mapDirectory: string, mapName: string, gameConfig: Partial<GameConfig> = {}, humans: PlayerInfo[] = [], + options: { suppressDebug?: boolean } = {}, ): Promise<Game> { - // Suppress console.debug for tests - console.debug = () => {}; + if (options.suppressDebug !== false) { + console.debug = () => {}; + }
208-228: Dynamic import forTestServerConfiginside async function.This works but adds latency each time
setupFromPathis called. For benchmarks running many scenarios, this could add up. Consider importing at module level if possible.♻️ Move import to module level
At the top of the file:
import { TestServerConfig } from "../util/TestServerConfig";Then in the function:
const config = new TestConfig( - new (await import("../util/TestServerConfig")).TestServerConfig(), + new TestServerConfig(), {
52-61: Add explicit comment clarifying intentional private field mutation.The cast
(game as any)._navMesh = navMeshat line 58 mutates a private field, which couples the benchmark to Game's internal structure. However, this is intentional—the code comment (lines 53-55) explains why: Game deliberately doesn't expose a publicnavMeshsetter to keep the API clean and avoid test-side-effect pollution.The design choice is sound. Add a focused comment at line 58 explaining:
// Intentional mutation of private field. Game doesn't expose a setter to keep API clean.This clarifies the trade-off without requiring API changes that would contradict the stated design goal.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/pathfinding/navmesh/FastAStarAdapter.tstests/pathfinding/benchmark/run.tstests/pathfinding/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/pathfinding/navmesh/FastAStarAdapter.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/pathfinding/benchmark/run.ts (5)
tests/pathfinding/utils.ts (8)
getScenario(69-102)getAdapter(45-67)printHeader(164-169)printRow(151-158)BenchmarkResult(30-34)measurePathLength(104-110)calculateStats(133-149)measureExecutionTime(119-131)src/server/GameManager.ts (1)
game(25-27)tests/pathfinding/playground/public/client.js (8)
result(68-72)result(803-803)result(854-854)r(1112-1112)i(1344-1344)i(1364-1364)i(1384-1384)mapName(64-64)src/core/game/GameImpl.ts (1)
stats(961-963)src/core/game/AllianceRequestImpl.ts (1)
status(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
🔇 Additional comments (9)
tests/pathfinding/benchmark/run.ts (3)
32-34: Good path resolution for scenario discovery.The path calculation correctly navigates from the current file to the synthetic scenarios directory. Clean and readable.
242-250: Single iteration may produce noisy timing results.When running all synthetic scenarios,
iterations: 1is hardcoded. This gives faster batch runs but less stable timing measurements. The tradeoff is reasonable for batch mode, but worth noting in the output or usage docs.
284-287: Good top-level error handling.Catching errors at the main level and exiting with code 1 is proper CLI behavior. Clean and simple.
tests/pathfinding/utils.ts (6)
24-43: Clean type definitions using typed unions.The benchmark types are simple, flat structures without class hierarchies. Good use of composition and plain types.
69-74: Dynamic import uses.jsextension for TypeScript source.This works with tsx/ts-node ESM mode where
.tsfiles are referenced as.jsat runtime. Just ensure this matches your build/runtime setup.
104-110: Simple and clear path length measurement.The function is short, readable, and handles the null case cleanly. Good.
112-117: Generic timing utility is reusable.
measureTimeis a clean generic function. Works for any synchronous operation. Nice composition pattern.
133-149: Statistics calculation handles edge cases well.The function filters nulls before summing and avoids division by zero for
avgTime. Clean logic.
185-196: Good early validation of required map files.Checking file existence before attempting reads prevents cryptic errors later. Clear error messages with full paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/core/pathfinding/PathFinder.test.ts (2)
64-77: Clarify the expected behavior for same-tile paths.The comment says "Should probably be fixed to return [] instead" but the test expects a single-element path. If this behavior should change, consider opening an issue to track it. If it's intentional, update the comment to explain why.
318-332: Document whether "Known bugs" are intentional or to be fixed.These tests verify that paths can cross land barriers, which seems like a pathfinding bug. If these are temporary limitations to be fixed later, consider adding TODO comments or issue references. If this is intentional behavior (e.g., allowing diagonal water movement), rename the section to clarify.
src/core/pathfinding/navmesh/NavMesh.ts (5)
216-241: Same-gateway paths skip smoothing.When start and end share the same gateway, the path is returned directly without smoothing (line 240). This is inconsistent with gateway-routed paths that always get smoothed (line 413). If smoothing is beneficial, consider applying it uniformly to all multi-tile paths.
♻️ Apply smoothing consistently
sameGatewayMeasure.duration; this.debugInfo!.timings.total += sameGatewayMeasure.duration; } - return path; + if (path) { + return this.smoothPath(path); + } + return path; }
356-366: Path caching creates reversed copy for bidirectional edges.Line 364 uses
slice().reverse()to cache the reversed path. For long paths (e.g., 10,000+ tiles), this creates a full copy in memory. If memory usage becomes a concern, consider lazy reversal (reversing during traversal) or storing only the forward path with a direction flag.
496-507: High iteration limit for gateway pathfinding.The maxIterations of 100,000 should handle most gateway graphs, but on extremely large maps with thousands of gateways, this could cause performance issues. Consider making this configurable or proportional to gateway count.
775-789: Smoothing skip increment might miss waypoints.Line 778 skips by
Math.floor(path.length / 20), so for a 100-tile path, only every 5th tile is checked for line-of-sight shortcuts. This could miss optimal shortcuts between skipped tiles. The final-tile special case (lines 791-800) helps but doesn't cover all gaps.Consider reducing the skip increment or using a fixed small skip (e.g., 2-3 tiles) to balance performance and path quality.
650-667: Start/end position fixup uses array search and splice.Lines 650-667 use
findIndex()andsplice()to fix start/end positions in the scaled path. For very long paths, this linear search and array manipulation could be slow. Since the path is built from miniMap waypoints, the start/end should typically be close to array boundaries.Consider checking the first/last few elements directly before falling back to
findIndex().
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eslint.config.jssrc/core/pathfinding/navmesh/NavMesh.tstests/core/pathfinding/PathFinder.test.ts
✅ Files skipped from review due to trivial changes (1)
- eslint.config.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-08T00:57:39.752Z
Learnt from: Aareksio
Repo: openfrontio/OpenFrontIO PR: 2815
File: tests/pathfinding/playground/server.ts:87-104
Timestamp: 2026-01-08T00:57:39.752Z
Learning: Playground code in tests/pathfinding/playground/ should not be subjected to detailed code review - working functionality is sufficient for these test/visualization utilities.
Applied to files:
tests/core/pathfinding/PathFinder.test.ts
📚 Learning: 2025-11-29T22:22:37.178Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Applied to files:
src/core/pathfinding/navmesh/NavMesh.ts
🧬 Code graph analysis (1)
tests/core/pathfinding/PathFinder.test.ts (5)
src/core/pathfinding/PathFinder.ts (1)
PathFinder(19-22)src/core/pathfinding/adapters/MiniAStarAdapter.ts (1)
MiniAStarAdapter(15-66)src/core/pathfinding/adapters/NavMeshAdapter.ts (1)
NavMeshAdapter(6-99)tests/util/Setup.ts (1)
setup(23-82)src/core/game/GameMap.ts (1)
TileRef(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
🔇 Additional comments (2)
tests/core/pathfinding/PathFinder.test.ts (1)
255-267: PENDING status is adapter-specific.The loop handling
PathStatus.PENDINGonly applies toMiniAStarAdapter.NavMeshAdapternever returns PENDING (it computes paths eagerly), so this loop exits immediately for that adapter. This is fine but worth noting that test behavior differs between adapters.src/core/pathfinding/navmesh/NavMesh.ts (1)
44-70: LGTM! Well-sized FastAStar buffers.The pre-allocated buffer sizes are well-chosen:
- Single sector: 32×32 = 1,024 nodes
- Multi-sector: 96×96 = 9,216 nodes (3×3 sectors)
- Gateway graph: dynamically sized by gateway count
… takes time to initialize)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/core/pathfinding/navmesh/FastAStarAdapter.ts:
- Around line 13-17: In getCost(from: number, to: number) replace the silent
fallback (edge?.cost ?? 1) with an explicit error throw when no matching edge is
found: retrieve edges via this.graph.getEdges(from), find the edge with
edges.find(e => e.to === to), and if edge is undefined throw a descriptive Error
(including from and to IDs) instead of returning 1, mirroring the error handling
approach used in heuristic().
🧹 Nitpick comments (1)
src/core/pathfinding/navmesh/FastAStarAdapter.ts (1)
84-88: Consider bounds validation innodeToTile.The method converts a node ID to a TileRef without validating that
node < this.numNodes. If an invalid node ID is passed (e.g., from a corrupted state), this could produce a TileRef outside the intended bounds.Adding a validation check would help catch bugs early:
🛡️ Optional validation guard
nodeToTile(node: number): TileRef { + if (node < 0 || node >= this.numNodes) { + throw new Error(`Invalid node ID: ${node} (valid range: 0-${this.numNodes - 1})`); + } const localX = node % this.width; const localY = Math.floor(node / this.width); return this.map.ref(localX + this.minX, localY + this.minY); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/pathfinding/navmesh/FastAStarAdapter.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-29T22:22:37.178Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Applied to files:
src/core/pathfinding/navmesh/FastAStarAdapter.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/pathfinding/navmesh/FastAStarAdapter.ts
📚 Learning: 2025-12-13T14:58:29.645Z
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.
Applied to files:
src/core/pathfinding/navmesh/FastAStarAdapter.ts
📚 Learning: 2026-01-08T00:57:39.752Z
Learnt from: Aareksio
Repo: openfrontio/OpenFrontIO PR: 2815
File: tests/pathfinding/playground/server.ts:87-104
Timestamp: 2026-01-08T00:57:39.752Z
Learning: Playground code in tests/pathfinding/playground/ should not be subjected to detailed code review - working functionality is sufficient for these test/visualization utilities.
Applied to files:
src/core/pathfinding/navmesh/FastAStarAdapter.ts
🔇 Additional comments (1)
src/core/pathfinding/navmesh/FastAStarAdapter.ts (1)
64-81: The clamping behavior is intentional design for sector-based pathfinding, not a correctness issue.This adapter is used in bounded sector regions (3×3 or 1×1 sectors). When start or goal tiles fall near sector edges, they are clamped to valid positions within the bounded region. While this creates a lossy mapping (multiple out-of-bounds positions collapse to the same node), this is by design—the code immediately validates both start and goal positions after conversion, rejecting the path if either returns
-1.However, the comment referencing "matching graph building behavior" is unverified. Consider either verifying this claim is accurate or removing it if the rationale for clamping is simply to allow bounded sector-based search.
| getCost(from: number, to: number): number { | ||
| const edges = this.graph.getEdges(from); | ||
| const edge = edges.find((edge) => edge.to === to); | ||
| return edge?.cost ?? 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the actual code in the file
cat -n src/core/pathfinding/navmesh/FastAStarAdapter.tsRepository: openfrontio/OpenFrontIO
Length of output: 4591
🏁 Script executed:
# Search for all getCost calls to understand usage patterns
rg -n "getCost\(" --type=ts -A 3 -B 3Repository: openfrontio/OpenFrontIO
Length of output: 2207
🏁 Script executed:
# Look for the FastAStar implementation to understand interface expectations
fd -n "FastAStar" --type=ts -o -type=jsRepository: openfrontio/OpenFrontIO
Length of output: 297
🏁 Script executed:
# Check how the adapters are used with FastAStar
rg -n "FastAStar\|new.*Adapter" --type=ts -A 2 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 49
Make getCost() throw when edge is not found, consistent with error handling in heuristic().
The ?? 1 fallback on line 16 silently masks a logic error. Since getCost is only called for neighbors returned by getNeighbors(), the edge should always exist. If it doesn't, that indicates a bug—either getNeighbors() returned an invalid neighbor, or getCost() has an implementation error.
Throw an error instead of returning a default value to catch this inconsistency early, matching the error handling approach used in heuristic() (lines 23-27).
🤖 Prompt for AI Agents
In @src/core/pathfinding/navmesh/FastAStarAdapter.ts around lines 13 - 17, In
getCost(from: number, to: number) replace the silent fallback (edge?.cost ?? 1)
with an explicit error throw when no matching edge is found: retrieve edges via
this.graph.getEdges(from), find the edge with edges.find(e => e.to === to), and
if edge is undefined throw a descriptive Error (including from and to IDs)
instead of returning 1, mirroring the error handling approach used in
heuristic().
2 participants
Pathfinding with HPA*
Hi! The primary objective of this PR is to replace per-tile A* with hierarchical pathfinding - HPA*. In practice, this means we create an abstract graph on top of the actual map with far fewer points and use it to decide on general path structure. Only then we go back to tile-level and build path between selected waypoints. This speeds up long distance pathfinding by over 1000x in some cases. To make the review easier, it comes with a benchmark and visual playground.
PREPROCESSING
H part of HPA* means "hierarchical" and requires preprocessing.
This PR includes pre-processing as part inside
new Game()constructor. It takes about 135ms forgiantworldmapon my machine, which increases the effective initialization from ~95ms to ~230ms. This time could be reduced in different ways, which are out of scope for this PR.After confirming the initialization time is bearable on low-end devices, I argue merging this PR as-is is acceptable tradeoff. It creates small lag at the beginning of a round but pays for itself in the first minute of the match.
Nerdy details
Architecture
Key Optimizations
Review Focus
Play with included tools, benchmark and visualization. Pathfinding should be safe to merge as a black box - you do not need to understand the details. Outcomes can be tested empirically in-game. Visualize (and share!) edge cases with included playground. Confirm the 100x speedup is real with benchmark.
If you plan to dive into the code, I suggest the following order:
src/core/pathfinding/tests/core/pathfinding/src/core/pathfinding/navmesh/+ integration withGame.tstests/pathfinding/benchmark/Do not look at playground's code, it has been created with a clanker. The design is 100% mine and I spent way too long polishing it, but I haven't even once edited the code manually. There is probably no abstraction whatsoever, just do not look at the code, let it play.
Core Changes
Pathfinding (
src/core/pathfinding/navmesh/)Pre-Processing (
src/core/pathfinding/navmesh/)Abstraction (
src/core/pathfinding/)PathFinderinterface that can return full path and also act as state machine (.next())Benchmark (
tests/pathfinding/benchmark/)npx tsx tests/pathfinding/benchmark/run.ts- no guesswork, numbersnpx tsx tests/pathfinding/benchmark/run.ts --synthetic- 1000s of synthetic pathsnpx tsc tests/pathfinding/benchmark/generate.ts- generate more as needed, test new mapsPlayground (
tests/pathfinding/playground/)npx tsx tests/pathfinding/playground/server.ts- visualize paths with both new and legacy algorithmBenchmarks
Compared with legacy in default - hand picked - scenario:
42,000 synthetic routes across all maps
Playground
That's the fun part. Watch NavMesh running circles around legacy
PathFinder.Miniin real time. Debug inner workings, test edge cases, share URLs for debugging.chrome_RdeeROFDRG.mp4
Discord Tag
moleole