TEST PR Refactor/webgl border layer #2451
Conversation
- Introduced ToggleTerritoryWebGLDebugBordersEvent to manage debug border visibility. - Updated TerritoryLayer to handle debug border toggling. - Enhanced TerritoryBorderWebGL to support pulsing effect for debug borders. - Added UI button in TerritoryWebGLStatus for toggling debug borders.
- Added flags to BorderEdge interface to manage border states. - Updated TerritoryBorderWebGL to utilize flags for rendering logic. - Implemented border relation flags in WebGLBorderRenderer for color adjustments based on tile relationships. - Refactored PlayerView to centralize border relation flag logic into a new method.
… logging for capacity management
…erer and TerritoryLayer - Added FrameProfiler to GameRenderer for detailed layer rendering timings. - Updated PerformanceOverlay to display per-layer render metrics and added a reset button for performance stats. - Enhanced TerritoryLayer with FrameProfiler to measure rendering performance of territory and border rendering operations.
…tes in alternative view mode. Added logic to conditionally skip drawing the territory canvas when WebGL borders are active, improving performance during rendering.
…tionality - Removed unused distance calculation functions from PlayerInfoOverlay. - Integrated resolveHoverTarget for streamlined player and unit targeting in PlayerInfoOverlay. - Updated TerritoryLayer to utilize resolveHoverTarget for territory highlighting. - Enhanced hover highlight options configuration for WebGL border rendering, allowing for customizable hover effects.
WalkthroughAdds a WebGL territory border renderer with a canvas fallback, new input events and UI controls, per-layer frame profiling and performance breakdown, a hover-target resolver for units/players, and relation-aware border color logic; integrates these across renderer, territory layer, overlays, and user settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TerritoryWebGLStatus
participant EB as EventBus
participant TL as TerritoryLayer
participant BR as BorderRenderer
User->>UI: Click "Toggle WebGL"
UI->>EB: emit ToggleTerritoryWebGLEvent
EB->>TL: receive ToggleTerritoryWebGLEvent
TL->>TL: configureBorderRenderer()
alt WebGL enabled & supported
TL->>BR: instantiate WebGLBorderRenderer
TL->>BR: setHoverHighlightOptions()
else fallback
TL->>BR: use NullBorderRenderer
end
TL->>EB: emit TerritoryWebGLStatusEvent(enabled, active, supported, message?)
EB->>UI: receive status -> update UI
sequenceDiagram
participant GR as GameRenderer
participant PF as FrameProfiler
participant L as Layers
participant PO as PerformanceOverlay
GR->>PF: clear()
loop per layer
GR->>PF: start() -> token
GR->>L: render_layer()
L->>PF: end(name, token)
end
GR->>PF: consume() -> layerDurations
GR->>PO: updateFrameMetrics(frameDuration, layerDurations)
PO->>PO: updateLayerStats(layerDurations) (EMA, max, total)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Files/areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/client/graphics/FrameProfiler.ts (1)
1-43: Frame profiling logic is soundThe static helper covers the basic per-frame use case well (clear → record/start/end → consume) and is safe against bad duration values.
If you ever want stronger composition/testability, you could move this to a plain module-level helper (or injected profiler object) instead of a static class, but it is fine as-is.
src/client/graphics/layers/BorderRenderer.ts (1)
1-36: Align NullBorderRenderer method signatures with the interfaceThe interface shape looks good for swapping different border renderers. For
NullBorderRendererit would be clearer and more type-safe to explicitly match the parameter lists, instead of using bare() => {}methods.For example:
-export class NullBorderRenderer implements BorderRenderer { - drawsOwnBorders(): boolean { - return false; - } - - setAlternativeView() {} - - setHoveredPlayerId() {} - - updateBorder() {} - - clearTile() {} - - render() {} -} +export class NullBorderRenderer implements BorderRenderer { + drawsOwnBorders(): boolean { + return false; + } + + setAlternativeView(_enabled: boolean): void {} + + setHoveredPlayerId(_playerSmallId: number | null): void {} + + updateBorder( + _tile: TileRef, + _owner: PlayerView | null, + _isBorder: boolean, + _isDefended: boolean, + _hasFallout: boolean, + ): void {} + + clearTile(_tile: TileRef): void {} + + render(_context: CanvasRenderingContext2D): void {} +}This keeps the implementation self-documenting and robust if someone later narrows the interface types.
src/client/graphics/GameRenderer.ts (1)
361-364: Per-layer frame profiling is correct and low-riskThe render loop now:
- Clears
FrameProfileronce per frame.- Measures each layer’s
renderLayercall withperformance.now(), aggregating intolayerDurationskeyed by constructor name.- Consumes internal timings from
FrameProfiler.consume()and merges them into acombinedDurationsmap passed toperformanceOverlay.updateFrameMetrics.This keeps the main-frame duration unchanged while exposing richer timing data. Using constructor names as keys is fine for debugging overlays, even if they are minified in production.
If this grows, consider extracting the profiling logic into a small helper to keep
renderGamefocused on the draw pipeline.Also applies to: 396-403, 410-417
src/client/graphics/layers/PerformanceOverlay.ts (1)
216-235: Make drag suppression robust for clicks inside nested button contentRight now you only check
e.target.classList.contains("close-button" | "reset-button"). If the button ever gets nested content (e.g. an icon span),e.targetwill be that inner element and drag will still start.Consider using
closestso any click inside the buttons is ignored for dragging:- private handleMouseDown = (e: MouseEvent) => { - // Don't start dragging if clicking on close button - const target = e.target as HTMLElement; - if ( - target.classList.contains("close-button") || - target.classList.contains("reset-button") - ) { + private handleMouseDown = (e: MouseEvent) => { + // Don't start dragging if clicking on control buttons + const target = e.target as HTMLElement | null; + if ( + target && + target.closest(".close-button, .reset-button") + ) { return; }Also applies to: 457-458
src/client/graphics/layers/TerritoryLayer.ts (1)
359-405: Remove debug logging fromredraw()in production code
console.log("redrew territory layer");will fire every redraw (init, WebGL toggle, myPlayer change, etc.). This can spam the console and make it harder to see real issues.Unless you have a central logging abstraction, it’s better to remove this, or guard behind an explicit debug flag.
src/client/graphics/layers/TerritoryBorderWebGL.ts (3)
47-60: Tune initial capacity and clean up stale comment
INITIAL_CHUNK_CAPACITYis set to65536with a comment// 256;, and you allocate:this.vertexData = new Float32Array( TerritoryBorderWebGL.INITIAL_CHUNK_CAPACITY * TerritoryBorderWebGL.FLOATS_PER_TILE, );For smaller maps, this can pre‑allocate tens of MB of vertex data that’s never used. It also looks like the comment is out of date.
Consider using a more conservative default (or deriving it from map size) and updating or removing the comment to avoid confusion, for example:
- private static readonly INITIAL_CHUNK_CAPACITY = 65536; // 256; + // Initial number of tiles worth of border data to allocate; grows as needed. + private static readonly INITIAL_CHUNK_CAPACITY = 2048;(or another tuned value based on typical map sizes).
Also applies to: 117-121, 124-129, 738-778
85-95:needsRedrawflag is never cleared and can be simplified
needsRedrawis initialized totrueand only ever set totrueagain (setAlternativeView,setHoveredPlayerId,setHoverHighlightOptions,setDebugPulseEnabled,updateEdges,removeTileEdges,ensureCapacity, and at the end ofrender()), but never tofalse.That makes the early‑return guard:
if (!this.needsRedraw && this.vertexCount === 0) { return; }effectively dead code;
!this.needsRedrawcan never betrue.Either:
- Actually use
needsRedraw(set it tofalseafter a successful draw and only set it totrueon state changes), or- Drop the flag and just rely on
vertexCountif you’re happy to redraw every frame for animation.This will make the intent clearer and avoid misleading code paths.
Also applies to: 479-481, 554-616
121-129: Hide WebGL debug logging behind a debug flag (or remove)The debug
console.logcalls in the constructor andensureCapacity:console.log("[TerritoryBorderWebGL] initial capacityChunks=", ...); console.log("[TerritoryBorderWebGL] growing capacityChunks", ...);are useful while tuning, but will be noisy in real games and can impact performance in tight render loops.
Similarly, the shader/program compile/link error logs are fine, but capacity logs should be removed or guarded behind some
DEBUG/ feature flag so production builds don’t spam the console.Also applies to: 748-756, 798-813, 815-834
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/client/InputHandler.ts(1 hunks)src/client/graphics/FrameProfiler.ts(1 hunks)src/client/graphics/GameRenderer.ts(6 hunks)src/client/graphics/HoverTargetResolver.ts(1 hunks)src/client/graphics/layers/BorderRenderer.ts(1 hunks)src/client/graphics/layers/PerformanceOverlay.ts(8 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(2 hunks)src/client/graphics/layers/TerritoryBorderWebGL.ts(1 hunks)src/client/graphics/layers/TerritoryLayer.ts(16 hunks)src/client/graphics/layers/TerritoryWebGLStatus.ts(1 hunks)src/client/graphics/layers/WebGLBorderRenderer.ts(1 hunks)src/core/game/GameView.ts(4 hunks)src/core/game/UserSettings.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/BorderRenderer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/PerformanceOverlay.tssrc/client/graphics/layers/TerritoryWebGLStatus.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/WebGLBorderRenderer.tssrc/client/graphics/layers/TerritoryBorderWebGL.tssrc/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.tssrc/client/InputHandler.tssrc/core/game/GameView.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/TerritoryWebGLStatus.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/TerritoryBorderWebGL.tssrc/core/game/GameView.tssrc/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-06-06T15:36:31.739Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/Colors.ts:114-330
Timestamp: 2025-06-06T15:36:31.739Z
Learning: In the OpenFrontIO project, color improvements are implemented incrementally. The current focus for player colors is ensuring sufficient unique color availability rather than optimizing for visual distinction or accessibility features.
Applied to files:
src/core/game/GameView.ts
🧬 Code graph analysis (10)
src/client/graphics/layers/BorderRenderer.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/GameView.ts (1)
PlayerView(185-558)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
src/client/graphics/HoverTargetResolver.ts (1)
resolveHoverTarget(40-70)
src/client/graphics/layers/TerritoryWebGLStatus.ts (4)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/EventBus.ts (1)
EventBus(7-44)src/core/game/UserSettings.ts (1)
UserSettings(6-209)src/client/InputHandler.ts (3)
TerritoryWebGLStatusEvent(86-93)ToggleTerritoryWebGLEvent(84-84)ToggleTerritoryWebGLDebugBordersEvent(95-97)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/FrameProfiler.ts (2)
FrameProfiler(1-43)start(23-25)
src/client/graphics/layers/WebGLBorderRenderer.ts (3)
src/client/graphics/layers/BorderRenderer.ts (1)
BorderRenderer(4-20)src/client/graphics/layers/TerritoryBorderWebGL.ts (3)
TerritoryBorderWebGL(47-835)HoverHighlightOptions(40-45)BorderEdge(12-21)src/core/game/GameView.ts (9)
GameView(560-886)tile(110-112)owner(113-115)owner(748-750)PlayerView(185-558)isBorder(834-836)x(783-785)y(786-788)myPlayer(704-706)
src/client/graphics/HoverTargetResolver.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/GameView.ts (3)
GameView(560-886)UnitView(48-183)PlayerView(185-558)
src/client/graphics/layers/TerritoryBorderWebGL.ts (2)
src/core/game/GameView.ts (2)
width(792-794)height(795-797)src/core/configuration/Config.ts (1)
Theme(188-211)
src/client/InputHandler.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/core/game/GameView.ts (3)
src/core/configuration/DefaultConfig.ts (1)
theme(637-641)src/core/game/GameMap.ts (3)
TileRef(3-3)x(122-124)y(126-128)src/core/game/GameImpl.ts (2)
x(798-800)y(801-803)
src/client/graphics/layers/TerritoryLayer.ts (5)
src/client/graphics/layers/BorderRenderer.ts (2)
BorderRenderer(4-20)NullBorderRenderer(22-36)src/client/graphics/layers/WebGLBorderRenderer.ts (1)
WebGLBorderRenderer(12-199)src/client/InputHandler.ts (3)
ToggleTerritoryWebGLEvent(84-84)ToggleTerritoryWebGLDebugBordersEvent(95-97)TerritoryWebGLStatusEvent(86-93)src/client/graphics/HoverTargetResolver.ts (1)
resolveHoverTarget(40-70)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-43)
🔇 Additional comments (11)
src/core/game/UserSettings.ts (1)
64-66: Territory WebGL setting follows existing settings patternGetter and toggle are consistent with the other boolean settings and look correct. Defaulting to
truematches the idea of “on unless explicitly disabled”.Also applies to: 122-124
src/client/InputHandler.ts (1)
84-97: New WebGL events integrate cleanly with existing EventBus model
ToggleTerritoryWebGLEvent,TerritoryWebGLStatusEvent, andToggleTerritoryWebGLDebugBordersEventfollow the existing “class per event” pattern and give just enough data for the UI and renderer wiring.src/client/graphics/layers/PlayerInfoOverlay.ts (1)
29-29: Centralizing hover resolution improves clarityUsing
resolveHoverTargetto pick either aplayerorunitkeepsPlayerInfoOverlayfocused on display logic instead of map probing, and the visibility handling remains straightforward.Also applies to: 96-106
src/client/graphics/GameRenderer.ts (1)
6-6: TerritoryWebGLStatus wiring is consistent with existing DOM-layer patternCreating or reusing the
<territory-webgl-status>element, assigningeventBusanduserSettings, and then including it at the front of thelayersarray matches how other DOM-backed layers (e.g. overlays and panels) are integrated. This should give the component a clean lifecycle viainit()without interfering with canvas transforms.Also applies to: 40-40, 224-235, 252-294
src/client/graphics/layers/TerritoryWebGLStatus.ts (1)
1-193: User-facing strings should use translateText / i18nThe status panel wiring (eventBus, userSettings, debug toggle) looks good, but all visible text is currently hard-coded in English:
- Labels like
"Territory Renderer","WebGL borders hidden","WebGL borders active","WebGL unsupported (fallback)","WebGL enabled (fallback)".- Button texts like
"Hide WebGL layer","Show WebGL layer","Disable GL border highlight","Highlight GL borders".Given the rest of the UI and the PR checklist, these should go through the translation layer and have keys added to
en.json, for example:import { translateText } from "../../Utils"; // or existing helper // examples private statusText(): string { if (!this.enabled) { return translateText("territory_webgl_status.hidden"); } if (!this.supported) { return translateText("territory_webgl_status.unsupported_fallback"); } if (this.active) { return translateText("territory_webgl_status.active"); } return translateText("territory_webgl_status.enabled_fallback"); }and update button labels similarly.
You might also consider collapsing
enabled/active/supportedinto a small discriminated union status type for clearer state handling, but that’s optional.⛔ Skipped due to learnings
Learnt from: mokizzz Repo: openfrontio/OpenFrontIO PR: 1940 File: resources/lang/en.json:763-766 Timestamp: 2025-08-27T08:12:19.610Z Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.Learnt from: devalnor Repo: openfrontio/OpenFrontIO PR: 1248 File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20 Timestamp: 2025-06-22T21:51:14.990Z Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.Learnt from: TheGiraffe3 Repo: openfrontio/OpenFrontIO PR: 1864 File: resources/maps/arabianpeninsula/manifest.json:13-170 Timestamp: 2025-08-19T11:00:55.422Z Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.Learnt from: NewYearNewPhil Repo: openfrontio/OpenFrontIO PR: 2230 File: src/client/graphics/GameRenderer.ts:269-277 Timestamp: 2025-10-18T11:00:57.142Z Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.src/client/graphics/layers/PerformanceOverlay.ts (1)
59-72: Per‑layer timing stats and overlay UI look correctThe EMA logic, total accumulation, reset handling, and the way you scale and render the per‑layer bars all look consistent and safe. The optional
layerDurationsintegration intoupdateFrameMetricsand reset behaviour forlayerStats/layerBreakdownare also coherent. I don’t see correctness issues here.Also applies to: 261-286, 288-350, 352-383, 446-450, 487-510
src/client/graphics/layers/TerritoryLayer.ts (3)
280-291: Hover resolution and WebGL hover wiring look goodThe switch to
resolveHoverTargetplus gating hover onalternativeView || borderRenderer.drawsOwnBorders()is clean, and forwardinghighlightedTerritory?.smallID()intosetHoveredPlayerIdwhile falling back toredrawBorderfor the canvas path keeps both renderers in sync without extra work in the non‑WebGL case.Also applies to: 313-357
498-573: Tile rendering + borderRenderer integration looks consistentThe new
renderLayerflow withskipTerritoryCanvasand the FrameProfiler spans is coherent:
- You skip
putImageDataanddrawImagewhenalternativeView && borderRenderer.drawsOwnBorders(), which avoids useless blits when WebGL borders are the only visible work.- Canvas drawing is still used for the main view and for the fallback
NullBorderRenderercase.- Border rendering (
borderRenderer.render) and spawn highlight drawing are individually timed.In
paintTerritoryandclearTile:
- The split between CPU tile painting and
borderRenderer.updateBorder(...)is clean.- The defended‑border detection (
hasUnitNearby) and relation‑based border colors viaowner.borderColor(tile, isDefended)match the new PlayerView border logic.- Calling
borderRenderer.clearTile(tile)insideclearTileensures WebGL and canvas state stay aligned when tiles are cleared.I don’t see correctness problems in these paths.
Also applies to: 598-671, 703-708
359-361: Avoid resettingwebglSupportedtotruewhen WebGL is disabledIn
configureBorderRenderer()you set:if (!this.useWebGL) { this.borderRenderer = new NullBorderRenderer(); this.webglSupported = true; this.emitWebGLStatus(false, false, this.webglSupported, "WebGL territory layer hidden."); return; }If you previously detected that WebGL is not supported (
isSupported() === false), toggling WebGL off will overwritewebglSupportedback totrue. Any UI listening toTerritoryWebGLStatusEventwill then think WebGL is supported even though you know it is not.You likely want to preserve the last known support flag instead of forcing it to
true, for example:- if (!this.useWebGL) { - this.borderRenderer = new NullBorderRenderer(); - this.webglSupported = true; - this.emitWebGLStatus( - false, - false, - this.webglSupported, - "WebGL territory layer hidden.", - ); - return; - } + if (!this.useWebGL) { + this.borderRenderer = new NullBorderRenderer(); + // Keep existing webglSupported value so status stays accurate. + this.emitWebGLStatus( + false, + false, + this.webglSupported, + "WebGL territory layer hidden.", + ); + return; + }Also applies to: 389-438
⛔ Skipped due to learnings
Learnt from: devalnor Repo: openfrontio/OpenFrontIO PR: 1248 File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20 Timestamp: 2025-06-22T21:51:14.990Z Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.src/client/graphics/layers/WebGLBorderRenderer.ts (1)
12-37: WebGL border renderer implementation and flag encoding look correctThe
WebGLBorderRenderercleanly implementsBorderRendererand delegates toTerritoryBorderWebGLwithout leaking WebGL details into the rest of the code. The edge construction:
- Uses tile world coordinates from
GameView.x/y.- Encodes
isDefended,hasFriendly,hasEmbargo, andlightTileinto bit flags that match the fragment shader’s decode order.- Resolves relations via
TileRelationin a way consistent withPlayerView.isFriendly/hasEmbargo.The render path (
renderer.render()+context.drawImage(...)) also lines up with how the territory canvas is positioned, so this looks solid.Also applies to: 54-74, 94-170, 172-198
src/core/game/GameView.ts (1)
44-47: Relation‑aware border colors and shared flags API look goodThe new border color flow in
PlayerViewis well structured:
- You precompute neutral/friendly/embargo base colors and defended variants once in the constructor using the theme, which keeps per‑tile work cheap.
borderColor(tile, isDefended)now usesborderRelationFlags(tile)so both canvas rendering and WebGL borders get consistent relationship‑based tinting and defended checkerboard behavior.borderRelationFlagsitself has clear semantics (hasEmbargoshort‑circuits,hasFriendlyaccumulates) and matches the usage in both the CPU path andWebGLBorderRenderer’s flag encoding.This shared API makes it much easier to keep the two renderers in sync.
Also applies to: 189-199, 215-253, 253-303, 321-356, 358-389
—falsy check fails for tile index 0. Since TileRef is a number and tile 0 is valid the !tile check was wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/HoverTargetResolver.ts (1)
49-52: Consider makingisPlayer()a type guard to eliminate the cast.The explicit cast to
PlayerViewat line 51 suggests thatisPlayer()isn't defined as a TypeScript type guard. If you update theisPlayer()method signature to returnthis is PlayerView, TypeScript will automatically narrow the type and the cast becomes unnecessary.Example signature for the method:
isPlayer(): this is PlayerView { // implementation }This change would be in the
GameViewor owner type definition file, not here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/HoverTargetResolver.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/client/graphics/HoverTargetResolver.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/client/graphics/HoverTargetResolver.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/client/graphics/HoverTargetResolver.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/client/graphics/HoverTargetResolver.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/graphics/HoverTargetResolver.ts
🧬 Code graph analysis (1)
src/client/graphics/HoverTargetResolver.ts (2)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/GameView.ts (3)
GameView(560-886)UnitView(48-183)PlayerView(185-558)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
🔇 Additional comments (5)
src/client/graphics/HoverTargetResolver.ts (5)
1-10: Clean imports and well-defined constants.The imports are focused and the constants are clearly named. Using a 5-pixel hover distance threshold and limiting to the three ship types makes sense for this hover detection system.
12-22: Distance calculation is correct and straightforward.The Euclidean distance calculation is implemented correctly. The function is pure, testable, and clear in its purpose.
24-33: Comparator function is well-structured.The distance-based sorting logic is correct and uses a clean functional approach. This will properly sort units from nearest to farthest.
35-38: Interface is clean and properly typed.The
HoverTargetResolutioninterface is straightforward with appropriate nullable types for cases where no hover target exists.
40-70: Hover target resolution logic is sound and the previous bug is fixed.The coordinate validation at line 44 correctly uses
isValidCoord()instead of a falsy check on the tile reference, which addresses the critical issue from the previous review. The overall resolution logic is clear and follows a sensible priority: territory owner → nearby ships → null.
- Imported `FrameProfiler` and instrumented:
- `updateBorder` with spans:
- `WebGLBorderRenderer:updateBorder.noRenderer`
- `WebGLBorderRenderer:updateBorder.clearTile`
- `WebGLBorderRenderer:updateBorder.buildBorderEdges`
- `WebGLBorderRenderer:updateBorder.noEdges`
- `WebGLBorderRenderer:updateBorder.renderer.updateEdges`
- `WebGLBorderRenderer:updateBorder.total`
- `render` with spans:
- `WebGLBorderRenderer:render.noRenderer`
- `WebGLBorderRenderer:render.renderer.render`
- `WebGLBorderRenderer:render.context.drawImage`
- `WebGLBorderRenderer:render.total`
- `buildBorderEdges` with:
- `WebGLBorderRenderer:buildBorderEdges`
- **Added profiling to `TerritoryBorderWebGL`**
- Imported `FrameProfiler` and instrumented:
- `create` with `TerritoryBorderWebGL:create`
- `updateEdges` with:
- `TerritoryBorderWebGL:updateEdges.noContextOrProgram`
- `TerritoryBorderWebGL:updateEdges.removeTileEdges`
- `TerritoryBorderWebGL:updateEdges.addTileChunk`
- `TerritoryBorderWebGL:updateEdges.writeChunk`
- `TerritoryBorderWebGL:updateEdges.total`
- `render` with:
- `TerritoryBorderWebGL:render.uploadDirtyChunks`
- `TerritoryBorderWebGL:render.draw`
- `TerritoryBorderWebGL:render.total`
- `addTileChunk` with `TerritoryBorderWebGL:addTileChunk.ensureCapacity`
- `removeTileEdges` with:
- `TerritoryBorderWebGL:removeTileEdges.noChunk`
- `TerritoryBorderWebGL:removeTileEdges.removedLastChunk`
- `TerritoryBorderWebGL:removeTileEdges.total`
- `writeChunk` with `TerritoryBorderWebGL:writeChunk`
- `ensureCapacity` growth path with `TerritoryBorderWebGL:ensureCapacity.grow`
…formance metrics - Introduced a button to copy current performance metrics as JSON. - Implemented clipboard handling with fallback for unsupported browsers. - Added visual feedback for copy status (idle, success, error). - Enhanced styling for the copy button and integrated translation for status messages.
…) set in renderTerritory in paintTerritory. reducing updated tiles
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/client/graphics/layers/TerritoryBorderWebGL.ts (2)
59-60: Extract magic number to a named constant.The value
4represents bytes per float (Float32). Consider extracting it as a constant for clarity:export class TerritoryBorderWebGL { + private static readonly BYTES_PER_FLOAT = 4; private static readonly INITIAL_CHUNK_CAPACITY = 65536; // 256; private static readonly MAX_EDGES_PER_TILE = 4; private static readonly VERTICES_PER_EDGE = 2; private static readonly MAX_VERTICES_PER_TILE = TerritoryBorderWebGL.MAX_EDGES_PER_TILE * TerritoryBorderWebGL.VERTICES_PER_EDGE; private static readonly FLOATS_PER_VERTEX = 9; private static readonly FLOATS_PER_TILE = TerritoryBorderWebGL.MAX_VERTICES_PER_TILE * TerritoryBorderWebGL.FLOATS_PER_VERTEX; private static readonly STRIDE_BYTES = - TerritoryBorderWebGL.FLOATS_PER_VERTEX * 4; + TerritoryBorderWebGL.FLOATS_PER_VERTEX * TerritoryBorderWebGL.BYTES_PER_FLOAT;You'll also need to update line 791 where
* 4is used inbufferSubData.
128-133: Remove or gate debug logging behind a flag.The console.log statements at both locations are labeled as debug logs for capacity tuning. These should either be removed before merging to main, or gated behind a debug configuration flag to avoid cluttering production console output.
Option 1 - Remove the logs entirely:
- console.log( - "[TerritoryBorderWebGL] initial capacityChunks=", - this.capacityChunks, - "for map size", - `${this.width}x${this.height}`, - );Option 2 - Gate behind a debug flag (add at top of class):
private static readonly DEBUG_CAPACITY = false; // Then in constructor: if (TerritoryBorderWebGL.DEBUG_CAPACITY) { console.log( "[TerritoryBorderWebGL] initial capacityChunks=", this.capacityChunks, "for map size", `${this.width}x${this.height}`, ); }Also applies to: 807-815
src/client/graphics/layers/TerritoryLayer.ts (1)
444-464: Consider extracting magic numbers for hover highlight styling.The hover highlight options contain several magic numbers (0.8, 0.45, 0.6, 0.35) that control visual behavior. Extracting these as named constants at the class level would make them easier to tune and document their purpose.
export class TerritoryLayer implements Layer { + // Hover highlight visual parameters + private static readonly HOVER_ALT_VIEW_STRENGTH = 0.8; + private static readonly HOVER_ALT_VIEW_PULSE = 0.45; + private static readonly HOVER_MAIN_VIEW_STRENGTH = 0.6; + private static readonly HOVER_MAIN_VIEW_PULSE = 0.35; + private static readonly HOVER_PULSE_SPEED = Math.PI * 2; + private userSettings: UserSettings; // ... private hoverHighlightOptions() { const baseColor = this.theme.spawnHighlightSelfColor(); if (this.alternativeView) { - // Alternate view: borders are the primary visual, so make hover stronger return { color: baseColor, - strength: 0.8, - pulseStrength: 0.45, - pulseSpeed: Math.PI * 2, + strength: TerritoryLayer.HOVER_ALT_VIEW_STRENGTH, + pulseStrength: TerritoryLayer.HOVER_ALT_VIEW_PULSE, + pulseSpeed: TerritoryLayer.HOVER_PULSE_SPEED, }; } - // Main view: keep highlight noticeable but a bit subtler return { color: baseColor, - strength: 0.6, - pulseStrength: 0.35, - pulseSpeed: Math.PI * 2, + strength: TerritoryLayer.HOVER_MAIN_VIEW_STRENGTH, + pulseStrength: TerritoryLayer.HOVER_MAIN_VIEW_PULSE, + pulseSpeed: TerritoryLayer.HOVER_PULSE_SPEED, }; }src/client/graphics/layers/WebGLBorderRenderer.ts (1)
129-209: Extract magic number and consider clearer flag construction.Two opportunities for improvement:
- The
insetvalue (0.1) is a magic number that affects how borders are drawn. Extract it as a named constant to document its purpose.- The bit flag construction (lines 145-149) works but could be more explicit.
export class WebGLBorderRenderer implements BorderRenderer { + private static readonly BORDER_INSET = 0.1; // Inset borders so both countries' borders are visible private readonly renderer: TerritoryBorderWebGL | null; // ... private buildBorderEdges( tile: TileRef, owner: PlayerView, isDefended: boolean, ): BorderEdge[] { const span = FrameProfiler.start(); const edges: BorderEdge[] = []; const x = this.game.x(tile); const y = this.game.y(tile); const ownerId = owner.smallID(); const relation = this.resolveRelation(owner); const color = owner.borderColor(); const { hasEmbargo, hasFriendly } = owner.borderRelationFlags(tile); const lightTile = (x % 2 === 0 && y % 2 === 0) || (y % 2 === 1 && x % 2 === 1); const flags = - (isDefended ? 1 : 0) | - (hasFriendly ? 2 : 0) | - (hasEmbargo ? 4 : 0) | - (lightTile ? 8 : 0); + (isDefended ? 1 << 0 : 0) | + (hasFriendly ? 1 << 1 : 0) | + (hasEmbargo ? 1 << 2 : 0) | + (lightTile ? 1 << 3 : 0); - // Inset borders by 1 tile (0.1 tiles inward) so both countries' borders can be drawn - const inset = 0.1; + const inset = WebGLBorderRenderer.BORDER_INSET; // ... rest of method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/PerformanceOverlay.ts(11 hunks)src/client/graphics/layers/TerritoryBorderWebGL.ts(1 hunks)src/client/graphics/layers/TerritoryLayer.ts(16 hunks)src/client/graphics/layers/WebGLBorderRenderer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/WebGLBorderRenderer.tssrc/client/graphics/layers/TerritoryBorderWebGL.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/WebGLBorderRenderer.tssrc/client/graphics/layers/TerritoryBorderWebGL.tssrc/client/graphics/layers/PerformanceOverlay.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/TerritoryLayer.ts
🧬 Code graph analysis (3)
src/client/graphics/layers/TerritoryLayer.ts (6)
src/client/graphics/layers/BorderRenderer.ts (2)
BorderRenderer(4-20)NullBorderRenderer(22-36)src/client/graphics/layers/WebGLBorderRenderer.ts (1)
WebGLBorderRenderer(13-238)src/client/InputHandler.ts (3)
ToggleTerritoryWebGLEvent(84-84)ToggleTerritoryWebGLDebugBordersEvent(95-97)TerritoryWebGLStatusEvent(86-93)src/client/graphics/HoverTargetResolver.ts (1)
resolveHoverTarget(40-70)src/core/game/GameView.ts (7)
PlayerView(185-558)tile(110-112)hasOwner(822-824)owner(113-115)owner(748-750)hasFallout(828-830)myPlayer(704-706)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-43)
src/client/graphics/layers/WebGLBorderRenderer.ts (2)
src/client/graphics/layers/BorderRenderer.ts (1)
BorderRenderer(4-20)src/client/graphics/layers/TerritoryBorderWebGL.ts (3)
TerritoryBorderWebGL(48-896)HoverHighlightOptions(41-46)BorderEdge(13-22)
src/client/graphics/layers/TerritoryBorderWebGL.ts (2)
src/core/configuration/Config.ts (1)
Theme(188-211)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-43)
🔇 Additional comments (10)
src/client/graphics/layers/TerritoryBorderWebGL.ts (2)
1-11: LGTM! Clean public API design.The enum and interface definitions are clear and well-typed. TileRelation values align with shader usage, and BorderEdge provides a complete edge descriptor.
49-49: The calculation of initial memory allocation is incorrect; verify your math and consider the dynamic growth strategy.The INITIAL_CHUNK_CAPACITY of 65,536 allocates approximately 18.9 MB upfront (65,536 chunks × 72 floats per chunk × 4 bytes per float), not 2.36 MB as stated. More importantly, this is not a concern because:
- Dynamic growth is in place: The
ensureCapacity()method doubles capacity on demand (line 802), so initial allocation is a tuned baseline, not a fixed limit.- Chunks are sparse: Only tiles with borders consume chunks via the
tileToChunkmap; unused capacity remains unallocated in practice.- Intentional tuning: The comment on line 49 ("// 256") and debug logging (lines 125–126, 807–815) show this value was deliberately increased and is monitored in production.
The code already has instrumentation to track real-world usage patterns. If memory efficiency on lower-end devices becomes a concern, adjust this constant based on console logs from actual gameplay.
src/client/graphics/layers/TerritoryLayer.ts (4)
21-30: LGTM! Clean imports for WebGL integration.All new imports are necessary and properly used throughout the file for WebGL border rendering, profiling, and hover resolution.
67-82: LGTM! Proper initialization of WebGL state.The new fields are correctly initialized, with NullBorderRenderer as a safe default before the actual renderer is configured. The lastMyPlayerSmallId tracking enables detection of player changes.
172-176: LGTM! Correct handling of player perspective changes.When the current player changes (e.g., in spectator mode or game transitions), all border relations (Self/Friendly/Neutral/Enemy) need to be recomputed. A full redraw is the appropriate response. Since this check is in
tick(), it won't cause excessive redraws.
598-678: LGTM! Complex but correct border rendering coordination.The modified
paintTerritorycorrectly coordinates between WebGL and canvas border rendering:
- When
borderRenderer.drawsOwnBorders()is true, it paints only territory fill colors to the 2D canvas- When false, it falls back to painting borders directly to canvas
- The
_maybeStaleBorder && !isBorderTilecheck (line 661) properly clears border data when a tile stops being a borderThe FrameProfiler tracking helps identify performance bottlenecks. The logic is sound.
src/client/graphics/layers/WebGLBorderRenderer.ts (3)
13-25: LGTM! Clean constructor with proper factory usage.The constructor correctly initializes the WebGL renderer using the factory method, which safely returns null when WebGL is unavailable.
211-237: LGTM! Clean helper methods.Both
resolveRelationandownerSmallIdAtare straightforward, correct, and handle edge cases (null owner, invalid coordinates) properly.
69-69: Type cast is safe - TileRef is a simple number alias.The verification confirms that
TileRefis a straightforward type alias fornumber(defined asexport type TileRef = number;in src/core/game/GameMap.ts). The casting tonumberat lines 69, 82, and 98 is type-safe with no risk of data loss or type safety issues.src/client/graphics/layers/PerformanceOverlay.ts (1)
315-318: Remove or use theframeDurationparameterThe parameter
frameDurationis genuinely unused in the method body (line 316). However, the original review comment's claim about compiler enforcement is incorrect:noUnusedParametersis not enabled intsconfig.json, so this won't cause a compilation error.That said, the underlying concern is valid—the engine is supplying timing information through
frameDurationthat we should use instead of recalculating it withperformance.now(). Either use the parameter as intended or remove it from the signature. If intentionally unused, prefix it with an underscore:_frameDuration: number.Likely an incorrect or invalid review comment.
…r is drawing borders
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/client/graphics/layers/TerritoryLayer.ts (3)
286-297: Avoid directWebGLBorderRendererchecks where possibleThe event wiring is clear, but
instanceof WebGLBorderRendererhere tiesTerritoryLayerto that specific implementation for hover options and debug pulse.To keep the design more compositional and easier to extend, consider in a follow‑up:
- Extending
BorderRendererwith optional methods likesetHoverHighlightOptions?(...)andsetDebugPulseEnabled?(...), and- Implementing these as no‑ops in
NullBorderRenderer.Then callers can just call the interface methods without
instanceof, and adding a new border renderer variant stays simple.Also applies to: 298-301
315-318: Hover resolution change may alter which player is highlightedSwitching to
resolveHoverTarget(this.game, cell).playermeans hover can now highlight:
- Territory owners (old behavior) and
- Owners of nearby units over water or neutral tiles (new behavior from
HoverTargetResolver).This is probably desired, but it does change UX: hovering near a ship may highlight that player even when the tile has no territory owner.
Please confirm this is the intended behavior in both main and alternative views, and that hover transitions still feel correct when moving between land, sea, and unit clusters.
Also applies to: 335-335, 344-356
595-595:paintTerritorylogic and border updates look correct; consider small cleanupsThe refactor of
paintTerritoryto:
- Compute
owner,isBorderTile,hasFallout,isDefendedonce,- Keep all 2D drawing decisions local, and
- Drive
borderRenderer.updateBorderwith a single call per tile,is clear and seems logically correct. The
_maybeStaleBorderflag is also handled safely when clearing old borders on neighbors.Two small optional tweaks you might consider later:
Parameter naming
_maybeStaleBorderis now used meaningfully. Dropping the leading underscore (e.g.maybeStaleBorder) would make intent clearer.Minor micro‑optimization
In the!ownerpath, you call bothclearTile(tile)(which already clears borderRenderer state) and then, for WebGL,borderRenderer.clearTile(tile)again via therendererHandlesBordersblock. Not a bug, just redundant work; you could rely on the first clear.These are non‑blocking and can wait for a follow‑up pass.
Also applies to: 600-661, 662-679
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/TerritoryLayer.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/client/graphics/layers/TerritoryLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/TerritoryLayer.ts (6)
src/client/graphics/layers/BorderRenderer.ts (2)
BorderRenderer(4-20)NullBorderRenderer(22-36)src/core/game/GameView.ts (7)
PlayerView(185-558)tile(110-112)hasOwner(822-824)owner(113-115)owner(748-750)hasFallout(828-830)myPlayer(704-706)src/client/graphics/layers/WebGLBorderRenderer.ts (1)
WebGLBorderRenderer(13-238)src/client/InputHandler.ts (3)
ToggleTerritoryWebGLEvent(84-84)ToggleTerritoryWebGLDebugBordersEvent(95-97)TerritoryWebGLStatusEvent(86-93)src/client/graphics/HoverTargetResolver.ts (1)
resolveHoverTarget(40-70)src/client/graphics/FrameProfiler.ts (1)
FrameProfiler(1-43)
🔇 Additional comments (5)
src/client/graphics/layers/TerritoryLayer.ts (5)
21-23: WebGL border wiring and defaults look solidUsing
BorderRendererwith aNullBorderRendererdefault plususeWebGL/webglSupportedflags is a clean way to keep the caller free from null checks and to favor composition over inheritance. No changes needed here.Also applies to: 25-30, 56-56, 67-69, 80-81
163-169: Focused-player border repaint + my-player change redrawThe split between WebGL (
drawsOwnBorders === true) and canvas paths on focus change makes sense and avoids double work for the WebGL renderer. Also, redrawing whenmyPlayer.smallID()changes is a simple and safe way to keep relation‑based colors in sync.Please just double‑check behaviors by:
- Switching focus between players with WebGL borders on and off.
- Changing controlled player (e.g. spectate → join) and confirming borders update as expected.
Also applies to: 174-177
442-466: Central hover styling helper is a good abstraction
hoverHighlightOptions()nicely centralizes the WebGL hover styling and makes alternate‑view tuning explicit. Returning simple plain objects here keeps the code easy to read and avoids extra types or classes. No changes needed.
502-504: Render path separation and profiling look reasonableThe
skipTerritoryCanvasflag andshouldBlitTerritoriesguard cleanly separate:
- “WebGL borders + alternative view only” (skip 2D territory blits), from
- Normal canvas rendering paths.
Wrapping
renderTerritory,putImageData,drawImage, border rendering, and highlight drawing inFrameProfilerspans should give good per‑step visibility without changing behavior.Given the perf focus here, it would be good to sanity‑check that:
- Alternative view + WebGL correctly shows only WebGL borders, and
- Main view + WebGL still shows the regular territory canvas with borders overlaid.
Also applies to: 510-512, 523-528, 529-541, 544-555, 556-561, 563-575
712-713: SyncingclearTilewithborderRendereris the right callHaving
clearTilealso clearborderRendererstate keeps the WebGL borders and canvas buffers in sync and avoids stale border edges when tiles are cleared for any reason (ownership loss, spawn highlight, etc.). This is a good use of composition through theBorderRendererinterface.
| this.configureBorderRenderer(); | ||
|
|
There was a problem hiding this comment.
Do not reset webglSupported to true when user disables WebGL
In configureBorderRenderer:
if (!this.useWebGL) {
this.borderRenderer = new NullBorderRenderer();
this.webglSupported = true; // ← problematic
this.emitWebGLStatus(false, false, this.webglSupported, ...);
return;
}If WebGL was previously probed and found unsupported (webglSupported === false), toggling the feature off will overwrite that info and start emitting supported: true, which is misleading for the UI.
A safer approach is to leave webglSupported unchanged in the “disabled” branch:
if (!this.useWebGL) {
this.borderRenderer = new NullBorderRenderer();
- this.webglSupported = true;
this.emitWebGLStatus(
false,
false,
this.webglSupported,
"WebGL territory layer hidden.",
);
return;
}This keeps “hardware / browser support” separate from “user has it turned on”.
Also applies to: 408-440, 468-477
🤖 Prompt for AI Agents
In src/client/graphics/layers/TerritoryLayer.ts around lines 391-392 (and
similar logic at 408-440 and 468-477), the configureBorderRenderer branch that
runs when this.useWebGL is false resets this.webglSupported to true and emits
supported: true; instead leave this.webglSupported unchanged so browser/hardware
support remains accurate. Change the disabled branch to instantiate
NullBorderRenderer, do not assign to this.webglSupported, and call
emitWebGLStatus passing the existing this.webglSupported value (or compute only
if needed) so toggling the feature off does not overwrite the probed support
state.
2 participants
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