testpr: Webgl terrain #2867
testpr: Webgl terrain #2867
Conversation
- Added `lastPaletteSignature` to track palette changes. - Implemented `computePaletteSignature()` to generate a unique signature based on player views and settings. - Introduced `refreshPaletteIfNeeded()` to refresh the palette when the signature changes, ensuring visual consistency in the TerritoryLayer.
- Introduced a new `TileTransition` type to manage tile transitions. - Added methods to handle the beginning and updating of tile transitions. - Enhanced `TerritoryRenderer` interfaces to support transition progress. - Updated `TerritoryWebGLRenderer` to manage transition textures and rendering. - Modified `GameView` to track owner changes for tiles, enabling transition effects during gameplay.
Refactor TerritoryLayer and Renderer for improved transition handling - Removed unused imports and types, streamlining the TerritoryLayer code. - Replaced the `TerritoryRendererStrategy` interface with direct usage of `TerritoryWebGLRenderer`. - Enhanced transition management by introducing epoch and progress tracking for tile transitions. - Updated rendering logic to utilize the new transition properties, ensuring smoother visual updates. - Removed the CanvasTerritoryRenderer and related logic, consolidating rendering responsibilities within the WebGL renderer.
…ition management - Updated transition handling in TerritoryLayer, replacing epoch and progress tracking with a more efficient system using arrays for start times and active masks. - Introduced new methods to manage transition states and ensure proper initialization of transition data. - Modified TerritoryWebGLRenderer to align with the new transition management, removing obsolete properties and updating uniform handling for transitions. - Improved rendering logic to support smoother visual updates during tile transitions.
…ws boats/ships too. - Added shared hover helper in HoverInfo.ts (same rules as PlayerInfoOverlay, including nearby ships on water). - PlayerInfoOverlay.ts now uses getHoverInfo(...) instead of its local hover logic. - TerritoryLayer.ts now uses getHoverInfo(...) and highlights the unit owner when hovering ships/boats.
…ment - Introduced contest management in TerritoryLayer, replacing previous transition handling with a new system for managing contests between tile owners. - Added methods to handle contest state, including starting, updating, and expiring contests. - Updated TerritoryWebGLRenderer to support rendering contest data, including new textures and uniforms for contest owners, IDs, and times. - Enhanced rendering logic to visually represent contest states, including color changes and border effects during contests.
…d wired it into the layer update loop. The renderer now builds a distance field from the previous‑owner border on the GPU and blends old/new colors over the 100 ms window; contested areas still use their checkerboard effect and skip smoothing. Details: Added per‑tile smoothing state in TerritoryLayer.ts (prev owner + change mask + 100 ms timing) and feed progress into the renderer each frame. Added JFA seed + ping‑pong passes, prevOwner/changeMask textures, and smooth uniforms in TerritoryWebGLRenderer.ts, plus shader blending based on JFA distance.
…hing management - Removed redundant smooth state handling in TerritoryLayer, replacing it with a snapshot mechanism for smoother transitions. - Updated rendering logic in TerritoryWebGLRenderer to utilize framebuffers for state management, enhancing performance and visual fidelity. - Adjusted shader logic to accommodate new smoothing conditions and removed obsolete properties related to smooth state tracking.
…color - Added playerHighlightColor method to Theme interface and implemented it in PastelTheme and PastelThemeDark classes. - Updated TerritoryLayer to redraw when the theme changes, ensuring consistent visual updates. - Modified hoverHighlightOptions to use the new player highlight color for improved territory visualization.
…ebGLRenderer - Introduced a constant for default contest duration in TerritoryLayer - Updated contest duration handling in TerritoryWebGLRenderer
- Removed unused variables and logic related to contest state, including attacker relations and border management. - Streamlined rendering logic by eliminating unnecessary checks for contested states, improving code clarity and performance.
feat: Add view transforms and contest speed/strength visualization - Add contest speed tracking with exponential moving average for dynamic contest visualization - Add contest strength calculation based on attacker/defender troop ratios - Implement view transform support (scale/offset) for zoom and pan functionality - Add animated contest effects with noise-based cloud rendering - Improve smooth territory transitions with better edge handling - Add debug overlay for development with contest and rendering stats - Refactor WebGL renderer to support world-space rendering instead of pixel-perfect
Refactor contest management in TerritoryLayer and TerritoryWebGLRenderer - Updated contest duration handling to use ticks instead of milliseconds for improved precision. - Introduced new tick-based state management for contest updates and rendering. - Enhanced interpolation logic for smoother transitions between contest states. - Removed obsolete smooth state handling and related properties to streamline code. - Added support for older contest states in the WebGL renderer for better visual fidelity.
- Added support for change mask textures and framebuffers to track territory ownership changes. - Introduced a new shader program for change mask processing, enhancing visual fidelity during transitions. - Updated rendering logic to incorporate change mask updates, improving performance and accuracy in contested areas. - Refactored related uniform and framebuffer management for better organization and clarity.
…Layer and TerritoryWebGLRenderer to simplify contest handling. - Updated contest strength calculations to utilize troop counts directly, enhancing accuracy in contest dynamics. - Streamlined rendering logic by eliminating unnecessary checks and textures related to contest speeds, improving performance and clarity. - Refactored related methods to focus on contest strength, ensuring a more cohesive approach to contest state management.
… tracking - Remove unused GLSL noise functions (hash12, valueNoise, fbm) from TerritoryWebGLRenderer - Remove contested fill smoke rendering code that used these noise functions - Add contestTileCount field to TerritoryLayer to track total contested tiles - Update debug output to include contest tile count for monitoring
- Introduced ENABLE_CONTEST_TRACKING constant to toggle contest tracking functionality. - Updated TerritoryLayer to conditionally handle contest updates and rendering based on contestEnabled state. - Refactored TerritoryWebGLRenderer to manage contest-related uploads and rendering based on contestEnabled flag. - Improved shader logic to incorporate contestEnabled uniform for better control over contest rendering.
…LRenderer - Deleted TerrainLayer class to streamline rendering architecture. - Updated GameRenderer to remove TerrainLayer instantiation. - Enhanced TerritoryWebGLRenderer to manage terrain textures and rendering directly. - Added terrain texture handling and shader updates for improved visual fidelity. - Introduced terrainView method in GameImpl and GameMap for terrain data access.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThis PR implements a defense-post territory influence system with WebGL-based territory rendering, removes legacy TerrainLayer rendering, centralizes hover information retrieval, adds player territory highlighting, and introduces defense state tracking across the game and map layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Game as Game
participant Map as GameMap
participant Impl as GameImpl
participant Renderer as TerritoryRenderer
Note over Game,Renderer: Defense Post Tile Ownership Change Flow
Game->>Impl: conquer(owner, tile)
Impl->>Map: ownership change
Impl->>Impl: updateDefendedStateForTileChange(tile, owner)
Impl->>Impl: checks tiles near DefensePosts
Impl->>Map: setDefended(nearbyTile, true/false)
Map->>Map: update DEFENDED_BIT
Impl->>Renderer: palette/state updates trigger
Renderer->>Renderer: redraws territory with defended highlight
sequenceDiagram
participant UI as PlayerInfoOverlay
participant Hover as HoverInfo
participant Game as GameView
participant Map as GameMap
Note over UI,Map: Hover Information Retrieval Flow
UI->>UI: mouse move event
UI->>Hover: getHoverInfo(game, worldCoord)
Hover->>Game: validate coordinate
Hover->>Map: get tile owner
alt Owner is player
Hover->>Hover: set player in HoverInfo
else Owner exists, tile is land
Hover->>Hover: mark wilderness/irradiated
else Tile is not land
Hover->>Game: find nearby ships
Hover->>Hover: sort by distance
Hover->>Hover: set closest unit
end
Hover-->>UI: return HoverInfo
UI->>UI: display based on info object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/client/graphics/layers/TerritoryLayer.ts:
- Line 28: The DEBUG_TERRITORY_OVERLAY constant is currently true and enabling a
debug overlay in production; change the definition of DEBUG_TERRITORY_OVERLAY to
false (or wire it to a development-only flag/env variable) so the overlay is
disabled by default, updating the constant declaration where
DEBUG_TERRITORY_OVERLAY is defined and any tests or dev-only paths that expect
it to remain true.
- Around line 1011-1018: The comparison using Math.max on packed timestamps
(target.lastActivityPacked and source.lastActivityPacked) fails across the wrap
boundary; replace the Math.max usage with the existing contestElapsed-based
comparison so the code chooses the newer timestamp correctly across wrap-around,
e.g., use contestElapsed(target.lastActivityPacked, source.lastActivityPacked,
CONTEST_TIME_WRAP) (or the project's equivalent helper) to decide which packed
timestamp to keep and then call territoryRenderer.setContestTime(target.id,
target.lastActivityPacked) with the selected value.
In @src/core/game/GameView.ts:
- Around line 885-887: The method setDefended declares a void return but uses
"return this._map.setDefended(ref, value);", which is unnecessary; change
setDefended(ref: TileRef, value: boolean): void to call
this._map.setDefended(ref, value); without returning its value so the function
matches its void signature and removes the redundant return. Ensure you update
the implementation in the GameView class (setDefended) and run type checks.
🧹 Nitpick comments (4)
src/client/graphics/layers/TerritoryLayer.ts (2)
601-614: Allocating large typed arrays on every call if size mismatches.
ensureContestScratchallocates four large typed arrays when size changes. This is fine for initialization, but consider logging or validating that this only happens once per map load to catch unexpected re-allocations during gameplay.
946-961: Non-null assertions on contest arrays are fragile.Methods like
addTileToComponentuse!assertions oncontestTileIndices,contestPrevOwners, etc. IfensureContestScratchwas not called or failed silently, these would throw at runtime.Consider using a guard or a single nullable container object that is checked once:
Example pattern
type ContestScratch = { componentIds: Uint16Array; prevOwners: Uint16Array; attackers: Uint16Array; tileIndices: Int32Array; }; private contestScratch: ContestScratch | null = null; private getContestScratch(): ContestScratch { if (!this.contestScratch) { throw new Error("Contest scratch not initialized"); } return this.contestScratch; }This consolidates the null check to one place and makes the intent clearer.
src/client/graphics/HoverInfo.ts (1)
61-70: Consider naming the magic number.The threshold
50for ship detection radius could be a named constant for clarity (e.g.,SHIP_HOVER_RADIUS). Not blocking, just a readability note.♻️ Optional: Extract constant
+const SHIP_HOVER_RADIUS = 50; + export function getHoverInfo( game: GameView, worldCoord: { x: number; y: number }, ): HoverInfo { // ... if (!game.isLand(tile)) { const units = game .units(UnitType.Warship, UnitType.TradeShip, UnitType.TransportShip) - .filter((u) => euclideanDistWorld(worldCoord, u.tile(), game) < 50) + .filter((u) => euclideanDistWorld(worldCoord, u.tile(), game) < SHIP_HOVER_RADIUS) .sort(distSortUnitWorld(worldCoord, game));src/core/game/GameImpl.ts (1)
1072-1094: LGTM – tile ownership change triggers defended state update.The logic correctly handles both:
- Updating the conquered tile's defended state based on new owner's defense posts
- Propagating updates if the conquered tile contains a defense post (line 1089-1093)
One minor observation:
hasUnitNearby(tile, 0, ...)with range 0 checks if a unit is at that exact tile. This works but the intent is slightly obscured. Consider adding a brief inline comment or extracting to ahasDefensePostAtTilehelper if this pattern repeats elsewhere.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/client/graphics/GameRenderer.tssrc/client/graphics/HoverInfo.tssrc/client/graphics/TransformHandler.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/TerrainLayer.tssrc/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/TerritoryWebGLRenderer.tssrc/core/configuration/Config.tssrc/core/configuration/PastelTheme.tssrc/core/configuration/PastelThemeDark.tssrc/core/game/GameImpl.tssrc/core/game/GameMap.tssrc/core/game/GameView.tssrc/core/game/UnitImpl.ts
💤 Files with no reviewable changes (2)
- src/client/graphics/layers/TerrainLayer.ts
- src/client/graphics/GameRenderer.ts
🧰 Additional context used
🧠 Learnings (19)
📓 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.
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
📚 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/configuration/PastelThemeDark.tssrc/core/configuration/Config.tssrc/core/configuration/PastelTheme.tssrc/client/graphics/layers/TerritoryLayer.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/configuration/PastelThemeDark.tssrc/core/configuration/Config.tssrc/core/configuration/PastelTheme.tssrc/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-06T21:43:15.706Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/Colors.ts:398-411
Timestamp: 2025-06-06T21:43:15.706Z
Learning: In ColorAllocator class, the assignPlayerColor method intentionally allows color duplicates when the humanColors palette is exhausted by resetting availableColors to the full humanColors array. This is done on purpose as an acceptable temporary limitation, with potential palette improvements planned for later.
Applied to files:
src/core/configuration/PastelThemeDark.tssrc/core/configuration/Config.tssrc/core/configuration/PastelTheme.tssrc/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-06T15:42:21.903Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:20-21
Timestamp: 2025-06-06T15:42:21.903Z
Learning: In ColorAllocator class, all methods (assignBotColor, assignPlayerColor, assignTeamColor) use the constructor colors from this.availableColors. Only the predefined team cases in assignTeamColor use hardcoded global colors, while the default case uses constructor colors.
Applied to files:
src/core/configuration/Config.tssrc/core/configuration/PastelTheme.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/game/UnitImpl.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameMap.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/game/GameImpl.tssrc/core/game/GameView.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/GameMap.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/game/GameView.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/game/GameMap.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/core/game/GameMap.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/core/configuration/PastelTheme.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/core/game/GameImpl.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/core/game/GameView.ts
📚 Learning: 2025-11-29T22:22:37.178Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.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/configuration/PastelTheme.tssrc/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-11-26T22:27:31.844Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
Applied to files:
src/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/core/game/GameImpl.ts
📚 Learning: 2025-08-20T10:06:20.469Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:551-568
Timestamp: 2025-08-20T10:06:20.469Z
Learning: Defense posts in OpenFrontIO provide area-of-effect defensive bonuses (5x defense, 3x speed) to any tile being attacked within 30 tiles of the post. They should be positioned with strategic depth - close enough to cover borders but far enough back (around 10-20 tiles) to avoid immediate capture by attackers.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-12-04T15:45:21.217Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2548
File: src/core/game/GameImpl.ts:0-0
Timestamp: 2025-12-04T15:45:21.217Z
Learning: In OpenFrontIO, UnitImpl.delete() marks units as inactive before calling GameImpl.removeUnit, and UnitGrid.hasUnitNearby ignores inactive units. This means units being removed are already "invisible" to nearby unit checks during the removal process.
Applied to files:
src/core/game/GameImpl.ts
🧬 Code graph analysis (6)
src/core/game/GameMap.ts (2)
src/core/game/GameImpl.ts (1)
ref(866-868)src/core/game/GameView.ts (1)
ref(825-827)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
src/client/graphics/HoverInfo.ts (1)
getHoverInfo(32-73)
src/client/graphics/HoverInfo.ts (2)
src/core/game/GameView.ts (3)
PlayerView(185-582)UnitView(48-183)GameView(584-953)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/layers/TerritoryLayer.ts (2)
src/core/game/GameMap.ts (4)
TileRef(3-3)width(144-146)height(147-149)y(136-138)src/client/graphics/HoverInfo.ts (1)
getHoverInfo(32-73)
src/core/game/GameImpl.ts (2)
src/core/game/PlayerImpl.ts (1)
PlayerImpl(65-1257)src/core/game/GameMap.ts (3)
ref(121-126)TileRef(3-3)isDefended(219-221)
src/core/game/GameView.ts (2)
src/core/game/GameMap.ts (2)
TileRef(3-3)ref(121-126)src/core/game/GameImpl.ts (1)
ref(866-868)
🪛 Biome (2.1.2)
src/core/game/GameView.ts
[error] 886-887: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (29)
src/core/configuration/Config.ts (1)
204-204: LGTM!The new
playerHighlightColor()method fits well with the existing color accessor pattern in the Theme interface. Clear naming and correct return type.src/core/configuration/PastelTheme.ts (1)
38-39: LGTM!The new highlight color field and accessor follow the same pattern as existing color properties like
_selfColorand_spawnHighlightColor. Consistent and easy to read.Also applies to: 214-216
src/core/configuration/PastelThemeDark.ts (1)
11-11: Verify highlight color consistency between themes.The light theme uses neutral gray
rgb(221, 221, 221)while the dark theme uses a reddish-brownrgb(99, 42, 42). These have different visual character - gray is neutral, while red-brown feels warmer or more alert.If intentional for dark mode visibility, this is fine. If not, consider a darker neutral gray like
rgb(80, 80, 80)to keep the highlight semantically consistent across themes.Also applies to: 64-66
src/client/graphics/layers/TerritoryLayer.ts (6)
1076-1081:contestElapsedwrap logic looks correct.The elapsed calculation properly handles the wrap-around case. This is the correct pattern to use elsewhere when comparing packed timestamps (see previous comment about
mergeContestComponents).
1122-1129: Palette signature is simple but effective.The signature string combines player count, max small ID, and pattern toggle. This catches the common cases for palette invalidation without over-complicating.
1142-1186: Debug overlay is well-structured but should be gated.The overlay itself is nicely formatted and provides useful stats. However, since
DEBUG_TERRITORY_OVERLAYistrue, this will render in production. Once the flag is fixed, this code is fine.
480-517: WebGL canvas composition approach looks good.Drawing the WebGL canvas in screen space while keeping overlays in world space is a clean separation. The
save/restorepattern with identity transform is correct.
380-408: Hover territory detection integrates cleanly with getHoverInfo.Using the shared
getHoverInfohelper consolidates the hover logic and correctly handles both player tiles and ships. The renderer update on hover change is efficient.
732-732: Use string keys or add validation for attacker/defender pair mapping.The bitshift operation
(component.attacker << 16) | component.defenderassumes bothsmallIDvalues fit in 16 bits (max 65535). SincenextPlayerIDincrements without bound and has no enforced maximum, IDs can grow larger, causing bitshift collisions. Either use a string key like${attacker}-${defender}or validate thatsmallIDvalues stay below 65536.⛔ Skipped due to learnings
Learnt from: scamiv Repo: openfrontio/OpenFrontIO PR: 2607 File: src/core/execution/PlayerExecution.ts:271-295 Timestamp: 2025-12-13T14:58:29.645Z Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.Learnt from: El-Magico777 Repo: openfrontio/OpenFrontIO PR: 1166 File: src/core/Schemas.ts:50-56 Timestamp: 2025-06-13T20:57:49.599Z Learning: In the OpenFrontIO codebase, player identification primarily uses numeric smallIDs throughout the system (e.g., playerBySmallID). When adding new intents or schemas, maintain consistency with this existing pattern rather than introducing mixed ID types, even if string IDs might be more standard elsewhere.src/client/graphics/TransformHandler.ts (1)
48-50: LGTM!Simple getter exposing the view offset for external use. Clean and correct.
src/core/game/UnitImpl.ts (1)
340-348: LGTM!Good addition. When a DefensePost changes construction state, refreshing its defended status makes sense. The check runs before broadcasting the update, so state is consistent.
src/client/graphics/layers/PlayerInfoOverlay.ts (2)
21-21: Good refactor.Centralizing hover logic in
getHoverInfokeeps this overlay focused on rendering.
98-117: Clean implementation.The
maybeShowmethod is now easier to read. The branching oninfo.player,info.isWilderness, andinfo.unitis clear and follows a sensible priority order.src/client/graphics/HoverInfo.ts (3)
5-10: Clean type definition.Using a plain object type with nullable fields is a good approach - simple and easy to understand.
12-30: Helper functions look correct.
euclideanDistWorldanddistSortUnitWorldare straightforward distance utilities. Good that these are module-private (not exported).
32-73: Logic flow is clear and correct.The function handles each case well:
- Invalid coords → default info
- Player-owned tile → return player
- Non-player owned land → wilderness/irradiated
- Water → search for nearby ships
Good centralization of hover logic.
src/core/game/GameMap.ts (3)
30-33: Interface additions look good.New methods for defense state and direct array access. Clean extension of the interface.
83-86: Bit allocation is correct.
DEFENDED_BIT = 12fits nicely after the 12-bit player ID mask (bits 0-11), with FALLOUT_BIT at 13 and DEFENSE_BONUS_BIT at 14. No overlap.
219-237: Implementation is correct.The bit manipulation for
isDefended/setDefendedfollows the same pattern ashasFallout/setFallout.Note:
tileStateView()returns the mutablestatearray directly. External code could modify it, bypassing setters. This is intentional for WebGL performance (avoiding copies), but callers should treat it as read-only.src/core/game/GameView.ts (4)
590-594: LGTM – clean tracking structure for ownership deltas.The typed object structure with
tile,previousOwner,newOwneris clear and easy to consume.
714-720: LGTM – accessor exposes ownership deltas cleanly.Simple delegation to the private array. Good for consumption by renderers tracking ownership transitions.
888-893: LGTM – view accessors expose underlying map data.Useful for WebGL renderers needing direct typed array access.
645-655: The code is correct for the current implementation.The
packedTileUpdatesarray always contains packedTileUpdatevalues in format(tileRef << 16n | state), as confirmed byGameMapImpl.toTileUpdate(). The right-shifttu >> 16ncorrectly extracts the tile reference.The suggestion to check
usesSharedTileStateis not applicable—this property does not exist onGameUpdateViewData, and SAB mode is not implemented in the current codebase. The learning referenced applies to a different PR, not this code.src/core/game/GameImpl.ts (6)
577-577: LGTM – defended state update on tile conquest.Correctly integrates the defense post state update after setting ownership and updating borders.
599-601: LGTM – clean defended flag cleanup on relinquish.Good practice to check before setting to avoid unnecessary updates.
825-829: LGTM – public refresh hook for defense post state.Useful for external callers (like UnitImpl) to trigger state re-evaluation.
984-989: LGTM – view accessors for renderer access.Clean delegation to the underlying map.
1042-1067: LGTM – defense post range calculation for border tiles.The logic correctly:
- Gets range from config and precomputes squared value
- Iterates only border tiles (not all tiles) for performance
- Uses
hasUnitNearbyto check for any defense post coverage- Only emits tile updates when state actually changes
923-925: Remove unnecessaryreturnin void function.Same issue as in GameView.ts – the
returnkeyword is not needed for a void function.🔧 Proposed fix
setDefended(ref: TileRef, value: boolean): void { - return this._map.setDefended(ref, value); + this._map.setDefended(ref, value); }⛔ Skipped due to learnings
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()`.
| const CONTEST_STRENGTH_EMA_ALPHA = 0.8; | ||
| const CONTEST_STRENGTH_MIN = 0.01; | ||
| const CONTEST_STRENGTH_MAX = 0.95; | ||
| const DEBUG_TERRITORY_OVERLAY = true; |
There was a problem hiding this comment.
Debug flag left enabled in production code.
DEBUG_TERRITORY_OVERLAY is set to true, which will render a debug overlay on every frame. This should be false before merging.
Suggested fix
-const DEBUG_TERRITORY_OVERLAY = true;
+const DEBUG_TERRITORY_OVERLAY = false;📝 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.
| const DEBUG_TERRITORY_OVERLAY = true; | |
| const DEBUG_TERRITORY_OVERLAY = false; |
🤖 Prompt for AI Agents
In @src/client/graphics/layers/TerritoryLayer.ts at line 28, The
DEBUG_TERRITORY_OVERLAY constant is currently true and enabling a debug overlay
in production; change the definition of DEBUG_TERRITORY_OVERLAY to false (or
wire it to a development-only flag/env variable) so the overlay is disabled by
default, updating the constant declaration where DEBUG_TERRITORY_OVERLAY is
defined and any tests or dev-only paths that expect it to remain true.
| private configureRenderers() { | ||
| const { renderer, reason } = TerritoryWebGLRenderer.create( | ||
| this.game, | ||
| this.theme, | ||
| ); | ||
| if (!renderer) { | ||
| throw new Error(reason ?? "WebGL2 is required for territory rendering."); | ||
| } | ||
|
|
||
| this.territoryRenderer = renderer; | ||
| this.territoryRenderer.setContestEnabled(this.contestEnabled); | ||
| this.territoryRenderer.setAlternativeView(this.alternativeView); | ||
| this.territoryRenderer.markAllDirty(); | ||
| this.territoryRenderer.refreshPalette(); | ||
| this.territoryRenderer.setHoverHighlightOptions( | ||
| this.hoverHighlightOptions(), | ||
| ); | ||
| this.territoryRenderer.setHoveredPlayerId( | ||
| this.highlightedTerritory?.smallID() ?? null, | ||
| ); | ||
| this.lastPaletteSignature = this.computePaletteSignature(); |
There was a problem hiding this comment.
Throwing on WebGL2 failure prevents graceful degradation.
If WebGL2 is unavailable, configureRenderers throws an error. This could crash the app on older devices or browsers. Consider a fallback path or at least a user-friendly error message instead of a hard crash.
Suggested approach
private configureRenderers() {
const { renderer, reason } = TerritoryWebGLRenderer.create(
this.game,
this.theme,
);
if (!renderer) {
- throw new Error(reason ?? "WebGL2 is required for territory rendering.");
+ console.error(
+ `Territory rendering unavailable: ${reason ?? "WebGL2 not supported"}`,
+ );
+ this.territoryRenderer = null;
+ return;
}Then guard renderLayer and other methods to handle territoryRenderer === null gracefully (some guards already exist).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private configureRenderers() { | |
| const { renderer, reason } = TerritoryWebGLRenderer.create( | |
| this.game, | |
| this.theme, | |
| ); | |
| if (!renderer) { | |
| throw new Error(reason ?? "WebGL2 is required for territory rendering."); | |
| } | |
| this.territoryRenderer = renderer; | |
| this.territoryRenderer.setContestEnabled(this.contestEnabled); | |
| this.territoryRenderer.setAlternativeView(this.alternativeView); | |
| this.territoryRenderer.markAllDirty(); | |
| this.territoryRenderer.refreshPalette(); | |
| this.territoryRenderer.setHoverHighlightOptions( | |
| this.hoverHighlightOptions(), | |
| ); | |
| this.territoryRenderer.setHoveredPlayerId( | |
| this.highlightedTerritory?.smallID() ?? null, | |
| ); | |
| this.lastPaletteSignature = this.computePaletteSignature(); | |
| private configureRenderers() { | |
| const { renderer, reason } = TerritoryWebGLRenderer.create( | |
| this.game, | |
| this.theme, | |
| ); | |
| if (!renderer) { | |
| console.error( | |
| `Territory rendering unavailable: ${reason ?? "WebGL2 not supported"}`, | |
| ); | |
| this.territoryRenderer = null; | |
| return; | |
| } | |
| this.territoryRenderer = renderer; | |
| this.territoryRenderer.setContestEnabled(this.contestEnabled); | |
| this.territoryRenderer.setAlternativeView(this.alternativeView); | |
| this.territoryRenderer.markAllDirty(); | |
| this.territoryRenderer.refreshPalette(); | |
| this.territoryRenderer.setHoverHighlightOptions( | |
| this.hoverHighlightOptions(), | |
| ); | |
| this.territoryRenderer.setHoveredPlayerId( | |
| this.highlightedTerritory?.smallID() ?? null, | |
| ); | |
| this.lastPaletteSignature = this.computePaletteSignature(); |
| target.lastActivityPacked = Math.max( | ||
| target.lastActivityPacked, | ||
| source.lastActivityPacked, | ||
| ); | ||
| this.territoryRenderer?.setContestTime( | ||
| target.id, | ||
| target.lastActivityPacked, | ||
| ); |
There was a problem hiding this comment.
Math.max on packed timestamps breaks near wrap boundary.
Packed timestamps wrap at CONTEST_TIME_WRAP (32768). Using Math.max to pick the latest timestamp fails when one value is near zero (just wrapped) and another is near 32767. The older value would incorrectly appear larger.
Suggested fix
- target.lastActivityPacked = Math.max(
- target.lastActivityPacked,
- source.lastActivityPacked,
- );
+ // Pick the more recent packed timestamp, accounting for wrap-around.
+ const elapsed = this.contestElapsed(target.lastActivityPacked, source.lastActivityPacked);
+ if (elapsed > CONTEST_TIME_WRAP / 2) {
+ // source is actually more recent
+ target.lastActivityPacked = source.lastActivityPacked;
+ }This uses the existing contestElapsed logic to correctly handle the wrap-around comparison.
🤖 Prompt for AI Agents
In @src/client/graphics/layers/TerritoryLayer.ts around lines 1011 - 1018, The
comparison using Math.max on packed timestamps (target.lastActivityPacked and
source.lastActivityPacked) fails across the wrap boundary; replace the Math.max
usage with the existing contestElapsed-based comparison so the code chooses the
newer timestamp correctly across wrap-around, e.g., use
contestElapsed(target.lastActivityPacked, source.lastActivityPacked,
CONTEST_TIME_WRAP) (or the project's equivalent helper) to decide which packed
timestamp to keep and then call territoryRenderer.setContestTime(target.id,
target.lastActivityPacked) with the selected value.
| if (u.type() === UnitType.DefensePost) { | ||
| this.updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl); | ||
| } | ||
| } | ||
| removeUnit(u: Unit) { | ||
| this.unitGrid.removeUnit(u); | ||
| if (u.type() === UnitType.DefensePost) { | ||
| this.updateDefendedStateForDefensePost(u.tile(), u.owner() as PlayerImpl); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find captureUnit implementation
rg -n "captureUnit" --type=ts -g '!*.test.ts' -A 8Repository: openfrontio/OpenFrontIO
Length of output: 4026
🏁 Script executed:
#!/bin/bash
# Search for the PlayerExecution file mentioned in learnings
fd "PlayerExecution" --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Look for unit removal/addition patterns during capture
rg -n "removeUnit|addUnit" --type=ts -g '!*.test.ts' -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 3154
DefensePost defended state is not updated when captured.
When captureUnit() is called (PlayerImpl.ts:859), it only transfers ownership via unit.setOwner() without removing and re-adding the unit. This means the addUnit() and removeUnit() hooks in GameImpl.ts (lines 806-814) never fire, so updateDefendedStateForDefensePost() is never called. After a DefensePost is captured, the old owner's tiles retain stale defended state while the new owner's tiles lack the correct defended state. Update the capture flow to properly refresh defended state for both the old and new owner.
| setDefended(ref: TileRef, value: boolean): void { | ||
| return this._map.setDefended(ref, value); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary return in void function.
The static analyzer correctly flags this: setDefended declares void return but uses return. The returned value from _map.setDefended is ignored by callers anyway.
🔧 Proposed fix
setDefended(ref: TileRef, value: boolean): void {
- return this._map.setDefended(ref, value);
+ this._map.setDefended(ref, value);
}🧰 Tools
🪛 Biome (2.1.2)
[error] 886-887: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🤖 Prompt for AI Agents
In @src/core/game/GameView.ts around lines 885 - 887, The method setDefended
declares a void return but uses "return this._map.setDefended(ref, value);",
which is unnecessary; change setDefended(ref: TileRef, value: boolean): void to
call this._map.setDefended(ref, value); without returning its value so the
function matches its void signature and removes the redundant return. Ensure you
update the implementation in the GameView class (setDefended) and run type
checks.
- Updated seed distance calculations to check for valid seeds before applying smooth logic, enhancing performance. - Simplified color mixing and edge handling based on the presence of old and new seeds, improving visual fidelity during transitions. - Streamlined shader logic to reduce unnecessary computations when no changes are detected.
- Updated seed validation checks to simplify smooth animation handling. - Adjusted comments for clarity regarding seed conditions and animation behavior. - Enhanced performance by skipping unnecessary computations when seed data is invalid.
- Adjusted shader logic to only draw borders in alternative view - Updated comments for better understanding of rendering behavior in alternative view mode. - Simplified color blending
- Introduced hoverStartTime to track the duration of player hover highlights. - Implemented logic to disable hover highlights after a specified duration of 5000 ms. - Updated setHoveredPlayerId method to reset hoverStartTime when a new player is hovered.
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