perf: improve performance overlay profiling#3271
Closed
scamiv wants to merge 9 commits intoopenfrontio:mainfrom
Closed
perf: improve performance overlay profiling#3271scamiv wants to merge 9 commits intoopenfrontio:mainfrom
scamiv wants to merge 9 commits intoopenfrontio:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-frame and per-tick profiling: GameRenderer records per-layer render and tick durations (using stable profile labels), aggregates frame counts, and forwards metrics to PerformanceOverlay. PerformanceOverlay maintains EMA-smoothed stats, TPS, last-tick summaries, expanded UI sections, and new public update APIs. Adds Layer.profileName, localization keys, and user-settings change events. Changes
Sequence DiagramsequenceDiagram
participant GameLoop as Game Loop
participant Renderer as GameRenderer
participant Overlay as PerformanceOverlay
participant UI as Display/UI
GameLoop->>Renderer: renderGame()
activate Renderer
Renderer->>Renderer: measure per-layer render times
Renderer->>Renderer: accumulate frame count & per-layer durations
Renderer->>Overlay: updateRenderPerTickMetrics(frames, layerDurations)
deactivate Renderer
activate Overlay
Overlay->>Overlay: update EMA render stats, renderPerTickLast, TPS
Overlay->>UI: requestUpdate / render UI
deactivate Overlay
GameLoop->>Renderer: tick()
activate Renderer
Renderer->>Renderer: measure per-layer tick times
Renderer->>Overlay: updateTickLayerMetrics(tickLayerDurations)
deactivate Renderer
activate Overlay
Overlay->>Overlay: update EMA tick stats and build sorted breakdown
Overlay->>UI: render() with updated render/tick/TPS metrics
deactivate Overlay
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/client/graphics/layers/PerformanceOverlay.ts (3)
681-682: Magic numbers — consider named constants.
10(max layers to show) and0.01(inactive threshold on lines 760, 797) are used as bare literals. Small named constants at the top of the class would make intent clearer:private static readonly MAX_VISIBLE_LAYERS = 10; private static readonly INACTIVE_THRESHOLD_MS = 0.01;This is a small readability nit, not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 681 - 682, Introduce named constants for the magic numbers used in PerformanceOverlay: add private static readonly MAX_VISIBLE_LAYERS = 10 and private static readonly INACTIVE_THRESHOLD_MS = 0.01 at the top of the PerformanceOverlay class, then replace the literal 10 in the computations that create renderLayersToShow and tickLayersToShow (which use layerBreakdown.slice and tickLayerBreakdown.slice) with MAX_VISIBLE_LAYERS, and replace the 0.01 inactive threshold literals used elsewhere (around the logic that filters/marks inactive layers) with INACTIVE_THRESHOLD_MS to make intent clearer.
80-116: Repeated stats shape — extract a shared type alias.The type
{ avg: number; max: number; last: number; total: number }appears three times (layerStats,tickLayerStats,renderPerTickLayerStats). A simple type alias reduces duplication and makes future changes easier:type LayerTimingStats = { avg: number; max: number; last: number; total: number };Then use
Map<string, LayerTimingStats>for all three.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 80 - 116, Extract a shared type alias like LayerTimingStats = { avg: number; max: number; last: number; total: number } and replace the inline object type usages with it (e.g. change tickLayerStats: Map<string, {...}> to tickLayerStats: Map<string, LayerTimingStats>, renderPerTickLayerStats: Map<string, LayerTimingStats>, and any other declarations using the same shape such as layerStats or similar). Ensure imports/exports or local scope is adjusted so the alias is visible where those symbols (tickLayerStats, renderPerTickLayerStats, layerStats) are declared and update any type annotations that reference the duplicated shape to use LayerTimingStats.
469-547: Three near-identical EMA update methods — extract a shared helper.
updateLayerStats(line 436),updateRenderPerTickMetrics(line 469), andupdateTickLayerMetrics(line 508) all do the same EMA+max+total calculation on aMap<string, Stats>. The only differences are which map they write to and the extra bookkeeping (breakdown derivation, last-tick snapshots).A small private helper would remove this duplication:
♻️ Suggested helper
+ private updateStatsMap( + statsMap: Map<string, LayerTimingStats>, + durations: Record<string, number>, + alpha: number = 0.2, + ): void { + for (const [name, duration] of Object.entries(durations)) { + const existing = statsMap.get(name); + if (!existing) { + statsMap.set(name, { avg: duration, max: duration, last: duration, total: duration }); + } else { + const avg = existing.avg + alpha * (duration - existing.avg); + const max = Math.max(existing.max, duration); + const total = existing.total + duration; + statsMap.set(name, { avg, max, last: duration, total }); + } + } + }Then each caller becomes a thin wrapper that calls
this.updateStatsMap(...)and does its specific bookkeeping.Also note the inconsistent iteration style:
updateRenderPerTickMetricsusesfor...ofwhileupdateTickLayerMetricsuses.forEach(). Pick one for consistency —for...ofis generally cleaner in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 469 - 547, The three methods updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics duplicate the EMA/max/total logic; extract a private helper (e.g. updateStatsMap(statsMap: Map<string,Stats>, durations: Record<string,number>, alpha = 0.2)) that performs the shared loop and updates avg/max/last/total for each name, then have each caller do only its snapshot/breakdown bookkeeping (renderPerTickLayerStats, tickLayerStats, renderLastTickLayerDurations, tickLayerLastDurations, tickLayerBreakdown, etc.) and call updateStatsMap; also standardize on for...of iteration inside the helper for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 466-468: getProfileLabel(layer) is being called every frame even
when profiling is disabled; change the sequence around
FrameProfiler.start()/end() so you only compute the label and call
FrameProfiler.end when profiling is active: call FrameProfiler.start() into
layerStart, invoke layer.renderLayer?.(this.context) as before, then only if
layerStart is truthy call FrameProfiler.end(getProfileLabel(layer), layerStart).
This avoids eager string allocation when profiling is off and references
getProfileLabel, FrameProfiler.start, FrameProfiler.end, and layer.renderLayer.
---
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 681-682: Introduce named constants for the magic numbers used in
PerformanceOverlay: add private static readonly MAX_VISIBLE_LAYERS = 10 and
private static readonly INACTIVE_THRESHOLD_MS = 0.01 at the top of the
PerformanceOverlay class, then replace the literal 10 in the computations that
create renderLayersToShow and tickLayersToShow (which use layerBreakdown.slice
and tickLayerBreakdown.slice) with MAX_VISIBLE_LAYERS, and replace the 0.01
inactive threshold literals used elsewhere (around the logic that filters/marks
inactive layers) with INACTIVE_THRESHOLD_MS to make intent clearer.
- Around line 80-116: Extract a shared type alias like LayerTimingStats = { avg:
number; max: number; last: number; total: number } and replace the inline object
type usages with it (e.g. change tickLayerStats: Map<string, {...}> to
tickLayerStats: Map<string, LayerTimingStats>, renderPerTickLayerStats:
Map<string, LayerTimingStats>, and any other declarations using the same shape
such as layerStats or similar). Ensure imports/exports or local scope is
adjusted so the alias is visible where those symbols (tickLayerStats,
renderPerTickLayerStats, layerStats) are declared and update any type
annotations that reference the duplicated shape to use LayerTimingStats.
- Around line 469-547: The three methods updateLayerStats,
updateRenderPerTickMetrics, and updateTickLayerMetrics duplicate the
EMA/max/total logic; extract a private helper (e.g. updateStatsMap(statsMap:
Map<string,Stats>, durations: Record<string,number>, alpha = 0.2)) that performs
the shared loop and updates avg/max/last/total for each name, then have each
caller do only its snapshot/breakdown bookkeeping (renderPerTickLayerStats,
tickLayerStats, renderLastTickLayerDurations, tickLayerLastDurations,
tickLayerBreakdown, etc.) and call updateStatsMap; also standardize on for...of
iteration inside the helper for consistency.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
scamiv
force-pushed
the
perf-overlay-rework
branch
from
63effdf to
3acb624
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/graphics/GameRenderer.ts (2)
526-532:tickStart !== 0check is redundant withshouldProfileTick.Because
tickStart = shouldProfileTick ? performance.now() : 0, the value is non-zero exactly whenshouldProfileTickis true. The extra!== 0condition adds confusion without adding safety.♻️ Proposed simplification
- const tickStart = shouldProfileTick ? performance.now() : 0; + const tickStart = shouldProfileTick ? performance.now() : 0; // keep for one-liner layer.tick(); - if (shouldProfileTick && tickStart !== 0) { + if (shouldProfileTick) { const duration = performance.now() - tickStart; const label = getProfileLabel(layer); tickLayerDurations[label] = (tickLayerDurations[label] ?? 0) + duration; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/GameRenderer.ts` around lines 526 - 532, The redundant check can be removed: because tickStart is set via "const tickStart = shouldProfileTick ? performance.now() : 0", the boolean shouldProfileTick already indicates whether profiling ran, so replace the conditional that checks "shouldProfileTick && tickStart !== 0" with just "shouldProfileTick". Update the block around layer.tick() and the profiling logic (symbols: shouldProfileTick, tickStart, layer.tick(), getProfileLabel, tickLayerDurations) to use shouldProfileTick alone when computing duration and updating tickLayerDurations.
54-61:getProfileLabel: HTMLElement branch adds unexpected label noise and uses unnecessary optional chain.Two issues:
layer.tagNameis always defined onHTMLElement—?.is not needed.- All layers already have a
profileNameset, so the branch always produces"EventsDisplay (events-display)"instead of the clean"EventsDisplay". This makes labels for Web Component layers visually inconsistent with canvas-only layers in the overlay, and produces longer-than-necessary strings as dictionary keys.Since
profileNameis now the stable identifier by design, the HTMLElement tag-name suffix no longer adds useful information.♻️ Proposed simplification
function getProfileLabel(layer: Layer): string { - const base = layer.profileName ?? "UnknownLayer"; - if (layer instanceof HTMLElement) { - const tag = layer.tagName?.toLowerCase(); - if (tag) return `${base} (${tag})`; - } - return base; + return layer.profileName ?? "UnknownLayer"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/GameRenderer.ts` around lines 54 - 61, getProfileLabel currently appends the HTMLElement tagName (using an unnecessary optional chain) which creates noisy labels; change getProfileLabel to simply return layer.profileName ?? "UnknownLayer" (i.e., remove the HTMLElement branch and the use of tagName and its optional chaining) so profileName is the stable identifier and labels remain consistent across layers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 466-468: The profiling label is being computed unconditionally
each frame because getProfileLabel(layer) is always called even when profiling
is disabled; change the rendering block to only call FrameProfiler.start(),
compute getProfileLabel(layer), and call FrameProfiler.end(...) when profiling
is active (i.e., guard both the start and end calls with FrameProfiler.isEnabled
or equivalent), otherwise call layer.renderLayer(this.context) directly without
evaluating getProfileLabel; update the code paths around FrameProfiler.start,
FrameProfiler.end, getProfileLabel, and layer.renderLayer to ensure
getProfileLabel is only invoked when profiling is enabled.
---
Nitpick comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 526-532: The redundant check can be removed: because tickStart is
set via "const tickStart = shouldProfileTick ? performance.now() : 0", the
boolean shouldProfileTick already indicates whether profiling ran, so replace
the conditional that checks "shouldProfileTick && tickStart !== 0" with just
"shouldProfileTick". Update the block around layer.tick() and the profiling
logic (symbols: shouldProfileTick, tickStart, layer.tick(), getProfileLabel,
tickLayerDurations) to use shouldProfileTick alone when computing duration and
updating tickLayerDurations.
- Around line 54-61: getProfileLabel currently appends the HTMLElement tagName
(using an unnecessary optional chain) which creates noisy labels; change
getProfileLabel to simply return layer.profileName ?? "UnknownLayer" (i.e.,
remove the HTMLElement branch and the use of tagName and its optional chaining)
so profileName is the stable identifier and labels remain consistent across
layers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/PerformanceOverlay.ts (1)
444-555: 🛠️ Refactor suggestion | 🟠 MajorThree copies of the same EMA-update-and-sort logic — extract a helper.
updateLayerStats,updateRenderPerTickMetrics, andupdateTickLayerMetricseach repeat the identical pattern: iterate entries → upsert{ avg, max, last, total }withalpha = 0.2→ optionally sort into a breakdown array. A single shared helper keeps the smoothing logic in one place and cuts ~50 lines.Proposed helper
// At class level or as a module-level pure function: private static readonly EMA_ALPHA = 0.2; private updateEmaStats( statsMap: Map<string, { avg: number; max: number; last: number; total: number }>, durations: Record<string, number>, ): void { const alpha = PerformanceOverlay.EMA_ALPHA; for (const [name, duration] of Object.entries(durations)) { const existing = statsMap.get(name); if (!existing) { statsMap.set(name, { avg: duration, max: duration, last: duration, total: duration }); } else { statsMap.set(name, { avg: existing.avg + alpha * (duration - existing.avg), max: Math.max(existing.max, duration), last: duration, total: existing.total + duration, }); } } } private deriveBreakdown( statsMap: Map<string, { avg: number; max: number; last: number; total: number }>, ): { name: string; avg: number; max: number; total: number }[] { return Array.from(statsMap.entries()) .map(([name, s]) => ({ name, avg: s.avg, max: s.max, total: s.total })) .sort((a, b) => b.total - a.total); }Then each call site collapses to:
// updateLayerStats this.updateEmaStats(this.layerStats, layerDurations); this.layerBreakdown = this.deriveBreakdown(this.layerStats); // updateTickLayerMetrics this.updateEmaStats(this.tickLayerStats, tickLayerDurations); this.tickLayerBreakdown = this.deriveBreakdown(this.tickLayerStats); // updateRenderPerTickMetrics this.updateEmaStats(this.renderPerTickLayerStats, layerDurations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 444 - 555, The three methods updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics duplicate the same EMA upsert and breakdown logic; extract a shared helper (e.g. add a private static EMA_ALPHA = 0.2 and a private updateEmaStats(statsMap, durations) that performs the upsert of {avg, max, last, total}) and a deriveBreakdown(statsMap) that returns the sorted array of {name, avg, max, total}; then replace the duplicated loops in updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics with calls to updateEmaStats(this.layerStats or this.renderPerTickLayerStats or this.tickLayerStats, durations) and assign breakdowns via deriveBreakdown where needed (this.layerBreakdown, this.tickLayerBreakdown).
🧹 Nitpick comments (2)
src/client/graphics/layers/PerformanceOverlay.ts (2)
94-116: Several@state()fields are never the sole trigger of a re-render — plain properties suffice.
tickLayerLastCount,tickLayerLastTotalMs,tickLayerLastDurations,renderLastTickFrameCount,renderLastTickLayerTotalMs, andrenderLastTickLayerDurationsare always written in the same synchronous call that also updates a@state()breakdown array (which already schedules the update). Making them plainprivatefields avoids redundant Lit dirty-check overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 94 - 116, The six fields tickLayerLastCount, tickLayerLastTotalMs, tickLayerLastDurations, renderLastTickFrameCount, renderLastTickLayerTotalMs, and renderLastTickLayerDurations are marked with `@state`() but are always updated alongside the stateful breakdown arrays, causing redundant re-render triggers; remove the `@state`() decorator from these properties and make them plain private fields (keep their current names and types) so only the actual breakdown state triggers Lit updates, while leaving renderPerTickLayerStats and any other true reactive fields unchanged.
769-829: Render-layer and tick-layer row templates are near-identical — consider a shared row helper.Both blocks build the same
.layer-rowstructure (width calc, inactive check, name span, metrics span). A small private method likerenderLayerRow(layer, maxAvg, lastDurations, extraMetricFn?)would remove the duplication and make future styling changes safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 769 - 829, Create a small private helper on the PerformanceOverlay class (e.g., renderLayerRow(layer, maxAvg, lastDurations?, extraMetricStringFn?)) that encapsulates the repeated .layer-row markup and calculations (width = Math.min(100, (layer.avg / maxAvg) * 100 || 0), isInactive check, title, layer-name span and layer-metrics span). Use that helper in both places: for renderLayersToShow pass maxLayerAvg, this.renderLastTickLayerDurations and a function to produce the extra per-tick metric string (using this.renderPerTickLayerStats.get(layer.name)?.avg), and for tickLayersToShow pass maxTickLayerAvg and this.tickLayerLastDurations and no extraMetricStringFn (or one that returns empty). Ensure the helper reads the same field names (renderLastTickLayerDurations, renderPerTickLayerStats, tickLayerLastDurations) and returns the html`<div ...>` fragment so both map() blocks become a single call to renderLayerRow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 688-691: The Y clamp in render() is weaker than in
handleMouseMove: update the clamping logic around clampedY in render() (where it
currently uses Math.min(this.position.y, viewportHeight)) to use the same lower
bound as handleMouseMove (use viewportHeight - 100) so the overlay cannot be
positioned entirely off-screen; adjust the expression that computes clampedY
(referencing this.position.y, viewportHeight, margin) to mirror the
handleMouseMove clamp.
- Around line 314-325: The overlay width constant is inconsistent (CSS uses
460px while JS uses 420), causing the overlay to escape the viewport; update the
JS to use a single source of truth by changing the overlay width clamp in both
handleMouseMove (where overlayWidth is computed) and render (the corresponding
Math.min(...) there) to match the CSS value (460) or extract a shared constant
(e.g., OVERLAY_MAX_WIDTH) used by both functions and by render so the clamp in
this.position and the render sizing use the same value.
---
Outside diff comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 444-555: The three methods updateLayerStats,
updateRenderPerTickMetrics, and updateTickLayerMetrics duplicate the same EMA
upsert and breakdown logic; extract a shared helper (e.g. add a private static
EMA_ALPHA = 0.2 and a private updateEmaStats(statsMap, durations) that performs
the upsert of {avg, max, last, total}) and a deriveBreakdown(statsMap) that
returns the sorted array of {name, avg, max, total}; then replace the duplicated
loops in updateLayerStats, updateRenderPerTickMetrics, and
updateTickLayerMetrics with calls to updateEmaStats(this.layerStats or
this.renderPerTickLayerStats or this.tickLayerStats, durations) and assign
breakdowns via deriveBreakdown where needed (this.layerBreakdown,
this.tickLayerBreakdown).
---
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 94-116: The six fields tickLayerLastCount, tickLayerLastTotalMs,
tickLayerLastDurations, renderLastTickFrameCount, renderLastTickLayerTotalMs,
and renderLastTickLayerDurations are marked with `@state`() but are always updated
alongside the stateful breakdown arrays, causing redundant re-render triggers;
remove the `@state`() decorator from these properties and make them plain private
fields (keep their current names and types) so only the actual breakdown state
triggers Lit updates, while leaving renderPerTickLayerStats and any other true
reactive fields unchanged.
- Around line 769-829: Create a small private helper on the PerformanceOverlay
class (e.g., renderLayerRow(layer, maxAvg, lastDurations?,
extraMetricStringFn?)) that encapsulates the repeated .layer-row markup and
calculations (width = Math.min(100, (layer.avg / maxAvg) * 100 || 0), isInactive
check, title, layer-name span and layer-metrics span). Use that helper in both
places: for renderLayersToShow pass maxLayerAvg,
this.renderLastTickLayerDurations and a function to produce the extra per-tick
metric string (using this.renderPerTickLayerStats.get(layer.name)?.avg), and for
tickLayersToShow pass maxTickLayerAvg and this.tickLayerLastDurations and no
extraMetricStringFn (or one that returns empty). Ensure the helper reads the
same field names (renderLastTickLayerDurations, renderPerTickLayerStats,
tickLayerLastDurations) and returns the html`<div ...>` fragment so both map()
blocks become a single call to renderLayerRow.
Comment on lines
314
to
325
| const margin = 8; | ||
| const viewportWidth = window.innerWidth; | ||
| const viewportHeight = window.innerHeight; | ||
| const overlayWidth = Math.min(420, Math.max(0, viewportWidth - margin * 2)); | ||
|
|
||
| this.position = { | ||
| x: Math.max(0, Math.min(viewportWidth - 100, newX)), // Keep within viewport bounds | ||
| y: Math.max(0, Math.min(viewportHeight - 100, newY)), | ||
| x: Math.max( | ||
| margin, | ||
| Math.min(viewportWidth - overlayWidth - margin, newX), | ||
| ), | ||
| y: Math.max(margin, Math.min(viewportHeight - 100, newY)), | ||
| }; |
Contributor
There was a problem hiding this comment.
Overlay width constant mismatch: CSS says 460 px, JS says 420 px.
The CSS at line 135 sets width: min(460px, …) but both handleMouseMove (line 317) and render() (line 685) use Math.min(420, …) for clamping. This 40 px gap lets the overlay's right edge escape the viewport when dragged.
Pick one source of truth (ideally a shared constant) and use it everywhere.
Proposed fix
+ // Must match the CSS `width: min(…)` value
+ private static readonly OVERLAY_MAX_WIDTH = 460;
+
private handleMouseMove = (e: MouseEvent) => {
if (!this.isDragging) return;
const newX = e.clientX - this.dragStart.x;
const newY = e.clientY - this.dragStart.y;
const margin = 8;
const viewportWidth = window.innerWidth;
const viewportHeight = window.innerHeight;
- const overlayWidth = Math.min(420, Math.max(0, viewportWidth - margin * 2));
+ const overlayWidth = Math.min(
+ PerformanceOverlay.OVERLAY_MAX_WIDTH,
+ Math.max(0, viewportWidth - margin * 2),
+ );Apply the same constant in render():
- const overlayWidth = Math.min(420, Math.max(0, viewportWidth - margin * 2));
+ const overlayWidth = Math.min(
+ PerformanceOverlay.OVERLAY_MAX_WIDTH,
+ Math.max(0, viewportWidth - margin * 2),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 314 - 325, The
overlay width constant is inconsistent (CSS uses 460px while JS uses 420),
causing the overlay to escape the viewport; update the JS to use a single source
of truth by changing the overlay width clamp in both handleMouseMove (where
overlayWidth is computed) and render (the corresponding Math.min(...) there) to
match the CSS value (460) or extract a shared constant (e.g., OVERLAY_MAX_WIDTH)
used by both functions and by render so the clamp in this.position and the
render sizing use the same value.
Comment on lines
+688
to
+691
| const clampedY = Math.max( | ||
| margin, | ||
| Math.min(this.position.y, viewportHeight), | ||
| ); |
Contributor
There was a problem hiding this comment.
Y-axis clamp in render() is weaker than in handleMouseMove.
handleMouseMove clamps Y to viewportHeight - 100, but here in render() you clamp to viewportHeight — that allows the overlay to sit entirely off the bottom edge. Use the same lower bound to keep the overlay reachable.
Proposed fix
const clampedY = Math.max(
margin,
- Math.min(this.position.y, viewportHeight),
+ Math.min(this.position.y, viewportHeight - 100),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 688 - 691, The
Y clamp in render() is weaker than in handleMouseMove: update the clamping logic
around clampedY in render() (where it currently uses Math.min(this.position.y,
viewportHeight)) to use the same lower bound as handleMouseMove (use
viewportHeight - 100) so the overlay cannot be positioned entirely off-screen;
adjust the expression that computes clampedY (referencing this.position.y,
viewportHeight, margin) to mirror the handleMouseMove clamp.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/client/graphics/GameRenderer.ts (1)
49-52:namedLayermutates its argument — consider a non-mutating approach.Setting
layer.profileNamedirectly is a side-effect on the passed object. It works here because all layers are freshly created increateRenderer, but it's easy to pass a shared reference and inadvertently overwrite a name already set by the layer itself viaprofileName?: string.♻️ Proposed non-mutating alternative
Since
profileNameis optional on theLayerinterface, the caller can just set it inline:-function namedLayer<T extends Layer>(layer: T, profileName: string): T { - layer.profileName = profileName; - return layer; -}And at each call site:
- namedLayer(new TerrainLayer(game, transformHandler), "TerrainLayer"), + Object.assign(new TerrainLayer(game, transformHandler), { profileName: "TerrainLayer" }),Or, more idiomatically, simply set it after construction since it is already a public property on the
Layerinterface:const terrainLayer = new TerrainLayer(game, transformHandler); terrainLayer.profileName = "TerrainLayer";Either way, the key improvement is that
namedLayerbecomes unnecessary and any layer that already declares its ownprofileName(e.g., one that sets it in its constructor) won't be silently overwritten.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/GameRenderer.ts` around lines 49 - 52, The helper namedLayer mutates its input by assigning layer.profileName which can overwrite an existing value; change call sites to stop using namedLayer and instead set the profileName non-mutatively (e.g., construct the Layer via its constructor and then assign profileName on the returned instance at the call site or let the Layer set profileName in its constructor), and remove or deprecate the namedLayer function; update references in createRenderer and any other callers to use the inline assignment or constructor-initialized profileName so no shared object is mutated unexpectedly.src/core/game/UserSettings.ts (1)
7-19: Extract"user-settings-changed"as a shared constant to prevent silent drift.The event name is a string literal in both
emitChangehere and in theglobalThis.addEventListenercall inPerformanceOverlay.ts. If one is updated without the other, the event silently stops working.♻️ Proposed fix — shared constant
Add to a shared location (e.g.,
src/core/game/UserSettings.tsitself, or a newsrc/core/Events.ts):+export const USER_SETTINGS_CHANGED_EVENT = "user-settings-changed";Then use it in both files:
- new CustomEvent("user-settings-changed", { + new CustomEvent(USER_SETTINGS_CHANGED_EVENT, {- globalThis.addEventListener("user-settings-changed", (event: Event) => { + globalThis.addEventListener(USER_SETTINGS_CHANGED_EVENT, (event: Event) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 7 - 19, Extract the "user-settings-changed" literal into a shared constant (e.g., export const USER_SETTINGS_CHANGED = "user-settings-changed") and replace the string literal in the emitChange method of UserSettings (emitChange) with that constant; then import and use the same constant in PerformanceOverlay where it currently calls globalThis.addEventListener("user-settings-changed", ...) so both the dispatch and listener reference the identical exported symbol (place the constant either at the top of src/core/game/UserSettings.ts or in a new shared module like src/core/Events.ts and update imports accordingly).src/client/graphics/layers/PerformanceOverlay.ts (2)
301-305:requestUpdate()is redundant —isVisibleis already@state().In Lit, setting a
@state()property automatically schedules a re-render. The explicitthis.requestUpdate()call is a no-op.♻️ Proposed cleanup
setVisible(visible: boolean) { this.isVisible = visible; FrameProfiler.setEnabled(visible); - this.requestUpdate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 301 - 305, The setVisible method in PerformanceOverlay sets the `@state`() property isVisible and then calls this.requestUpdate(), which is redundant; remove the explicit this.requestUpdate() call from the setVisible(visible: boolean) method and keep only this.isVisible = visible and FrameProfiler.setEnabled(visible) so Lit will automatically schedule the update for the PerformanceOverlay component.
810-817:averageTPSis not color-coded, unlikeaverageFPSandcurrentTPS.
currentTPSat line 812 getsgetTPSColor, andaverageFPSat line 800 getsgetPerformanceColor.averageTPSis rendered as a plain<span>with no class, making it visually inconsistent.♻️ Proposed fix
- <span>${this.averageTPS}</span>) + <span class="${this.getTPSColor(this.averageTPS)}">${this.averageTPS}</span>)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 810 - 817, The averageTPS value is not color-coded like the other metrics; add the same color logic used for currentTPS to the averageTPS span by applying the class produced by getTPSColor( this.averageTPS ) so the <span> for averageTPS uses the TPS color styling (mirror how currentTPS uses getTPSColor); update the span around averageTPS to include class="${this.getTPSColor(this.averageTPS)}" in the PerformanceOverlay render where averageTPS is output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 288-298: The event listener added with
globalThis.addEventListener("user-settings-changed", ...) in init() is never
removed, causing a lifecycle leak; store the listener callback reference (the
function wrapping the CustomEvent handling), attach it via
globalThis.addEventListener in init(), and call globalThis.removeEventListener
with that same reference in disconnectedCallback (or when tearing down) so the
overlay stops receiving events after disconnect; optionally guard init() to
avoid re-adding if a stored listener already exists. Ensure references to
setVisible and isVisible remain unchanged and use the same callback identity
when removing.
---
Duplicate comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 341-352: The JS hardcoded max width (overlayWidth uses 420) is out
of sync with the CSS (460); update the implementation that computes overlayWidth
(the overlayWidth const and any usages of margin/newX/newY in
PerformanceOverlay) so the max value is sourced from a single shared constant or
CSS variable (e.g., OVERLAY_MAX_WIDTH or a --overlay-max-width CSS custom
property) and make the CSS use the same value; either create and import/export a
module-level OVERLAY_MAX_WIDTH used by PerformanceOverlay and the stylesheet, or
read the CSS variable via getComputedStyle and parse it before computing
overlayWidth so both JS and CSS use the identical max width.
- Around line 747-753: The overlay sizing/clamping is inconsistent: change the
fixed max width from 420 to 460 and make the vertical clamp use viewportHeight -
100; specifically update the overlayWidth calculation to use 460 (const
overlayWidth = Math.min(460, Math.max(0, viewportWidth - margin * 2))) and
update clampedY so it clamps this.position.y between margin and viewportHeight -
100 (Math.max(margin, Math.min(this.position.y, viewportHeight - 100))); leave
the existing maxLeft/clampedX logic that uses overlayWidth, margin and
viewportWidth unchanged so horizontal clamping still respects the new
overlayWidth.
---
Nitpick comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 49-52: The helper namedLayer mutates its input by assigning
layer.profileName which can overwrite an existing value; change call sites to
stop using namedLayer and instead set the profileName non-mutatively (e.g.,
construct the Layer via its constructor and then assign profileName on the
returned instance at the call site or let the Layer set profileName in its
constructor), and remove or deprecate the namedLayer function; update references
in createRenderer and any other callers to use the inline assignment or
constructor-initialized profileName so no shared object is mutated unexpectedly.
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 301-305: The setVisible method in PerformanceOverlay sets the
`@state`() property isVisible and then calls this.requestUpdate(), which is
redundant; remove the explicit this.requestUpdate() call from the
setVisible(visible: boolean) method and keep only this.isVisible = visible and
FrameProfiler.setEnabled(visible) so Lit will automatically schedule the update
for the PerformanceOverlay component.
- Around line 810-817: The averageTPS value is not color-coded like the other
metrics; add the same color logic used for currentTPS to the averageTPS span by
applying the class produced by getTPSColor( this.averageTPS ) so the <span> for
averageTPS uses the TPS color styling (mirror how currentTPS uses getTPSColor);
update the span around averageTPS to include
class="${this.getTPSColor(this.averageTPS)}" in the PerformanceOverlay render
where averageTPS is output.
In `@src/core/game/UserSettings.ts`:
- Around line 7-19: Extract the "user-settings-changed" literal into a shared
constant (e.g., export const USER_SETTINGS_CHANGED = "user-settings-changed")
and replace the string literal in the emitChange method of UserSettings
(emitChange) with that constant; then import and use the same constant in
PerformanceOverlay where it currently calls
globalThis.addEventListener("user-settings-changed", ...) so both the dispatch
and listener reference the identical exported symbol (place the constant either
at the top of src/core/game/UserSettings.ts or in a new shared module like
src/core/Events.ts and update imports accordingly).
Comment on lines
+288
to
+298
| globalThis.addEventListener("user-settings-changed", (event: Event) => { | ||
| const customEvent = event as CustomEvent<{ | ||
| key?: string; | ||
| value?: unknown; | ||
| }>; | ||
| if (customEvent.detail?.key !== "settings.performanceOverlay") return; | ||
|
|
||
| const nextVisible = customEvent.detail.value === true; | ||
| if (this.isVisible === nextVisible) return; | ||
| this.setVisible(nextVisible); | ||
| }); |
Contributor
There was a problem hiding this comment.
globalThis.addEventListener listener is never removed — minor lifecycle leak.
The listener added in init() lives on globalThis for the entire page lifetime. In a dev-reload scenario (or if init is ever called more than once), listeners stack up and the overlay would respond multiple times per settings change.
Consider removing it in disconnectedCallback:
🛡️ Proposed fix
+ private onUserSettingsChanged = (event: Event) => {
+ const customEvent = event as CustomEvent<{ key?: string; value?: unknown }>;
+ if (customEvent.detail?.key !== "settings.performanceOverlay") return;
+ const nextVisible = customEvent.detail.value === true;
+ if (this.isVisible === nextVisible) return;
+ this.setVisible(nextVisible);
+ };
init() {
this.setVisible(this.userSettings.performanceOverlay());
// ...existing eventBus.on calls...
- globalThis.addEventListener("user-settings-changed", (event: Event) => {
- const customEvent = event as CustomEvent<{ key?: string; value?: unknown }>;
- if (customEvent.detail?.key !== "settings.performanceOverlay") return;
- const nextVisible = customEvent.detail.value === true;
- if (this.isVisible === nextVisible) return;
- this.setVisible(nextVisible);
- });
+ globalThis.addEventListener("user-settings-changed", this.onUserSettingsChanged);
}
+
+ disconnectedCallback() {
+ super.disconnectedCallback();
+ globalThis.removeEventListener("user-settings-changed", this.onUserSettingsChanged);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 288 - 298, The
event listener added with globalThis.addEventListener("user-settings-changed",
...) in init() is never removed, causing a lifecycle leak; store the listener
callback reference (the function wrapping the CustomEvent handling), attach it
via globalThis.addEventListener in init(), and call
globalThis.removeEventListener with that same reference in disconnectedCallback
(or when tearing down) so the overlay stops receiving events after disconnect;
optionally guard init() to avoid re-adding if a stored listener already exists.
Ensure references to setVisible and isVisible remain unchanged and use the same
callback identity when removing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/graphics/layers/PerformanceOverlay.ts (1)
519-630: Identical EMA update block is repeated three times — extract a shared helper.
updateLayerStats,updateRenderPerTickMetrics, andupdateTickLayerMetricsall contain the same get-or-create + EMA + max + total pattern. A small private helper removes the duplication:♻️ Proposed refactor
+ private static applyEMA( + stats: Map<string, { avg: number; max: number; last: number; total: number }>, + durations: Record<string, number>, + alpha = 0.2, + ): void { + for (const [name, duration] of Object.entries(durations)) { + const existing = stats.get(name); + if (!existing) { + stats.set(name, { avg: duration, max: duration, last: duration, total: duration }); + } else { + stats.set(name, { + avg: existing.avg + alpha * (duration - existing.avg), + max: Math.max(existing.max, duration), + last: duration, + total: existing.total + duration, + }); + } + } + }Then replace the three inline EMA blocks:
private updateLayerStats(layerDurations: Record<string, number>) { - const alpha = 0.2; - Object.entries(layerDurations).forEach(([name, duration]) => { - const existing = this.layerStats.get(name); - ... - }); + PerformanceOverlay.applyEMA(this.layerStats, layerDurations); const breakdown = Array.from(this.layerStats.entries()) .map(...) .sort(...); this.layerBreakdown = breakdown; }Apply the same replacement in
updateRenderPerTickMetricsandupdateTickLayerMetrics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 519 - 630, Extract the repeated EMA update logic into a private helper (e.g., updateStatsForLayer) and call it from updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics: the helper should accept the Map to update (this.layerStats, this.renderPerTickLayerStats, or this.tickLayerStats), the layer name, and the new duration, then perform the get-or-create + EMA avg, max, last, total update using the same alpha. Replace each inline block that reads/sets entry and computes avg/max/total with a call to this new helper so all three functions reuse the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/lang/en.json`:
- Around line 858-861: The two translation keys render_layers_summary and
tick_layers_summary use raw numeric placeholders that produce incorrect grammar
for singular values; update them to use ICU pluralization for their placeholders
(e.g., replace {frames} and {count} with ICU plural expressions like "{frames,
plural, one {# frame} other {# frames}}" and similarly for {count}) so the
strings render "1 frame" / "1 layer" when appropriate while preserving the ms
suffix and existing text.
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Line 891: Replace hardcoded English title strings in PerformanceOverlay with
translateText usage: add keys performance_overlay.collapse,
performance_overlay.expand, performance_overlay.last_tick_render, and
performance_overlay.last_tick to en.json, then update the title attributes in
the PerformanceOverlay component (the button title that depends on
this.renderLayersExpanded and the per-row title strings built around "last tick
render:" / "last tick:") to call translateText for those keys and interpolate
the dynamic values. Ensure you use the same translateText helper used elsewhere
in this component so tooltips are localized consistently.
---
Duplicate comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 378-392: The overlay width constant in PerformanceOverlay is still
using 420px causing layout mismatch; update the Math.min(420, …) usage in the
mouse-move positioning logic (PerformanceOverlay.handleMouseMove / position
calculation where overlayWidth is computed) to Math.min(460, …) and make the
identical change in render() where overlayWidth is computed so both JS
calculations match the CSS width: min(460px, …) value.
- Around line 798-807: In PerformanceOverlay.render update the clamping values
to match the CSS and handleMouseMove logic: change the hardcoded overlay max
width from 420 to 460 (affects overlayWidth calculation) and make the vertical
clamp use the same lower bound as handleMouseMove (use viewportHeight - 100
instead of viewportHeight) when computing clampedY so this.position.y cannot
slide the overlay fully off the bottom; adjust the expressions around
clampedX/clampedY and overlayWidth in render accordingly to keep consistency
with handleMouseMove and the CSS.
- Around line 324-334: Store the event handler as a named class field on
PerformanceOverlay (e.g., private settingsChangedListener = (event: Event) => {
... }) instead of an anonymous inline callback passed to
globalThis.addEventListener, register that field with
globalThis.addEventListener in the constructor/initializer, and remove it with
globalThis.removeEventListener(this.settingsChangedListener) at teardown/destroy
time (implement or extend the class's destroy/cleanup method). Keep the handler
logic unchanged (checking customEvent.detail.key ===
"settings.performanceOverlay", computing nextVisible, comparing this.isVisible
and calling this.setVisible(nextVisible)) so the only change is binding it to a
field and unregistering it in destroy to prevent lifecycle leaks.
---
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 519-630: Extract the repeated EMA update logic into a private
helper (e.g., updateStatsForLayer) and call it from updateLayerStats,
updateRenderPerTickMetrics, and updateTickLayerMetrics: the helper should accept
the Map to update (this.layerStats, this.renderPerTickLayerStats, or
this.tickLayerStats), the layer name, and the new duration, then perform the
get-or-create + EMA avg, max, last, total update using the same alpha. Replace
each inline block that reads/sets entry and computes avg/max/total with a call
to this new helper so all three functions reuse the same implementation.
Comment on lines
+858
to
+861
| "render_layers_summary": "Last tick: {frames} frames, {ms}ms", | ||
| "tick_layers_header": "Tick Layers", | ||
| "tick_layers_table_header": "avg / max", | ||
| "tick_layers_summary": "Last tick: {count} layers, {ms}ms" |
Contributor
There was a problem hiding this comment.
Missing plural forms for {frames} and {count}
At low frame rates (FPS near TPS) {frames} can be 1, rendering "Last tick: 1 frames, Xms". Similarly, {count} could be 1. The file already uses ICU plural syntax elsewhere (e.g., events_display.alliance_nukes_destroyed_incoming).
💬 Suggested plural forms
- "render_layers_summary": "Last tick: {frames} frames, {ms}ms",
+ "render_layers_summary": "Last tick: {frames, plural, one {# frame} other {# frames}}, {ms}ms",
- "tick_layers_summary": "Last tick: {count} layers, {ms}ms"
+ "tick_layers_summary": "Last tick: {count, plural, one {# layer} other {# layers}}, {ms}ms"📝 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
| "render_layers_summary": "Last tick: {frames} frames, {ms}ms", | |
| "tick_layers_header": "Tick Layers", | |
| "tick_layers_table_header": "avg / max", | |
| "tick_layers_summary": "Last tick: {count} layers, {ms}ms" | |
| "render_layers_summary": "Last tick: {frames, plural, one {# frame} other {# frames}}, {ms}ms", | |
| "tick_layers_header": "Tick Layers", | |
| "tick_layers_table_header": "avg / max", | |
| "tick_layers_summary": "Last tick: {count, plural, one {# layer} other {# layers}}, {ms}ms" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/en.json` around lines 858 - 861, The two translation keys
render_layers_summary and tick_layers_summary use raw numeric placeholders that
produce incorrect grammar for singular values; update them to use ICU
pluralization for their placeholders (e.g., replace {frames} and {count} with
ICU plural expressions like "{frames, plural, one {# frame} other {# frames}}"
and similarly for {count}) so the strings render "1 frame" / "1 layer" when
appropriate while preserving the ms suffix and existing text.
| <button | ||
| class="collapse-button" | ||
| @click=${this.toggleRenderLayersExpanded} | ||
| title=${this.renderLayersExpanded ? "Collapse" : "Expand"} |
Contributor
There was a problem hiding this comment.
Hardcoded English strings in title attributes bypass translateText.
Four places use raw English strings instead of translateText:
- Lines 891 and 952:
"Collapse"/"Expand"button tooltips. - Lines 921–923 and 980: the per-row
titleattributes embed"last tick render:"/"last tick:"literals.
These are tooltips surfaced to end users. Add keys like performance_overlay.collapse, performance_overlay.expand, performance_overlay.last_tick_render, and performance_overlay.last_tick to en.json, then use translateText to build these strings.
Also applies to: 921-923, 952-952, 980-980
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/PerformanceOverlay.ts` at line 891, Replace
hardcoded English title strings in PerformanceOverlay with translateText usage:
add keys performance_overlay.collapse, performance_overlay.expand,
performance_overlay.last_tick_render, and performance_overlay.last_tick to
en.json, then update the title attributes in the PerformanceOverlay component
(the button title that depends on this.renderLayersExpanded and the per-row
title strings built around "last tick render:" / "last tick:") to call
translateText for those keys and interpolate the dynamic values. Ensure you use
the same translateText helper used elsewhere in this component so tooltips are
localized consistently.
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