Sab 021225 #2553
Sab 021225 #2553
Conversation
Increase worker heartbeats per frame when far behind server to fast-forward simulation. Track backlog and expose catch-up status via TickMetricsEvent. Extend performance overlay to display backlog turns and indicate active catch-up mode.
- Refactor worker update handling into processPendingUpdates so multiple GameUpdateViewData objects are batched per frame. - Combine all tick updates in a batch into a single GameUpdateViewData before applying it to GameView, while still running per-tick side effects (turnComplete, hashes, backlog metrics, win saving). - Ensure layers using updatesSinceLastTick and recentlyUpdatedTiles see all events in a batch, fixing visual artifacts during fast-forward resync.
GameRunner exposes pending work via a new hasPendingTurns() so the worker can check whether more ticks need to be processed. Worker auto-runs ticks: as soon as it initializes or receives a new turn, it calls processPendingTurns() and loops executeNextTick() while hasPendingTurns() is true. No more "heartbeat" message type; the worker no longer depends on the main thread’s RAF loop to advance the simulation. Client main thread simplified: Removed CATCH_UP_HEARTBEATS_PER_FRAME, the heartbeat loop, and the lastBeatsPerFrame tracking. keepWorkerAlive now just manages frame skipping + draining. When it decides to render (based on renderEveryN), it drains pendingUpdates, merges them, updates GameView, and runs renderer.tick(). Because rendering a batch always implies draining, we restored the invariant that every GameView.update is paired with a layer tick() (no more lost incremental updates). MAX_RENDER_EVERY_N is now 5 to keep the queue from growing too large while the worker sprints.
removed: - catchUpMode and its CATCH_UP_ENTER/EXIT thresholds in ClientGameRunner - tick metrics fields and overlay UI for inCatchUpMode and beatsPerFrame - leftover worker heartbeat plumbing (message type + WorkerClient.sendHeartbeat) that was no longer used after self-clocking changed: - backlog tracking: keep serverTurnHighWater / lastProcessedTick / backlogTurns, but simplify it to just compute backlog and a backlogGrowing flag instead of driving a dedicated catch-up mode - frame skip: adaptRenderFrequency now only increases renderEveryN when backlog > 0 and still growing; when backlog is stable/shrinking or zero, it decays renderEveryN back toward 1 - render loop: uses the backlog-aware renderEveryN unconditionally (no catch-up flag), and resets skipping completely when backlog reaches 0 - metrics/overlay: TickMetricsEvent now carries backlogTurns and renderEveryN; the performance overlay displays backlog and current “render every N frames” but no longer mentions catch-up or heartbeats Learnings during branch development leading to this Once the worker self-clocks, a separate “catch-up mode” and beats-per-frame knob don’t add real control; they just complicate the model. Backlog is still a valuable signal, but it’s more effective as a quantitative input (backlog size and whether it’s growing) than as a boolean mode toggle. Frame skipping should be driven by actual backlog pressure plus frame cost: throttle only while backlog is growing and frames are heavy, and automatically relax back to full-rate rendering once the simulation catches up.
…ponsive. src/client/ClientGameRunner.ts now drains pending game updates in small chunks (max 100 updates or ~8ms per slice) via requestAnimationFrame, merging and rendering per slice, and only clears the processing flag when the queue is empty.
…log handling. Introduced dynamic slice budget scaling based on backlog size, allowing for longer processing times when necessary while maintaining UI responsiveness.
process updates in a single budgeted loop per RAF and render once track queue head with pendingStart and compact to avoid array shifts
- Refactor rendering and metrics emission in ClientGameRunner to ensure updates occur only after all processing is complete - Throttle renderGame() based on the current backlog
- Introduced new metrics in ClientGameRunner to track worker simulation ticks and render tick calls per second. - Updated TickMetricsEvent to include these new metrics. - Enhanced PerformanceOverlay to display worker and render ticks per second, improving performance monitoring capabilities. - Adjusted minimum FPS in GameRenderer
Added src/core/worker/SharedTileRing.ts, which defines a SharedArrayBuffer-backed ring buffer (SharedTileRingBuffers/SharedTileRingViews) and helpers pushTileUpdate (worker-side writer) and drainTileUpdates (main-thread reader) using Atomics. Extended GameRunner (src/core/GameRunner.ts) with an optional tileUpdateSink?: (update: bigint) => void; when provided, tile updates are sent to the sink instead of being packed into GameUpdateViewData.packedTileUpdates (those become an empty BigUint64Array in this mode). Extended the worker protocol (src/core/worker/WorkerMessages.ts) so the init message can optionally carry sharedTileRingHeader and sharedTileRingData (the two SABs for the ring). Updated WorkerClient (src/core/worker/WorkerClient.ts) to accept optional SharedTileRingBuffers in its constructor and, during initialize(), include those SABs in the init message sent to Worker.worker.ts. Updated the worker implementation (src/core/worker/Worker.worker.ts) to build sharedTileRing views from the SABs on "init" and pass a (update: bigint) => pushTileUpdate(...) sink into createGameRunner, so every tile update from the simulation goes into the shared ring instead of the message payload. Updated the client side (src/client/ClientGameRunner.ts): In createClientGame, when SharedArrayBuffer/Atomics are available and crossOriginIsolated is true, it allocates the ring (createSharedTileRingBuffers with capacity 262144), creates views, passes buffers into WorkerClient, and keeps the views in ClientGameRunner. ClientGameRunner now has a tileRingViews field and, in mergeGameUpdates, if the ring is present it ignores gu.packedTileUpdates and instead drains up to MAX_TILE_UPDATES_PER_RENDER (100 000) tile updates from the SAB ring into combinedPackedTileUpdates before constructing the merged GameUpdateViewData passed to GameView.update. If SABs are unavailable, it falls back to the original behaviour (packing gu.packedTileUpdates from messages), so the feature is opt-in and backward compatible. Net effect: when the page is cross-origin isolated, the worker no longer sends per-tick BigUint64Array tile payloads via postMessage; it streams tile updates into a shared ring buffer, and the main thread pulls and applies them at its own pace while still batching non-tile updates and rendering exactly as before. If you’d like, I can next run the existing tests/build in your environment to sanity-check everything.
- Share GameMapImpl tile state between worker and main via SharedArrayBuffer - Add SAB-backed tile update ring buffer to stream tile changes instead of postMessage payloads - Wire shared state/ring through WorkerClient, Worker.worker, GameRunner, and ClientGameRunner - Update GameView to skip updateTile when shared state is enabled and consume tile refs from the ring
Store only TileRef instead of packed tile+state values
This reverts commit 08a2ff9.
- Extend SharedTileRing to include a shared dirtyFlags buffer alongside header and data - Pass shared dirty buffer through WorkerClient/WorkerMessages and initialize views in Worker.worker - In SAB mode, mark tiles dirty via Atomics.compareExchange before enqueuing to ensure each tile is queued at most once until processed - On the main thread, clear dirty flags when draining the ring and build packedTileUpdates from distinct tile refs - Keep non-SAB behaviour unchanged while reducing ring pressure and making overflows reflect true backlog, not duplicate updates
WalkthroughIntroduces shared memory tile ring buffers for efficient inter-thread tile update communication and replaces heartbeat-driven game loop with controlled turn-by-turn processing. Implements backlog-based batching, dynamic frame throttling, and extended metrics tracking across client rendering and performance monitoring layers. Changes
Sequence DiagramsequenceDiagram
actor Worker as Worker Thread
participant GR as GameRunner
participant STR as SharedTileRing
participant WC as WorkerClient
participant CGR as ClientGameRunner
participant GRend as GameRenderer
participant PerfUI as PerformanceOverlay
Worker->>GR: executeNextTick()
activate GR
GR->>STR: pushTileUpdate(tileRef)
Note over STR: Lock-free enqueue via Atomics
GR-->>Worker: GameUpdateViewData (empty packedTileUpdates)
deactivate GR
Worker->>WC: send GameUpdateViewData (via postMessage)
activate WC
WC-->>CGR: (message received)
deactivate WC
activate CGR
CGR->>CGR: pendingUpdates.push(update)
CGR->>CGR: processPendingUpdates()
rect rgb(200, 220, 255)
Note over CGR: Batched Processing Loop
CGR->>CGR: mergeGameUpdates(batch)
CGR->>STR: drainTileUpdates(maxItems)
Note over STR: Lock-free dequeue via Atomics
STR-->>CGR: packed tile metrics
CGR->>CGR: updateBacklogMetrics()
end
CGR->>CGR: emit TickMetricsEvent (with backlog, tile metrics)
deactivate CGR
CGR->>GRend: TickMetricsEvent
activate GRend
GRend->>GRend: backlogTurns = metrics.backlogTurns
alt backlog > 0
GRend->>GRend: compute throttled FPS
GRend->>GRend: skip frame if interval too short
else backlog = 0
GRend->>GRend: render at BASE_FPS
end
deactivate GRend
CGR->>PerfUI: TickMetricsEvent
activate PerfUI
PerfUI->>PerfUI: updateTickMetrics(backlog, tiles, ringBuffer...)
PerfUI->>PerfUI: render overlay (backlog, tps, tile updates/s)
deactivate PerfUI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (3 warnings)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/client/InputHandler.ts (1)
128-146: Consider using an options object for TickMetricsEvent constructor.With 10 optional parameters, the constructor becomes hard to read and use correctly. Callers must pass
undefinedfor unused fields. A single options object would be cleaner:+interface TickMetricsData { + 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, - // ... many more - ) {} + constructor(public readonly data: TickMetricsData = {}) {} }This also makes adding future metrics simpler.
src/client/graphics/GameRenderer.ts (1)
298-300:backlogGrowingis set but never used.The field is updated on line 320 but never referenced in the rendering logic. Either remove it or use it (e.g., to show a visual indicator or further adjust throttling).
#!/bin/bash # Check if backlogGrowing is used anywhere in the codebase rg -n "backlogGrowing" --type=tssrc/client/ClientGameRunner.ts (1)
680-686: Avoid double type assertion withas unknown as any[].The pattern
gu.updates[type] as unknown as any[]bypasses type safety entirely. SinceGameUpdatesis a mapped type with known update types, consider using a type guard or a more specific approach.One approach is to accept the dynamic nature here with a single cast:
- const updatesForType = gu.updates[type] as unknown as any[]; - (combinedUpdates[type] as unknown as any[]).push(...updatesForType); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const src = gu.updates[type] as any[]; + const dst = combinedUpdates[type] as any[]; + dst.push(...src);This keeps the cast explicit while reducing the chain length.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/client/ClientGameRunner.ts(11 hunks)src/client/InputHandler.ts(1 hunks)src/client/graphics/GameRenderer.ts(4 hunks)src/client/graphics/layers/PerformanceOverlay.ts(6 hunks)src/core/GameRunner.ts(5 hunks)src/core/game/GameMap.ts(2 hunks)src/core/game/GameView.ts(3 hunks)src/core/game/TerrainMapLoader.ts(4 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)
🧰 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.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-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/client/graphics/layers/PerformanceOverlay.tssrc/core/GameRunner.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/SharedTileRing.tssrc/client/graphics/layers/PerformanceOverlay.tssrc/core/worker/WorkerClient.tssrc/core/game/TerrainMapLoader.tssrc/core/worker/Worker.worker.tssrc/client/ClientGameRunner.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/SharedTileRing.tssrc/core/worker/WorkerClient.tssrc/core/GameRunner.tssrc/core/game/TerrainMapLoader.tssrc/core/game/GameMap.tssrc/core/worker/Worker.worker.tssrc/core/game/GameView.tssrc/client/ClientGameRunner.ts
📚 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/worker/SharedTileRing.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/GameRunner.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-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/GameRenderer.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/GameRenderer.tssrc/client/ClientGameRunner.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/ClientGameRunner.ts
🧬 Code graph analysis (8)
src/client/InputHandler.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/core/worker/SharedTileRing.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/worker/WorkerClient.ts (1)
src/core/worker/SharedTileRing.ts (1)
SharedTileRingBuffers(3-7)
src/core/GameRunner.ts (5)
src/core/game/GameView.ts (2)
tile(106-108)config(670-672)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/configuration/ConfigLoader.ts (1)
getConfig(11-27)
src/core/game/TerrainMapLoader.ts (2)
src/core/game/GameImpl.ts (2)
map(197-199)miniMap(200-202)src/client/TerrainMapFileLoader.ts (1)
terrainMapFileLoader(4-4)
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 (1)
src/client/InputHandler.ts (1)
BacklogStatusEvent(148-153)
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/core/game/GameUpdates.ts (2)
GameUpdateViewData(17-23)HashUpdate(252-256)src/client/Transport.ts (1)
SendHashEvent(157-162)src/client/InputHandler.ts (2)
TickMetricsEvent(128-146)BacklogStatusEvent(148-153)src/core/game/Game.ts (1)
GameUpdates(33-35)src/core/game/GameMap.ts (2)
TileRef(3-3)ref(122-127)
🔇 Additional comments (27)
src/client/InputHandler.ts (1)
148-153: LGTM!
BacklogStatusEventis clean and straightforward. Both fields are required since the event always carries current backlog state.src/core/game/GameMap.ts (1)
78-103: LGTM!The optional
stateBufferparameter enables shared memory usage while preserving backward compatibility. The length validation is correct and mirrors the terrain data check above.src/core/worker/WorkerMessages.ts (1)
38-41: Naming inconsistency:sharedDirtyBuffervsSharedTileRingBuffers.dirty.Looking at
SharedTileRingBuffers(from relevant snippets), it has adirtyfield. However,InitMessagepasses it assharedDirtyBufferseparately rather than as part of the ring buffers. This works but is slightly inconsistent sincedirtyis conceptually part of the tile ring.Consider either:
- Keeping it separate (current approach) if dirty flags may be used independently, or
- Documenting why it's passed separately
This is minor and the current approach works fine.
src/core/game/GameView.ts (1)
518-527: LGTM!The conditional logic correctly handles the two tile update modes:
- SAB mode:
Number(tu)extracts the tile ref directly (state lives in shared buffer)- Non-SAB mode:
updateTile(tu)decodes packed format(tileRef << 16n | state)This matches the documented behavior in the retrieved learnings.
src/core/worker/WorkerClient.ts (2)
23-29: LGTM!Constructor extension is clean. Optional parameters preserve backward compatibility for callers not using shared memory.
77-80: LGTM!Shared buffer fields are correctly extracted and passed to the worker. Optional chaining handles the case when
sharedTileRingBuffersis undefined.src/client/graphics/GameRenderer.ts (1)
356-378: Frame throttling logic looks correct.The linear interpolation from 60 FPS down to 10 FPS as backlog grows is a reasonable approach. A few notes:
- The early return still calls
requestAnimationFrame, maintaining the render loop.lastRenderTimeis updated after the check, so the first frame always renders.- Constants are local to the function; consider extracting if they need tuning.
src/core/worker/Worker.worker.ts (2)
44-65: LGTM!The
processPendingTurnsfunction correctly:
- Guards against re-entrancy with
isProcessingTurns- Early-exits if no runner or no pending turns
- Uses try/finally to always reset the flag
- Drains all pending turns in a loop
90-99: Tile update callback logic is correct.The nested ternary creates three paths:
- Ring + dirty flags: Atomics-based deduplication before push
- Ring only: Direct push (every tile update goes to ring)
- No ring: undefined callback
Per learnings, the dirty flag deduplication bounds entries to map size, making overflow extremely rare.
src/core/GameRunner.ts (3)
35-48: Clean API extension for shared memory support.The optional parameters
tileUpdateSinkandsharedStateBufferextend the factory function nicely without breaking existing callers. The threading ofsharedStateBuffertoloadGameMapaligns with the SAB initialization flow.
183-196: Tile update dispatch logic is correct.The branching between sink-based streaming and packed array modes is clean. The extraction of
tileReffrom the packed update usingNumber(u.update >> 16n)correctly decodes the format documented in the learnings (packed astileRef << 16n | state).
292-295:hasPendingTurnsis actively used in the codebase.The method is called in
src/core/worker/Worker.worker.tsat lines 53 and 59, where it's used both as a conditional check and in a while loop for turn processing logic. The public API addition is justified and should be retained.Likely an incorrect or invalid review comment.
src/core/game/TerrainMapLoader.ts (3)
42-55: Feature detection and cache gating is well designed.The triple check for
SharedArrayBuffer,Atomics, andcrossOriginIsolatedensures SAB is only used in properly isolated contexts. Disabling cache when SAB is available but not provided prevents stale buffer reuse.
69-76: Confirm 4x map intentionally skips shared state buffer.The normal map path (line 71-75) passes
stateBuffer, but the 4x map path (line 76) does not. This appears intentional since the 4x map is used as a mini-map and likely does not need shared state. Please confirm this is the expected behavior.
95-110: Result construction and caching logic looks correct.The
sharedDirtyBufferis set toundefinedhere and populated by the consumer later, which aligns with the design whereClientGameRunnercreates the ring buffers. The conditional caching only for non-SAB paths prevents issues with shared buffer reuse.src/client/graphics/layers/PerformanceOverlay.ts (3)
440-468: New state properties for extended metrics are well structured.Each new metric has its own reactive
@stateproperty, which is clean and aligns with the existing pattern. The properties are appropriately typed.
545-548: Overflow tracking only records presence, not count.Setting
ringBufferOverflows = 1when any overflow occurs means you lose the actual overflow count. Based on the learnings, overflow should be extremely rare, so this is acceptable for a diagnostic flag. If detailed counts become needed, consider accumulating the value instead.
326-332: Reset method correctly initializes all new tile metrics.All new fields are properly reset, matching their initial state values.
src/core/worker/SharedTileRing.ts (3)
3-14: Clean typed interfaces for shared memory structures.Using interfaces
SharedTileRingBuffersandSharedTileRingViewswith composition is idiomatic TypeScript. This avoids class hierarchies and keeps the memory layout explicit.
45-64: Non-atomic buffer write is acceptable for this use case.Line 62 (
buffer[write] = value) uses a regular array assignment rather thanAtomics.store. While this could theoretically cause visibility issues between threads, based on learnings, any race would only manifest as redundant tile refs being rendered (not memory corruption), which is acceptable for a rendering path. The deduplication and dirty-flag mechanism in the consumer provides additional safety.
66-85: Drain function correctly advances read pointer.The loop correctly reads up to
maxItemsentries and updates the read index atomically at the end. The modulo arithmetic handles wrap-around properly.src/client/ClientGameRunner.ts (6)
183-213: Shared tile ring setup is well structured.The feature detection mirrors
TerrainMapLoader.tsfor consistency. Ring capacity is set tonumTiles(map dimensions), which aligns with the learnings that overflow should be extremely rare due to dirty-flag deduplication.
510-569: Dynamic budget scaling for backlog catch-up is well designed.The sliding scale from
BASE_SLICE_BUDGET_MS(8ms) toMAX_SLICE_BUDGET_MS(1000ms) based on backlog depth allows the client to catch up aggressively when far behind while staying responsive when nearly caught up. The constantsBACKLOG_FREE_TURNSandBACKLOG_MAX_TURNSprovide clear tuning points.
581-641: Rendering only after all processing is complete.The condition at line 582-585 ensures rendering only happens when the pending queue is fully drained for this slice. This batches multiple ticks into a single render, which is efficient. The metrics calculation and emission is clean.
696-733: Tile ring drain and deduplication logic is correct.The flow is:
- Drain tile refs from ring buffer
- Deduplicate using a
Set- Calculate utilization metrics
- Clear dirty flags atomically
- Build packed tile updates
The use of
Atomics.storeto clear dirty flags (line 730) ensures visibility to the producer thread. The deduplication step prevents redundant tile processing.
755-766: Backlog metrics tracking is straightforward.The calculation of
backlogTurnsasserverTurnHighWater - lastProcessedTickcorrectly reflects how far behind the client is. EmittingBacklogStatusEventallows other components to react to backlog changes.
572-579: Queue compaction prevents unbounded memory growth.The condition triggers compaction when
pendingStart > 1024or when it exceeds half the queue length. This is a reasonable heuristic to balance memory usage against array copy overhead.
| <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.
Use translateText() for new UI strings.
These new metrics labels are hardcoded in English:
- "Worker ticks/s:"
- "Render ticks/s:"
- "Ticks per render:"
- "Backlog turns:"
- "Tile updates/render:"
- "Ring buffer:"
- "Ring drain time:"
The existing pattern uses translateText() for user-facing text. Please add translation keys to en.json and wrap these strings.
Example fix for one line:
- <div class="performance-line">
- Worker ticks/s:
- <span>${this.workerTicksPerSecond.toFixed(1)}</span>
- </div>
+ <div class="performance-line">
+ ${translateText("performance_overlay.worker_ticks_per_second")}
+ <span>${this.workerTicksPerSecond.toFixed(1)}</span>
+ </div>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/client/graphics/layers/PerformanceOverlay.ts around lines 707 to 737 the
new UI metric labels are hardcoded in English; wrap each user-facing label with
translateText(...) and add corresponding translation keys to the English locale
(en.json) (e.g. workerTicksPerSecond, renderTicksPerSecond, ticksPerRender,
backlogTurns, tileUpdatesPerRender, ringBuffer, ringDrainTime) ensuring string
values match the original text, then import/ensure translateText is available in
this module and replace each literal label with the translateText call using the
new keys.
| if (message.sharedTileRingHeader && message.sharedTileRingData) { | ||
| sharedTileRing = createSharedTileRingViews({ | ||
| header: message.sharedTileRingHeader, | ||
| data: message.sharedTileRingData, | ||
| dirty: message.sharedDirtyBuffer!, | ||
| }); | ||
| dirtyFlags = sharedTileRing.dirtyFlags; | ||
| } else { | ||
| sharedTileRing = null; | ||
| dirtyFlags = null; | ||
| } |
There was a problem hiding this comment.
Non-null assertion on sharedDirtyBuffer may throw.
Line 77 uses message.sharedDirtyBuffer! but only checks for sharedTileRingHeader and sharedTileRingData. If the caller passes header and data but not dirty, this will fail at runtime when creating the views.
Consider adding explicit validation:
- if (message.sharedTileRingHeader && message.sharedTileRingData) {
+ if (
+ message.sharedTileRingHeader &&
+ message.sharedTileRingData &&
+ message.sharedDirtyBuffer
+ ) {
sharedTileRing = createSharedTileRingViews({
header: message.sharedTileRingHeader,
data: message.sharedTileRingData,
- dirty: message.sharedDirtyBuffer!,
+ dirty: message.sharedDirtyBuffer,
});🤖 Prompt for AI Agents
In src/core/worker/Worker.worker.ts around lines 73 to 83, the code currently
uses a non-null assertion on message.sharedDirtyBuffer when only header and data
are checked, which can throw at runtime; change the conditional to require
message.sharedDirtyBuffer as well (or handle its absence explicitly) before
calling createSharedTileRingViews, and remove the non-null assertion — either
pass the buffer as-is when present, or if absent set sharedTileRing = null and
dirtyFlags = null (or throw/log a clear error) so we never dereference an
undefined buffer.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
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