Skip to content

Conversation

@bibizu
Copy link
Contributor

@bibizu bibizu commented Jan 17, 2026

Description:

Adds a new layer, NukeRenderUtilLayer, which does not draw anything but consolidates all nuke prediction code to be used by trajectory drawing layer, SAM radii drawing layer, and structure icon layer rather than recalculated each time. The layer is calculated first, and a reference of this layer is passed to all layers needing it.

pr_ex.1.mp4

SAM radii now change color/transparency when in "nuke mode":

  • Opaque and highlighted radii means the owner will intercept this nuke.
  • Other opaque radii means that the owner is "stressed" by this nuke, and launching it will break alliances, or owner is enemy.
  • Orange radii symbolize SAMs who are friendly but will break alliance if nuke is launched.

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

bibizu

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 17, 2026

Walkthrough

A new NukeRenderUtilLayer centralizes nuke ghost state, trajectory/spawn selection, interceptor detection, and sets of affected players/structures. GameRenderer instantiates and injects it into StructureIconsLayer, SAMRadiusLayer, and NukeTrajectoryPreviewLayer; several constructors and core util functions were updated to use the new API.

Changes

Cohort / File(s) Summary
Core utilities & execution
src/core/execution/Util.ts, src/core/execution/NukeExecution.ts
Added listAffectedByNuke(params: NukeAllianceCheckParams): AffectedByNuke and widened NukeAllianceCheckParams to accept `Game
New render util layer
src/client/graphics/layers/NukeRenderUtilLayer.ts
New NukeRenderUtilLayer class that listens for mouse/ghost/rocket events, computes target/spawn, trajectory points, untargetable segment bounds, interceptor SAMs/players, and affected players/structures. Exposes lifecycle methods and getters: init(), tick(), renderLayer(), isNukeGhostActive(), getAffectedPlayers(), getAffectedStructures(), getInterceptingPlayers(), getInterceptingSAMs(), getTrajectoryInfo().
Renderer wiring
src/client/graphics/GameRenderer.ts
Instantiates NukeRenderUtilLayer and inserts it early in the layers array; passes it into dependent layers (StructureIconsLayer, SAMRadiusLayer, NukeTrajectoryPreviewLayer).
Trajectory preview simplified
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
Constructor changed to (game, nukeRenderUtilLayer); removed internal event/trajectory state and now reads trajectory/target data from NukeRenderUtilLayer for rendering.
SAM radius rendering
src/client/graphics/layers/SAMRadiusLayer.ts
Constructor now accepts NukeRenderUtilLayer; added theme use and AlternateViewEvent handling; rendering adapts outline/inline colors for nuke-mode using data from NukeRenderUtilLayer (stressed/intercepting SAMs/players).
Structure icons nuke logic
src/client/graphics/layers/StructureIconsLayer.ts
Injects NukeRenderUtilLayer; replaces previous would-nuke-alliance checks with NukeRenderUtilLayer.getAffectedPlayers() membership for targeting decisions; minor event binding change (ToggleStructuresEventToggleStructureEvent) and guard in modifyVisibility.

Sequence Diagram(s)

sequenceDiagram
  participant GR as GameRenderer
  participant EB as EventBus
  participant NR as NukeRenderUtilLayer
  participant CV as CanvasLoop
  participant SR as SAMRadiusLayer
  participant SI as StructureIconsLayer
  participant NT as NukeTrajectoryPreviewLayer

  GR->>NR: instantiate NR(game, eventBus, uiState, transform)
  GR->>SR: construct(..., nukeRenderUtilLayer)
  GR->>SI: construct(..., nukeRenderUtilLayer)
  GR->>NT: construct(game, nukeRenderUtilLayer)

  EB->>NR: MouseMove / GhostStructureChanged / SwapRocketDirection
  NR->>NR: compute target, spawn, trajectory, interceptors, affected players/structures

  CV->>NR: tick() / renderLayer()
  CV->>SR: render() -> NR.isNukeGhostActive(), NR.getInterceptingPlayers(), NR.getInterceptingSAMs()
  CV->>SI: render() -> NR.isNukeGhostActive(), NR.getAffectedPlayers()
  CV->>NT: render() -> NR.getTrajectoryInfo()
  SR->>CV: draw SAM arcs (theme + nuke-mode)
  SI->>CV: draw ghost icons (ally/targeting)
  NT->>CV: draw trajectory preview
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

A single layer hears the mouse and sky,
Maps spawn, path, who’ll duck or cry,
Renderers sip that shared report,
SAMs and icons draw from one court,
Small code change, clearer sky.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main changes: introduction of NukeRenderUtilLayer and SAM color/transparency updates based on nuke prediction.
Description check ✅ Passed The description is directly related to the changeset, explaining the new NukeRenderUtilLayer consolidation approach and the SAM visual feedback system in nuke mode.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/NukeRenderUtilLayer.ts`:
- Around line 255-259: The early return in renderLayer (after checking
this.findTargetTile() === null || !this.spawnTile) leaves stale state in
this.allianceStressedPlayers and this.interceptingPlayers; update the block so
that before returning you also clear this.allianceStressedPlayers = [] and
this.interceptingPlayers = [] (and keep clearing this.trajectoryPoints = [] as
already done) so no old colors or intercept markers remain when there is no
valid target or spawnTile.
- Around line 54-64: When ghost structure changes (in the
GhostStructureChangedEvent handler) reset the cached target by setting
lastTargetTile = null (like SwapRocketDirectionEvent does) and also make
trajectoryPreviewTick resilient to stale async results by recording the
currentGhostStructure (or a request ID) before awaiting player.actions() and,
after the await, verify that currentGhostStructure (and any relevant request ID)
still matches before assigning spawnTile; update both the
GhostStructureChangedEvent handler (references: currentGhostStructure,
nukeGhostActive, lastTargetTile) and the async logic inside
trajectoryPreviewTick (references: player.actions(), spawnTile) — apply the same
guard for the other similar block noted (lines ~105-127).
🧹 Nitpick comments (1)
src/client/graphics/layers/SAMRadiusLayer.ts (1)

274-349: Check intercepting opacity for allies/self.
In nuke mode, friendly/self inline strokes stay translucent unless stressed is true. If getInterceptingPlayers() can include allies or self, the intercepting cue becomes too subtle. Please confirm the set excludes them; if not, make intercepting force full opacity. Also, nukeMode / stressed / intercepting are constant per SAM, so you can compute them once outside the arc loop to avoid repeated lookups.

Possible tweak if intercepting can include allies/self
-        if (a.owner.isMe()) {
-          ctx.strokeStyle = lineColorSelf.alpha(0.3).toRgbString();
-        } else if (this.game.myPlayer()?.isFriendly(a.owner)) {
-          if (stressed) {
-            ctx.strokeStyle = lineColorStressed.toRgbString();
-          } else {
-            ctx.strokeStyle = lineColorFriend.alpha(0.3).toRgbString();
-          }
-        } else {
-          if (intercepting || stressed) {
-            ctx.strokeStyle = lineColorEnemy.toRgbString();
-          } else {
-            ctx.strokeStyle = lineColorEnemy.alpha(0.3).toRgbString();
-          }
-        }
+        if (a.owner.isMe()) {
+          ctx.strokeStyle = (
+            intercepting ? lineColorSelf : lineColorSelf.alpha(0.3)
+          ).toRgbString();
+        } else if (this.game.myPlayer()?.isFriendly(a.owner)) {
+          if (stressed) {
+            ctx.strokeStyle = lineColorStressed.toRgbString();
+          } else if (intercepting) {
+            ctx.strokeStyle = lineColorFriend.toRgbString();
+          } else {
+            ctx.strokeStyle = lineColorFriend.alpha(0.3).toRgbString();
+          }
+        } else {
+          if (intercepting || stressed) {
+            ctx.strokeStyle = lineColorEnemy.toRgbString();
+          } else {
+            ctx.strokeStyle = lineColorEnemy.alpha(0.3).toRgbString();
+          }
+        }

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/NukeRenderUtilLayer.ts`:
- Around line 273-276: When the early return after calling this.game.myPlayer()
executes, ensure you clear any lingering state to avoid stale data: reset
this.allianceStressedPlayers and any trajectory-related state (e.g.
this.trajectory*, trajectoryPoints, collision/preview state) before returning.
Update the method in NukeRenderUtilLayer where you call this.game.myPlayer() so
that on !player you explicitly clear those fields and then return.
🧹 Nitpick comments (1)
src/client/graphics/layers/NukeRenderUtilLayer.ts (1)

267-267: Prefix unused parameter with underscore.

The context parameter is intentionally unused since this is a utility layer that doesn't draw. Prefix it with underscore to signal intent and silence linter warnings.

Suggested fix
-  renderLayer(context: CanvasRenderingContext2D) {
+  renderLayer(_context: CanvasRenderingContext2D) {

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
@bibizu bibizu marked this pull request as ready for review January 20, 2026 17:38
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
@evanpelle evanpelle requested a review from a team as a code owner January 22, 2026 23:08
@evanpelle evanpelle added this to the v30 milestone Jan 22, 2026
Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the friendly SAMs yellow, since that's what we use for friendly trade ships

Comment on lines +273 to +274
new SAMRadiusLayer(game, eventBus, uiState, nukeRenderUtilLayer),
new NukeTrajectoryPreviewLayer(game, nukeRenderUtilLayer),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a bit cleaner to merge these two layers into one layer, and then that layer has a nukeRenderUtil layer that it calls into?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure Icon Layer was also running more or less the same logic, so there are three layers that all require the same calculation. This also makes it easy if I want to add more layers as well needing it as well. (such as I might wanna work on showing which structures in StructureLayer get destroyed by the nuke).

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 25, 2026
@bibizu bibizu marked this pull request as draft January 26, 2026 00:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/NukeRenderUtilLayer.ts`:
- Around line 226-229: The loop that jumps over the untargetable segment in
NukeRenderUtilLayer uses this.untargetableSegmentBounds[1] without validating
it; guard against an invalid second bound (e.g., -1) before assigning i =
this.untargetableSegmentBounds[1] - 1. Update the code around the jump (where
untargetableSegmentBounds is referenced) to check that
untargetableSegmentBounds[1] is a valid index (> -1 and >
untargetableSegmentBounds[0]) and only then set i = untargetableSegmentBounds[1]
- 1; otherwise skip the jump (e.g., continue or break) so the loop does not
reset to negative indices and reprocess points.
- Around line 239-254: In NukeRenderUtilLayer, remove the intermediate
canvas-relative coordinate calculation (localX/localY) and call
transformHandler.screenToWorldCoordinates with the raw screen coordinates
this.mousePos.x and this.mousePos.y, then use the returned worldCoords with
game.isValidCoord and game.ref to set this.targetTile; also apply the same
change in StructureIconsLayer by passing raw this.mousePos values into
screenToWorldCoordinates and deleting the manual rect subtraction so the canvas
offset isn’t subtracted twice.

In `@src/client/graphics/layers/SAMRadiusLayer.ts`:
- Around line 283-286: The variable lineColorStressed currently calls
this.theme.allyColor() making it identical to lineColorFriend; either add a
stressedColor() method to the Theme interface and implement it in concrete theme
classes, then change lineColorStressed to this.theme.stressedColor(), or remove
the separate variable and instead apply the stressed opacity/alpha when
rendering (use this.theme.allyColor() at draw time and modulate its alpha for
stressed vs non-stressed). Update any references to lineColorStressed and the
Theme interface accordingly to keep types consistent.
🧹 Nitpick comments (4)
src/client/graphics/layers/NukeRenderUtilLayer.ts (2)

109-134: Consider logging errors in the catch handler.

The .catch() silently swallows errors. While this prevents crashes, it can hide issues during development or debugging. Consider adding a debug log or using a conditional log based on environment.

♻️ Suggested improvement
       .catch(() => {
-        // Handle errors silently
+        // Handle errors silently in production
+        if (process.env.NODE_ENV === 'development') {
+          console.warn('Failed to get player actions for nuke trajectory');
+        }
         this.spawnTile = null;
       });

292-297: Use already-fetched player variable instead of calling myPlayer() again.

Line 279 already fetched player with a null check. Lines 294 uses this.game.myPlayer()?.allies() which is redundant and adds unnecessary optional chaining.

♻️ Suggested simplification
       allySmallIds: new Set(
-        this.game
-          .myPlayer()
-          ?.allies()
-          .map((a) => a.smallID()),
+        player.allies().map((a) => a.smallID()),
       ),
src/client/graphics/layers/StructureIconsLayer.ts (1)

264-276: Consider using .some() instead of .filter().length !== 0.

Using .some() is clearer and exits early when a match is found, avoiding unnecessary iterations.

♻️ Suggested improvement
       if (allies.length > 0) {
-        targetingAlly =
-          allies.filter((p) =>
-            this.nukeRenderUtilLayer.getAffectedPlayers().has(p.smallID()),
-          ).length !== 0;
+        targetingAlly = allies.some((p) =>
+          this.nukeRenderUtilLayer.getAffectedPlayers().has(p.smallID()),
+        );
       }
src/core/execution/Util.ts (1)

38-45: Remove unused allySmallIds parameter from NukeAllianceCheckParams.

The allySmallIds field is declared in the interface but listAffectedByNuke never reads it. Callers are computing and passing this value unnecessarily. Remove it from the interface and update the call sites in NukeExecution.ts and NukeRenderUtilLayer.ts to keep the API clean and avoid wasted computation.

Comment on lines +239 to +254
// Convert mouse position to world coordinates
const rect = this.transformHandler.boundingRect();
if (!rect) {
return null;
}
const localX = this.mousePos.x - rect.left;
const localY = this.mousePos.y - rect.top;
const worldCoords = this.transformHandler.screenToWorldCoordinates(
localX,
localY,
);
if (!this.game.isValidCoord(worldCoords.x, worldCoords.y)) {
return null;
}
this.targetTile = this.game.ref(worldCoords.x, worldCoords.y);
return this.targetTile;
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find and read the full findTargetTile method in NukeRenderUtilLayer.ts
fd NukeRenderUtilLayer.ts --type f

Repository: openfrontio/OpenFrontIO

Length of output: 116


🏁 Script executed:

# Find TransformHandler to understand screenToWorldCoordinates implementation
fd -e ts | xargs rg -l 'class TransformHandler|screenToWorldCoordinates' | head -5

Repository: openfrontio/OpenFrontIO

Length of output: 299


🏁 Script executed:

# Find StructureIconsLayer.ts and check the renderGhost method mentioned
fd StructureIconsLayer.ts --type f

Repository: openfrontio/OpenFrontIO

Length of output: 116


🏁 Script executed:

# Read the full findTargetTile method and surrounding context
rg -n 'findTargetTile' src/client/graphics/layers/NukeRenderUtilLayer.ts -B5 -A30

Repository: openfrontio/OpenFrontIO

Length of output: 3670


🏁 Script executed:

# Find where TransformHandler is defined
rg -n 'class TransformHandler|interface TransformHandler' --type=ts

Repository: openfrontio/OpenFrontIO

Length of output: 141


🏁 Script executed:

# Find the screenToWorldCoordinates method implementation
rg -n 'screenToWorldCoordinates' --type=ts -B2 -A15 | head -80

Repository: openfrontio/OpenFrontIO

Length of output: 6045


🏁 Script executed:

# Now check StructureIconsLayer.ts renderGhost method mentioned in the review
rg -n 'renderGhost' src/client/graphics/layers/StructureIconsLayer.ts -B5 -A20

Repository: openfrontio/OpenFrontIO

Length of output: 1662


🏁 Script executed:

# Also check all usages of screenToWorldCoordinates to see the pattern
rg -n 'screenToWorldCoordinates' --type=ts -B3 -A3 | grep -E "(\.x|\.y|-|screenToWorldCoordinates)"

Repository: openfrontio/OpenFrontIO

Length of output: 7540


🏁 Script executed:

# Verify the exact context - check if rect is the canvas bounding rect
rg -n 'boundingRect' src/client/graphics/TransformHandler.ts -B2 -A8

Repository: openfrontio/OpenFrontIO

Length of output: 4010


Remove the intermediate canvas-relative coordinate calculation—pass raw screen coordinates directly to screenToWorldCoordinates().

The method expects raw screen coordinates and internally subtracts the canvas bounding rect. Lines 244–245 already subtract rect.left/top, causing those offsets to be subtracted twice: once here, and again inside screenToWorldCoordinates() (TransformHandler.ts:102–103).

Compare with all other usages in the codebase (UnitLayer, TerritoryLayer, EmojiTable, BuildMenu, etc.)—they pass raw coordinates directly. Change to:

const worldCoords = this.transformHandler.screenToWorldCoordinates(
  this.mousePos.x,
  this.mousePos.y,
);

Then remove lines 244–245. This same bug exists in StructureIconsLayer.ts lines 255–259.

🤖 Prompt for AI Agents
In `@src/client/graphics/layers/NukeRenderUtilLayer.ts` around lines 239 - 254, In
NukeRenderUtilLayer, remove the intermediate canvas-relative coordinate
calculation (localX/localY) and call transformHandler.screenToWorldCoordinates
with the raw screen coordinates this.mousePos.x and this.mousePos.y, then use
the returned worldCoords with game.isValidCoord and game.ref to set
this.targetTile; also apply the same change in StructureIconsLayer by passing
raw this.mousePos values into screenToWorldCoordinates and deleting the manual
rect subtraction so the canvas offset isn’t subtracted twice.

Copy link
Contributor Author

@bibizu bibizu Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing breaks in testing, and I suspect this is not an issue since we use mouse position directly, which needs the window position subtracted first, or something similar, to get it into "screen" coords. If there actually was a double subtraction, the trajectory should be way off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find MouseMoveEvent definition to see what coordinates it provides
rg -n 'class MouseMoveEvent|interface MouseMoveEvent' --type=ts -A10

Length of output: 597


🏁 Script executed:

#!/bin/bash
# Find where MouseMoveEvent is created/emitted to see what values are passed
rg -n 'new MouseMoveEvent|MouseMoveEvent\(' --type=ts -B2 -A2

Length of output: 330


🏁 Script executed:

#!/bin/bash
# Check other usages of MouseMoveEvent to see if they subtract rect
rg -n 'MouseMoveEvent.*=>|on\(MouseMoveEvent' --type=ts -B2 -A8

Length of output: 3517


🏁 Script executed:

#!/bin/bash
# Specifically check StructureIconsLayer's mouse handling
rg -n 'mousePos|MouseMoveEvent' src/client/graphics/layers/StructureIconsLayer.ts -B2 -A8

Length of output: 2490


🏁 Script executed:

#!/bin/bash
# Check if the canvas is typically fullscreen or positioned at (0,0)
rg -n 'canvas|position.*fixed|position.*absolute|fullscreen' src/client/graphics/GameRenderer.ts -B2 -A3 | head -50

Length of output: 1579


🏁 Script executed:

#!/bin/bash
# Check TransformHandler for canvas setup
rg -n 'canvas.*style|position|getBoundingClientRect' src/client/graphics/TransformHandler.ts -B2 -A5 | head -60

Length of output: 1172


🏁 Script executed:

#!/bin/bash
# Check for canvas CSS styling in the main application files
rg -n 'canvas.*{|#game-canvas|canvas.*style' src/client --type=ts -B2 -A5

Length of output: 127


🏁 Script executed:

#!/bin/bash
# Check the HTML file for canvas positioning
fd -e html | xargs cat | grep -A10 -B5 canvas

Length of output: 743


🏁 Script executed:

#!/bin/bash
# Check for any CSS files that might style the canvas
fd -e css | xargs cat

Length of output: 44050


@bibizu You're right that the canvas is likely at position (0, 0) during your testing, which masks the bug! When rect.left and rect.top are both 0, the double subtraction has no visible effect (0 - 0 - 0 = 0).

However, the double subtraction mathematically exists:

Current code flow:

  1. MouseMoveEvent provides clientX/clientY (raw viewport coordinates) — Line 246 in InputHandler.ts shows new MouseMoveEvent(e.clientX, e.clientY)
  2. NukeRenderUtilLayer subtracts rect.left/rect.top → creates localX/localY
  3. screenToWorldCoordinates(localX, localY) internally subtracts rect.left/rect.top again (TransformHandler.ts lines 102-103)

When the bug would surface:

  • Canvas embedded in a container with offset positioning
  • Mobile layouts with top navigation bars
  • Any scenario where the canvas bounding rect isn't at viewport (0, 0)

Evidence from codebase:
All other layers pass raw mousePos coordinates directly to screenToWorldCoordinates:

  • UnitLayer, TerritoryLayer, EmojiTable, BuildMenu (from earlier searches)
  • Even StructureIconsLayer's moveGhost method at line 417 correctly positions the ghost with raw coordinates, but then renderGhost at lines 255-259 has the same double-subtraction bug

Recommendation:
Fix it for robustness even though it works in your current test scenarios. The trajectory would be off by (rect.left, rect.top) pixels in non-fullscreen contexts.


🧠 Learnings used
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.

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: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:30.357Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.

Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.

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.

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.

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.

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.

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.

Learnt from: Aareksio
Repo: openfrontio/OpenFrontIO PR: 2815
File: tests/pathfinding/playground/server.ts:87-104
Timestamp: 2026-01-08T00:57:47.789Z
Learning: Playground code in tests/pathfinding/playground/ should not be subjected to detailed code review - working functionality is sufficient for these test/visualization utilities.

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: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants