webgl #2548
webgl #2548
Conversation
Replace BFS with DFS and eliminate GC pressure in calculateClusters() hot path: - Switch from O(N) queue.shift() to O(1) stack.pop() operations - Replace Set.has()/Set.add() with Uint8Array bitfield - Add reusable buffer management to avoid repeated allocations - Implement callback-based neighbor iteration to eliminate array allocations - Add forEachNeighborWithDiag() method to Game interface and GameImpl - Remove now unused GameImpl import from PlayerExecution
- Introduce ClusterTraversalState and a WeakMap<Game, ClusterTraversalState> in PlayerExecution.ts to store visited: Uint32Array and gen per game instance. - Remove the per-player _visitedBuffer - Update calculateClusters() to allocate/resize a single Uint32Array(totalTiles) per Game, use a generation counter instead of calling fill(0) to clear. - Switch visited checks to visited[tile] === currentGen.
- Convert neighbor loops in surroundedBySamePlayer, isSurrounded, getCapturingPlayer to forEachNeighbor callbacks - Implement floodFillWithGen() method for configurable zero-allocation flood fill - Replace BFS in removeCluster() with floodFillWithGen using cardinal neighbors - Refactor calculateClusters() to use floodFillWithGen with diagonal neighbors - Add generational state management and forEachNeighbor interface method
WalkthroughAdds SharedArrayBuffer-backed tile ring buffers and worker plumbing, WebGL territory renderer (Canvas/WebGL strategy + UI toggle), backlog-aware batched client update processing and render throttling, defended-tile and zero-allocation neighbor APIs, and dev HTTPS/proxy improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Main Thread (UI)
participant Worker as Worker Thread
participant GR as GameRunner (server-side)
participant Client as ClientGameRunner
participant Renderer as TerritoryRenderer
UI->>Worker: init({sharedTileRingHeader, sharedTileRingData, sharedStateBuffer, sharedDirtyBuffer})
Worker->>GR: createGameRunner(..., tileUpdateSink -> pushTileUpdate / Atomics dirtyFlags)
GR->>Worker: tileUpdateSink(tileRef) -- writes into SAB or sets dirty flag
UI->>Worker: postMessage("turn", turnData)
Worker->>Worker: processPendingTurns() -> drainTileUpdates()
Worker->>Client: send merged GameUpdateViewData / tick messages
Client->>Client: enqueue pendingUpdates
Client->>Client: processPendingUpdates(timeBudget) -> merged = mergeGameUpdates(batch)
Client->>Renderer: render(merged)
alt Renderer.isWebGL
Renderer->>Renderer: upload state/palette (incremental), draw via shader
else Canvas
Renderer->>Renderer: paint ImageData and blit
end
Renderer->>UI: emit TickMetricsEvent + BacklogStatusEvent
UI->>Client: BacklogStatusEvent consumed -> adjust render throttle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing close attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 2 inconclusive)
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 |
This reverts commit 18d04a5.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (13)
src/client/InputHandler.ts (1)
139-157: Consider using an options object for clarity.The constructor now has 10 optional parameters. An options object would improve readability and make it easier to pass only specific metrics:
+export interface TickMetricsEventData { + tickExecutionDuration?: number; + tickDelay?: number; + backlogTurns?: number; + ticksPerRender?: number; + workerTicksPerSecond?: number; + renderTicksPerSecond?: number; + tileUpdatesCount?: number; + ringBufferUtilization?: number; + ringBufferOverflows?: number; + ringDrainTime?: number; +} + export class TickMetricsEvent implements GameEvent { - constructor( - public readonly tickExecutionDuration?: number, - public readonly tickDelay?: number, - ... - ) {} + constructor(public readonly data: TickMetricsEventData = {}) {} }src/core/worker/Worker.worker.ts (1)
90-99: Nested ternary is hard to follow.The logic branches:
sharedTileRing && dirtyFlags→ atomic dedup + pushsharedTileRingonly → direct push- Neither → undefined
Branch 2 seems like dead code since
dirtyFlagsis set fromsharedTileRing.dirtyFlags(line 79). Consider simplifying:- sharedTileRing && dirtyFlags - ? (tile: TileRef) => { - if (Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0) { - pushTileUpdate(sharedTileRing!, tile); - } - } - : sharedTileRing - ? (tile: TileRef) => pushTileUpdate(sharedTileRing!, tile) - : undefined, + sharedTileRing + ? (tile: TileRef) => { + if (Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0) { + pushTileUpdate(sharedTileRing!, tile); + } + } + : undefined,src/client/graphics/HoverTarget.ts (2)
37-39: Avoidanycast; use a proper type guard.The pattern
(owner as any).isPlayer?.()bypasses type safety. Theownerresult fromgame.owner(tile)likely returns a union type. Use an explicit type guard or method check without theanycast.- if ((owner as any).isPlayer?.()) { - return { player: owner as PlayerView, unit: null }; + if (owner && "isPlayer" in owner && owner.isPlayer()) { + return { player: owner, unit: null }; }
45-56: UsenearbyUnits()spatial query instead of filtering all units.The code calls
game.units(...HOVER_UNIT_TYPES)to fetch all matching units globally, then filters by distance. Sincegame.nearbyUnits(tile, searchRange, types)is available and designed for this use case—returning distance-sorted results from a spatial index—consider using it withHOVER_DISTANCE_PXas the search range to avoid scanning the entire map.src/client/graphics/layers/TerritoryWebGLStatus.ts (2)
116-127: No cleanup for event subscription.The
eventBus.on()call ininit()registers a listener that is never removed. If this component is removed from the DOM and re-added, you'll accumulate duplicate listeners. Consider storing the handler and unsubscribing indisconnectedCallback().+ private statusHandler?: (event: TerritoryWebGLStatusEvent) => void; + init() { this.enabled = this.userSettings?.territoryWebGL() ?? true; if (this.eventBus) { - this.eventBus.on(TerritoryWebGLStatusEvent, (event) => { + this.statusHandler = (event) => { this.enabled = event.enabled; this.active = event.active; this.supported = event.supported; this.lastMessage = event.message ?? null; this.requestUpdate(); - }); + }; + this.eventBus.on(TerritoryWebGLStatusEvent, this.statusHandler); } } + + disconnectedCallback() { + super.disconnectedCallback(); + if (this.eventBus && this.statusHandler) { + this.eventBus.off(TerritoryWebGLStatusEvent, this.statusHandler); + } + }
138-143: Simplify redundant condition.Line 141 checks
this.enabled && this.active, butthis.enabledbeing false was already handled at line 139. You can simplify:private statusClass(): string { if (!this.enabled) return "status-disabled"; - if (this.enabled && this.active) return "status-active"; + if (this.active) return "status-active"; if (!this.supported) return "status-disabled"; return "status-fallback"; }src/client/graphics/GameRenderer.ts (1)
312-314:backlogGrowingis tracked but unused.The field
backlogGrowingis updated fromBacklogStatusEventbut never read withinGameRenderer. If it's for future use, consider adding a TODO comment. Otherwise, remove it to avoid confusion.src/core/game/TerrainMapLoader.ts (1)
43-47: Consider extracting crossOriginIsolated check.The SAB availability check is duplicated on line 100. Extract to a helper or constant:
+const canUseSharedBuffers = + typeof SharedArrayBuffer !== "undefined" && + typeof Atomics !== "undefined" && + typeof (globalThis as any).crossOriginIsolated === "boolean" && + (globalThis as any).crossOriginIsolated === true; export async function loadTerrainMap( ... ): Promise<TerrainMapData> { - const useCache = sharedStateBuffer === undefined; - const canUseSharedBuffers = - typeof SharedArrayBuffer !== "undefined" && - typeof Atomics !== "undefined" && - typeof (globalThis as any).crossOriginIsolated === "boolean" && - (globalThis as any).crossOriginIsolated === true; + const useCache = sharedStateBuffer === undefined;Alternatively, keep it as a function if it needs to be evaluated lazily.
src/client/graphics/layers/PerformanceOverlay.ts (1)
518-553: Metrics update logic is straightforward.The overflow handling (line 545-548) is a bit unusual—it sets to 1 if any overflow occurred rather than accumulating. This is intentional per the comment ("at least once"). Consider if showing a count would be more useful for debugging.
The rounding for utilization and drain time (lines 541-542, 551) keeps display values clean.
Consider accumulating overflow count instead of just flagging "at least once":
- if (ringBufferOverflows !== undefined && ringBufferOverflows !== 0) { - // Remember that an overflow has occurred at least once this run. - this.ringBufferOverflows = 1; + if (ringBufferOverflows !== undefined) { + this.ringBufferOverflows += ringBufferOverflows; }src/client/graphics/layers/TerritoryRenderers.ts (2)
78-83: Use a type guard instead ofas anycasting.The pattern
(rawOwner as any).isPlayer === "function"breaks type safety. Consider adding a type guard function or use a discriminated union.+function isPlayerView(view: unknown): view is PlayerView { + return view !== null && typeof view === "object" && "isPlayer" in view && typeof (view as any).isPlayer === "function" && (view as any).isPlayer(); +} + const owner = - rawOwner && - typeof (rawOwner as any).isPlayer === "function" && - (rawOwner as any).isPlayer() - ? (rawOwner as PlayerView) - : null; + rawOwner && isPlayerView(rawOwner) ? rawOwner : null;
270-274: Same type guard issue.Apply the same type guard refactor suggested for
CanvasTerritoryRendererto keep both implementations consistent.src/client/graphics/layers/TerritoryWebGLRenderer.ts (2)
110-138: Duplicated uniform defaults.The null uniform object is defined twice with identical structure. Consider extracting to a constant or helper to reduce duplication.
const NULL_UNIFORMS = { resolution: null, state: null, palette: null, // ... rest of uniform locations } as const;Then use
this.uniforms = { ...NULL_UNIFORMS };in both branches.Also applies to: 151-179
864-867: Extract magic numbers to constants.The bit masks
0xFFFu,0x2000u,0x1000u,0xC000uare hard to understand. Add comments or GLSL constants.+ // State bit layout: + // bits 0-11: owner ID (0xFFF) + // bit 12: isDefended (0x1000) + // bit 13: hasFallout (0x2000) + // bits 14-15: relation (0xC000) + const uint OWNER_MASK = 0xFFFu; + const uint DEFENDED_BIT = 0x1000u; + const uint FALLOUT_BIT = 0x2000u; + const uint RELATION_MASK = 0xC000u; + const uint RELATION_SHIFT = 14u; + uint state = texelFetch(u_state, texCoord, 0).r; - uint owner = state & 0xFFFu; - bool hasFallout = (state & 0x2000u) != 0u; // bit 13 - bool isDefended = (state & 0x1000u) != 0u; // bit 12 - uint relation = (state & 0xC000u) >> 14u; // bits 14-15 + uint owner = state & OWNER_MASK; + bool hasFallout = (state & FALLOUT_BIT) != 0u; + bool isDefended = (state & DEFENDED_BIT) != 0u; + uint tileRelation = (state & RELATION_MASK) >> RELATION_SHIFT;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.gitignore(1 hunks)package.json(1 hunks)src/client/ClientGameRunner.ts(11 hunks)src/client/InputHandler.ts(2 hunks)src/client/LocalServer.ts(4 hunks)src/client/graphics/GameRenderer.ts(8 hunks)src/client/graphics/HoverTarget.ts(1 hunks)src/client/graphics/layers/Layer.ts(1 hunks)src/client/graphics/layers/PerformanceOverlay.ts(6 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(3 hunks)src/client/graphics/layers/TerritoryLayer.ts(14 hunks)src/client/graphics/layers/TerritoryRenderers.ts(1 hunks)src/client/graphics/layers/TerritoryWebGLRenderer.ts(1 hunks)src/client/graphics/layers/TerritoryWebGLStatus.ts(1 hunks)src/core/GameRunner.ts(5 hunks)src/core/execution/PlayerExecution.ts(8 hunks)src/core/game/Game.ts(1 hunks)src/core/game/GameImpl.ts(2 hunks)src/core/game/GameMap.ts(4 hunks)src/core/game/GameView.ts(10 hunks)src/core/game/TerrainMapLoader.ts(4 hunks)src/core/game/UserSettings.ts(2 hunks)src/core/worker/SharedTileRing.ts(1 hunks)src/core/worker/Worker.worker.ts(4 hunks)src/core/worker/WorkerClient.ts(3 hunks)src/core/worker/WorkerMessages.ts(1 hunks)webpack.config.js(3 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.808Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
📚 Learning: 2025-11-29T22:22:37.170Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.170Z
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/game/Game.tssrc/core/game/GameImpl.tssrc/client/graphics/layers/TerritoryLayer.tssrc/core/worker/SharedTileRing.tssrc/core/execution/PlayerExecution.ts
📚 Learning: 2025-11-26T22:27:31.808Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.808Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
Applied to files:
src/core/worker/WorkerClient.tssrc/core/game/GameImpl.tssrc/client/graphics/layers/TerritoryLayer.tssrc/core/game/TerrainMapLoader.tssrc/core/worker/SharedTileRing.tssrc/core/worker/Worker.worker.tssrc/client/ClientGameRunner.tssrc/core/execution/PlayerExecution.tssrc/client/graphics/layers/TerritoryWebGLRenderer.tssrc/client/graphics/layers/PerformanceOverlay.ts
📚 Learning: 2025-11-26T20:49:29.110Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/worker/WorkerClient.tssrc/core/game/TerrainMapLoader.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/GameRunner.tssrc/core/worker/SharedTileRing.tssrc/core/worker/Worker.worker.tssrc/client/ClientGameRunner.tssrc/core/game/GameMap.tssrc/core/game/GameView.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/core/game/UserSettings.tssrc/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLStatus.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/TerritoryRenderers.tssrc/client/graphics/layers/TerritoryWebGLRenderer.tssrc/core/game/GameView.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLStatus.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/TerritoryRenderers.tssrc/client/graphics/layers/TerritoryWebGLRenderer.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/client/graphics/layers/TerritoryLayer.tssrc/core/game/GameView.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/core/game/GameView.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/GameRunner.tssrc/client/ClientGameRunner.tssrc/core/game/GameView.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:
src/client/graphics/layers/TerritoryWebGLStatus.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/GameRenderer.tssrc/client/ClientGameRunner.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/TerrainMapLoader.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.tssrc/client/ClientGameRunner.tssrc/core/execution/PlayerExecution.tssrc/core/game/GameView.ts
📚 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/client/InputHandler.tssrc/core/worker/Worker.worker.tssrc/core/execution/PlayerExecution.tssrc/core/game/GameView.tssrc/client/graphics/layers/PerformanceOverlay.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/PlayerExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/PlayerExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/PlayerExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/PlayerExecution.ts
🧬 Code graph analysis (16)
src/core/game/Game.ts (4)
src/core/game/GameView.ts (1)
tile(110-112)src/core/game/UnitImpl.ts (1)
tile(176-178)src/core/game/TrainStation.ts (1)
tile(141-143)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/worker/WorkerClient.ts (2)
src/core/worker/SharedTileRing.ts (1)
SharedTileRingBuffers(3-7)src/core/game/GameView.ts (1)
sharedStateBuffer(926-932)
src/core/game/GameImpl.ts (3)
src/core/game/UnitImpl.ts (1)
tile(176-178)src/core/game/TrainStation.ts (1)
tile(141-143)src/core/game/GameMap.ts (4)
TileRef(3-3)x(142-144)y(146-148)ref(131-136)
src/client/graphics/layers/TerritoryLayer.ts (2)
src/client/graphics/layers/TerritoryRenderers.ts (3)
TerritoryRendererStrategy(12-27)CanvasTerritoryRenderer(29-246)WebglTerritoryRenderer(248-342)src/client/graphics/HoverTarget.ts (1)
resolveHoverTarget(28-63)
src/client/graphics/layers/TerritoryWebGLStatus.ts (4)
src/client/graphics/layers/Layer.ts (1)
Layer(1-8)src/core/EventBus.ts (1)
EventBus(7-44)src/core/game/UserSettings.ts (1)
UserSettings(6-209)src/client/InputHandler.ts (2)
TerritoryWebGLStatusEvent(86-93)ToggleTerritoryWebGLEvent(84-84)
src/core/game/TerrainMapLoader.ts (1)
src/core/game/GameView.ts (1)
sharedStateBuffer(926-932)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
src/client/graphics/HoverTarget.ts (1)
resolveHoverTarget(28-63)
src/core/GameRunner.ts (4)
src/core/game/GameView.ts (2)
tile(110-112)config(782-784)src/core/game/UnitImpl.ts (1)
tile(176-178)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/GameImpl.ts (1)
config(336-338)
src/core/worker/SharedTileRing.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/InputHandler.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/core/worker/Worker.worker.ts (3)
src/core/worker/SharedTileRing.ts (3)
SharedTileRingViews(9-14)createSharedTileRingViews(31-43)pushTileUpdate(45-64)src/core/GameRunner.ts (1)
createGameRunner(35-95)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/GameRenderer.ts (4)
src/core/configuration/DefaultConfig.ts (1)
userSettings(274-279)src/client/InputHandler.ts (1)
BacklogStatusEvent(159-164)src/client/graphics/layers/TerritoryLayer.ts (1)
profileName(36-38)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-62)
src/client/ClientGameRunner.ts (6)
src/core/worker/SharedTileRing.ts (6)
SharedTileRingBuffers(3-7)SharedTileRingViews(9-14)createSharedTileRingBuffers(21-29)createSharedTileRingViews(31-43)drainTileUpdates(66-85)TILE_RING_HEADER_OVERFLOW(19-19)src/client/Transport.ts (1)
SendHashEvent(157-162)src/client/InputHandler.ts (2)
TickMetricsEvent(139-157)BacklogStatusEvent(159-164)src/core/game/Game.ts (1)
GameUpdates(33-35)src/core/game/GameMap.ts (2)
TileRef(3-3)ref(131-136)src/core/game/GameImpl.ts (1)
ref(825-827)
src/client/LocalServer.ts (4)
src/client/InputHandler.ts (1)
BacklogStatusEvent(159-164)src/core/configuration/DefaultConfig.ts (1)
turnIntervalMs(173-175)tests/util/TestServerConfig.ts (1)
turnIntervalMs(55-57)src/client/graphics/layers/BuildMenu.ts (1)
count(380-387)
src/client/graphics/layers/TerritoryWebGLRenderer.ts (4)
src/core/game/GameView.ts (9)
GameView(572-937)tile(110-112)x(806-808)y(809-811)id(86-88)id(458-460)PlayerView(185-570)type(98-100)type(464-466)src/core/configuration/Config.ts (1)
Theme(189-214)src/core/game/GameMap.ts (3)
TileRef(3-3)x(142-144)y(146-148)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-62)
src/core/game/GameView.ts (5)
src/core/configuration/DefaultConfig.ts (1)
theme(632-636)src/core/game/GameMap.ts (5)
TileRef(3-3)isDefended(229-231)x(142-144)y(146-148)ref(131-136)src/core/game/GameImpl.ts (4)
isDefended(880-882)x(831-833)y(834-836)ref(825-827)src/core/game/TerraNulliusImpl.ts (2)
TerraNulliusImpl(4-20)id(13-15)src/core/game/PlayerImpl.ts (1)
id(201-203)
🪛 Biome (2.1.2)
src/core/game/GameImpl.ts
[error] 885-885: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 893-893: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
src/core/game/GameView.ts
[error] 863-864: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 871-872: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (79)
.gitignore (1)
9-9: LGTM!Correctly excludes local development certificates from version control, supporting the new HTTPS dev workflow.
webpack.config.js (6)
4-4: LGTM!The fs import is correctly used for reading certificate files in the HTTPS configuration.
13-18: LGTM!Cross-origin headers correctly configured for SharedArrayBuffer and cross-origin isolation. These enable WebGL and shared memory features mentioned in the PR summary.
20-27: LGTM!HTTPS environment variable handling is clean and the default certificate paths are consistent with the package.json dev:secure script.
29-33: LGTM!Clean helper that applies cross-origin headers to proxied responses.
118-136: LGTM!HTTPS configuration correctly handles missing certificates with clear error messages and fail-fast behavior. The synchronous file reads are appropriate for startup configuration.
143-144: LGTM!The HTTPS server configuration, cross-origin headers, and proxy configuration are properly wired into the devServer, with correct production mode handling.
Also applies to: 304-305, 313-313
src/core/GameRunner.ts (3)
40-48: LGTM! Clean extension of the factory function.The optional parameters for shared tile state (
tileUpdateSink,sharedStateBuffer) are wired through cleanly. This allows callers to opt into the shared memory path without breaking existing consumers.
183-196: Clear branching for tile update dispatch.The logic cleanly separates shared sink mode (publish each tile ref) from the legacy path (pack into BigUint64Array). The bit shift
>> 16ncorrectly extracts the tile reference from the packed update format(tileRef << 16n | state).
292-295: Simple and effective.Clean helper to check if turns are waiting to be processed. Useful for backlog monitoring.
src/client/graphics/layers/Layer.ts (1)
7-7: LGTM! Good addition for profiling.Adding an optional
profileNamemethod follows the existing interface pattern and enables per-layer performance tracking.src/core/game/UserSettings.ts (2)
64-66: LGTM! Consistent with existing patterns.The getter follows the same structure as other boolean settings. Defaulting to
truemakes sense since WebGL is the preferred rendering path.
122-124: LGTM!Toggle method follows the established pattern used by other settings.
src/core/worker/WorkerClient.ts (2)
72-81: Init message wiring looks correct.The shared buffers are passed to the worker via the init message. Optional chaining handles the undefined case cleanly.
26-28: No changes needed.
sharedDirtyBufferandsharedTileRingBuffers.dirtyare not redundant. TheSharedTileRingBuffersinterface contains three buffers:header,data, anddirty. InClientGameRunner,sharedDirtyBufferis assigned fromsharedTileRingBuffers.dirty(line 201) and passed separately toWorkerClientbecause SharedArrayBuffers must be transferred individually through worker messages—they cannot be passed as nested object properties.WorkerClientdecomposes the buffers (sharedTileRingHeader,sharedTileRingData,sharedDirtyBuffer) and sends them in the init message, which the worker then uses to reconstruct the ring buffer views. This is the necessary and correct pattern for cross-worker buffer sharing.src/core/game/Game.ts (1)
667-675: LGTM! Zero-allocation iteration is a solid performance optimization.These callback-based methods avoid creating intermediate arrays during neighbor traversal. The comments clearly explain the purpose. Based on learnings, the intentional duplication between these and array-returning methods is acceptable for performance reasons.
src/client/graphics/layers/TerritoryLayer.ts (12)
36-38: LGTM - Simple profile name accessor.Clean addition for profiling purposes.
59-59: LGTM - Clean field declarations.Typed unions used correctly (
TerritoryRendererStrategy | null,number | null).Also applies to: 70-72
177-190: Verify: redraw vs border repaint logic.When
territoryRendererexists, a fullredraw()is triggered. For the canvas path, individual player borders are repainted. This seems intentional for WebGL (GPU texture sync), but confirm a fullredraw()is not too expensive when focus changes frequently during spawn phase.
192-196: LGTM - Player change detection.Triggers redraw when the local player changes, keeping rendering in sync.
312-316: LGTM - WebGL toggle event handler.Toggling WebGL updates user settings and triggers a full redraw to reconfigure renderers.
354-367: LGTM - Efficient hover handling split.WebGL path uses
setHover()for GPU-based highlighting; canvas path repaints borders. Clean separation.
392-441: LGTM - Clean renderer configuration with fallback.Good composition pattern. Creates WebGL renderer when available, falls back to canvas with appropriate status messages. All renderer state (alternativeView, hoverHighlightOptions, etc.) is properly initialized.
447-468: LGTM - Central hover highlight configuration.Clear, tweakable configuration. Good use of typed object literal instead of scattered constants.
470-479: LGTM - Event emission helper.Reduces repetition in
configureRenderers().
504-523: LGTM - Viewport-based rendering delegation.Correct viewport bounds calculation with clamping. Renderer receives proper viewport and blit flag.
546-552: Good optimization for WebGL path.WebGL renders the entire queue at once, while canvas path uses gradual rendering. This makes sense since WebGL batches draw calls efficiently.
588-594: LGTM - Clean delegation to renderer.Parameter
_maybeStaleBorderprefixed with underscore to indicate it's unused in the new path.src/core/worker/WorkerMessages.ts (1)
38-41: LGTM - Optional shared memory buffer fields.Clean extension of
InitMessagefor shared memory support. Optional fields handle graceful degradation whenSharedArrayBufferis unavailable.src/client/graphics/layers/PlayerInfoOverlay.ts (2)
97-107: LGTM - Simplified hover logic.Now uses shared
resolveHoverTargetutility. Cleaner and consistent withTerritoryLayer.
157-162: LGTM - Clean delegation wrapper.Encapsulates the game instance while delegating to the shared utility.
src/client/InputHandler.ts (2)
84-93: LGTM - New WebGL toggle and status events.
ToggleTerritoryWebGLEventis a simple marker event.TerritoryWebGLStatusEventcarries clear status fields.
159-164: LGTM - Simple backlog status event.Clear payload with required fields for backlog monitoring.
src/core/worker/Worker.worker.ts (3)
25-27: LGTM - State variables for shared tile ring.Clean module-level state for shared memory synchronization.
44-65: Verify: message starvation during long turn processing.The
while (gr.hasPendingTurns())loop processes all pending turns before returning. If many turns queue up, this could delay handling of other messages (e.g.,player_actions). Is this intentional?Based on learnings, the ring buffer has deduplication, so individual turns should be fast. If you observe UI lag during catch-up, consider processing turns in smaller batches.
114-127: LGTM - Turn processing integration.Calls
processPendingTurns()after adding a turn to drain the queue.src/client/graphics/GameRenderer.ts (1)
370-391: Good adaptive frame throttling.The scaling from 60 FPS down to 30 FPS based on backlog size is a reasonable approach to reduce rendering pressure when the game loop is falling behind. The logic is clear and the first-frame bypass (
lastRenderTime !== 0) ensures immediate initial render.src/core/game/GameImpl.ts (2)
520-542: Good zero-allocation neighbor iteration.This intentionally duplicates the coordinate logic from
neighborsWithDiag()to avoid allocations on hot paths. The callback-based approach is efficient for performance-critical code. Based on learnings, this is the intended pattern.
901-909: Clean cardinal neighbor iteration.The direct bound checks and callback invocations are efficient. No allocations, no array creation.
src/core/execution/PlayerExecution.ts (3)
6-12: Good use of WeakMap for per-game traversal state.Using
WeakMap<Game, ClusterTraversalState>ensures the traversal buffers are garbage collected when the Game instance is no longer referenced. The generation-based visitation avoids clearing the entire array each frame.
325-333: Correct generation wraparound handling.The check for
0xffffffffand reset to 1 (not 0) ensures generation 0 remains the "unvisited" sentinel. Clean solution.
335-369: Clean flood fill with generation tracking.Stack-based iteration, proper generation checks, and callback-based neighbor access. This is efficient and avoids allocations for the visited tracking.
src/core/game/TerrainMapLoader.ts (2)
69-76: Only normal-size maps receive the shared state buffer.The 4x map path (line 76) and all mini-map paths do not receive
stateBuffer. This appears intentional since shared state is likely only needed for the main game map. If 4x maps should also use shared state in some scenarios, this would need adjustment.Is it correct that only
GameMapSize.Normalmaps use the shared state buffer?
42-55: Cache bypass when SharedArrayBuffer is available.The logic correctly avoids returning cached map data when SAB support is detected but no buffer was provided. This ensures fresh SharedArrayBuffer instances are created for each game session.
src/core/worker/SharedTileRing.ts (6)
1-14: Clean type definitions for shared memory buffers.The interfaces are well-structured and clear. Using typed arrays (
Int32Array,Uint32Array,Uint8Array) backed bySharedArrayBufferis the right approach for cross-thread communication.
16-19: Header constants are clear and self-documenting.Using named constants for header indices is good practice for readability and maintainability.
21-29: Buffer allocation looks correct.The function correctly allocates three separate
SharedArrayBufferinstances for header (3 int32s), data (capacity uint32s), and dirty flags (numTiles uint8s).
31-43: View creation correctly derives capacity from buffer length.
66-85: drainTileUpdates correctly advances read pointer.The drain logic is straightforward: read items from the ring buffer up to
maxItems, push them to the output array, and update the read index. The single-consumer assumption (render thread) makes this safe.One small note: the
outarray is mutated viapush(). This is fine for the current usage but caller must be aware.
45-64: Single producer confirmed—no race condition concern.The current implementation is correct.
pushTileUpdateis called exclusively fromWorker.worker.ts(Web Worker context), which executes on a single thread. Both call sites (lines 93 and 97) occur within the same worker, so concurrent producer access is not possible. The theoretical race condition between loading and storing indices cannot occur in practice with this design.src/core/game/GameMap.ts (4)
30-33: Interface extension for defense and relation state is clear.The new methods follow the existing pattern of getters/setters for tile state. Clean API design.
79-85: Bit layout for state fits within Uint16.Let me verify the bit allocation:
- Bits 0-11: PLAYER_ID_MASK (0xfff) — 12 bits
- Bit 12: DEFENDED_BIT
- Bit 13: FALLOUT_BIT
- Bits 14-15: RELATION_MASK (0xc000)
Total: 16 bits. This fits perfectly in
Uint16Array.The relation values (0, 1, 2) use only 2 bits, which is correct for the 2-bit mask.
92-112: Optional stateBuffer allows shared memory integration.The constructor correctly validates the external buffer length before using it, falling back to a new allocation when not provided. Good defensive coding.
229-254: Bit manipulation for defended and relation state is correct.
isDefended/setDefended: Standard single-bit operations using shift and mask.getRelation/setRelation: Properly clears existing bits before setting new value with mask.The masking in
setRelation(line 253) ensures that even if an invalid relation value is passed, only bits 14-15 are affected.src/client/ClientGameRunner.ts (7)
173-203: Shared buffer setup with proper feature detection.Good defensive checks:
- Checks
crossOriginIsolatedfor SAB availability- Verifies
SharedArrayBufferandAtomicsexist- Only creates ring buffers when all conditions are met
The ring capacity scaling to
numTilesaligns with the learning about natural bounds.
258-272: State fields for backlog tracking and batch processing.Clear naming for the new metrics fields. Using separate counters for worker ticks vs render ticks is good for understanding throughput differences.
500-569: Adaptive batch processing with time budgets.The backlog-aware budget scaling is clever:
- Base budget of 8ms keeps UI responsive
- Scales up to 1000ms when severely behind (500+ turns)
- Per-slice cap of 1000 ticks prevents runaway processing
The queue compaction logic (lines 562-569) avoids unbounded memory growth by splicing when the start pointer is large enough.
One concern:
requestAnimationFramechains could pile up if updates arrive faster than frames process. However, theisProcessingUpdatesflag prevents re-entry, which is correct.
571-632: Render only after all pending updates processed.Batching all updates before a single render is efficient. The merged view data approach (line 577-581) reduces redundant state updates.
The metrics sampling (lines 591-605) resets counters on each sample, which gives per-render-frame rates rather than rolling averages. This matches the expected behavior for a performance overlay.
637-743: mergeGameUpdates combines batched updates correctly.The function:
- Merges all update arrays across batch entries
- Drains tile ring when SAB mode is active
- Deduplicates tile refs via Set
- Clears dirty flags after drain
- Falls back to counting packed updates in non-SAB mode
One note: The cast
as unknown as any[](lines 674-675) is a bit loose, but acceptable given the dynamic nature ofGameUpdatesenum keys.
745-756: Backlog metrics emit events for UI.Simple and correct. The
backlogGrowingflag helps the UI show whether the client is catching up or falling further behind.
409-425: Server turn high-water tracking during game start.Correctly tracks the maximum turn number seen from the server. This establishes the baseline for backlog calculation.
src/client/graphics/layers/PerformanceOverlay.ts (3)
231-244: Extended metrics forwarding to updateTickMetrics.All new metrics from
TickMetricsEventare properly passed through. Clean integration with the existing pattern.
326-333: Reset clears all new tile-related metrics.Complete reset of the new state fields, maintaining consistency with the existing reset pattern.
440-468: New state properties for tile metrics.All new fields are properly decorated with
@state()for Lit reactivity. Initial values are sensible (0 for counters).src/core/game/GameView.ts (7)
44-46: Border tint constants are clear and readable.Using named constants for tint targets and ratio is good for maintainability. The 35% blend ratio is reasonable for visual distinction without being jarring.
195-310: Pre-computed border color variants avoid runtime overhead.Computing all color variants at construction time (neutral, friendly, embargo, plus defended versions of each) is efficient. The color blending math is correct—linear interpolation between base and tint target.
333-401: borderColor and borderRelationFlags work together cleanly.The
borderRelationFlagsmethod correctly:
- Iterates neighbors
- Skips unowned and same-owner tiles
- Checks embargo first (takes priority)
- Falls back to friendly detection
The
borderColormethod uses these flags to select the appropriate pre-computed color variant, with defended state adding the checkerboard pattern.
588-601: Shared tile state flag and terraNullius singleton.Using a singleton
terraNulliusinstance instead of creating newTerraNulliusImplobjects is more efficient and semantically correct—there's only one "no owner" state.The
usesSharedTileStateflag cleanly separates the two update paths.
630-639: Tile update branch based on shared state mode.Per learnings: in SAB mode,
packedTileUpdatescontains bare tile refs (justBigInt(tileRef)), while non-SAB mode contains packed state. The branching correctly handles both cases.
743-752: playerBySmallID returns singleton terraNullius.Returning the shared
terraNulliusinstance for ID 0 or missing IDs is correct. This avoids creating throwaway objects.
922-932: Shared state buffer accessor is defensive.The method correctly checks
usesSharedTileStatefirst and validates the buffer is actually aSharedArrayBufferbefore returning. Good defensive coding.src/client/graphics/layers/TerritoryRenderers.ts (2)
12-27: Clean strategy interface.Good use of a simple interface to abstract canvas vs WebGL rendering. The method surface is minimal and focused.
292-297: BothsetDefendedandsetRelationmethods are properly defined onGameView(lines 862 and 870 insrc/core/game/GameView.ts). The methods delegate to the underlying map instance and are correctly implemented. No issues found with these method calls.src/client/graphics/layers/TerritoryWebGLRenderer.ts (6)
6-18: Good type definitions.Simple, clear interfaces. Using optional fields for
HoverHighlightOptionsmakes the API flexible.
25-80: Well-organized properties.Good separation of WebGL resources, dirty tracking state, and animation parameters.
406-433: Good factory pattern.Validation of SharedArrayBuffer and size with clear error reasons. This makes debugging easier.
469-486: Good defensive clamping.The
Math.max(0, Math.min(1, ...))pattern for strength values and unbounded lower limit for speed is appropriate.
532-582: Clean render loop.Proper separation of upload and draw phases with profiling. Using
gl.clearColor(0, 0, 0, 0)for transparent background is correct for layered compositing.
783-800: Relation code mapping looks correct.Codes 1 (Self), 2 (Ally), 3 (Neutral), 4 (Enemy) match the shader's alternative view logic. Good.
| "start:server": "node --loader ts-node/esm --experimental-specifier-resolution=node src/server/Server.ts", | ||
| "start:server-dev": "cross-env GAME_ENV=dev node --loader ts-node/esm --experimental-specifier-resolution=node src/server/Server.ts", | ||
| "dev": "cross-env GAME_ENV=dev concurrently \"npm run start:client\" \"npm run start:server-dev\"", | ||
| "dev:secure": "cross-env GAME_ENV=dev DEV_HTTPS=1 DEV_CERT=resources/certs/dev.crt DEV_KEY=resources/certs/dev.key concurrently \"npm run start:client\" \"npm run start:server-dev\"", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for any documentation about certificates, HTTPS setup, or certificate generation
rg -i "certificate|https.*dev|dev.*crt|dev.*key|mkcert|openssl.*dev" --type md --type txt -C 2Repository: openfrontio/OpenFrontIO
Length of output: 320
🏁 Script executed:
# Check if certificate directory is gitignored
cat .gitignore 2>/dev/null | grep -i "cert\|crt\|key" || echo "No certificate entries found in .gitignore"Repository: openfrontio/OpenFrontIO
Length of output: 83
🏁 Script executed:
# Look for README or setup documentation
find . -maxdepth 2 -type f \( -name "README*" -o -name "SETUP*" -o -name "CONTRIBUTING*" -o -name "*.md" \) | head -20Repository: openfrontio/OpenFrontIO
Length of output: 201
🏁 Script executed:
# Check the actual structure around package.json for any dev setup docs
ls -la | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1887
🏁 Script executed:
cat README.md | head -100Repository: openfrontio/OpenFrontIO
Length of output: 3634
🏁 Script executed:
cat example.envRepository: openfrontio/OpenFrontIO
Length of output: 707
🏁 Script executed:
# Check webpack.config.js for certificate handling
sed -n '20,35p' webpack.config.jsRepository: openfrontio/OpenFrontIO
Length of output: 563
🏁 Script executed:
# Check if there's any script to generate certificates
rg "mkcert|keytool|certificate|cert.*generate" --type=js --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Search for dev:secure references
rg "dev:secure" --type=js --type=json --type=mdRepository: openfrontio/OpenFrontIO
Length of output: 267
🏁 Script executed:
# Check CONTRIBUTING or other guidance docs
find . -maxdepth 2 -type f \( -name "CONTRIBUTING*" -o -name ".github/*" \) | xargs cat 2>/dev/null | head -50Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check package.json for all dev-related scripts and context
cat package.json | head -50Repository: openfrontio/OpenFrontIO
Length of output: 2342
Add documentation for HTTPS certificate setup in local development.
The dev:secure npm script requires certificate files at resources/certs/dev.crt and resources/certs/dev.key, which are gitignored and not provided in the repository. Developers attempting to use this script will encounter missing file errors without guidance. Add instructions in the README or a setup guide explaining how to generate self-signed certificates using OpenSSL or mkcert for local HTTPS development.
🤖 Prompt for AI Agents
In package.json around line 10 the dev:secure script expects
resources/certs/dev.crt and resources/certs/dev.key which are gitignored and not
provided; update the README (or a dedicated SETUP.md) with concise steps to
generate and install local dev certificates (both an OpenSSL one-liner and a
mkcert workflow), specify the exact target paths (resources/certs/dev.crt and
resources/certs/dev.key), note file permissions if needed, and instruct
developers to set DEV_HTTPS=1 or use the npm script; also mention that the cert
files are intentionally gitignored and should not be checked in.
| <div class="performance-line"> | ||
| Worker ticks/s: | ||
| <span>${this.workerTicksPerSecond.toFixed(1)}</span> | ||
| </div> | ||
| <div class="performance-line"> | ||
| Render ticks/s: | ||
| <span>${this.renderTicksPerSecond.toFixed(1)}</span> | ||
| </div> | ||
| <div class="performance-line"> | ||
| Ticks per render: | ||
| <span>${this.ticksPerRender}</span> | ||
| </div> | ||
| <div class="performance-line"> | ||
| Backlog turns: | ||
| <span>${this.backlogTurns}</span> | ||
| </div> | ||
| <div class="performance-line"> | ||
| Tile updates/render: | ||
| <span>${this.tileUpdatesPerRender}</span> | ||
| (peak: <span>${this.tileUpdatesPeak}</span>) | ||
| </div> | ||
| <div class="performance-line"> | ||
| Ring buffer: | ||
| <span>${this.ringBufferUtilization}%</span> | ||
| (${this.totalTilesUpdated} total, ${this.ringBufferOverflows} | ||
| overflows) | ||
| </div> | ||
| <div class="performance-line"> | ||
| Ring drain time: | ||
| <span>${this.ringDrainTime.toFixed(2)}ms</span> | ||
| </div> |
There was a problem hiding this comment.
Hardcoded English strings in new metric labels.
The new performance lines use hardcoded strings like "Worker ticks/s:", "Render ticks/s:", etc., while other labels use translateText().
For consistency with the i18n approach used elsewhere in this file, these should be translated.
Consider adding translation keys for the new metric labels:
<div class="performance-line">
- Worker ticks/s:
+ ${translateText("performance_overlay.worker_ticks")}
<span>${this.workerTicksPerSecond.toFixed(1)}</span>
</div>📝 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.
| <div class="performance-line"> | |
| Worker ticks/s: | |
| <span>${this.workerTicksPerSecond.toFixed(1)}</span> | |
| </div> | |
| <div class="performance-line"> | |
| Render ticks/s: | |
| <span>${this.renderTicksPerSecond.toFixed(1)}</span> | |
| </div> | |
| <div class="performance-line"> | |
| Ticks per render: | |
| <span>${this.ticksPerRender}</span> | |
| </div> | |
| <div class="performance-line"> | |
| Backlog turns: | |
| <span>${this.backlogTurns}</span> | |
| </div> | |
| <div class="performance-line"> | |
| Tile updates/render: | |
| <span>${this.tileUpdatesPerRender}</span> | |
| (peak: <span>${this.tileUpdatesPeak}</span>) | |
| </div> | |
| <div class="performance-line"> | |
| Ring buffer: | |
| <span>${this.ringBufferUtilization}%</span> | |
| (${this.totalTilesUpdated} total, ${this.ringBufferOverflows} | |
| overflows) | |
| </div> | |
| <div class="performance-line"> | |
| Ring drain time: | |
| <span>${this.ringDrainTime.toFixed(2)}ms</span> | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.worker_ticks")} | |
| <span>${this.workerTicksPerSecond.toFixed(1)}</span> | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.render_ticks")} | |
| <span>${this.renderTicksPerSecond.toFixed(1)}</span> | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.ticks_per_render")} | |
| <span>${this.ticksPerRender}</span> | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.backlog_turns")} | |
| <span>${this.backlogTurns}</span> | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.tile_updates")} | |
| <span>${this.tileUpdatesPerRender}</span> | |
| (${translateText("performance_overlay.peak")}: <span>${this.tileUpdatesPeak}</span>) | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.ring_buffer")} | |
| <span>${this.ringBufferUtilization}%</span> | |
| (${this.totalTilesUpdated} ${translateText("performance_overlay.total")}, ${this.ringBufferOverflows} | |
| ${translateText("performance_overlay.overflows")}) | |
| </div> | |
| <div class="performance-line"> | |
| ${translateText("performance_overlay.ring_drain_time")} | |
| <span>${this.ringDrainTime.toFixed(2)}ms</span> | |
| </div> |
🤖 Prompt for AI Agents
In src/client/graphics/layers/PerformanceOverlay.ts around lines 707 to 737 the
new metric labels are hardcoded in English; replace each literal label (e.g.
"Worker ticks/s:", "Render ticks/s:", "Ticks per render:", "Backlog turns:",
"Tile updates/render:", "(peak:", "Ring buffer:", "Ring drain time:") with the
existing translateText(...) call pattern used elsewhere in this file, add
corresponding i18n keys to the translation resource files (keeping any
punctuation and parentheses separate or included per convention), and ensure
numeric values continue to be formatted as before (toFixed / % / units) while
passing only the label strings through translation so the UI remains consistent
across locales.
| private labelUpload( | ||
| base: string, | ||
| metrics: { rows: number; bytes: number }, | ||
| ): string { | ||
| if (metrics.rows === 0 || metrics.bytes === 0) { | ||
| return `${base} (skip)`; | ||
| } | ||
| const rowBucket = | ||
| metrics.rows >= this.canvas.height | ||
| ? "full" | ||
| : `${Math.ceil(metrics.rows / 50) * 50}`; | ||
| const kb = Math.max(1, Math.round(metrics.bytes / 1024)); | ||
| const kbBucket = kb > 1024 ? `${Math.round(kb / 1024)}MB` : `${kb}KB`; | ||
| return `${base} rows:${rowBucket} bytes:${kbBucket}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dead code: labelUpload is never called.
This method appears to be unused. Either remove it or integrate it with the profiler output.
- private labelUpload(
- base: string,
- metrics: { rows: number; bytes: number },
- ): string {
- if (metrics.rows === 0 || metrics.bytes === 0) {
- return `${base} (skip)`;
- }
- const rowBucket =
- metrics.rows >= this.canvas.height
- ? "full"
- : `${Math.ceil(metrics.rows / 50) * 50}`;
- const kb = Math.max(1, Math.round(metrics.bytes / 1024));
- const kbBucket = kb > 1024 ? `${Math.round(kb / 1024)}MB` : `${kb}KB`;
- return `${base} rows:${rowBucket} bytes:${kbBucket}`;
- }📝 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.
| private labelUpload( | |
| base: string, | |
| metrics: { rows: number; bytes: number }, | |
| ): string { | |
| if (metrics.rows === 0 || metrics.bytes === 0) { | |
| return `${base} (skip)`; | |
| } | |
| const rowBucket = | |
| metrics.rows >= this.canvas.height | |
| ? "full" | |
| : `${Math.ceil(metrics.rows / 50) * 50}`; | |
| const kb = Math.max(1, Math.round(metrics.bytes / 1024)); | |
| const kbBucket = kb > 1024 ? `${Math.round(kb / 1024)}MB` : `${kb}KB`; | |
| return `${base} rows:${rowBucket} bytes:${kbBucket}`; | |
| } |
src/client/LocalServer.ts
Outdated
| private sendTurnBatch(count: number) { | ||
| for (let i = 0; i < count; i++) { | ||
| if (this.turnsExecuted === this.turns.length) { | ||
| this.endTurn(); | ||
| } else { | ||
| break; // No more turns available | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This batch loop sends at most one turn per call.
The condition this.turnsExecuted === this.turns.length is true only when the client has caught up. After endTurn() pushes a new turn, this.turns.length increases, so the condition becomes false and the loop breaks immediately. This means sendTurnBatch(count) always sends exactly 0 or 1 turns.
If batch sending is intended, consider tracking how many turns were actually sent:
private sendTurnBatch(count: number) {
for (let i = 0; i < count; i++) {
- if (this.turnsExecuted === this.turns.length) {
- this.endTurn();
- } else {
- break; // No more turns available
+ if (!this.endTurn()) {
+ break; // No more turns available or paused
}
}
}Then have endTurn() return a boolean indicating success.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/client/LocalServer.ts around lines 118-126, the for-loop logic only ever
sends 0 or 1 turns because it checks an equality that flips after endTurn()
grows turns, then immediately breaks; change the loop to attempt up to `count`
sends by looping while `i < count` and `this.turnsExecuted < this.turns.length`,
increment a local sent counter each successful send, call `endTurn()` for each
turn and treat it as a boolean-returning operation (modify endTurn() to return
true on success, false on failure), and break the loop if endTurn() returns
false so you stop on failure. Ensure you update `this.turnsExecuted` only when a
turn was actually sent and return/record the number of turns sent if needed.
| const buildDevProxyConfig = () => | ||
| [ | ||
| // WebSocket proxies | ||
| { | ||
| context: ["/socket"], | ||
| target: "ws://localhost:3000", | ||
| ws: true, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| // Worker WebSocket proxies - using direct paths without /socket suffix | ||
| { | ||
| context: ["/w0"], | ||
| target: "ws://localhost:3001", | ||
| ws: true, | ||
| secure: false, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| { | ||
| context: ["/w1"], | ||
| target: "ws://localhost:3002", | ||
| ws: true, | ||
| secure: false, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| { | ||
| context: ["/w2"], | ||
| target: "ws://localhost:3003", | ||
| ws: true, | ||
| secure: false, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| // Worker proxies for HTTP requests | ||
| { | ||
| context: ["/w0"], | ||
| target: "http://localhost:3001", | ||
| pathRewrite: { "^/w0": "" }, | ||
| secure: false, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| { | ||
| context: ["/w1"], | ||
| target: "http://localhost:3002", | ||
| pathRewrite: { "^/w1": "" }, | ||
| secure: false, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| { | ||
| context: ["/w2"], | ||
| target: "http://localhost:3003", | ||
| pathRewrite: { "^/w2": "" }, | ||
| secure: false, | ||
| changeOrigin: true, | ||
| logLevel: "debug", | ||
| }, | ||
| // Original API endpoints | ||
| { | ||
| context: [ | ||
| "/api/env", | ||
| "/api/game", | ||
| "/api/public_lobbies", | ||
| "/api/join_game", | ||
| "/api/start_game", | ||
| "/api/create_game", | ||
| "/api/archive_singleplayer_game", | ||
| "/api/auth/callback", | ||
| "/api/auth/discord", | ||
| "/api/kick_player", | ||
| ], | ||
| target: "http://localhost:3000", | ||
| secure: false, | ||
| changeOrigin: true, | ||
| }, | ||
| ].map((proxyEntry) => ({ | ||
| onProxyRes: addProxyHeaders, | ||
| ...proxyEntry, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
webpack-dev-server proxy duplicate context paths ws true
💡 Result:
Short answer: The dev-server's HTTP proxy (http-proxy-middleware) will also try to handle WebSocket upgrade requests if the WS path matches your proxy context. If your devServer's client/server WS path is included in the proxy context you get duplicate/looping "Upgrading to WebSocket" messages or duplicated requests. Fixes: make the WS path not match the proxy context (exclude it), or configure the proxy so it doesn't handle upgrades for that path and let webpack-dev-server handle its own webSocketServer.
How to fix (examples)
-
Exclude the WS path from proxy context (use array/object or a function):
- context that excludes /ws (regex or function):
context: (pathname, req) => !pathname.startsWith('/dev-server-ws') && (pathname.startsWith('/api') || pathname.startsWith('/auth'))
- context that excludes /ws (regex or function):
-
Use a separate proxy entry that doesn't include the dev-server WS pathname; set ws: true only for the backend WS route you actually want proxied.
-
Or move the dev-server WS to a path that’s not proxied and set client.webSocketURL.pathname / webSocketServer.options.path accordingly so the dev server handles HMR itself.
References: webpack-dev-server proxy docs; http-proxy-middleware websocket docs; community reports showing the reconnection/duplicate-upgrade issue and the recommended workaround to avoid having the dev-server WS path match proxy.context. [1][2][3]
Sources
[1] webpack-dev-server proxy and webSocketServer docs. [2] http-proxy-middleware — WebSocket / pathRewrite / ws options. [3] StackOverflow thread describing reconnection / duplicate upgrade loop when WS path is inside proxy.context.
🌐 Web query:
proxy-middleware ws WebSocket multiple entries same context
💡 Result:
Short answer: this is a known behavior of http-proxy-middleware (and underlying http-proxy) — WebSocket "upgrade" handling happens on the raw HTTP server upgrade event (outside Express routing), so if you mount multiple proxy middlewares without giving each a concrete context when creating them, the first proxy instance that listens for upgrades can "claim" all WS upgrades and break the others. [1][2]
Recommendations / fixes
- Create proxy instances with explicit context (use createProxyMiddleware(context, options) or pass the context string/array as first arg) and mount them on the app. That ensures the proxy instance is bound to the context for both HTTP and WS. Example: app.use('/sockjs-node', createProxyMiddleware('/sockjs-node',{ target, ws:true })); [2][3]
- OR handle upgrades yourself and dispatch by req.url: attach server.on('upgrade', ...) and call the appropriate proxy.ws(req,socket,head) for the correct target (look at http-proxy example). This is the robust solution when you need multiple WS backends. [7]
- If you see "invalid frame header" / reserved bits errors, it’s usually the wrong proxy handling the upgrade — fix by using explicit contexts or custom upgrade dispatch. [3][5]
Sources
- http-proxy-middleware GitHub issues and discussion (upgrade/WS behavior). [4]
- http-proxy-middleware docs / examples showing createProxyMiddleware(context, options) and ws: true. [3]
- Examples showing manual server.on('upgrade') dispatch and http-proxy ws usage. [7]
If you want, tell me your current proxy code and I’ll show the minimal change to make multiple WS routes work.
🏁 Script executed:
# Check if there's any configuration documentation or tests in the repo related to this
find . -type f \( -name "*.md" -o -name "*.json" \) | xargs grep -l "proxy\|webpack" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 138
🏁 Script executed:
# Check webpack.config.js for any comments or context about testing this proxy setup
cat webpack.config.js | grep -A 5 -B 5 "test\|verify\|TODO\|FIXME\|NOTE" || echo "No special notes found"Repository: openfrontio/OpenFrontIO
Length of output: 2249
🏁 Script executed:
# Look for test files or documentation about the proxy configuration
find . -type f \( -name "*.test.js" -o -name "*.spec.js" -o -name "*proxy*" \) | head -20Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check package.json for dev-server related config
cat package.json | grep -A 30 "devServer"Repository: openfrontio/OpenFrontIO
Length of output: 49
Fix duplicate proxy contexts – this configuration is fragile and follows a known problematic pattern.
The proxy configuration duplicates context paths (/w0, /w1, /w2) with separate entries for WebSocket and HTTP requests. While this may work due to execution order, it violates http-proxy-middleware's WebSocket handling design: upgrade requests are claimed at the HTTP server level by the first proxy instance listening for them, not routed at the application level. This means the first proxy entry for each path captures all WebSocket upgrades, leaving the second entry to handle only regular HTTP requests.
This pattern is fragile and documented as problematic. Consolidate each worker endpoint into a single proxy entry that handles both WebSocket and HTTP, using pathRewrite for HTTP while allowing WebSocket through:
{
context: ["/w0"],
target: "http://localhost:3001", // target changes per request type
ws: true,
pathRewrite: { "^/w0": "" }, // rewrite HTTP paths
secure: false,
changeOrigin: true,
logLevel: "debug",
},Or use a context function to exclude WebSocket paths and mount them separately with explicit handling.
🤖 Prompt for AI Agents
In webpack.config.js around lines 35 to 116, the proxy config duplicates
contexts for /w0, /w1, /w2 with separate WebSocket and HTTP entries which is
fragile because upgrade requests are claimed by the first matching proxy;
consolidate each worker path into a single proxy entry that sets ws: true and
includes pathRewrite (e.g., "^/w0": "") so the same proxy handles both WebSocket
upgrades and rewritten HTTP requests, remove the duplicate standalone ws
entries, keep secure/changeOrigin/logLevel and the onProxyRes mapper, and ensure
targets point to the correct worker host for each consolidated entry.
- GPU Relation Calculations: Moved border diplomatic relation logic from CPU buffer to WebGL shader, eliminating per-tile CPU computation - Relation Matrix: Converted 1D relation array to 2D owner×other matrix for O(1) GPU lookups - Palette Refresh: Batched palette refreshes to single call per update cycle WebGL Shader Updates - Added u_viewerId uniform and bitmask relation helper functions (isFriendly(), isEmbargo(), isSelf()) - Enhanced border detection with per-neighbor relation evaluation Removed CPU-side relation state management Files: TerritoryLayer.ts, TerritoryRenderers.ts, TerritoryWebGLRenderer.ts (+102/-39 lines)
…Renderer - Consolidated defended state logic for tiles into dedicated methods in GameImpl to improve clarity and maintainability. - Updated CanvasTerritoryRenderer to utilize the new isDefended method for determining tile defense status. - Removed redundant checks and streamlined the painting logic for territory tiles.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/core/game/GameImpl.ts (1)
900-910: Already flagged: Remove return from void functions.This issue was already identified in previous review comments for lines 900-910.
src/client/graphics/layers/TerritoryWebGLRenderer.ts (2)
709-723: Dead code:labelUploadis never called.This method appears unused. Remove it to reduce maintenance burden.
- private labelUpload( - base: string, - metrics: { rows: number; bytes: number }, - ): string { - if (metrics.rows === 0 || metrics.bytes === 0) { - return `${base} (skip)`; - } - const rowBucket = - metrics.rows >= this.canvas.height - ? "full" - : `${Math.ceil(metrics.rows / 50) * 50}`; - const kb = Math.max(1, Math.round(metrics.bytes / 1024)); - const kbBucket = kb > 1024 ? `${Math.round(kb / 1024)}MB` : `${kb}KB`; - return `${base} rows:${rowBucket} bytes:${kbBucket}`; - }
1006-1012: Brightness clamping missing for defended tiles.Multiplying by
LIGHT_FACTOR = 1.2can push RGB values above 1.0, causing clamping artifacts. This was flagged in a previous review but appears unaddressed.if (isDefended) { bool isLightTile = ((texCoord.x % 2) == (texCoord.y % 2)); const float LIGHT_FACTOR = 1.2; const float DARK_FACTOR = 0.8; - borderColor *= isLightTile ? LIGHT_FACTOR : DARK_FACTOR; + borderColor = min(borderColor * (isLightTile ? LIGHT_FACTOR : DARK_FACTOR), vec3(1.0)); }
🧹 Nitpick comments (6)
src/client/graphics/layers/TerritoryLayer.ts (3)
196-199: Missing state update forlastMyPlayerSmallId.After detecting a player change and calling
redraw(), the local tracking variablelastMyPlayerSmallIdis not updated here. Whileredraw()does update it internally (line 377), relying on this side effect is fragile.Consider making the update explicit here for clarity:
const currentMyPlayer = this.game.myPlayer()?.smallID() ?? null; if (currentMyPlayer !== this.lastMyPlayerSmallId) { + this.lastMyPlayerSmallId = currentMyPlayer; this.redraw(); }Or document that
redraw()handles this update.
558-576: Consider simplifying the conditional.The
useNeighborPaintvariable is computed as!(this.territoryRenderer?.isWebGL() ?? false). Since you already checkedterritoryRendereris not null at line 546, this can be simplified:- const useNeighborPaint = !(this.territoryRenderer?.isWebGL() ?? false); + const useNeighborPaint = !this.territoryRenderer.isWebGL();
592-594: Unused parameter_maybeStaleBorder.The parameter is unused in the new implementation. Consider removing it if it's no longer needed, or document why it's kept for API compatibility.
src/client/graphics/layers/TerritoryRenderers.ts (1)
77-82: Type assertion smell with(rawOwner as any).This type narrowing via
as anyis fragile. Consider using a type guard or checking against the interface directly.- const owner = - rawOwner && - typeof (rawOwner as any).isPlayer === "function" && - (rawOwner as any).isPlayer() - ? (rawOwner as PlayerView) - : null; + const owner = + rawOwner && "isPlayer" in rawOwner && rawOwner.isPlayer() + ? (rawOwner as PlayerView) + : null;This uses
inoperator for property check, which is cleaner and avoidsas any.src/client/graphics/layers/TerritoryWebGLRenderer.ts (2)
97-101: Consider context loss handling.WebGL contexts can be lost. While
preserveDrawingBuffer: trueis set, there's nowebglcontextlostevent handler. For a long-running game, this could cause silent failures.Consider adding context loss detection:
this.canvas.addEventListener('webglcontextlost', (e) => { e.preventDefault(); console.warn('[TerritoryWebGLRenderer] WebGL context lost'); });
1018-1024: Float comparison for player ID matching.Using
abs(float(owner) - u_hoveredPlayerId) < 0.5works for integer IDs but is unusual. SinceownerisuintandhoveredPlayerIdis a float uniform, consider using int uniform and direct comparison for clarity:uniform int u_hoveredPlayerId; // ... if (u_hoveredPlayerId >= 0 && int(owner) == u_hoveredPlayerId) {This is minor since the current approach works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/TerritoryLayer.ts(16 hunks)src/client/graphics/layers/TerritoryRenderers.ts(1 hunks)src/client/graphics/layers/TerritoryWebGLRenderer.ts(1 hunks)src/core/game/GameImpl.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
📚 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/game/GameImpl.tssrc/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/GameImpl.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/GameImpl.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.tssrc/client/graphics/layers/TerritoryWebGLRenderer.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
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
📚 Learning: 2025-11-26T22:27:31.844Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
Applied to files:
src/core/game/GameImpl.tssrc/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLRenderer.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLRenderer.tssrc/client/graphics/layers/TerritoryRenderers.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLRenderer.tssrc/client/graphics/layers/TerritoryRenderers.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/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLRenderer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/TerritoryWebGLRenderer.ts
🧬 Code graph analysis (3)
src/core/game/GameImpl.ts (1)
src/core/game/GameMap.ts (5)
TileRef(3-3)x(142-144)y(146-148)ref(131-136)isDefended(229-231)
src/client/graphics/layers/TerritoryWebGLRenderer.ts (4)
src/core/game/GameView.ts (13)
GameView(572-937)tile(110-112)x(806-808)y(809-811)players(739-741)id(86-88)id(458-460)owner(113-115)owner(771-773)PlayerView(185-570)player(731-737)type(98-100)type(464-466)src/core/configuration/Config.ts (1)
Theme(189-214)src/core/game/GameMap.ts (3)
TileRef(3-3)x(142-144)y(146-148)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-62)
src/client/graphics/layers/TerritoryRenderers.ts (4)
src/core/game/GameMap.ts (5)
TileRef(3-3)hasOwner(196-198)isDefended(229-231)x(142-144)y(146-148)src/client/graphics/layers/TerritoryWebGLRenderer.ts (2)
HoverHighlightOptions(13-18)TerritoryWebGLRenderer(25-1079)src/core/configuration/Config.ts (1)
Theme(189-214)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-62)
🪛 Biome (2.1.2)
src/core/game/GameImpl.ts
[error] 901-901: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 909-909: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ 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: Deploy to openfront.dev
🔇 Additional comments (26)
src/core/game/GameImpl.ts (6)
520-542: LGTM: Zero-allocation neighbor iterator.The implementation correctly iterates all 8 surrounding tiles with proper bounds checks. Based on learnings, this intentional duplication of coordinate logic (versus the array-returning
neighborsWithDiag()) is a hot-path optimization to avoid allocation overhead.
559-561: Good integration of defense state updates.Calling
updateDefendedStateForTileChangeafter ownership transfer ensures the conquered tile's defended state reflects nearby defense posts of the new owner.
584-586: Correct defended state cleanup.Clearing the defended flag when relinquishing ownership is appropriate, and the conditional check avoids unnecessary map updates.
917-925: LGTM: Clean cardinal neighbor iterator.The zero-allocation pattern with inline bounds checks is efficient and correct. Each direction is independently validated before the callback is invoked.
1008-1063: Well-structured defense state management.Both methods correctly:
- Check state before updating (avoiding unnecessary writes)
- Emit tile updates when state changes
- Handle the edge case where a conquered tile contains a DefensePost (line 1059)
The iteration over border tiles in
updateDefendedStateForDefensePostis bounded by map size and only runs when defense posts are added/removed/moved, making the performance impact acceptable.
799-804: No issue found with the ordering of DefensePost updates.The method correctly calls
updateDefendedStateForDefensePost()beforeunitGrid.updateUnitCell(). Sinceu.tile()returns a property that is already updated byUnitImpl.move()beforeupdateUnitTile()is called, the coordinates are current, not stale. This order is consistent with the patterns used inaddUnit()andremoveUnit().src/client/graphics/layers/TerritoryLayer.ts (8)
36-38: LGTM: Clean profile name hook.Simple method for performance overlay integration. Clear and direct.
83-84: LGTM: Initialization looks good.Proper null handling for player ID and settings read for WebGL toggle.
175-177: Good: Palette refresh on relation changes.Correctly triggers GPU palette update when alliances or embargos change.
305-326: LGTM: Event handlers are well structured.Clean integration of new events. The WebGL toggle correctly updates settings and triggers a full redraw.
334-338: Verify hover support condition.The condition
alternativeView || territoryRenderer !== nullmeans hover is always supported when a renderer exists. This seems correct since both renderers now handle hover, but the canvas renderer'ssetHover()is a no-op (line 178-180 in TerritoryRenderers.ts).The logic works, but consider documenting why canvas path still needs the
alternativeViewcheck when the renderer is always present afterinit().
396-445: Good: Clean renderer configuration with fallback.The factory pattern with explicit fallback is clear and handles all cases well. Status events provide good visibility into the rendering mode.
447-472: LGTM: Clear hover configuration.Good separation of hover styling. The stronger highlight in alternative view makes sense since borders are the primary visual there.
508-527: LGTM: Clean viewport-based rendering delegation.Viewport calculation with proper bounds clamping. The renderer abstraction handles the actual drawing.
src/client/graphics/layers/TerritoryRenderers.ts (6)
11-26: Good: Clean strategy interface.Well-defined interface with clear responsibilities. All methods have explicit types.
35-45: LGTM: Constructor with proper initialization.Initial 1x1 placeholder is fine since
redraw()is called during layer initialization.
140-172: LGTM: Efficient partial update with putImageData.Good use of viewport dirty rect for partial updates. The profiling instrumentation is helpful.
196-211: LGTM: Clear relationship color mapping.Simple and readable. The priority (self > friendly > embargo > neutral) is correct.
213-224: Verify TileRef layout assumption.The code assumes
TileRefdirectly maps to pixel offset (tile * 4). Based onGameMap.tswhereTileRef = number, this should be the linear index. This is correct if the map uses row-major order matching ImageData.
240-301: LGTM: Clean wrapper implementation.Good delegation pattern. The no-op
clearTile()is appropriate since WebGL manages tile state differently.src/client/graphics/layers/TerritoryWebGLRenderer.ts (6)
6-18: LGTM: Clean type definitions.
DirtySpanandHoverHighlightOptionsare well-typed. Simple and clear.
414-441: LGTM: Good factory with validation.Proper precondition checks before construction. Clear fallback reasons are helpful for debugging.
496-531: LGTM: Efficient dirty tracking.Row-span tracking enables partial texture updates. The early returns when full upload is needed prevent redundant work.
725-800: LGTM: Palette and relation texture setup.Clean encoding of player colors and relationships. The bit-based relation codes are efficient for shader lookups.
927-958: LGTM: Border detection with relation tracking.The shader correctly detects borders and accumulates relationship flags from neighbors. The repeated pattern is acceptable in GLSL for performance.
960-981: LGTM: Alternative view rendering consistent with canvas path.Color priority matches
CanvasTerritoryRenderer.alternateViewColor(). Good consistency between renderers.
… the u_relations texture in TerritoryWebGLRenderer
In src/client/graphics/layers/TerritoryWebGLRenderer.ts: - Deleted borderColorTexture, borderColorData, borderDirtyRows, borderNeedsFullUpload. - Removed setBorderColor, clearBorderColor, markBorderDirty, and uploadBorderTexture, plus the extra profiling calls. - Dropped the u_borderColor uniform from both the uniform map and the fragment shader. - Removed all texture creation/binding/uniform setup for the old border color texture; only u_state, u_palette, and u_relations remain. The WebGL renderer now relies solely on the palette + relations + state buffers for borders.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/client/graphics/layers/TerritoryWebGLRenderer.ts (1)
575-589: Dead code:labelUploadis never called.The past review comment correctly identifies this as unused code. Either integrate it with the profiler output or remove it to reduce clutter.
- private labelUpload( - base: string, - metrics: { rows: number; bytes: number }, - ): string { - if (metrics.rows === 0 || metrics.bytes === 0) { - return `${base} (skip)`; - } - const rowBucket = - metrics.rows >= this.canvas.height - ? "full" - : `${Math.ceil(metrics.rows / 50) * 50}`; - const kb = Math.max(1, Math.round(metrics.bytes / 1024)); - const kbBucket = kb > 1024 ? `${Math.round(kb / 1024)}MB` : `${kb}KB`; - return `${base} rows:${rowBucket} bytes:${kbBucket}`; - }src/core/game/GameImpl.ts (2)
790-794: Critical: Wrong order in removeUnit causes incorrect defended state.The past review comment correctly identifies this critical bug.
updateDefendedStateForDefensePostis called beforeunitGrid.removeUnit(u), sohasUnitNearbywill still find the unit being removed, causing border tiles to incorrectly retain defended status.Apply the fix from the past review:
removeUnit(u: Unit) { - if (u.type() === UnitType.DefensePost) { - this.updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl); - } this.unitGrid.removeUnit(u); + if (u.type() === UnitType.DefensePost) { + this.updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl); + } if (u.hasTrainStation()) { this._railNetwork.removeStation(u); } }
895-902: Removereturnfrom void function.The past review comment correctly identifies this static analysis warning.
setDefendedis declared void but usesreturn.Apply the suggested fix:
setDefended(ref: TileRef, value: boolean): void { - return this._map.setDefended(ref, value); + this._map.setDefended(ref, value); }src/core/game/GameView.ts (1)
858-864: Removereturnfrom void setter method.The past review comment correctly identifies this static analysis warning.
setDefendedis declared void but includes areturnstatement.Apply the suggested fix:
setDefended(ref: TileRef, value: boolean): void { - return this._map.setDefended(ref, value); + this._map.setDefended(ref, value); }
🧹 Nitpick comments (1)
src/core/game/GameImpl.ts (1)
799-804: Ordering consideration: Defense update before grid update.Unlike
addUnit(which updates defense after grid update), hereupdateDefendedStateForDefensePostis called beforeunitGrid.updateUnitCell(u). While likely not a bug (since the unit's tile is already updated), this inconsistency could be confusing.Consider moving the defense update after line 803 for consistency with
addUnit:updateUnitTile(u: Unit) { + this.unitGrid.updateUnitCell(u); if (u.type() === UnitType.DefensePost) { this.updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl); } - this.unitGrid.updateUnitCell(u); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/TerritoryWebGLRenderer.ts(1 hunks)src/core/game/GameImpl.ts(6 hunks)src/core/game/GameMap.ts(4 hunks)src/core/game/GameView.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/GameMap.ts
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
📚 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/game/GameImpl.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/GameImpl.tssrc/core/game/GameView.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/GameImpl.tssrc/core/game/GameView.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
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
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.tssrc/core/game/GameView.tssrc/client/graphics/layers/TerritoryWebGLRenderer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/core/game/GameView.tssrc/client/graphics/layers/TerritoryWebGLRenderer.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/GameView.ts
📚 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/game/GameView.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/core/game/GameView.tssrc/client/graphics/layers/TerritoryWebGLRenderer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/TerritoryWebGLRenderer.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/graphics/layers/TerritoryWebGLRenderer.ts
🧬 Code graph analysis (2)
src/core/game/GameImpl.ts (1)
src/core/game/GameMap.ts (5)
TileRef(3-3)x(134-136)y(138-140)ref(123-128)isDefended(221-223)
src/client/graphics/layers/TerritoryWebGLRenderer.ts (3)
src/core/game/GameView.ts (13)
GameView(572-929)tile(110-112)x(806-808)y(809-811)players(739-741)id(86-88)id(458-460)owner(113-115)owner(771-773)PlayerView(185-570)player(731-737)type(98-100)type(464-466)src/core/configuration/Config.ts (1)
Theme(189-214)src/core/game/GameMap.ts (3)
TileRef(3-3)x(134-136)y(138-140)
🪛 Biome (2.1.2)
src/core/game/GameImpl.ts
[error] 901-901: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
src/core/game/GameView.ts
[error] 863-864: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ 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: Deploy to openfront.dev
🔇 Additional comments (22)
src/core/game/GameImpl.ts (7)
520-542: LGTM: Zero-allocation neighbor iteration.The callback-based approach correctly avoids array allocation for hot-path code. Bounds checking is thorough, and the iteration logic is sound.
Based on learnings, the duplication between this and
neighborsWithDiag()is intentional for performance optimization.
559-561: LGTM: Defended state updated after tile conquest.The call to
updateDefendedStateForTileChangeis correctly placed after ownership assignment and border updates.
584-586: LGTM: Defended state cleared on relinquish.The defensive check with
isDefendedbefore clearing is a good optimization. Logic is correct.
786-788: LGTM: Defended state updated after adding DefensePost.The update is correctly placed after
unitGrid.addUnit(u), ensuring the unit is in the grid before recalculating defended borders.
909-917: LGTM: Cardinal neighbor iteration is efficient.The callback-based approach avoids array allocation. Each cardinal direction is checked with proper bounds validation.
1000-1028: LGTM: Defense post range logic is sound.The implementation correctly:
- Filters border tiles within defense post range using squared distance
- Recalculates defended state via
hasUnitNearby- Only emits updates when state actually changes
1030-1055: LGTM: Tile ownership change handles defended state correctly.The logic properly:
- Updates defended state for the newly conquered tile
- Checks if the tile has a defense post (via
hasUnitNearbywith range 0)- Propagates defense updates to nearby border tiles when needed
src/core/game/GameView.ts (8)
44-46: LGTM: Border tint constants are reasonable.The color targets (green for friendly, red for embargo) and 35% blend ratio provide clear visual distinction without overwhelming the base border colors.
191-310: LGTM: Border color pre-computation optimizes rendering.Pre-computing all border color variants (neutral, friendly, embargo, and their defended counterparts) avoids repeated runtime calculations. The tinting math correctly blends base colors with relation targets at the specified ratio.
333-368: LGTM: Border color logic handles relations and defended state.The method correctly:
- Determines neighbor relations via
borderRelationFlags- Prioritizes embargo over friendly tinting
- Applies defended checkerboard pattern when needed
- Uses pre-computed colors for efficiency
The checkerboard parity logic
(x % 2 === 0 && y % 2 === 0) || (y % 2 === 1 && x % 2 === 1)correctly identifies light tiles.
370-401: LGTM: Relation flag computation is efficient and correct.The method properly:
- Iterates only cardinal neighbors (matches border detection)
- Skips unowned tiles and same-owner neighbors
- Checks bilateral friendly relations (either direction)
- Uses early break when embargo is detected
588-589: LGTM: Shared tile state flag added correctly.The readonly field with constructor parameter (defaulting to
false) maintains backward compatibility while enabling SAB mode when available.Also applies to: 598-598, 601-601
630-639: LGTM: Tile update branching matches SAB mode requirements.Based on learnings, the implementation correctly handles the two modes:
- SAB mode:
packedTileUpdatescontains unpacked tile references, soNumber(tu)is correct- Non-SAB mode:
packedTileUpdatescontains packed bigints that needupdateTile(tu)decoding
743-750: LGTM: Defensive player lookup prevents crashes.Returning
terraNulliusfor invalid IDs (0 or missing) instead of throwing is a good defensive pattern. The return typePlayerView | TerraNulliuscorrectly reflects this behavior.
914-924: LGTM: Shared state API correctly validates buffer type.The
instanceof SharedArrayBuffercheck insharedStateBuffer()ensures the buffer is actually shared memory, not a regularArrayBuffer. Returningundefinedwhen not available is the right pattern.src/client/graphics/layers/TerritoryWebGLRenderer.ts (7)
1-411: LGTM: Factory pattern with proper WebGL2 initialization.The static
create()method correctly validates:
- SharedArrayBuffer availability
- Buffer size matches map dimensions
- WebGL2 context creation
- Program compilation and linking
The WebGL setup uses appropriate texture parameters (NEAREST filtering, CLAMP_TO_EDGE wrapping) and pixel formats (R16UI for state, RGBA8 for palette, R8UI for relations).
441-459: LGTM: Dirty tracking optimizes texture uploads.The row-based dirty span tracking correctly:
- Expands spans to cover new dirty pixels via
Math.min/max- Uses early return when full upload is pending
- Provides granular incremental updates
468-573: LGTM: Render pipeline with efficient incremental uploads.The implementation correctly:
- Updates time-based uniforms (time, hoveredPlayerId, hover parameters) each frame
- Branches between full texture upload and dirty-row-based incremental updates
- Uses
texSubImage2Dfor dirty rows to minimize GPU uploads- Tracks upload metrics for profiling
591-689: LGTM: Palette and relation textures correctly encode player data.The implementation properly:
- Packs territory and border colors into 8 bytes per player
- Builds a relation matrix encoding friendly/embargo/self flags (bits 0, 1, 2)
- Uses
safePlayerBySmallIdto handle invalid IDs defensively- Uploads with appropriate formats (RGBA8 for colors, R8UI for relations)
697-707: LGTM: Vertex shader correctly transforms full-screen quad.The shader properly converts vertex positions to clip space and flips the Y axis to match OpenGL's bottom-left origin with the canvas's top-left orientation.
709-893: LGTM: Fragment shader implements territory rendering with complex features.The shader correctly handles:
- State unpacking (owner, fallout, defended bits)
- Border detection via 4-neighbor comparison
- Relation-based tinting matching TypeScript constants
- Alternative view mode with relation-colored borders
- Defended checkerboard pattern
- Hover highlighting with pulse animation
The logic matches the CPU-side border color computation in
GameView.ts, ensuring visual consistency.
895-943: LGTM: Shader compilation with proper error handling.The compilation helpers correctly:
- Check compilation/linking status
- Log errors with descriptive prefixes for debugging
- Clean up resources (delete program/shader) on failure
- Return null to signal failure
…ring - Move unitGrid.removeUnit() call to beginning of removeUnit method for cleaner code organization
…tatements - Updated setOwnerID, setDefended, forEachTile, and setFallout methods in GameImpl and GameView to eliminate redundant return statements
- Updated the initialization logic to include a check for sharedDirtyBuffer alongside sharedTileRingHeader and sharedTileRingData, ensuring all necessary data is present before creating sharedTileRing views.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/game/GameImpl.ts (1)
900-902: Removereturnfrom void function.This is the same issue flagged in the previous review. The
setDefendedmethod has a void return type but usesreturnwhen calling the map method.Apply this diff:
setDefended(ref: TileRef, value: boolean): void { - return this._map.setDefended(ref, value); + this._map.setDefended(ref, value); }
🧹 Nitpick comments (1)
src/core/worker/Worker.worker.ts (1)
94-102: Simplify the tile update callback logic.The second branch (lines 100-102) checks only
sharedTileRingwithoutdirtyFlags, but based on the initialization logic at lines 78-86, both are always set together or both are null. This branch appears unreachable. The nested ternary can be simplified for better readability.Apply this diff to simplify:
- sharedTileRing && dirtyFlags - ? (tile: TileRef) => { - if (Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0) { - pushTileUpdate(sharedTileRing!, tile); - } - } - : sharedTileRing - ? (tile: TileRef) => pushTileUpdate(sharedTileRing!, tile) - : undefined, + sharedTileRing + ? (tile: TileRef) => { + if (Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0) { + pushTileUpdate(sharedTileRing!, tile); + } + } + : undefined,This removes the dead branch and makes the logic clearer, since
dirtyFlagsis always defined whensharedTileRingis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/TerritoryWebGLRenderer.ts(1 hunks)src/core/game/GameImpl.ts(7 hunks)src/core/game/GameView.ts(11 hunks)src/core/worker/Worker.worker.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/layers/TerritoryWebGLRenderer.ts
- src/core/game/GameView.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
📚 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/game/GameImpl.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/GameImpl.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
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
📚 Learning: 2025-12-04T15:45:21.200Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2548
File: src/core/game/GameImpl.ts:0-0
Timestamp: 2025-12-04T15:45:21.200Z
Learning: In OpenFrontIO, UnitImpl.delete() marks units as inactive before calling GameImpl.removeUnit, and UnitGrid.hasUnitNearby ignores inactive units. This means units being removed are already "invisible" to nearby unit checks during the removal process.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.tssrc/core/worker/Worker.worker.ts
📚 Learning: 2025-11-26T22:27:31.844Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
Applied to files:
src/core/worker/Worker.worker.ts
🧬 Code graph analysis (2)
src/core/game/GameImpl.ts (2)
src/core/game/GameView.ts (4)
x(806-808)y(809-811)ref(800-802)isDefended(858-860)src/core/game/GameMap.ts (5)
TileRef(3-3)x(134-136)y(138-140)ref(123-128)isDefended(221-223)
src/core/worker/Worker.worker.ts (2)
src/core/worker/SharedTileRing.ts (3)
SharedTileRingViews(9-14)createSharedTileRingViews(31-43)pushTileUpdate(45-64)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: Deploy to openfront.dev
🔇 Additional comments (11)
src/core/worker/Worker.worker.ts (3)
44-65: Good re-entrancy protection for turn processing.The
isProcessingTurnsguard prevents overlapping execution, and thewhileloop efficiently drains all pending turns. Thetry-finallyensures the flag is reset even ifexecuteNextTick()throws, allowing errors to propagate to the worker error handler.
73-87: Past issue resolved: all three buffers now required.The code now correctly checks for all three buffers (
sharedTileRingHeader,sharedTileRingData,sharedDirtyBuffer) before creating the views, preventing the unsafe non-null assertion issue from the previous review.
109-109: Good placement of turn processing triggers.Calling
processPendingTurns()after initialization and after each turn ensures pending work is processed promptly without relying on heartbeat events. This approach provides controlled, batched execution.Also applies to: 126-126
src/core/game/GameImpl.ts (8)
520-542: LGTM: Zero-allocation neighbor iteration is solid.The callback-based iteration avoids array allocations while correctly iterating all 8 neighbors with proper bounds checking. Based on learnings, the duplication with
neighborsWithDiag()is intentional for performance.
559-561: LGTM: Defended-state update is correctly placed.The update runs after ownership and border changes, ensuring the defended state reflects the new owner and any nearby defense posts.
584-586: LGTM: Clearing defended state on relinquish is correct.When a tile becomes unowned, its defended status is properly cleared. The TileUpdate at lines 587-590 will reflect this change to clients.
909-917: LGTM: Cardinal neighbor iteration is correct.The implementation correctly iterates the 4 cardinal neighbors (N/S/E/W) with proper bounds checking, providing a zero-allocation alternative when diagonals aren't needed.
786-788: LGTM: Defense post addition triggers defended-state update.When a defense post is added to the grid, the defended state of nearby border tiles is correctly updated.
790-794: LGTM: Removal order is now correct.The defense post is removed from the grid before updating defended state for nearby border tiles, ensuring
hasUnitNearbydoesn't find the removed unit. Based on learnings, units are marked inactive before removal, making them invisible to nearby checks.
1000-1028: LGTM: Defended-state propagation is efficient.The method correctly iterates only the owner's border tiles within range of the defense post, then rechecks each tile using
hasUnitNearbyto account for other defense posts. The distance filter (line 1011) optimizes by skipping tiles outside range of the changed defense post, while thehasUnitNearbycall (lines 1013-1018) correctly determines the tile's final defended state based on all nearby defense posts.
1030-1055: LGTM: Tile ownership change correctly updates defended state.The method handles two cases:
- Update the conquered tile's defended state based on nearby defense posts (lines 1034-1047)
- If the tile itself contains a defense post, update nearby border tiles (lines 1049-1054)
The
hasUnitNearbycall with range 0 (line 1051) correctly checks if a defense post exists on the exact tile.
| updateUnitTile(u: Unit) { | ||
| if (u.type() === UnitType.DefensePost) { | ||
| this.updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl); | ||
| } | ||
| this.unitGrid.updateUnitCell(u); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for updateDefendedStateForDefensePost implementation
rg -nP --type=ts "updateDefendedStateForDefensePost" --max-count=20Repository: openfrontio/OpenFrontIO
Length of output: 565
🏁 Script executed:
# Search for tests involving defense posts and position/movement
rg -nP --type=ts -C8 "(DefensePost|defense.*post)" -g "**/*.test.ts" -g "**/*.spec.ts" | head -100Repository: openfrontio/OpenFrontIO
Length of output: 7387
🏁 Script executed:
# Check the broader context of GameImpl.ts around the updateUnitTile method
fd -e ts -path "**/GameImpl.ts" -exec cat -n {} \; | sed -n '790,820p'Repository: openfrontio/OpenFrontIO
Length of output: 235
🏁 Script executed:
# Read the updateDefendedStateForDefensePost implementation
sed -n '1003,1100p' src/core/game/GameImpl.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 2415
🏁 Script executed:
# Get more context around updateUnitTile to understand the flow
sed -n '795,810p' src/core/game/GameImpl.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 569
🏁 Script executed:
# Search for any tests involving defense post movement or position changes
rg -nP --type=ts "(updateUnitTile|move.*defensePost|DefensePost.*move)" tests/ -A 5 -B 5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there are any tile updates or position-related defended state tests
rg -nP --type=ts "defended|isDefended" tests/ -i | grep -i "defensepost\|defense.*post" -A 2 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Find hasUnitNearby implementation to understand how it uses unitGrid
rg -nP --type=ts "hasUnitNearby\s*\(" --max-count=5 -A 15 src/core/game/UnitGrid.tsRepository: openfrontio/OpenFrontIO
Length of output: 677
🏁 Script executed:
# Search for any test that moves a unit (especially DefensePost) and checks defended state
rg -nP --type=ts "(move|setTile|updateUnitTile)" tests/ -B 5 -A 10 | grep -i "defense\|defended" -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check if there are any tests for DefensePost movement or position changes
rg -nP --type=ts "DefensePost" tests/ -B 2 -A 15 | grep -i "move\|position\|tile\|updateUnitTile" -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 5800
🏁 Script executed:
# Check when updateUnitTile is called - search for callers
rg -nP --type=ts "updateUnitTile\(" src/ -B 3 -A 1Repository: openfrontio/OpenFrontIO
Length of output: 630
🏁 Script executed:
# Look at the removeUnit method to see if it also updates defended state
rg -nP --type=ts "removeUnit" src/core/game/GameImpl.ts -A 8Repository: openfrontio/OpenFrontIO
Length of output: 406
🏁 Script executed:
# Check if there's any handling for old position in defended state updates
rg -nP --type=ts "defended" src/core/game/GameImpl.ts -B 2 -A 2 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 598
🏁 Script executed:
# Check the exact sequence in UnitImpl.setTile - get more context
sed -n '150,170p' src/core/game/UnitImpl.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 649
🏁 Script executed:
# Check if there are integration tests that would catch movement issues
rg -nP --type=ts "setTile|move.*tile" tests/ -B 3 -A 8 | grep -i "defensepost\|defended" -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Look for any other calls to updateDefendedStateForDefensePost
rg -nP --type=ts "updateDefendedStateForDefensePost" src/ -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 1722
🏁 Script executed:
# Verify there's no other mechanism that updates defended state for the new position
rg -nP --type=ts "setDefended|defended.*update" src/core/game/GameImpl.ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 762
🏁 Script executed:
# Check if there are any tests that would catch this bug (moving a DefensePost and checking defended state of new tiles)
rg -nP --type=ts "move\(" tests/ -B 5 -A 10 | grep -C 10 "DefensePost\|defended"Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Double-check the order in addUnit, removeUnit, and updateUnitTile side by side
echo "=== addUnit ===" && sed -n '784,789p' src/core/game/GameImpl.ts && echo -e "\n=== removeUnit ===" && sed -n '790,795p' src/core/game/GameImpl.ts && echo -e "\n=== updateUnitTile ===" && sed -n '799,804p' src/core/game/GameImpl.tsRepository: openfrontio/OpenFrontIO
Length of output: 734
Reverse the order of defended-state update and grid update in updateUnitTile.
In the updateUnitTile method (lines 800–803), updateDefendedStateForDefensePost is called before unitGrid.updateUnitCell. This means the defended state is recalculated while the unit's position in the grid is still at its old location. When hasUnitNearby searches the grid, it will not find the unit at its new position, causing border tiles near the new position to incorrectly remain undefended.
The correct order—used consistently in addUnit and removeUnit—is to update the grid first, then recalculate defended state. Swap these two method calls so unitGrid.updateUnitCell(u) is called before updateDefendedStateForDefensePost.
🤖 Prompt for AI Agents
In src/core/game/GameImpl.ts around lines 799 to 804, the calls in
updateUnitTile are in the wrong order: updateDefendedStateForDefensePost is
invoked before unitGrid.updateUnitCell, which causes defended-state to be
computed against the unit's old grid position. Fix by swapping the two calls so
unitGrid.updateUnitCell(u) runs first, then if u.type() === UnitType.DefensePost
call updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl).
Description:
sab+webgl+playerExecution patches
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME