Draft
Conversation
Contributor
WalkthroughAdds WebGPU-based territory rendering with worker-backed renderers, extensive WebGPU compute/render passes and shaders, worker metrics and batching, UI controls (WebGPU debug overlay, setting select), and refactors client and worker messaging to support renderer initialization, tile context queries, and batched turn processing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerClient
participant Worker
participant Renderer
Client->>WorkerClient: init renderer (offscreenCanvas, darkMode, backend)
WorkerClient->>Worker: postMessage init_renderer (transfer OffscreenCanvas)
Worker->>Renderer: init(offscreenCanvas, gameState, theme)
Renderer-->>Worker: renderer_ready
Worker-->>WorkerClient: renderer_ready
WorkerClient-->>Client: renderer ready
sequenceDiagram
participant Client
participant WorkerClient
participant Worker
participant Renderer
Client->>WorkerClient: send turns (batched: turn_batch)
WorkerClient->>Worker: postMessage turn_batch
Worker->>Worker: scheduleSimPump/process turns
Worker->>Renderer: mark tiles / update resources
Renderer-->>Worker: render_done (timings)
Worker-->>WorkerClient: render_done
WorkerClient-->>Client: WorkerMetricsEvent / RenderDone
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/EmojiTable.ts (1)
18-55:⚠️ Potential issue | 🟡 MinorFix emoji table visibility on failed owner lookup.
Right now the table becomes visible before owner validation and stays open if the worker call fails or has no owner. Hide the table on failure and only show after a valid player is found.Proposed fix
- eventBus.on(ShowEmojiMenuEvent, (e) => { - this.isVisible = true; + eventBus.on(ShowEmojiMenuEvent, async (e) => { const cell = this.transformHandler.screenToWorldCoordinates(e.x, e.y); if (!this.game.isValidCoord(cell.x, cell.y)) { + this.hideTable(); return; } const tile = this.game.ref(cell.x, cell.y); - this.game.worker.tileContext(tile).then((ctx) => { + try { + const ctx = await this.game.worker.tileContext(tile); if (!ctx.ownerId) { + this.hideTable(); return; } let targetPlayer: PlayerView | null = null; - try { - const maybe = this.game.player(ctx.ownerId); - targetPlayer = - maybe && maybe.isPlayer() ? (maybe as PlayerView) : null; - } catch { - targetPlayer = null; - } + try { + const maybe = this.game.player(ctx.ownerId); + targetPlayer = + maybe && maybe.isPlayer() ? (maybe as PlayerView) : null; + } catch { + targetPlayer = null; + } if (!targetPlayer) { + this.hideTable(); return; } this.showTable((emoji) => { const recipient = targetPlayer === this.game.myPlayer() ? AllPlayers : (targetPlayer as PlayerView); eventBus.emit( new SendEmojiIntentEvent( recipient, flattenedEmojiTable.indexOf(emoji as Emoji), ), ); this.hideTable(); }); - }); + } catch { + this.hideTable(); + } });
🤖 Fix all issues with AI agents
In `@src/client/components/baseComponents/setting/SettingSelect.ts`:
- Around line 8-13: The label is a DIV and the component mirrors the host id
onto the inner <select>, causing duplicate IDs and poor a11y; update the
SettingSelect component by adding a new property selectId (e.g., `@property`()
selectId = "") and stop using the host-level id for the select element, keep the
existing id property for the host only, then change the render method (around
the current render lines ~40–52) to render a proper <label
for={this.selectId}>{this.label}</label> and set the <select id={this.selectId}>
(remove id binding to the host id), ensuring the selectId is generated or set by
consumers when not provided so the label correctly associates with the select
without duplicating IDs.
- Around line 19-28: The native change event from the inner <select> is
currently allowed to bubble in handleChange, causing consumers to receive both
the native event and the component's custom change event; inside the
handleChange method (in SettingSelect), call e.stopPropagation() at the start
(before dispatching your CustomEvent) to prevent the native event from
propagating so only your composed CustomEvent("change", { detail: { value:
this.value }, bubbles: true, composed: true }) is seen by outer listeners.
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 851-1016: The new "Worker" UI labels in PerformanceOverlay are
hard-coded English; wrap all user-visible strings in this render block (e.g.,
"Worker", "metrics age", "metrics interval (worker)", "event loop lag (avg /
max)", "sim pump delay (avg / max)", "sim pump exec (avg / max)", "render_frame
queue (avg / max)", "render_frame handler (avg / max)", "render submits /
noops", "render CPU breakdown (avg/max, submitted frames)", the cpu sublabels
like "cpu total", "getCurrentTexture", "frame compute", "territory pass",
"temporal resolve", "submit", the "top msgs..." header, and the "trace" block
including the checkbox label "include") with translateText (or mark debug-only)
so they are localized; update the JSX/HTML template in PerformanceOverlay.ts
around the worker rendering block to call translateText(...) for each visible
string and add the import/use of translateText at the top if not present.
- Around line 251-255: The UI select can get out of sync because the handler
registered with this.eventBus.on(WorkerMetricsEvent, ...) updates workerMetrics,
lastWorkerMetricsWallMs, workerMetricsAgeMs and calls requestUpdate() but does
not update workerIntervalMs; modify that handler to also set
this.workerIntervalMs from the incoming metrics (e.g. assign the interval field
on event.metrics, such as event.metrics.intervalMs or equivalent) before calling
requestUpdate() so the dropdown reflects the real worker interval.
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 506-527: Replace the hard-coded UI strings in SettingsModal.ts
with translateText calls: change "WebGPU Debug", "Territory shader selection +
options", and the alt text "webgpuDebugIcon" to use new translation keys (e.g.,
"settings.webgpuDebug.title", "settings.webgpuDebug.description",
"settings.webgpuDebug.iconAlt") and keep the existing userSettings.webgpuDebug()
toggle label logic using translateText; update your language files to include
those new keys and translations. Ensure you call translateText(...) where the
title/description/alt are rendered and keep the existing
onToggleWebgpuDebugOverlayButtonClick and userSettings.webgpuDebug() usage
unchanged.
In `@src/client/graphics/webgpu/compute/DefendedStrengthPass.ts`:
- Around line 13-20: Add tracking for all six bind group resources so the bind
group rebuild logic triggers correctly: declare private fields
boundOwnerOffsetsBuffer, boundParamsBuffer, boundStateTexture, and
boundDefendedStrengthTexture alongside the existing boundDirtyTilesBuffer and
boundPostsByOwnerBuffer in the class; update shouldRebuildBindGroup to
compare/validate all six tracked fields (boundParamsBuffer,
boundDirtyTilesBuffer, boundStateTexture, boundDefendedStrengthTexture,
boundOwnerOffsetsBuffer, boundPostsByOwnerBuffer) against current resources;
after rebuildBindGroup completes, set each of the six bound* fields to the newly
used GPUBuffer/GPUTexture objects; and clear all six bound* fields in dispose().
In `@src/client/graphics/webgpu/core/GroundTruthData.ts`:
- Around line 142-149: The code uses (globalThis as
any).GPUBufferUsage/GPUTextureUsage and fallbacks (UNIFORM, COPY_DST_BUF,
COPY_DST_TEX, COPY_SRC_TEX, TEXTURE_BINDING, STORAGE_BINDING); update TypeScript
configuration so WebGPU types are available (either add "DOM"/WebGPU libs to
tsconfig.json "lib" or install `@webgpu/types`) and then remove the unsafe `as
any` casts, referencing GPUBufferUsage and GPUTextureUsage directly while
keeping the existing numeric fallback values for runtime safety (leave the
UNIFORM/COPY_*/TEXTURE_BINDING/STORAGE_BINDING fallback logic intact).
In `@src/client/graphics/webgpu/render/TemporalResolvePass.ts`:
- Around line 210-217: In TemporalResolvePass.dispose(), before nulling out
this.paramsBuffer call its destroy() method to free the underlying GPU resource
(and optionally guard with a null check like if (this.paramsBuffer)
this.paramsBuffer.destroy()), then set this.paramsBuffer = null; do the same for
any other GPU resources that require explicit cleanup (e.g., buffers or
textures) so they don't leak; update the dispose() method in the
TemporalResolvePass class to perform these destroy calls prior to null
assignments.
In `@src/client/graphics/webgpu/TerritoryRenderer.ts`:
- Around line 134-136: The code is currently fire-and-forgetting the shader load
for TerrainComputePass by calling void
this.terrainComputePass.setShader(this.terrainShaderPath), which can cause
first-frame rendering failures; change this to await the promise returned by
TerrainComputePass.setShader (or otherwise gate render until the pass reports
ready) so the shader at terrainShaderPath is fully loaded before render is
called—locate the TerrainComputePass instantiation and setShader call and
replace the void-prefixed invocation with an awaited call (or add a readiness
flag on terrainComputePass checked in render), consistent with how
StateUpdatePass.setShader is awaited elsewhere.
In `@src/client/graphics/webgpu/TerritoryRendererProxy.ts`:
- Around line 114-115: The current dark mode detection uses themeAny.darkShore
!== undefined which treats a light theme setting darkShore: false as dark;
change detection to use an explicit boolean (e.g., const darkMode =
themeAny.darkMode === true || themeAny.darkShore === true) and update the Theme
type to include darkMode?: boolean so callers can explicitly set darkMode;
alternatively, if you cannot change the type now, at minimum check for true (===
true) on darkShore when computing the darkMode variable in
TerritoryRendererProxy (look for themeAny and darkMode).
In `@src/core/game/GameView.ts`:
- Around line 886-888: The setDefended method in GameView (setDefended(ref:
TileRef, value: boolean): void) incorrectly uses "return" when calling
this._map.setDefended(ref, value); remove the "return" keyword so the method
simply calls this._map.setDefended(ref, value) and remains void; update the
setDefended implementation accordingly.
In `@src/core/game/UserSettings.ts`:
- Around line 68-79: The webgpuDebug() method currently returns true by default
which enables a noisy debug overlay on first load; change the default in
UserSettings.webgpuDebug() by calling this.get("settings.webgpuDebug", false) so
the default is off, leaving set/get semantics unchanged (refer to the
webgpuDebug() method in UserSettings).
In `@src/core/GameRunner.ts`:
- Around line 89-96: The comment on GameRunner.tileUpdateSink is ambiguous about
packed format; update it to document both modes: when usesSharedTileState (SAB)
is true, the sink receives plain BigInt(tileRef) values (since state lives in
the shared Uint16Array), and when not using SAB the sink receives packed updates
formatted as (tileRef << 16n) | state (decodeable by updateTile). Reference the
GameRunner.tileUpdateSink property and GameView.usesSharedTileState / updateTile
to clarify which decoder to use in each case so consumers decode Number(tu) in
SAB mode and shift/mask in non-SAB mode.
In `@src/core/worker/GameViewAdapter.ts`:
- Around line 536-547: The recentlyUpdatedTiles method is dead or incomplete: it
unpacks packedTileUpdates by shifting (packed[i] >> 16n) which only works in
non-SAB mode and doesn't handle the SAB (unpacked) case; either remove
recentlyUpdatedTiles from GameViewAdapter if it's unused, or add SAB detection
and handling consistent with GameView.updateTile()/updateTileFromWorker (i.e.,
check whether this.lastUpdate.packedTileUpdates contains bigints packed as
(tileRef<<16|state) or raw tile refs and branch accordingly), ensuring you
reference recentlyUpdatedTiles, this.lastUpdate.packedTileUpdates, and the
GameView.updateTile logic to keep behavior consistent.
In `@src/core/worker/Worker.worker.ts`:
- Line 42: Remove the unused constant MAX_TICKS_PER_HEARTBEAT from
Worker.worker.ts: locate the declaration "const MAX_TICKS_PER_HEARTBEAT = 4;"
and delete it (and any now-empty surrounding whitespace), ensuring no other code
references MAX_TICKS_PER_HEARTBEAT remains; run tests/linters to confirm no
use-site errors.
In `@src/core/worker/WorkerClient.ts`:
- Around line 355-376: tileContext currently returns a promise that can hang
forever if the worker never responds; update the tileContext method to add a
timeout (e.g., 20 seconds like initialize()) that rejects the promise if no
"tile_context_result" arrives, ensure you clear the timeout and remove the
message handler from this.messageHandlers when either the result is received or
the timeout fires, and reject with a clear Error (e.g., "Timed out waiting for
tile_context_result"); reference the tileContext function, the generated
messageId from generateID(), the entry in this.messageHandlers, and the
postMessage call so you add the timer and cleanup in those places.
In `@src/core/worker/WorkerTerritoryRenderer.ts`:
- Around line 136-143: The catch block on
terrainComputePass.setShader(this.terrainShaderPath) in WorkerTerritoryRenderer
silently swallows errors; update the promise handling for
TerrainComputePass#setShader so failures are surfaced—e.g., log the error via
the renderer/process logger and/or set a boolean flag (like
this.terrainShaderLoaded = false) on failure, and only call
computeTerrainImmediate() when the shader successfully loads (or bail out/skip
rendering when the flag indicates failure) so the code does not continue with
undefined shader state.
- Around line 583-601: dispose() currently only nulls references and can leak
GPU memory; update WorkerTerritoryRenderer.dispose() to explicitly destroy GPU
resources before nulling: call this.device.destroy() (if supported) after first
destroying all GPU objects; iterate this.computePasses, this.frameComputePasses,
this.renderPasses and call destroy() on any contained
GPUBuffer/GPUTexture/GPUTextureView/GPUExternalTexture/sampler/GPURenderPipeline/GPUComputePipeline
as appropriate, also destroy single-pass resources referenced by
terrainComputePass, stateUpdatePass, defendedStrengthFullPass,
defendedStrengthPass, visualStateSmoothingPass, territoryRenderPass,
temporalResolvePass and any entries in this.resources; catch and log errors
during destroy, then clear arrays and set fields (this.gameViewAdapter,
this.device, this.canvas, etc.) to null.
🧹 Nitpick comments (31)
src/client/graphics/layers/PlayerPanel.ts (1)
132-134: Empty catch swallows errors silently—consider logging for debugging.The catch block discards all errors without any logging. While keeping the panel visible on transient failures is reasonable, completely silent failures can mask real bugs (e.g., worker communication issues).
Consider logging at debug level so developers can trace problems:
Proposed change
- } catch { - // If tile context fails (rare), keep the panel as-is. + } catch (err) { + // If tile context fails (rare), keep the panel as-is. + console.debug("PlayerPanel: tile context fetch failed", err); }src/client/graphics/HoverInfo.ts (1)
61-69: Optional: avoid sort + sqrt on every hover.This runs on mouse move; a single pass with squared distance avoids sorting work and extra sqrt calls.
src/client/graphics/webgpu/compute/VisualStateSmoothingPass.ts (1)
195-202: Consider destroying GPU buffers on dispose.This helps release GPU memory promptly when the pass is rebuilt or torn down.
Please confirm `GPUBuffer.destroy()` is supported for your target browsers/devices.♻️ Suggested change
dispose(): void { + this.paramsBuffer?.destroy(); this.pipeline = null; this.bindGroupLayout = null; this.bindGroup = null; this.device = null; this.resources = null; this.paramsBuffer = null; }src/client/graphics/layers/WebGPUDebugOverlay.ts (1)
184-193: Ensure the EventBus listener is cleaned up.If the overlay is re-mounted or
init()runs more than once, this can leak handlers or double-update. Please store an unsubscribe handle and clean it indisconnectedCallback()(or an equivalent teardown path).Please confirm the EventBus API supports unsubscribe or return-disposer patterns.
src/client/graphics/webgpu/core/Pipeline.ts (1)
1-7: Consider exporting types if used externally.
SlowPipelineKindandSlowPipelineEventare not exported. If external code needs to type the__webgpuSlowPipelineHookcallback, these types would be useful exports.Export types for external hook consumers
-type AnyFn = (...args: any[]) => any; -type SlowPipelineKind = "compute" | "render"; -type SlowPipelineEvent = { +type AnyFn = (...args: any[]) => any; +export type SlowPipelineKind = "compute" | "render"; +export type SlowPipelineEvent = { kind: SlowPipelineKind; ms: number; label?: string; };src/client/graphics/webgpu/shaders/compute/terrain-compute-improved-lite.wgsl (1)
60-68: Redundant negative checks on unsigned integers.
globalIdisvec3<u32>, soxandyare always non-negative after conversion toi32from a positiveu32. The checksx < 0andy < 0are always false.Remove dead code for cleaner shader
`@compute` `@workgroup_size`(8, 8) fn main(`@builtin`(global_invocation_id) globalId: vec3<u32>) { - let x = i32(globalId.x); - let y = i32(globalId.y); + let x = globalId.x; + let y = globalId.y; let dims = textureDimensions(terrainDataTex); - if (x < 0 || y < 0 || u32(x) >= dims.x || u32(y) >= dims.y) { + if (x >= dims.x || y >= dims.y) { return; } + + let texCoord = vec2i(i32(x), i32(y));src/client/graphics/webgpu/render/TerritoryPreSmoothingRegistry.ts (1)
100-113: Duplicated shader path string.Line 110 hardcodes
"compute/visual-state-smoothing.wgsl", but this is already defined in theTERRITORY_PRE_SMOOTHINGarray entries. Consider looking up the definition to avoid drift.Use definition's wgslPath instead of hardcoding
export function buildTerritoryPreSmoothingParams( userSettings: { getFloat: (key: string, defaultValue: number) => number; }, smoothingId: TerritoryPreSmoothingId, ): { enabled: boolean; shaderPath: string; params0: Float32Array; params1: Float32Array; } { if (smoothingId === "off") { return { enabled: false, shaderPath: "", params0: new Float32Array(4), params1: new Float32Array(4), }; } + const definition = TERRITORY_PRE_SMOOTHING.find((d) => d.id === smoothingId); const curveExp = userSettings.getFloat( "settings.webgpu.territory.preSmoothing.curveExp", 1, ); const mode = smoothingId === "dissolve" ? 1 : 2; const params0 = new Float32Array([mode, curveExp, 0, 0]); const params1 = new Float32Array([0, 0, 0, 0]); return { enabled: true, - shaderPath: "compute/visual-state-smoothing.wgsl", + shaderPath: definition?.wgslPath ?? "compute/visual-state-smoothing.wgsl", params0, params1, }; }src/client/graphics/webgpu/TerritoryRendererProxy.ts (1)
22-25: Interface name is misleading.
TerritoryWebGLCreateResultsuggests WebGL, but this proxy is for WebGPU. Consider renaming toTerritoryWebGPUCreateResultfor clarity.Rename interface to match technology
-export interface TerritoryWebGLCreateResult { +export interface TerritoryWebGPUCreateResult { renderer: TerritoryRendererProxy | null; reason?: string; }src/client/graphics/webgpu/TerritoryRenderer.ts (2)
17-20: Same interface naming issue as proxy.
TerritoryWebGLCreateResultshould beTerritoryWebGPUCreateResultsince this is WebGPU, not WebGL.
475-480: Unsafe cast to call rebuildBindGroup.Casting to
anyto callrebuildBindGroupbreaks type safety. If this method exists and is needed, add it to theRenderPassinterface orTerritoryRenderPasstype.Add method to interface instead of casting
Either add
rebuildBindGroup?(): voidto theRenderPassinterface, or check the type explicitly:- if (this.territoryRenderPass) { - (this.territoryRenderPass as any).rebuildBindGroup?.(); - } + if (this.territoryRenderPass?.rebuildBindGroup) { + this.territoryRenderPass.rebuildBindGroup(); + }This requires
TerritoryRenderPassto haverebuildBindGroupas a public method.src/core/game/GameMap.ts (1)
30-33: Clarify mutation rules for the exposed state/terrain views.
tileStateView()returns the mutable backing array; direct writes can bypasssetFallout()/updateTile()and desync_numTilesWithFallout. If callers are read‑only, add a clear guard comment; if writes are expected, consider a safer mutation API.💡 Suggested doc guard
setDefended(ref: TileRef, value: boolean): void; - tileStateView(): Uint16Array; - terrainDataView(): Uint8Array; + /** + * Direct view of mutable state. Avoid writing to it unless you also keep + * fallout counters in sync (prefer updateTile/setFallout). + */ + tileStateView(): Uint16Array; + /** Direct view of immutable terrain data. Treat as read-only. */ + terrainDataView(): Uint8Array;Also applies to: 231-237
src/client/graphics/webgpu/render/TerritoryRenderPass.ts (1)
122-199: Avoid rebuilding the bind group every frame.
rebuildBindGroup()is called on eachexecute, which can create new GPU objects per frame. Cache the last bound textures and rebuild only when they change. Please verify with profiling that bind group creation is not happening per frame after caching.Example cache to avoid per-frame bind group rebuild
export class TerritoryRenderPass implements RenderPass { name = "territory"; dependencies: string[] = []; @@ private clearR = 0; private clearG = 0; private clearB = 0; + private boundStateTexture: GPUTexture | null = null; + private boundDefendedStrengthTexture: GPUTexture | null = null; + private boundPaletteTexture: GPUTexture | null = null; + private boundTerrainTexture: GPUTexture | null = null; + private boundOwnerIndexTexture: GPUTexture | null = null; + private boundRelationsTexture: GPUTexture | null = null; @@ rebuildBindGroup(): void { @@ const stateTexture = this.resources.getRenderStateTexture(); + if ( + this.bindGroup && + this.boundStateTexture === stateTexture && + this.boundDefendedStrengthTexture === this.resources.defendedStrengthTexture && + this.boundPaletteTexture === this.resources.paletteTexture && + this.boundTerrainTexture === this.resources.terrainTexture && + this.boundOwnerIndexTexture === this.resources.ownerIndexTexture && + this.boundRelationsTexture === this.resources.relationsTexture + ) { + return; + } this.bindGroup = this.device.createBindGroup({ layout: this.bindGroupLayout, entries: [ { binding: 0, resource: { buffer: this.resources.uniformBuffer } }, { binding: 1, resource: stateTexture.createView(), }, { binding: 2, resource: this.resources.defendedStrengthTexture.createView(), }, { binding: 3, resource: this.resources.paletteTexture.createView(), }, { binding: 4, resource: this.resources.terrainTexture.createView(), }, { binding: 5, resource: this.resources.ownerIndexTexture.createView(), }, { binding: 6, resource: this.resources.relationsTexture.createView(), }, ], }); + this.boundStateTexture = stateTexture; + this.boundDefendedStrengthTexture = this.resources.defendedStrengthTexture; + this.boundPaletteTexture = this.resources.paletteTexture; + this.boundTerrainTexture = this.resources.terrainTexture; + this.boundOwnerIndexTexture = this.resources.ownerIndexTexture; + this.boundRelationsTexture = this.resources.relationsTexture; }src/client/graphics/webgpu/compute/StateUpdatePass.ts (1)
28-61: Consider using named constants for visibility flags.The magic number
4forGPUShaderStage.COMPUTEis correct but less readable. Consider extracting it:♻️ Optional: Use named constant for clarity
+const COMPUTE_STAGE = GPUShaderStage.COMPUTE; // 4 + this.bindGroupLayout = device.createBindGroupLayout({ entries: [ { binding: 0, - visibility: 4 /* COMPUTE */, + visibility: COMPUTE_STAGE, buffer: { type: "uniform" }, }, // ... and so onsrc/core/worker/Worker.worker.ts (1)
55-406: Consider extracting WorkerProfiler to its own file.The
WorkerProfilerclass is ~350 lines and has a clear, self-contained responsibility. Moving it toWorkerProfiler.tswould improve readability and testability of this file.src/client/graphics/webgpu/render/TemporalResolvePass.ts (2)
33-39: Consider using WebGPU constants directly or documenting the fallback pattern.The
globalThiscast pattern with fallback hex values works but is non-obvious. If this is needed for worker context compatibility, a brief comment explaining why would help future readers.+ // Access GPUBufferUsage via globalThis for worker compatibility const GPUBufferUsage = (globalThis as any).GPUBufferUsage;
150-150: Texture view created per-frame inside render pass.
historyWrite.createView()is called every frame insideexecute(). Consider caching this view and rebuilding only when textures change, similar to howrebuildBindGroup()tracksboundHistoryTexture.src/client/graphics/webgpu/render/TerritoryPostSmoothingRegistry.ts (1)
119-127:params1is always zeroed and unused.The function always returns
params1as[0,0,0,0]. If this is a placeholder for future use, consider documenting it. Otherwise, remove it from the return type to avoid confusion.src/client/graphics/webgpu/render/TerrainShaderRegistry.ts (1)
3-25: Duplicate type definition:TerrainShaderOptionmatchesTerritoryShaderOption.This type is identical to
TerritoryShaderOptioninTerritoryShaderRegistry.ts. Consider extracting a sharedShaderOptiontype to a common module to avoid duplication and keep them in sync.src/client/graphics/webgpu/render/TerritoryShaderRegistry.ts (1)
3-25: SameShaderOptiontype appears in multiple registries.As noted in
TerrainShaderRegistry.ts, this type is duplicated. A shared type would reduce maintenance burden.src/client/graphics/webgpu/shaders/compute/terrain-compute-improved-heavy.wgsl (1)
71-79: Redundant negative check on unsigned coordinates.
globalIdisvec3<u32>, soxandy(cast toi32) will always be non-negative. The checksx < 0 || y < 0are always false.Proposed simplification
`@compute` `@workgroup_size`(8, 8) fn main(`@builtin`(global_invocation_id) globalId: vec3<u32>) { - let x = i32(globalId.x); - let y = i32(globalId.y); + let x = globalId.x; + let y = globalId.y; let dims = textureDimensions(terrainDataTex); - if (x < 0 || y < 0 || u32(x) >= dims.x || u32(y) >= dims.y) { + if (x >= dims.x || y >= dims.y) { return; } + + let texCoord = vec2i(i32(x), i32(y));src/client/graphics/canvas2d/Canvas2DRendererProxy.ts (1)
360-426: Consider extracting timing metrics parsing to a helper function.The
render_donehandler has extensive optional field extraction (lines 365-414). Extracting this to a separate function likeparseRenderMetrics(m: any)would improve readability and testability.src/core/worker/GameViewAdapter.ts (1)
206-208: Remove empty statementvoid 0;in constructor.This appears to be leftover debug code or placeholder. It does nothing and adds noise.
Proposed fix
constructor( private tileState: Uint16Array, private terrainData: Uint8Array, private readonly mapWidth: number, private readonly mapHeight: number, private theme: Theme, private readonly myClientId: ClientID | null, private readonly cosmeticsByClientID: Map<ClientID, PlayerCosmetics>, - ) { - void 0; - } + ) {}src/core/worker/WorkerTerritoryRenderer.ts (2)
116-122: Type cast toanyhides potential interface mismatch.The
as anycast on line 118 bypasses type checking. IfGameViewAdapterdoes not fully implement whatGroundTruthData.create()expects, this will fail at runtime without compile-time warning.Consider having
GameViewAdapterimplement the required interface explicitly, or define a minimal interface that both satisfy.
707-743: Tick loop disables GPU wait permanently on first timeout.When
awaitGpuWorktimes out (line 718-719 or 731-732),gpuWaitEnabledis set tofalseand never re-enabled. A single slow frame permanently disables synchronization for the lifetime of the renderer.Consider a recovery mechanism or exponential backoff:
Proposed approach
+ private gpuWaitDisabledUntil = 0; + private readonly gpuWaitCooldownMs = 5000; // In runTickLoop: - if (r.timedOut) { - this.gpuWaitEnabled = false; - } + if (r.timedOut) { + this.gpuWaitDisabledUntil = performance.now() + this.gpuWaitCooldownMs; + } // Then check: - if (this.gpuWaitEnabled && this.lastGpuWork) { + const waitEnabled = performance.now() > this.gpuWaitDisabledUntil; + if (waitEnabled && this.lastGpuWork) {src/core/worker/WorkerCanvas2DRenderer.ts (2)
461-536: Terrain color logic duplicates PastelTheme implementation.The hardcoded color formulas for shore, water, plains, highland, and mountain match the
PastelThemeimplementation. If the theme colors change, this code must be updated manually.This is acceptable for a performance-critical path, but consider extracting shared constants or documenting the dependency on
PastelThemecolor formulas.
456-459: Add a property to theThemeinterface to avoid instanceof checks on concrete classes.Using
instanceof PastelThemeorPastelThemeDarkties this code to specific implementation classes. If a new Pastel-style theme is added that doesn't extendPastelTheme, it won't benefit from the optimization here.Consider adding a method like
isPastelStyle(): booleanto theThemeinterface instead of checking class types. This aligns with TypeScript best practices and makes the code work with any Pastel-style theme, not just classes in thePastelThemehierarchy.src/core/worker/WorkerMessages.ts (2)
272-314:RenderDoneMessagehas many optional fields that could be grouped.The message has 16+ optional timing fields. Consider grouping related fields into a nested object for clarity:
Example grouping
export interface RenderDoneMessage extends BaseWorkerMessage { type: "render_done"; timing?: { startedAt?: number; endedAt?: number; startedAtWallMs?: number; endedAtWallMs?: number; }; renderMetrics?: { waitPrevGpuMs?: number; cpuMs?: number; getTextureMs?: number; // ... }; }This is a style preference and not blocking.
334-369:WorkerMetricsMessagemixes primitive fields with record fields.The message combines many numeric fields with
Record<string, number>fields for message counts and timings. This is functional but makes the type harder to work with.Consider using a more structured approach or documenting the expected keys for
msgCounts,msgHandlerMsAvg, etc.src/client/graphics/webgpu/core/GroundTruthData.ts (3)
804-817: Destroying and recreating texture usesanycast.Lines 807 and 811 use
(this.paletteTexture as any).destroy?.()and(this as any).paletteTexture = .... This circumvents thereadonlymodifier.If the texture needs to be recreated, consider:
- Not marking it as
readonly, or- Using a wrapper pattern that allows internal mutation
629-725: Terrain params upload extracts colors directly from theme object.The code accesses theme internals via
themeAny.darkShore,themeAny.water, etc. (lines 636-671). This assumes the theme has these specific properties, which may not be true for all theme implementations.Consider adding these color accessors to the
Themeinterface or documenting this coupling.
16-251: Large class could benefit from decomposition.
GroundTruthDataat ~1400 lines manages textures, buffers, uploads, defense posts, relations, and palette data. Consider extracting cohesive subsets:
TextureManagerfor texture creation/destructionDefensePostManagerfor defense post logicRelationsManagerfor player relationsPaletteManagerfor palette generationThis is optional given the current scope but would improve maintainability.
Comment on lines
+8
to
+13
| @property() label = "Setting"; | ||
| @property() description = ""; | ||
| @property() id = ""; | ||
| @property() value = ""; | ||
| @property({ attribute: false }) options: SettingSelectOption[] = []; | ||
| @property({ type: Boolean }) easter = false; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/client/components/baseComponents/setting/SettingSelect.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 2517
🏁 Script executed:
rg "setting-select" --type html --type ts -A 2 -B 2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 559
🏁 Script executed:
rg "<setting-select" -A 1 -B 1Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
rg "SettingSelect" --type ts -lRepository: openfrontio/OpenFrontIO
Length of output: 159
🏁 Script executed:
cat -n src/client/UserSettingModal.ts | head -100Repository: openfrontio/OpenFrontIO
Length of output: 3791
🏁 Script executed:
cat -n src/client/UserSettingModal.ts | grep -A 10 -B 2 "setting-select"Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
cat -n src/client/UserSettingModal.ts | tail -100Repository: openfrontio/OpenFrontIO
Length of output: 4296
🏁 Script executed:
rg "setting-select" src/client/UserSettingModal.ts -A 5 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
wc -l src/client/UserSettingModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 101
🏁 Script executed:
cat -n src/client/UserSettingModal.ts | grep -i selectRepository: openfrontio/OpenFrontIO
Length of output: 448
🏁 Script executed:
find . -name "*.ts" -o -name "*.html" | xargs rg "setting-select" 2>/dev/nullRepository: openfrontio/OpenFrontIO
Length of output: 163
Use <label> with for attribute and separate the select ID from the host element ID.
The label is currently a <div>, so screen readers won't associate it with the <select>. Additionally, if the host element receives an id attribute, it duplicates on the inner <select> because @property() id mirrors to the select element, creating two elements with the same ID. Use a dedicated selectId property and properly labeled <label> element instead.
Suggested structure
-export class SettingSelect extends LitElement {
+export class SettingSelect extends LitElement {
`@property`() label = "Setting";
`@property`() description = "";
- `@property`() id = "";
+ `@property`({ attribute: "select-id" }) selectId = "";
`@property`() value = "";
`@property`({ attribute: false }) options: SettingSelectOption[] = [];
`@property`({ type: Boolean }) easter = false;
@@
- <div class="text-white font-bold text-base block mb-1">
- ${this.label}
- </div>
+ <label class="text-white font-bold text-base block mb-1" for=${this.selectId}>
+ ${this.label}
+ </label>
@@
- <select
- id=${this.id}
+ <select
+ id=${this.selectId}
class="shrink-0 bg-black/60 border border-white/10 text-white/90 rounded-lg px-3 py-2 focus:outline-none focus:ring-2 focus:ring-blue-600"This also applies to lines 40–52 in the render method.
🤖 Prompt for AI Agents
In `@src/client/components/baseComponents/setting/SettingSelect.ts` around lines 8
- 13, The label is a DIV and the component mirrors the host id onto the inner
<select>, causing duplicate IDs and poor a11y; update the SettingSelect
component by adding a new property selectId (e.g., `@property`() selectId = "")
and stop using the host-level id for the select element, keep the existing id
property for the host only, then change the render method (around the current
render lines ~40–52) to render a proper <label
for={this.selectId}>{this.label}</label> and set the <select id={this.selectId}>
(remove id binding to the host id), ensuring the selectId is generated or set by
consumers when not provided so the label correctly associates with the select
without duplicating IDs.
Comment on lines
+19
to
+28
| private handleChange(e: Event) { | ||
| const select = e.target as HTMLSelectElement; | ||
| this.value = select.value; | ||
| this.dispatchEvent( | ||
| new CustomEvent("change", { | ||
| detail: { value: this.value }, | ||
| bubbles: true, | ||
| composed: true, | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/client/components/baseComponents/setting/SettingSelect.tsRepository: openfrontio/OpenFrontIO
Length of output: 2537
🌐 Web query:
LitElement light DOM createRenderRoot native event bubbling double dispatch custom element
💡 Result:
-
Light DOM in LitElement / Lit: override
createRenderRoot()and returnthisto render your template into the element’s own children (no shadow root). This is the “light DOM” pattern. It’s supported but generally not recommended because you lose DOM/style scoping and other Shadow DOM composition benefits. [1]import {LitElement, html} from 'lit'; class MyEl extends LitElement { createRenderRoot() { return this; } // light DOM render() { return html`<button `@click`=${this._onClick}>Click</button>`; } } customElements.define('my-el', MyEl);
-
Native event bubbling & “double dispatch”:
- With light DOM, events behave like normal DOM: bubbling goes up through your rendered children → your custom element → ancestors.
- With shadow DOM (Lit’s default), bubbling events coming from inside the shadow root are retargeted so outside listeners see
event.targetas the host element, not the internal node—this can look like “double dispatch” if you log from both inside and outside, but it’s the same event observed at different points with retargeting. Lit explicitly calls out this retargeting behavior and recommends usingevent.composedPath()to find the original source node. [2]
-
Custom events crossing the shadow boundary:
- By default, a bubbling CustomEvent created inside shadow DOM stops at the shadow root.
- To let it escape to the outside DOM, set both
bubbles: trueandcomposed: true. [2] composedis specifically the flag that allows an event to cross the Shadow DOM boundary (and it only propagates outward if it also bubbles). [3]
Sources: Lit “Working with Shadow DOM” (render root / createRenderRoot) [1], Lit “Events” (retargeting, composedPath(), bubbles+composed for custom events) [2], MDN Event.composed (crossing Shadow DOM boundary) [3].
Stop the native change event from propagating.
This component uses light DOM (see createRenderRoot()). When the native <select> element fires its change event, it will bubble up through the custom element to listeners above. But you also dispatch your own change event. This means listeners receive two separate events with the same name.
Use e.stopPropagation() to prevent the native event from bubbling, leaving only your custom event:
Fix: prevent native event from bubbling
private handleChange(e: Event) {
+ e.stopPropagation();
const select = e.target as HTMLSelectElement;
this.value = select.value;
this.dispatchEvent(
new CustomEvent("change", {
detail: { value: this.value },
bubbles: true,
composed: true,
}),
);📝 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.
Suggested change
| private handleChange(e: Event) { | |
| const select = e.target as HTMLSelectElement; | |
| this.value = select.value; | |
| this.dispatchEvent( | |
| new CustomEvent("change", { | |
| detail: { value: this.value }, | |
| bubbles: true, | |
| composed: true, | |
| }), | |
| ); | |
| private handleChange(e: Event) { | |
| e.stopPropagation(); | |
| const select = e.target as HTMLSelectElement; | |
| this.value = select.value; | |
| this.dispatchEvent( | |
| new CustomEvent("change", { | |
| detail: { value: this.value }, | |
| bubbles: true, | |
| composed: true, | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In `@src/client/components/baseComponents/setting/SettingSelect.ts` around lines
19 - 28, The native change event from the inner <select> is currently allowed to
bubble in handleChange, causing consumers to receive both the native event and
the component's custom change event; inside the handleChange method (in
SettingSelect), call e.stopPropagation() at the start (before dispatching your
CustomEvent) to prevent the native event from propagating so only your composed
CustomEvent("change", { detail: { value: this.value }, bubbles: true, composed:
true }) is seen by outer listeners.
Comment on lines
+251
to
+255
| this.eventBus.on(WorkerMetricsEvent, (event: WorkerMetricsEvent) => { | ||
| this.workerMetrics = event.metrics; | ||
| this.lastWorkerMetricsWallMs = Date.now(); | ||
| this.workerMetricsAgeMs = 0; | ||
| this.requestUpdate(); |
Contributor
There was a problem hiding this comment.
Sync the interval dropdown with incoming worker metrics.
Right now the select can show a value that does not match the worker’s actual interval. Update workerIntervalMs when metrics arrive so the UI stays in sync.
Suggested fix
this.eventBus.on(WorkerMetricsEvent, (event: WorkerMetricsEvent) => {
this.workerMetrics = event.metrics;
+ this.workerIntervalMs = event.metrics.intervalMs;
this.lastWorkerMetricsWallMs = Date.now();
this.workerMetricsAgeMs = 0;
this.requestUpdate();
});📝 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.
Suggested change
| this.eventBus.on(WorkerMetricsEvent, (event: WorkerMetricsEvent) => { | |
| this.workerMetrics = event.metrics; | |
| this.lastWorkerMetricsWallMs = Date.now(); | |
| this.workerMetricsAgeMs = 0; | |
| this.requestUpdate(); | |
| this.eventBus.on(WorkerMetricsEvent, (event: WorkerMetricsEvent) => { | |
| this.workerMetrics = event.metrics; | |
| this.workerIntervalMs = event.metrics.intervalMs; | |
| this.lastWorkerMetricsWallMs = Date.now(); | |
| this.workerMetricsAgeMs = 0; | |
| this.requestUpdate(); |
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 251 - 255, The
UI select can get out of sync because the handler registered with
this.eventBus.on(WorkerMetricsEvent, ...) updates workerMetrics,
lastWorkerMetricsWallMs, workerMetricsAgeMs and calls requestUpdate() but does
not update workerIntervalMs; modify that handler to also set
this.workerIntervalMs from the incoming metrics (e.g. assign the interval field
on event.metrics, such as event.metrics.intervalMs or equivalent) before calling
requestUpdate() so the dropdown reflects the real worker interval.
Comment on lines
+851
to
+1016
| <div class="layers-section"> | ||
| <div class="performance-line">Worker</div> | ||
| <div class="layer-row" style="margin-top: 4px;"> | ||
| <span class="layer-name">metrics age</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(this.workerMetricsAgeMs, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">metrics interval (worker)</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.intervalMs, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">event loop lag (avg / max)</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.loopLagAvg)} / | ||
| ${this.formatMs(worker.loopLagMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">sim pump delay (avg / max)</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.simDelayAvg)} / | ||
| ${this.formatMs(worker.simDelayMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">sim pump exec (avg / max)</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.simExecAvg)} / | ||
| ${this.formatMs(worker.simExecMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">render_frame queue (avg / max)</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.rfQueueAvg, 0)} / | ||
| ${this.formatMs(worker.rfQueueMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">render_frame handler (avg / max)</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.rfHandlerAvg, 0)} / | ||
| ${this.formatMs(worker.rfHandlerMax, 0)}</span | ||
| > | ||
| </div> | ||
| ${worker.renderSubmittedCount !== null || | ||
| worker.renderNoopCount !== null | ||
| ? html`<div class="layer-row"> | ||
| <span class="layer-name">render submits / noops</span> | ||
| <span class="layer-metrics" | ||
| >${worker.renderSubmittedCount ?? 0} / | ||
| ${worker.renderNoopCount ?? 0}</span | ||
| > | ||
| </div>` | ||
| : html``} | ||
| ${worker.renderCpuTotalAvg !== null | ||
| ? html`<div class="performance-line" style="margin-top: 6px;"> | ||
| render CPU breakdown (avg/max, submitted frames) | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">cpu total</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.renderCpuTotalAvg)} / | ||
| ${this.formatMs(worker.renderCpuTotalMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">getCurrentTexture</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.renderGetTextureAvg)} / | ||
| ${this.formatMs(worker.renderGetTextureMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">frame compute</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.renderFrameComputeAvg)} / | ||
| ${this.formatMs(worker.renderFrameComputeMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">territory pass</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.renderTerritoryPassAvg)} / | ||
| ${this.formatMs(worker.renderTerritoryPassMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">temporal resolve</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.renderTemporalResolveAvg)} / | ||
| ${this.formatMs(worker.renderTemporalResolveMax, 0)}</span | ||
| > | ||
| </div> | ||
| <div class="layer-row"> | ||
| <span class="layer-name">submit</span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(worker.renderSubmitAvg)} / | ||
| ${this.formatMs(worker.renderSubmitMax, 0)}</span | ||
| > | ||
| </div>` | ||
| : html``} | ||
| ${worker.topMsgs.length | ||
| ? html`<div class="performance-line" style="margin-top: 6px;"> | ||
| top msgs (count | queue avg/max | handler avg/max) | ||
| </div> | ||
| ${worker.topMsgs.map( | ||
| (m) => | ||
| html`<div class="layer-row"> | ||
| <span class="layer-name" title=${m.type} | ||
| >${m.type} (${m.count}) | ||
| </span> | ||
| <span class="layer-metrics" | ||
| >${this.formatMs(m.queueAvg, 0)}/${this.formatMs( | ||
| m.queueMax, | ||
| 0, | ||
| )} | ||
| | | ||
| ${this.formatMs(m.handlerAvg, 0)}/${this.formatMs( | ||
| m.handlerMax, | ||
| 0, | ||
| )} | ||
| </span> | ||
| </div>`, | ||
| )}` | ||
| : html``} | ||
| <div class="layer-row" style="margin-top: 4px;"> | ||
| <span class="layer-name">trace</span> | ||
| <span class="layer-metrics"> | ||
| <label style="cursor: pointer;"> | ||
| <input | ||
| type="checkbox" | ||
| .checked=${this.workerIncludeTrace} | ||
| @change=${this.onWorkerTraceToggle} | ||
| /> | ||
| include | ||
| </label> | ||
| <select | ||
| style="margin-left: 8px;" | ||
| .value=${String(this.workerIntervalMs)} | ||
| @change=${this.onWorkerIntervalChange} | ||
| > | ||
| <option value="250">250ms</option> | ||
| <option value="500">500ms</option> | ||
| <option value="1000">1000ms</option> | ||
| <option value="2000">2000ms</option> | ||
| </select> | ||
| </span> | ||
| </div> | ||
| ${worker.traceLines.length | ||
| ? html`<div | ||
| class="performance-line" | ||
| style="margin-top: 4px; opacity: 0.85;" | ||
| > | ||
| <div | ||
| style="white-space: pre-wrap; font-size: 10px; line-height: 1.2;" | ||
| > | ||
| ${worker.traceLines.join("\n")} | ||
| </div> | ||
| </div>` | ||
| : html``} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Localize the new Worker labels.
These strings are hard-coded English; wrap them with translateText (or mark as debug-only) to keep i18n consistent.
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 851 - 1016,
The new "Worker" UI labels in PerformanceOverlay are hard-coded English; wrap
all user-visible strings in this render block (e.g., "Worker", "metrics age",
"metrics interval (worker)", "event loop lag (avg / max)", "sim pump delay (avg
/ max)", "sim pump exec (avg / max)", "render_frame queue (avg / max)",
"render_frame handler (avg / max)", "render submits / noops", "render CPU
breakdown (avg/max, submitted frames)", the cpu sublabels like "cpu total",
"getCurrentTexture", "frame compute", "territory pass", "temporal resolve",
"submit", the "top msgs..." header, and the "trace" block including the checkbox
label "include") with translateText (or mark debug-only) so they are localized;
update the JSX/HTML template in PerformanceOverlay.ts around the worker
rendering block to call translateText(...) for each visible string and add the
import/use of translateText at the top if not present.
Comment on lines
+506
to
+527
| <button | ||
| class="flex gap-3 items-center w-full text-left p-3 hover:bg-slate-700 rounded-sm text-white transition-colors" | ||
| @click="${this.onToggleWebgpuDebugOverlayButtonClick}" | ||
| > | ||
| <img | ||
| src=${settingsIcon} | ||
| alt="webgpuDebugIcon" | ||
| width="20" | ||
| height="20" | ||
| /> | ||
| <div class="flex-1"> | ||
| <div class="font-medium">WebGPU Debug</div> | ||
| <div class="text-sm text-slate-400"> | ||
| Territory shader selection + options | ||
| </div> | ||
| </div> | ||
| <div class="text-sm text-slate-400"> | ||
| ${this.userSettings.webgpuDebug() | ||
| ? translateText("user_setting.on") | ||
| : translateText("user_setting.off")} | ||
| </div> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Localize the new WebGPU Debug strings.
Other settings use translateText, so these should too for consistent i18n.
✅ Suggested change
- <div class="font-medium">WebGPU Debug</div>
- <div class="text-sm text-slate-400">
- Territory shader selection + options
- </div>
+ <div class="font-medium">
+ ${translateText("user_setting.webgpu_debug_label")}
+ </div>
+ <div class="text-sm text-slate-400">
+ ${translateText("user_setting.webgpu_debug_desc")}
+ </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.
Suggested change
| <button | |
| class="flex gap-3 items-center w-full text-left p-3 hover:bg-slate-700 rounded-sm text-white transition-colors" | |
| @click="${this.onToggleWebgpuDebugOverlayButtonClick}" | |
| > | |
| <img | |
| src=${settingsIcon} | |
| alt="webgpuDebugIcon" | |
| width="20" | |
| height="20" | |
| /> | |
| <div class="flex-1"> | |
| <div class="font-medium">WebGPU Debug</div> | |
| <div class="text-sm text-slate-400"> | |
| Territory shader selection + options | |
| </div> | |
| </div> | |
| <div class="text-sm text-slate-400"> | |
| ${this.userSettings.webgpuDebug() | |
| ? translateText("user_setting.on") | |
| : translateText("user_setting.off")} | |
| </div> | |
| </button> | |
| <button | |
| class="flex gap-3 items-center w-full text-left p-3 hover:bg-slate-700 rounded-sm text-white transition-colors" | |
| `@click`="${this.onToggleWebgpuDebugOverlayButtonClick}" | |
| > | |
| <img | |
| src=${settingsIcon} | |
| alt="webgpuDebugIcon" | |
| width="20" | |
| height="20" | |
| /> | |
| <div class="flex-1"> | |
| <div class="font-medium"> | |
| ${translateText("user_setting.webgpu_debug_label")} | |
| </div> | |
| <div class="text-sm text-slate-400"> | |
| ${translateText("user_setting.webgpu_debug_desc")} | |
| </div> | |
| </div> | |
| <div class="text-sm text-slate-400"> | |
| ${this.userSettings.webgpuDebug() | |
| ? translateText("user_setting.on") | |
| : translateText("user_setting.off")} | |
| </div> | |
| </button> |
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/SettingsModal.ts` around lines 506 - 527, Replace
the hard-coded UI strings in SettingsModal.ts with translateText calls: change
"WebGPU Debug", "Territory shader selection + options", and the alt text
"webgpuDebugIcon" to use new translation keys (e.g.,
"settings.webgpuDebug.title", "settings.webgpuDebug.description",
"settings.webgpuDebug.iconAlt") and keep the existing userSettings.webgpuDebug()
toggle label logic using translateText; update your language files to include
those new keys and translations. Ensure you call translateText(...) where the
title/description/alt are rendered and keep the existing
onToggleWebgpuDebugOverlayButtonClick and userSettings.webgpuDebug() usage
unchanged.
Comment on lines
+536
to
+547
| recentlyUpdatedTiles(): TileRef[] { | ||
| if (!this.lastUpdate) { | ||
| return []; | ||
| } | ||
| // packedTileUpdates encode [tileRef << 16 | state] as bigint. | ||
| const packed = this.lastUpdate.packedTileUpdates; | ||
| const out: TileRef[] = new Array(packed.length); | ||
| for (let i = 0; i < packed.length; i++) { | ||
| out[i] = Number(packed[i] >> 16n); | ||
| } | ||
| return out; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check SAB mode usage in GameViewAdapter and surrounding code
echo "=== SAB mode references in core/worker ==="
rg -n "usesSharedTileState|SAB" src/core/worker/ --type=ts -B2 -A2
echo -e "\n=== recentlyUpdatedTiles usage ==="
rg -n "recentlyUpdatedTiles" --type=ts
echo -e "\n=== lastUpdate packedTileUpdates handling ==="
rg -n "packedTileUpdates" src/core/worker/ --type=ts -B2 -A2Repository: openfrontio/OpenFrontIO
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Check how packedTileUpdates is populated in GameView
echo "=== packedTileUpdates assignment in GameView.ts ==="
rg -n "packedTileUpdates" src/core/game/GameView.ts --type=ts -B3 -A3
echo -e "\n=== lastUpdate structure definition ==="
rg -n "lastUpdate" src/core/game/GameView.ts --type=ts -B1 -A1Repository: openfrontio/OpenFrontIO
Length of output: 1181
🏁 Script executed:
#!/bin/bash
# Get recentlyUpdatedTiles() implementation from GameView.ts
echo "=== recentlyUpdatedTiles in GameView.ts (line 694) ==="
sed -n '694,710p' src/core/game/GameView.ts
echo -e "\n=== SAB mode context from GameView.ts (lines 516-525 from learning) ==="
sed -n '516,530p' src/core/game/GameView.ts
echo -e "\n=== updateTile method in GameView.ts ==="
rg -n "updateTile\(" src/core/game/GameView.ts --type=ts -B1 -A8 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1736
🏁 Script executed:
#!/bin/bash
# Check GameViewAdapter class definition and properties
echo "=== GameViewAdapter class definition and constructor ==="
sed -n '1,100p' src/core/worker/GameViewAdapter.ts | head -80
echo -e "\n=== Check if GameViewAdapter has usesSharedTileState ==="
rg -n "usesSharedTileState" src/core/worker/GameViewAdapter.ts --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 1910
🏁 Script executed:
#!/bin/bash
# Check the map/tile update handling to understand format differences
echo "=== GameMap.updateTile implementation ==="
rg -n "updateTile\(tu:" src/core/game/ --type=ts -B2 -A10
echo -e "\n=== Check if SAB mode is accessible from GameViewAdapter context ==="
rg -n "class GameViewAdapter" src/core/worker/GameViewAdapter.ts -A50 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 5112
🏁 Script executed:
#!/bin/bash
# Search for where GameViewAdapter.recentlyUpdatedTiles is called
echo "=== Callers of recentlyUpdatedTiles ==="
rg -n "\.recentlyUpdatedTiles\(\)" --type=ts -B3 -A3
# Check if there's SAB mode detection anywhere
echo -e "\n=== SAB or SharedArrayBuffer detection in worker ==="
rg -n "SharedArrayBuffer|SAB" src/core/worker/ --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 275
Remove unused method or add SAB mode handling.
The unpacking logic (>> 16n) is correct for non-SAB mode, matching GameMap.updateTile(). However, this method is never called anywhere in the codebase, and it does not account for SAB mode, where packedTileUpdates contains unpacked tile references (no packing). If this method is needed, either add SAB mode detection (similar to GameView's approach of processing updates through updateTile()) or remove it as dead code.
🤖 Prompt for AI Agents
In `@src/core/worker/GameViewAdapter.ts` around lines 536 - 547, The
recentlyUpdatedTiles method is dead or incomplete: it unpacks packedTileUpdates
by shifting (packed[i] >> 16n) which only works in non-SAB mode and doesn't
handle the SAB (unpacked) case; either remove recentlyUpdatedTiles from
GameViewAdapter if it's unused, or add SAB detection and handling consistent
with GameView.updateTile()/updateTileFromWorker (i.e., check whether
this.lastUpdate.packedTileUpdates contains bigints packed as (tileRef<<16|state)
or raw tile refs and branch accordingly), ensuring you reference
recentlyUpdatedTiles, this.lastUpdate.packedTileUpdates, and the
GameView.updateTile logic to keep behavior consistent.
Comment on lines
+355
to
+376
| tileContext(tile: TileRef): Promise<TileContext> { | ||
| return new Promise((resolve, reject) => { | ||
| if (!this.isInitialized) { | ||
| reject(new Error("Worker not initialized")); | ||
| return; | ||
| } | ||
|
|
||
| const messageId = generateID(); | ||
|
|
||
| this.messageHandlers.set(messageId, (message) => { | ||
| if (message.type === "tile_context_result" && message.result) { | ||
| resolve(message.result); | ||
| } | ||
| }); | ||
|
|
||
| this.postMessage({ | ||
| type: "tile_context", | ||
| id: messageId, | ||
| tile, | ||
| }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing timeout for tileContext promise.
Unlike initialize() which has a 20-second timeout, tileContext can hang indefinitely if the worker never responds. Consider adding a timeout:
🛡️ Suggested fix: Add timeout
tileContext(tile: TileRef): Promise<TileContext> {
return new Promise((resolve, reject) => {
if (!this.isInitialized) {
reject(new Error("Worker not initialized"));
return;
}
const messageId = generateID();
+ const timeoutId = setTimeout(() => {
+ this.messageHandlers.delete(messageId);
+ reject(new Error("tileContext timeout"));
+ }, 5000);
this.messageHandlers.set(messageId, (message) => {
if (message.type === "tile_context_result" && message.result) {
+ clearTimeout(timeoutId);
resolve(message.result);
}
});
this.postMessage({
type: "tile_context",
id: messageId,
tile,
});
});
}🤖 Prompt for AI Agents
In `@src/core/worker/WorkerClient.ts` around lines 355 - 376, tileContext
currently returns a promise that can hang forever if the worker never responds;
update the tileContext method to add a timeout (e.g., 20 seconds like
initialize()) that rejects the promise if no "tile_context_result" arrives,
ensure you clear the timeout and remove the message handler from
this.messageHandlers when either the result is received or the timeout fires,
and reject with a clear Error (e.g., "Timed out waiting for
tile_context_result"); reference the tileContext function, the generated
messageId from generateID(), the entry in this.messageHandlers, and the
postMessage call so you add the timer and cleanup in those places.
Comment on lines
+136
to
+143
| // Create compute passes | ||
| this.terrainComputePass = new TerrainComputePass(); | ||
| void this.terrainComputePass | ||
| .setShader(this.terrainShaderPath) | ||
| .then(() => { | ||
| this.computeTerrainImmediate(); | ||
| }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
Silently swallowed promise rejection hides shader loading failures.
The .catch(() => {}) on line 143 discards any error from setShader(). If the terrain shader fails to load, the renderer will silently continue with undefined behavior.
Consider logging the error or setting a flag to indicate shader load failure:
Proposed fix
void this.terrainComputePass
.setShader(this.terrainShaderPath)
.then(() => {
this.computeTerrainImmediate();
})
- .catch(() => {});
+ .catch((err) => {
+ console.error("Failed to load terrain shader:", err);
+ });📝 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.
Suggested change
| // Create compute passes | |
| this.terrainComputePass = new TerrainComputePass(); | |
| void this.terrainComputePass | |
| .setShader(this.terrainShaderPath) | |
| .then(() => { | |
| this.computeTerrainImmediate(); | |
| }) | |
| .catch(() => {}); | |
| // Create compute passes | |
| this.terrainComputePass = new TerrainComputePass(); | |
| void this.terrainComputePass | |
| .setShader(this.terrainShaderPath) | |
| .then(() => { | |
| this.computeTerrainImmediate(); | |
| }) | |
| .catch((err) => { | |
| console.error("Failed to load terrain shader:", err); | |
| }); |
🤖 Prompt for AI Agents
In `@src/core/worker/WorkerTerritoryRenderer.ts` around lines 136 - 143, The catch
block on terrainComputePass.setShader(this.terrainShaderPath) in
WorkerTerritoryRenderer silently swallows errors; update the promise handling
for TerrainComputePass#setShader so failures are surfaced—e.g., log the error
via the renderer/process logger and/or set a boolean flag (like
this.terrainShaderLoaded = false) on failure, and only call
computeTerrainImmediate() when the shader successfully loads (or bail out/skip
rendering when the flag indicates failure) so the code does not continue with
undefined shader state.
Comment on lines
+583
to
+601
| dispose(): void { | ||
| this.ready = false; | ||
| this.computePasses = []; | ||
| this.computePassOrder = []; | ||
| this.frameComputePasses = []; | ||
| this.renderPasses = []; | ||
| this.renderPassOrder = []; | ||
| this.terrainComputePass = null; | ||
| this.stateUpdatePass = null; | ||
| this.defendedStrengthFullPass = null; | ||
| this.defendedStrengthPass = null; | ||
| this.visualStateSmoothingPass = null; | ||
| this.territoryRenderPass = null; | ||
| this.temporalResolvePass = null; | ||
| this.resources = null; | ||
| this.gameViewAdapter = null; | ||
| this.device = null; | ||
| this.canvas = null; | ||
| } |
Contributor
There was a problem hiding this comment.
dispose() does not destroy GPU resources.
The method nulls out references but does not call destroy() on the WebGPU device, buffers, or textures. This may cause GPU memory to leak until garbage collection, which is unreliable for GPU resources.
Consider explicitly destroying resources:
Proposed cleanup
dispose(): void {
this.ready = false;
+ this.resources?.destroy?.(); // if GroundTruthData has a destroy method
+ this.device?.destroy?.();
this.computePasses = [];
// ... rest of cleanup📝 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.
Suggested change
| dispose(): void { | |
| this.ready = false; | |
| this.computePasses = []; | |
| this.computePassOrder = []; | |
| this.frameComputePasses = []; | |
| this.renderPasses = []; | |
| this.renderPassOrder = []; | |
| this.terrainComputePass = null; | |
| this.stateUpdatePass = null; | |
| this.defendedStrengthFullPass = null; | |
| this.defendedStrengthPass = null; | |
| this.visualStateSmoothingPass = null; | |
| this.territoryRenderPass = null; | |
| this.temporalResolvePass = null; | |
| this.resources = null; | |
| this.gameViewAdapter = null; | |
| this.device = null; | |
| this.canvas = null; | |
| } | |
| dispose(): void { | |
| this.ready = false; | |
| this.resources?.destroy?.(); | |
| this.device?.destroy?.(); | |
| this.computePasses = []; | |
| this.computePassOrder = []; | |
| this.frameComputePasses = []; | |
| this.renderPasses = []; | |
| this.renderPassOrder = []; | |
| this.terrainComputePass = null; | |
| this.stateUpdatePass = null; | |
| this.defendedStrengthFullPass = null; | |
| this.defendedStrengthPass = null; | |
| this.visualStateSmoothingPass = null; | |
| this.territoryRenderPass = null; | |
| this.temporalResolvePass = null; | |
| this.resources = null; | |
| this.gameViewAdapter = null; | |
| this.device = null; | |
| this.canvas = null; | |
| } |
🤖 Prompt for AI Agents
In `@src/core/worker/WorkerTerritoryRenderer.ts` around lines 583 - 601, dispose()
currently only nulls references and can leak GPU memory; update
WorkerTerritoryRenderer.dispose() to explicitly destroy GPU resources before
nulling: call this.device.destroy() (if supported) after first destroying all
GPU objects; iterate this.computePasses, this.frameComputePasses,
this.renderPasses and call destroy() on any contained
GPUBuffer/GPUTexture/GPUTextureView/GPUExternalTexture/sampler/GPURenderPipeline/GPUComputePipeline
as appropriate, also destroy single-pass resources referenced by
terrainComputePass, stateUpdatePass, defendedStrengthFullPass,
defendedStrengthPass, visualStateSmoothingPass, territoryRenderPass,
temporalResolvePass and any entries in this.resources; catch and log errors
during destroy, then clear arrays and set fields (this.gameViewAdapter,
this.device, this.canvas, etc.) to null.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
…based architecture Refactor the monolithic TerritoryWebGLRenderer into a modular, extensible architecture that separates ground truth computation from rendering passes. This change also includes related improvements to game state management and hover information handling. WebGPU Architecture Refactor: - Extract all shaders to external .wgsl files (no inlined shaders) - Separate ground truth data management (GroundTruthData) from rendering - Create pass-based architecture with ComputePass and RenderPass interfaces - Implement compute passes: StateUpdatePass, DefendedClearPass, DefendedUpdatePass - Implement render pass: TerritoryRenderPass - Add TerritoryRenderer orchestrator with dependency-based execution ordering - Add WebGPUDevice for device initialization and management - Add ShaderLoader utility for loading .wgsl files via Vite ?raw imports Performance Optimizations: - Dependency order computed once at init (topological sort) - Early exit checks at orchestrator and pass levels - Bind groups rebuilt when textures/buffers are recreated - Zero per-frame allocations (reuse command encoders and staging buffers) Architecture Benefits: - Easy to extend with new compute/render passes (borders, temporal smoothing, etc.) - Clear separation between tick-based compute and frame-based rendering - All shaders in external files for better maintainability - Ground truth data computed once and reused by all passes Related Changes: - Add defended tile state support to GameMap (isDefended/setDefended) - Expose tileStateView() for direct GPU state access - Extract hover info logic to HoverInfo utility - Remove TerrainLayer (terrain now rendered by WebGPU territory pass) - Update GameRenderer to use transparent overlay canvas - Add viewOffset() method to TransformHandler Files: - Deleted: TerritoryWebGLRenderer.ts (1217 lines), TerrainLayer.ts (77 lines) - Added: 17 new files in webgpu/ directory structure - Updated: TerritoryLayer.ts, GameRenderer.ts, PlayerInfoOverlay.ts, GameMap.ts, GameView.ts, GameImpl.ts, TransformHandler.ts, vite-env.d.ts
Updated the terrain recomputation logic to trigger asynchronously, improving performance by allowing rendering to continue without blocking. This change ensures that the terrain will be ready for the next frame, which may result in displaying stale terrain for one frame but enhances overall rendering efficiency.
Modified the workgroup size in the state-update compute shader from 1 to 64 for improved parallel processing. Adjusted the dispatch logic in StateUpdatePass to calculate the correct number of workgroups based on the new size, enhancing performance during state updates. Removed unnecessary terrain parameter upload in TerritoryRenderer to streamline resource management.
Replaced tile sampling for terrain colors with direct extraction from the theme object, significantly improving performance. Updated shore, water, shoreline water, plains, highland, and mountain color computations to utilize theme properties, eliminating the need for tile searches. This change enhances efficiency in terrain color management while maintaining visual fidelity.
Replace epoch-based defended texture tracking with hard-clear approach for improved reliability and performance. Remove complex dirty state tracking and epoch incrementing logic in favor of direct hard clears before rebuilds. Changes: - Remove wasDefensePostsDirty tracking from TerritoryRenderer - Replace numUpdates > 0 checks with hasStateUpdates boolean - Hard-clear defended texture before restamping instead of epoch management - Mark DefendedUpdatePass as dirty when rebuilding defended state - Rebuild bind group in DefendedUpdatePass when missing, not just on buffer change This eliminates potential transient mismatches where defended rendering disappeared between rebuilds and simplifies the update pipeline.
Store defended influence in defendedStrengthTexture and sample it in territory render shader Recompute defended strength on tick for state-updated tiles and for post-change dirty tiles, with full-map fallback when diffs are large Pack defense posts by owner on GPU (owner offsets + posts buffer) Remove old defended clear/update passes and epoch-based params
…t.meta.glob(..., { as: "raw", eager: true })
- Introduced WebGPUComputeMetricsEvent to track compute timing. - Added WebGPUDebugOverlay component for displaying WebGPU performance metrics. - Refactored TerritoryLayer to utilize new shader management for territory rendering. - Updated shaders to support new parameters for enhanced visual effects. - Removed deprecated territory border mode settings from UserSettingModal and SettingsModal. - Enhanced GroundTruthData to manage new textures for owner indices and relations. - Improved shader parameter handling in TerritoryRenderer and related classes. This commit enhances the WebGPU rendering pipeline, providing better performance insights and visual fidelity through improved shader management and debugging capabilities.
- Included <webgpu-debug-overlay> component in the main layout
Add user-selectable temporal smoothing pipeline to create smooth visual transitions between simulation ticks (~10Hz) and display frames (~60Hz). Pre-render smoothing provides sharp tile dissolve transitions using compute shaders, while post-render smoothing blends frames for fluid animation. Includes tick timing with exponential moving averages for stable temporal parameters. New Components: - TerritoryPreSmoothingRegistry & TerritoryPostSmoothingRegistry for mode selection - VisualStateSmoothingPass compute shader for pre-render dissolve effects - TemporalResolvePass render shader for post-render temporal compositing - Enhanced GroundTruthData with temporal uniforms and history textures Performance: No full-map per-frame compute, single fullscreen post-render pass. Compatible with all territory shaders (classic, retro, future variants). Files: 12 files changed, 1357 insertions(+), 8 deletions(-)
- Add terrain-compute-improved-lite.wgsl and terrain-compute-improved-heavy.wgsl - Create TerrainShaderRegistry.ts for shader management - Refactor TerrainComputePass to support dynamic shader switching - Update TerritoryRenderer, TerritoryLayer, and GroundTruthData for new shader integration - Enhance WebGPUDebugOverlay with additional debugging capabilities
…ault values - Changed section title from "Shaders" to "Terrain" in WebGPUDebugOverlay. - Updated default values for various terrain shader parameters to improve rendering quality, including noise strength, blend width, lighting strength, and cavity strength.
- Modified terrain shader parameters in GroundTruthData for better rendering. - Added new user-configurable settings for water effects in TerrainShaderRegistry. - Enhanced terrain compute shaders to incorporate water depth and blur adjustments. - Refactored shader logic to improve water color blending and depth calculations
- Added support for terrain base RGBA values to optimize rendering. - Refactored tile marking logic to utilize map dimensions for improved accuracy. - Updated view size handling to prevent unnecessary canvas resizing. - Introduced methods to rebuild terrain base and refresh terrain for better visual fidelity. - Cleaned up initialization and cleanup processes for better resource management.
- Introduced new properties and methods in GroundTruthData for handling relations, including `needsRelationsUpload`, `relationsDenseBySmallId`, and `pendingRelationsPairs`. - Implemented logic to mark relations as dirty and upload relations conditionally based on changes in diplomacy. - Updated Worker.worker.ts to synchronize relations updates with the renderer, optimizing performance by only refreshing when necessary. - Enhanced WorkerTerritoryRenderer with methods to mark relations dirty, ensuring proper resource management during updates.
- GameViewAdapter: build from tileState/terrainData buffers and game updates (players, defense posts, embargo/alliance) instead of Game + TerrainMapData; add DefensePostUnit/PlayerLiteView and drop config(). - Worker: keep local renderTileState; tileUpdateSink receives packed bigint and updates buffer + dirty queue; no terrain map load in worker. - Proxies: send view size/transform only when changed, inline in render_frame (optional viewSize/viewTransform); remove separate set_view_size/set_view_transform messages. - Simulation: remove main-thread RAF heartbeat loop; worker uses scheduleSimPump() on heartbeat/addTurn to coalesce ticks. - GroundTruthData: take defensePostRange at construction; Territory renderer passes it through; remove runtime defensePostRange change check. - GameRunner: tileUpdateSink(packedTileUpdate: bigint); add hasPendingTurns().
- Add render_done worker message and tag render_frame with an id. - Gate TerritoryRendererProxy/Canvas2DRendererProxy to one in-flight render (2s safety timeout) to prevent render queue buildup. - Split roster vs palette dirtiness in GameViewAdapter and only force full relations rebuilds on roster/team changes. - Only markRelationsDirty() on roster changes in WorkerTerritoryRenderer to avoid repeated expensive uploadRelations() while paused.
- Introduced WorkerMetricsEvent and SetWorkerDebugEvent to facilitate communication between the main thread and worker for performance monitoring. - Enhanced ClientGameRunner to emit worker metrics and handle debug configuration updates. - Updated PerformanceOverlay to display worker metrics and allow toggling of debug settings. - Refactored Canvas2DRendererProxy and TerritoryRendererProxy to improve rendering performance and manage render cooldowns. - Added profiling capabilities in Worker.worker.ts to track event loop lag, simulation delays, and message handling metrics.
- Introduced batching for turn messages in ClientGameRunner to reduce the frequency of worker communication - Updated WorkerClient to manage pending turns and schedule flushes - Enhanced Worker.worker.ts to process turn batches - Added new TurnBatchMessage type for better organization of turn data sent between the main thread and worker. - Improved PerformanceOverlay to display additional metrics related to worker performance and turn processing.
…tracking - Introduced new properties to track view dimensions, scale, and offsets for improved rendering efficiency. - Updated rendering logic to set frameDirty flag based on changes in view parameters and shader settings. - Refactored methods to prevent unnecessary updates when view parameters remain unchanged. - Improved error handling in shader setting to ensure smoother rendering operations.
- Added new metrics to PerformanceOverlay for tracking render submissions, noops, and various CPU timings. - Updated TerritoryLayer to optimize user settings synchronization and reduce unnecessary refresh calls. - Enhanced Worker components to collect and report detailed rendering performance metrics, including frame compute and territory pass timings. - Improved WorkerTerritoryRenderer to profile rendering phases
…ment - Introduced getOffsetX() and getOffsetY() methods in TransformHandler for better access to offset values. - Updated TerritoryLayer to manage attached canvases more effectively, ensuring proper synchronization of view dimensions and transformations. - Improved rendering logic to prevent unnecessary updates when view parameters remain unchanged, enhancing overall performance.
- Added an optional `onTileStateChanged` hook in the Game interface - Refactored GameImpl to utilize the new hook for reporting tile state changes instead of emitting GameUpdateType.Tile updates. - Updated Worker.worker.ts to integrate the new tile state change handling
- Move profiling/metrics logic out of Worker.worker.ts into WorkerProfiler.ts - Drop unused MAX_TICKS_PER_HEARTBEAT and TransformHandler.viewOffset() - Simplify TerritoryLayer overlay parenting; remove unused markTile() wrapper
scamiv
force-pushed
the
webgpu-worker-rebased
branch
from
1bcdc3c to
813c502
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/client/graphics/canvas2d/Canvas2DRendererProxy.ts`:
- Around line 101-102: The dark-mode check in Canvas2DRendererProxy using const
themeAny = this.theme as any; const darkMode = themeAny.darkShore !== undefined
is brittle; update the Theme interface to include an explicit isDarkMode():
boolean (or a boolean property like isDarkMode) and replace the current
detection in Canvas2DRendererProxy with this.theme.isDarkMode() (or read the new
property) so detection is type-safe and resilient to theme changes.
In `@src/client/graphics/layers/TerritoryLayer.ts`:
- Around line 121-167: configureRenderer() always allocates a new renderer and
nulls attachedTerritoryCanvas/attachedMainCanvas, causing leaked DOM nodes and
renderer resources; change it to check the current backend via
this.userSettings.backgroundRenderer() and reuse this.territoryRenderer when the
backend type and worker/theme-compatible state haven't changed, otherwise before
creating a new one explicitly detach and dispose the old renderer (call its
dispose/destroy/detach methods) and remove any attached canvases from the DOM
(clear this.attachedTerritoryCanvas and this.attachedMainCanvas after removal)
so TerritoryRendererProxy.create/Canvas2DRendererProxy.create only run when
necessary and resources are released when swapping backends.
In `@src/client/graphics/webgpu/compute/VisualStateSmoothingPass.ts`:
- Around line 195-202: In the VisualStateSmoothingPass.dispose() method, clear
the remaining tracked resource references by setting the boundUpdatesBuffer and
boundVisualStateTexture fields to null in addition to the existing fields;
update the dispose() implementation (method: dispose) to assign null to
boundUpdatesBuffer and boundVisualStateTexture so no stale GPU resources remain
referenced after disposal.
In `@src/client/graphics/webgpu/core/GroundTruthData.ts`:
- Around line 1310-1342: The ensureUpdatesBuffer method can produce NaN sizes if
capacity is non-finite or negative; before computing the power-of-two, validate
and clamp capacity (e.g. if !Number.isFinite(capacity) or capacity < 0, set
capacity to 0) and use Math.max(1, capacity) when computing Math.log2 so the
Math.pow/ceil path never receives NaN/Infinity; update ensureUpdatesBuffer (and
similarly getUpdatesStaging usage of this.updatesCapacity) to use the clamped
value so updatesCapacity and bufferSize are always a finite integer.
In `@src/client/graphics/webgpu/TerritoryRenderer.ts`:
- Around line 420-423: markAllDirty currently only calls
this.resources?.markDefensePostsDirty(), which leaves state, palette and
defended-strength stale; update TerritoryRenderer.markAllDirty to perform a full
GPU-state reset by invoking the corresponding resource invalidation methods on
this.resources (e.g. call this.resources?.markStateDirty(),
this.resources?.markPaletteDirty(), this.resources?.markDefendedStrengthDirty()
in addition to this.resources?.markDefensePostsDirty()), or if a worker-renderer
style fullReset/resetAll method exists on the resources object, call that
instead so state/palette/defended-strength recompute is forced.
In `@src/core/worker/WorkerTerritoryRenderer.ts`:
- Around line 369-377: In setTerrainShader, the promise returned by
this.terrainComputePass.setShader(shaderPath) needs proper error handling:
update the call in setTerrainShader so its then() continues to call
this.refreshTerrain() on success and add a .catch(err => ...) that logs or
handles the error (and still sets this.frameDirty appropriately) following the
same pattern used elsewhere in this file for shader loading; reference the
methods terrainComputePass.setShader, refreshTerrain, and the frameDirty flag
when making the change.
🧹 Nitpick comments (12)
src/client/graphics/PlayerIcons.ts (1)
143-149: Confirm removal of the “targeted at me” nuke warning.
isMyPlayerTargetis now alwaysfalse, so the red icon path is never used. If this is intentional because the main thread is non-authoritative, please confirm. If not, consider plumbing a target flag from the worker. If removal is intended, simplify the branch to avoid dead logic.♻️ Optional simplification (if red icon is intentionally removed)
- // Main thread does not maintain authoritative tile ownership; treat this icon - // as informational only (no "targeted at me" specialization here). - const isMyPlayerTarget = false; - if (nukesSentByOtherPlayer.length > 0) { - const icon = isMyPlayerTarget ? nukeRedIcon : nukeWhiteIcon; + const icon = nukeWhiteIcon; icons.push({ id: "nuke", kind: "image", src: icon }); }src/client/graphics/HoverInfo.ts (1)
12-66: Avoidsqrtand double distance work in hover path.This runs on hover; squared distances are cheaper and still correct for sorting and thresholds.
♻️ Optional refactor
-function euclideanDistWorld( +function euclideanDistSqWorld( coord: { x: number; y: number }, tileRef: TileRef, game: GameView, ): number { const x = game.x(tileRef); const y = game.y(tileRef); const dx = coord.x - x; const dy = coord.y - y; - return Math.sqrt(dx * dx + dy * dy); + return dx * dx + dy * dy; } function distSortUnitWorld(coord: { x: number; y: number }, game: GameView) { return (a: UnitView, b: UnitView) => { - const distA = euclideanDistWorld(coord, a.tile(), game); - const distB = euclideanDistWorld(coord, b.tile(), game); + const distA = euclideanDistSqWorld(coord, a.tile(), game); + const distB = euclideanDistSqWorld(coord, b.tile(), game); return distA - distB; }; } @@ - .filter((u) => euclideanDistWorld(worldCoord, u.tile(), game) < 50) + .filter( + (u) => euclideanDistSqWorld(worldCoord, u.tile(), game) < 50 * 50, + )src/client/graphics/webgpu/render/TerrainShaderRegistry.ts (1)
186-284: Default values are duplicated between TERRAIN_SHADERS and buildTerrainShaderParams.The default values for shader options are defined in two places:
- In
TERRAIN_SHADERSarray (lines 47-155)- Hardcoded again in
buildTerrainShaderParams(e.g., lines 199, 203, 225-254)This creates a maintenance risk — if you update a default in one place, you might forget the other.
Also,
waterDepthStrengthDefault,waterDepthCurveDefault, andwaterDepthBlurDefault(lines 192-194) are only used in the "classic" branch but declared at the top of the function.♻️ Suggested approach
Consider a helper that reads defaults from the shader definition:
function getOptionDefault(shaderId: TerrainShaderId, key: string): number { const shader = getTerrainShaderById(shaderId); const opt = shader.options.find(o => o.key === key); if (opt && opt.kind === "range") return opt.defaultValue; return 0; }Then use it in
buildTerrainShaderParamsto avoid duplication.src/client/graphics/canvas2d/Canvas2DRendererProxy.ts (1)
345-358: 15-second render timeout is quite long.A 15-second timeout for a single render frame seems excessive. If rendering takes that long, the user experience is already severely degraded. A shorter timeout (e.g., 2-5 seconds) would surface issues faster.
- }, 15000); + }, 5000);src/client/graphics/layers/EmojiTable.ts (1)
43-55: Redundant cast at line 47.Since
targetPlayeris already typed asPlayerView | nulland the null case returns early at line 39-41, the cast(targetPlayer as PlayerView)at line 47 is unnecessary —targetPlayeris already narrowed toPlayerView.const recipient = targetPlayer === this.game.myPlayer() ? AllPlayers - : (targetPlayer as PlayerView); + : targetPlayer;src/client/graphics/layers/PlayerInfoOverlay.ts (2)
68-69: Unused fieldshowDetails.The field
showDetailsis declared but never read or written anywhere in this file. Either remove it or wire it up to the intended functionality.- private showDetails = true; private hoverSeq = 0;
146-160: Minor: Use squared distance in filter to avoidsqrt.The filter computes
Math.sqrt(dx * dx + dy * dy) < 50but you could compare squared distances directly to skip thesqrtcall. This is a small optimization..filter((u) => { const dx = worldCoord.x - this.game.x(u.tile()); const dy = worldCoord.y - this.game.y(u.tile()); - return Math.sqrt(dx * dx + dy * dy) < 50; + return dx * dx + dy * dy < 2500; // 50^2 })src/client/graphics/webgpu/core/WebGPUDevice.ts (1)
38-42: Consider handling device loss.
adapter.requestDevice()can fail, and the device can be lost during operation. Consider wrapping in try/catch and exposing a way for callers to handle device loss:device.lost.then((info) => { console.error(`WebGPU device lost: ${info.message}`); });This helps diagnose issues when the GPU context is lost (e.g., driver crash, resource exhaustion).
src/client/graphics/webgpu/compute/VisualStateSmoothingPass.ts (1)
149-167: Redundant null check forvisualStateTexture.
getVisualStateTexture()is called and checked for null on line 157, then called again on lines 163-167 with another null check. Consider storing the result in a local variable earlier:private rebuildBindGroup(): void { + const visualStateTexture = this.resources?.getVisualStateTexture(); if ( !this.device || !this.bindGroupLayout || !this.resources || !this.resources.temporalUniformBuffer || !this.paramsBuffer || !this.resources.updatesBuffer || - !this.resources.getVisualStateTexture() + !visualStateTexture ) { this.bindGroup = null; return; } - const visualStateTexture = this.resources.getVisualStateTexture(); - if (!visualStateTexture) { - this.bindGroup = null; - return; - } - this.bindGroup = this.device.createBindGroup({src/client/graphics/layers/WebGPUDebugOverlay.ts (1)
184-193: Unregister the metrics listener when the overlay is detached.
init()registers an EventBus listener but never removes it. If the element is re-created or removed, the callback can keep firing.Proposed cleanup
+ private onComputeMetrics = (e: WebGPUComputeMetricsEvent) => { + this.syncDebugVisibility(); + if (typeof e.computeMs === "number" && Number.isFinite(e.computeMs)) { + this.tickComputeMs = e.computeMs; + this.requestUpdate(); + } + }; + init() { - this.eventBus.on(WebGPUComputeMetricsEvent, (e) => { - this.syncDebugVisibility(); - if (typeof e.computeMs === "number" && Number.isFinite(e.computeMs)) { - this.tickComputeMs = e.computeMs; - this.requestUpdate(); - } - }); + this.eventBus.on(WebGPUComputeMetricsEvent, this.onComputeMetrics); this.requestUpdate(); } + + disconnectedCallback(): void { + super.disconnectedCallback(); + this.eventBus?.off(WebGPUComputeMetricsEvent, this.onComputeMetrics); + }src/client/graphics/webgpu/core/GroundTruthData.ts (1)
16-124: Consider splitting this large class into smaller helpers.This class owns palette, relations, defense-post, and view-state logic. Small composed helpers would keep the surface smaller and easier to test.
src/core/worker/WorkerMessages.ts (1)
92-109: Use a discriminated union forTileContextto prevent state inconsistency.The current interface allows
hasOwnerand the nullable ID fields to drift independently. A union type enforces the invariant: whenhasOwneris true, bothownerSmallIdandownerIdmust be non-null; when false, both must be null. This aligns with how the codebase represents typed game state.🔧 Suggested shape
-export interface TileContext { - hasOwner: boolean; - ownerSmallId: number | null; - ownerId: PlayerID | null; - hasFallout: boolean; - isDefended: boolean; -} +export type TileContext = + | { + hasOwner: false; + ownerSmallId: null; + ownerId: null; + hasFallout: boolean; + isDefended: boolean; + } + | { + hasOwner: true; + ownerSmallId: number; + ownerId: PlayerID; + hasFallout: boolean; + isDefended: boolean; + };
Comment on lines
+101
to
+102
| const themeAny = this.theme as any; | ||
| const darkMode = themeAny.darkShore !== undefined; |
Contributor
There was a problem hiding this comment.
Brittle dark mode detection.
Checking themeAny.darkShore !== undefined to detect dark mode is fragile. If the theme interface changes, this will silently break.
🛠️ Suggested fix
- const themeAny = this.theme as any;
- const darkMode = themeAny.darkShore !== undefined;
+ // Consider adding a method to Theme interface:
+ // isDarkMode(): boolean
+ // For now, check if backgroundColor is dark
+ const bg = this.theme.backgroundColor();
+ const darkMode = bg.brightness() < 0.5;Or better, add an isDarkMode() method to the Theme interface so this is explicit.
🤖 Prompt for AI Agents
In `@src/client/graphics/canvas2d/Canvas2DRendererProxy.ts` around lines 101 -
102, The dark-mode check in Canvas2DRendererProxy using const themeAny =
this.theme as any; const darkMode = themeAny.darkShore !== undefined is brittle;
update the Theme interface to include an explicit isDarkMode(): boolean (or a
boolean property like isDarkMode) and replace the current detection in
Canvas2DRendererProxy with this.theme.isDarkMode() (or read the new property) so
detection is type-safe and resilient to theme changes.
Comment on lines
+121
to
+167
| private configureRenderer() { | ||
| const backend = this.userSettings.backgroundRenderer(); | ||
| const create = (b: "webgpu" | "canvas2d") => | ||
| b === "canvas2d" | ||
| ? Canvas2DRendererProxy.create(this.game, this.theme, this.game.worker) | ||
| : TerritoryRendererProxy.create( | ||
| this.game, | ||
| this.theme, | ||
| this.game.worker, | ||
| ); | ||
|
|
||
| let { renderer, reason } = create(backend); | ||
| if (!renderer && backend === "webgpu") { | ||
| // Graceful fallback: allow the game to run even without WebGPU. | ||
| console.warn( | ||
| `WebGPU renderer unavailable (${reason ?? "unknown"}); falling back to Canvas2D worker renderer.`, | ||
| ); | ||
| ({ renderer, reason } = create("canvas2d")); | ||
| } | ||
|
|
||
| for (const tile of this.game.bfs( | ||
| centerTile, | ||
| euclDistFN(centerTile, 9, true), | ||
| )) { | ||
| if (!this.game.hasOwner(tile)) { | ||
| this.paintHighlightTile(tile, color, 255); | ||
| } | ||
| } | ||
| if (!renderer) { | ||
| throw new Error(reason ?? "No supported background renderer available."); | ||
| } | ||
| } | ||
|
|
||
| private drawFocusedPlayerHighlight() { | ||
| const focusedPlayer = this.game.focusedPlayer(); | ||
| this.territoryRenderer = renderer; | ||
| this.attachedTerritoryCanvas = null; | ||
| this.attachedMainCanvas = null; | ||
| this.lastViewWidth = 0; | ||
| this.lastViewHeight = 0; | ||
| this.viewTransformSynced = false; | ||
| const patternsEnabled = this.userSettings.territoryPatterns(); | ||
| this.lastPatternsEnabled = patternsEnabled; | ||
| this.territoryRenderer.setPatternsEnabled(patternsEnabled); | ||
| this.territoryRenderer.setAlternativeView(this.alternativeView); | ||
| this.territoryRenderer.setHighlightedOwnerId(this.hoveredOwnerSmallId); | ||
| this.applyTerrainShaderSettings(true); | ||
| this.applyTerritoryShaderSettings(true); | ||
| this.applyTerritorySmoothingSettings(true); | ||
| this.territoryRenderer.markAllDirty(); | ||
| this.territoryRenderer.refreshPalette(); | ||
| this.lastPaletteSignature = this.computePaletteSignature(); | ||
|
|
||
| // Run an initial tick to upload state and build the colour texture. Without | ||
| // this, the first render call may occur before the initial compute pass | ||
| // has been executed, resulting in undefined colours. | ||
| // Note: compute passes are ticked in the worker at simulation cadence. | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent renderer churn and leaked overlay canvases.
configureRenderer() always creates a new renderer and resets attachedTerritoryCanvas to null, so old canvases are never removed and old renderer resources are never released. This can leak DOM nodes and worker/GPU state on every redraw or theme change (Line 121+). Consider reusing the renderer when the backend is unchanged, and explicitly detach/dispose when it changes.
♻️ Suggested fix
+ private currentBackend: "webgpu" | "canvas2d" | null = null;
+
private configureRenderer() {
const backend = this.userSettings.backgroundRenderer();
+ if (this.territoryRenderer && this.currentBackend === backend) {
+ return;
+ }
+ this.attachedTerritoryCanvas?.remove();
+ this.territoryRenderer?.dispose?.();
+
const create = (b: "webgpu" | "canvas2d") =>
b === "canvas2d"
? Canvas2DRendererProxy.create(this.game, this.theme, this.game.worker)
: TerritoryRendererProxy.create(
this.game,
this.theme,
this.game.worker,
);
@@
this.territoryRenderer = renderer;
+ this.currentBackend = backend;
this.attachedTerritoryCanvas = null;📝 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.
Suggested change
| private configureRenderer() { | |
| const backend = this.userSettings.backgroundRenderer(); | |
| const create = (b: "webgpu" | "canvas2d") => | |
| b === "canvas2d" | |
| ? Canvas2DRendererProxy.create(this.game, this.theme, this.game.worker) | |
| : TerritoryRendererProxy.create( | |
| this.game, | |
| this.theme, | |
| this.game.worker, | |
| ); | |
| let { renderer, reason } = create(backend); | |
| if (!renderer && backend === "webgpu") { | |
| // Graceful fallback: allow the game to run even without WebGPU. | |
| console.warn( | |
| `WebGPU renderer unavailable (${reason ?? "unknown"}); falling back to Canvas2D worker renderer.`, | |
| ); | |
| ({ renderer, reason } = create("canvas2d")); | |
| } | |
| for (const tile of this.game.bfs( | |
| centerTile, | |
| euclDistFN(centerTile, 9, true), | |
| )) { | |
| if (!this.game.hasOwner(tile)) { | |
| this.paintHighlightTile(tile, color, 255); | |
| } | |
| } | |
| if (!renderer) { | |
| throw new Error(reason ?? "No supported background renderer available."); | |
| } | |
| } | |
| private drawFocusedPlayerHighlight() { | |
| const focusedPlayer = this.game.focusedPlayer(); | |
| this.territoryRenderer = renderer; | |
| this.attachedTerritoryCanvas = null; | |
| this.attachedMainCanvas = null; | |
| this.lastViewWidth = 0; | |
| this.lastViewHeight = 0; | |
| this.viewTransformSynced = false; | |
| const patternsEnabled = this.userSettings.territoryPatterns(); | |
| this.lastPatternsEnabled = patternsEnabled; | |
| this.territoryRenderer.setPatternsEnabled(patternsEnabled); | |
| this.territoryRenderer.setAlternativeView(this.alternativeView); | |
| this.territoryRenderer.setHighlightedOwnerId(this.hoveredOwnerSmallId); | |
| this.applyTerrainShaderSettings(true); | |
| this.applyTerritoryShaderSettings(true); | |
| this.applyTerritorySmoothingSettings(true); | |
| this.territoryRenderer.markAllDirty(); | |
| this.territoryRenderer.refreshPalette(); | |
| this.lastPaletteSignature = this.computePaletteSignature(); | |
| // Run an initial tick to upload state and build the colour texture. Without | |
| // this, the first render call may occur before the initial compute pass | |
| // has been executed, resulting in undefined colours. | |
| // Note: compute passes are ticked in the worker at simulation cadence. | |
| } | |
| private currentBackend: "webgpu" | "canvas2d" | null = null; | |
| private configureRenderer() { | |
| const backend = this.userSettings.backgroundRenderer(); | |
| if (this.territoryRenderer && this.currentBackend === backend) { | |
| return; | |
| } | |
| this.attachedTerritoryCanvas?.remove(); | |
| this.territoryRenderer?.dispose?.(); | |
| const create = (b: "webgpu" | "canvas2d") => | |
| b === "canvas2d" | |
| ? Canvas2DRendererProxy.create(this.game, this.theme, this.game.worker) | |
| : TerritoryRendererProxy.create( | |
| this.game, | |
| this.theme, | |
| this.game.worker, | |
| ); | |
| let { renderer, reason } = create(backend); | |
| if (!renderer && backend === "webgpu") { | |
| // Graceful fallback: allow the game to run even without WebGPU. | |
| console.warn( | |
| `WebGPU renderer unavailable (${reason ?? "unknown"}); falling back to Canvas2D worker renderer.`, | |
| ); | |
| ({ renderer, reason } = create("canvas2d")); | |
| } | |
| if (!renderer) { | |
| throw new Error(reason ?? "No supported background renderer available."); | |
| } | |
| this.territoryRenderer = renderer; | |
| this.currentBackend = backend; | |
| this.attachedTerritoryCanvas = null; | |
| this.attachedMainCanvas = null; | |
| this.lastViewWidth = 0; | |
| this.lastViewHeight = 0; | |
| this.viewTransformSynced = false; | |
| const patternsEnabled = this.userSettings.territoryPatterns(); | |
| this.lastPatternsEnabled = patternsEnabled; | |
| this.territoryRenderer.setPatternsEnabled(patternsEnabled); | |
| this.territoryRenderer.setAlternativeView(this.alternativeView); | |
| this.territoryRenderer.setHighlightedOwnerId(this.hoveredOwnerSmallId); | |
| this.applyTerrainShaderSettings(true); | |
| this.applyTerritoryShaderSettings(true); | |
| this.applyTerritorySmoothingSettings(true); | |
| this.territoryRenderer.markAllDirty(); | |
| this.territoryRenderer.refreshPalette(); | |
| this.lastPaletteSignature = this.computePaletteSignature(); | |
| // Run an initial tick to upload state and build the colour texture. Without | |
| // this, the first render call may occur before the initial compute pass | |
| // has been executed, resulting in undefined colours. | |
| // Note: compute passes are ticked in the worker at simulation cadence. | |
| } |
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/TerritoryLayer.ts` around lines 121 - 167,
configureRenderer() always allocates a new renderer and nulls
attachedTerritoryCanvas/attachedMainCanvas, causing leaked DOM nodes and
renderer resources; change it to check the current backend via
this.userSettings.backgroundRenderer() and reuse this.territoryRenderer when the
backend type and worker/theme-compatible state haven't changed, otherwise before
creating a new one explicitly detach and dispose the old renderer (call its
dispose/destroy/detach methods) and remove any attached canvases from the DOM
(clear this.attachedTerritoryCanvas and this.attachedMainCanvas after removal)
so TerritoryRendererProxy.create/Canvas2DRendererProxy.create only run when
necessary and resources are released when swapping backends.
Comment on lines
+195
to
+202
| dispose(): void { | ||
| this.pipeline = null; | ||
| this.bindGroupLayout = null; | ||
| this.bindGroup = null; | ||
| this.device = null; | ||
| this.resources = null; | ||
| this.paramsBuffer = null; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clear tracked resource references in dispose().
The boundUpdatesBuffer and boundVisualStateTexture fields should be cleared to avoid stale references.
dispose(): void {
this.pipeline = null;
this.bindGroupLayout = null;
this.bindGroup = null;
this.device = null;
this.resources = null;
this.paramsBuffer = null;
+ this.boundUpdatesBuffer = null;
+ this.boundVisualStateTexture = null;
}📝 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.
Suggested change
| dispose(): void { | |
| this.pipeline = null; | |
| this.bindGroupLayout = null; | |
| this.bindGroup = null; | |
| this.device = null; | |
| this.resources = null; | |
| this.paramsBuffer = null; | |
| } | |
| dispose(): void { | |
| this.pipeline = null; | |
| this.bindGroupLayout = null; | |
| this.bindGroup = null; | |
| this.device = null; | |
| this.resources = null; | |
| this.paramsBuffer = null; | |
| this.boundUpdatesBuffer = null; | |
| this.boundVisualStateTexture = null; | |
| } |
🤖 Prompt for AI Agents
In `@src/client/graphics/webgpu/compute/VisualStateSmoothingPass.ts` around lines
195 - 202, In the VisualStateSmoothingPass.dispose() method, clear the remaining
tracked resource references by setting the boundUpdatesBuffer and
boundVisualStateTexture fields to null in addition to the existing fields;
update the dispose() implementation (method: dispose) to assign null to
boundUpdatesBuffer and boundVisualStateTexture so no stale GPU resources remain
referenced after disposal.
Comment on lines
+1310
to
+1342
| ensureUpdatesBuffer(capacity: number): GPUBuffer { | ||
| if (this.updatesBuffer && capacity <= this.updatesCapacity) { | ||
| return this.updatesBuffer; | ||
| } | ||
|
|
||
| const GPUBufferUsage = (globalThis as any).GPUBufferUsage; | ||
| const STORAGE = GPUBufferUsage?.STORAGE ?? 0x10; | ||
| const COPY_DST_BUF = GPUBufferUsage?.COPY_DST ?? 0x8; | ||
|
|
||
| this.updatesCapacity = Math.max( | ||
| 256, | ||
| Math.pow(2, Math.ceil(Math.log2(capacity))), | ||
| ); | ||
| const bufferSize = this.updatesCapacity * 8; // Each update is 8 bytes | ||
|
|
||
| if (this.updatesBuffer) { | ||
| (this.updatesBuffer as any).destroy?.(); | ||
| } | ||
|
|
||
| const buffer = this.device.createBuffer({ | ||
| size: bufferSize, | ||
| usage: STORAGE | COPY_DST_BUF, | ||
| }); | ||
| (this as any).updatesBuffer = buffer; | ||
|
|
||
| this.updatesStaging = new Uint32Array(this.updatesCapacity * 2); | ||
| return buffer; | ||
| } | ||
|
|
||
| getUpdatesStaging(): Uint32Array { | ||
| this.updatesStaging ??= new Uint32Array(this.updatesCapacity * 2); | ||
| return this.updatesStaging; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard capacity before computing power-of-two.
A negative or non-finite capacity can lead to NaN sizes. Clamp like the other ensure* methods.
🛠️ Proposed fix
- this.updatesCapacity = Math.max(
- 256,
- Math.pow(2, Math.ceil(Math.log2(capacity))),
- );
+ const safeCapacity = Math.max(1, capacity);
+ this.updatesCapacity = Math.max(
+ 256,
+ Math.pow(2, Math.ceil(Math.log2(safeCapacity))),
+ );📝 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.
Suggested change
| ensureUpdatesBuffer(capacity: number): GPUBuffer { | |
| if (this.updatesBuffer && capacity <= this.updatesCapacity) { | |
| return this.updatesBuffer; | |
| } | |
| const GPUBufferUsage = (globalThis as any).GPUBufferUsage; | |
| const STORAGE = GPUBufferUsage?.STORAGE ?? 0x10; | |
| const COPY_DST_BUF = GPUBufferUsage?.COPY_DST ?? 0x8; | |
| this.updatesCapacity = Math.max( | |
| 256, | |
| Math.pow(2, Math.ceil(Math.log2(capacity))), | |
| ); | |
| const bufferSize = this.updatesCapacity * 8; // Each update is 8 bytes | |
| if (this.updatesBuffer) { | |
| (this.updatesBuffer as any).destroy?.(); | |
| } | |
| const buffer = this.device.createBuffer({ | |
| size: bufferSize, | |
| usage: STORAGE | COPY_DST_BUF, | |
| }); | |
| (this as any).updatesBuffer = buffer; | |
| this.updatesStaging = new Uint32Array(this.updatesCapacity * 2); | |
| return buffer; | |
| } | |
| getUpdatesStaging(): Uint32Array { | |
| this.updatesStaging ??= new Uint32Array(this.updatesCapacity * 2); | |
| return this.updatesStaging; | |
| } | |
| ensureUpdatesBuffer(capacity: number): GPUBuffer { | |
| if (this.updatesBuffer && capacity <= this.updatesCapacity) { | |
| return this.updatesBuffer; | |
| } | |
| const GPUBufferUsage = (globalThis as any).GPUBufferUsage; | |
| const STORAGE = GPUBufferUsage?.STORAGE ?? 0x10; | |
| const COPY_DST_BUF = GPUBufferUsage?.COPY_DST ?? 0x8; | |
| const safeCapacity = Math.max(1, capacity); | |
| this.updatesCapacity = Math.max( | |
| 256, | |
| Math.pow(2, Math.ceil(Math.log2(safeCapacity))), | |
| ); | |
| const bufferSize = this.updatesCapacity * 8; // Each update is 8 bytes | |
| if (this.updatesBuffer) { | |
| (this.updatesBuffer as any).destroy?.(); | |
| } | |
| const buffer = this.device.createBuffer({ | |
| size: bufferSize, | |
| usage: STORAGE | COPY_DST_BUF, | |
| }); | |
| (this as any).updatesBuffer = buffer; | |
| this.updatesStaging = new Uint32Array(this.updatesCapacity * 2); | |
| return buffer; | |
| } | |
| getUpdatesStaging(): Uint32Array { | |
| this.updatesStaging ??= new Uint32Array(this.updatesCapacity * 2); | |
| return this.updatesStaging; | |
| } |
🤖 Prompt for AI Agents
In `@src/client/graphics/webgpu/core/GroundTruthData.ts` around lines 1310 - 1342,
The ensureUpdatesBuffer method can produce NaN sizes if capacity is non-finite
or negative; before computing the power-of-two, validate and clamp capacity
(e.g. if !Number.isFinite(capacity) or capacity < 0, set capacity to 0) and use
Math.max(1, capacity) when computing Math.log2 so the Math.pow/ceil path never
receives NaN/Infinity; update ensureUpdatesBuffer (and similarly
getUpdatesStaging usage of this.updatesCapacity) to use the clamped value so
updatesCapacity and bufferSize are always a finite integer.
Comment on lines
+420
to
+423
| markAllDirty(): void { | ||
| this.resources?.markDefensePostsDirty(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
markAllDirty should really mark all GPU state.
markAllDirty() only calls markDefensePostsDirty() (Line 420+). That does not force state/palette/defended‑strength recompute, so a “full refresh” can still render stale data. Consider aligning with the worker renderer’s full reset.
🛠️ Suggested fix
markAllDirty(): void {
- this.resources?.markDefensePostsDirty();
+ if (!this.resources) return;
+ this.resources.markStateDirty();
+ this.resources.markDefensePostsDirty();
+ this.resources.markDefendedFullRecompute();
+ this.resources.markPaletteDirty();
+ this.resources.invalidateHistory();
}🤖 Prompt for AI Agents
In `@src/client/graphics/webgpu/TerritoryRenderer.ts` around lines 420 - 423,
markAllDirty currently only calls this.resources?.markDefensePostsDirty(), which
leaves state, palette and defended-strength stale; update
TerritoryRenderer.markAllDirty to perform a full GPU-state reset by invoking the
corresponding resource invalidation methods on this.resources (e.g. call
this.resources?.markStateDirty(), this.resources?.markPaletteDirty(),
this.resources?.markDefendedStrengthDirty() in addition to
this.resources?.markDefensePostsDirty()), or if a worker-renderer style
fullReset/resetAll method exists on the resources object, call that instead so
state/palette/defended-strength recompute is forced.
Comment on lines
+369
to
+377
| setTerrainShader(shaderPath: string): void { | ||
| this.terrainShaderPath = shaderPath; | ||
| if (!this.terrainComputePass) { | ||
| return; | ||
| } | ||
| void this.terrainComputePass.setShader(shaderPath).then(() => { | ||
| this.refreshTerrain(); | ||
| }); | ||
| this.frameDirty = true; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/core/worker/WorkerTerritoryRenderer.ts | sed -n '360,385p'Repository: openfrontio/OpenFrontIO
Length of output: 1011
🏁 Script executed:
rg "terrainComputePass" src/core/worker/WorkerTerritoryRenderer.ts -B 2 -A 2 | head -40Repository: openfrontio/OpenFrontIO
Length of output: 1141
🏁 Script executed:
rg "\.then\(" src/core/worker/WorkerTerritoryRenderer.ts -B 1 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 478
Add error handling to the shader loading promise in setTerrainShader().
The promise from this.terrainComputePass.setShader(shaderPath) at line 374 lacks a .catch() handler. If shader loading fails, the rejection goes unhandled and this.refreshTerrain() won't execute, leaving the terrain in a bad state. Similar code elsewhere in this file includes error handling—apply the same pattern here.
Suggested fix
void this.terrainComputePass.setShader(shaderPath).then(() => {
this.refreshTerrain();
- });
+ }).catch((err) => {
+ console.error("Failed to load terrain shader:", err);
+ });📝 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.
Suggested change
| setTerrainShader(shaderPath: string): void { | |
| this.terrainShaderPath = shaderPath; | |
| if (!this.terrainComputePass) { | |
| return; | |
| } | |
| void this.terrainComputePass.setShader(shaderPath).then(() => { | |
| this.refreshTerrain(); | |
| }); | |
| this.frameDirty = true; | |
| setTerrainShader(shaderPath: string): void { | |
| this.terrainShaderPath = shaderPath; | |
| if (!this.terrainComputePass) { | |
| return; | |
| } | |
| void this.terrainComputePass.setShader(shaderPath).then(() => { | |
| this.refreshTerrain(); | |
| }).catch((err) => { | |
| console.error("Failed to load terrain shader:", err); | |
| }); | |
| this.frameDirty = true; |
🤖 Prompt for AI Agents
In `@src/core/worker/WorkerTerritoryRenderer.ts` around lines 369 - 377, In
setTerrainShader, the promise returned by
this.terrainComputePass.setShader(shaderPath) needs proper error handling:
update the call in setTerrainShader so its then() continues to call
this.refreshTerrain() on success and add a .catch(err => ...) that logs or
handles the error (and still sets this.frameDirty appropriately) following the
same pattern used elsewhere in this file for shader loading; reference the
methods terrainComputePass.setShader, refreshTerrain, and the frameDirty flag
when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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