Add warship patrol area indicator #2543
Conversation
- Show patrol area circle when warship is selected (solid line) - Show preview circle at cursor when selecting new patrol location (dashed line) - ESC key deselects warship - Expose patrolTile to client via UnitUpdate for visualization
|
|
WalkthroughAdds a new WarshipRadiusLayer that renders selected and placement patrol indicators, wires it into the renderer layer stack, subscribes selection/mouse events, and propagates an optional Changes
Sequence Diagram(s)mermaid Note over Renderer,WarshipLayer: Initialization Note over EventBus,WarshipLayer: Runtime interactions Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/UnitLayer.ts(3 hunks)src/client/graphics/layers/WarshipRadiusLayer.ts(1 hunks)src/core/game/GameUpdates.ts(1 hunks)src/core/game/GameView.ts(1 hunks)src/core/game/UnitImpl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T20:49:29.110Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
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/GameView.tssrc/core/game/UnitImpl.tssrc/core/game/GameUpdates.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/WarshipRadiusLayer.tssrc/client/graphics/GameRenderer.ts
🧬 Code graph analysis (5)
src/core/game/GameView.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/layers/UnitLayer.ts (1)
src/client/InputHandler.ts (2)
CloseViewEvent(78-78)UnitSelectionEvent(31-36)
src/client/graphics/layers/WarshipRadiusLayer.ts (6)
src/core/game/GameView.ts (5)
UnitView(44-182)GameView(462-788)patrolTile(133-135)x(685-687)y(688-690)src/server/GameManager.ts (1)
game(24-26)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)src/client/graphics/UIState.ts (1)
UIState(3-6)src/client/InputHandler.ts (2)
UnitSelectionEvent(31-36)MouseMoveEvent(45-50)src/core/game/UnitImpl.ts (1)
patrolTile(97-99)
src/core/game/GameUpdates.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/WarshipRadiusLayer.ts (1)
WarshipRadiusLayer(15-189)
🔇 Additional comments (8)
src/client/graphics/GameRenderer.ts (1)
214-219: LGTM! Consistent with existing layer patterns.The
WarshipRadiusLayeris instantiated and inserted in the correct position, following the same pattern asSAMRadiusLayer. The layer ordering (radius layers before unit layer) makes sense for visual layering.Also applies to: 252-252
src/core/game/GameView.ts (1)
133-135: LGTM! Clean accessor following existing patterns.The
patrolTile()accessor matches the style oftargetTile()directly above it.src/core/game/UnitImpl.ts (1)
143-143: LGTM! Proper serialization of patrol tile.The
patrolTilefield is correctly added to the update payload, matching the pattern of other optional fields liketargetTile.src/core/game/GameUpdates.ts (1)
137-137: LGTM! Clear interface extension with good documentation.The
patrolTilefield follows the existing pattern of optional unit-specific fields with a helpful comment.src/client/graphics/layers/UnitLayer.ts (1)
194-201: LGTM! Simple and effective ESC deselection handler.The
onCloseViewmethod cleanly handles ESC key deselection by emitting aUnitSelectionEvent. The existingonUnitSelectionChangehandler will update the state correctly.src/client/graphics/layers/WarshipRadiusLayer.ts (3)
1-57: Good layer setup following existing patterns.Clean imports with proper
typeannotations. Canvas setup and event subscriptions follow the established layer patterns (similar toSAMRadiusLayer).
59-94: Tick and animation logic is well-structured.The animation timing, ghost mode detection, and destroyed warship cleanup are all handled correctly. The modulo guard on
dashOffsetprevents numeric overflow over long sessions.
133-188: Clean drawing implementation.The
redraw(),drawCurrentPatrol(), anddrawPreviewPatrol()methods are straightforward and follow canvas best practices with propersave()/restore()pairs.
|
Good idea. I thought warships patrol square areas instead of circle ones though? |
Warships patrol in a square area (random x/y within ±range/2), not a circular one. Updated the visual indicator to match the actual behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/WarshipRadiusLayer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/WarshipRadiusLayer.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/WarshipRadiusLayer.ts
📚 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/layers/WarshipRadiusLayer.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/WarshipRadiusLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/WarshipRadiusLayer.ts (1)
src/client/graphics/UIState.ts (1)
UIState(3-6)
🔇 Additional comments (8)
src/client/graphics/layers/WarshipRadiusLayer.ts (8)
1-7: LGTM!Imports are clean and necessary for the layer implementation.
9-31: LGTM!Field declarations are well-organized with clear state tracking, animation, and cursor management. The note about matching SAMRadiusLayer's animation speed is helpful for consistency.
33-47: LGTM!Constructor properly initializes canvas and context with good error handling.
49-57: LGTM!Standard layer initialization with proper event subscriptions and initial redraw.
96-104: LGTM!Standard layer rendering implementation with correct centered coordinate offset.
117-131: LGTM!Mouse handling logic is correct with proper early returns and coordinate conversion.
133-150: LGTM!Redraw logic correctly handles both solid patrol area and dashed preview with proper conditional rendering and defensive checks.
59-94: Verify thatisActive()method exists on UnitView.The tick method calls
this.selectedWarship.isActive()on line 71. Ensure this method exists on the UnitView interface/class and returns a boolean indicating whether the unit is still active.
BCNelson
changed the title
|
Looks quite nice (and also kinda highlights how gigantic the patrol area is lol). Would it be possible to update the preview a little faster? When I compare it to the rocket path previews, those are quite a lot smoother than your implementation |
@Lavodan Are you referring to the frame rate? if so yes i can i was copying the current SAM implementation but can tie it to the fame rate of the web page.
A future change i would like to make is setting the patrol area dynamically but that has broader game play implications. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/client/graphics/WarshipRadiusLayer.test.ts (1)
16-36: Reset non-function canvas state between tests to avoid order-dependence.
mockContext.lineDashOffset,lineWidth,strokeStyle,fillStylecan be mutated by the layer but aren’t reset inbeforeEach.beforeEach(() => { // Reset mock context Object.values(mockContext).forEach((fn) => { if (typeof fn === "function") { (fn as jest.Mock).mockClear(); } }); + mockContext.lineDashOffset = 0; + mockContext.lineWidth = 1; + mockContext.strokeStyle = ""; + mockContext.fillStyle = "";Also applies to: 65-72
src/client/graphics/layers/WarshipRadiusLayer.ts (1)
33-47: Consider resize/DPR handling for the offscreen canvas (blurry after resize/retina).
Canvas size is fixed in the constructor, and the layer is then scaled viadrawImage(). If the game canvas resizes or DPR changes, this overlay can blur or misalign.export class WarshipRadiusLayer implements Layer { private readonly canvas: HTMLCanvasElement; private readonly context: CanvasRenderingContext2D; + private resizeIfNeeded() { + const dpr = window.devicePixelRatio || 1; + const w = Math.floor(this.game.width() * dpr); + const h = Math.floor(this.game.height() * dpr); + if (this.canvas.width !== w || this.canvas.height !== h) { + this.canvas.width = w; + this.canvas.height = h; + this.context.setTransform(dpr, 0, 0, dpr, 0, 0); + this.needsRedraw = true; + } + } renderLayer(context: CanvasRenderingContext2D) { + this.resizeIfNeeded(); // Animate dash offset every frame for smooth animation on high refresh rate displays const now = Date.now();Also applies to: 80-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)src/client/graphics/layers/WarshipRadiusLayer.ts(1 hunks)tests/client/graphics/WarshipRadiusLayer.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/WarshipRadiusLayer.tstests/client/graphics/WarshipRadiusLayer.test.ts
📚 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/layers/WarshipRadiusLayer.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/layers/WarshipRadiusLayer.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/WarshipRadiusLayer.ts
📚 Learning: 2025-06-22T05:48:19.241Z
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.
Applied to files:
src/client/graphics/layers/WarshipRadiusLayer.ts
🧬 Code graph analysis (2)
src/client/graphics/layers/WarshipRadiusLayer.ts (5)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (5)
UnitView(44-182)GameView(462-788)patrolTile(133-135)x(685-687)y(688-690)src/client/graphics/TransformHandler.ts (1)
TransformHandler(15-333)src/client/graphics/UIState.ts (1)
UIState(3-6)src/client/InputHandler.ts (2)
UnitSelectionEvent(31-36)MouseMoveEvent(45-50)
tests/client/graphics/WarshipRadiusLayer.test.ts (2)
src/client/graphics/layers/WarshipRadiusLayer.ts (1)
WarshipRadiusLayer(15-196)src/client/InputHandler.ts (2)
UnitSelectionEvent(31-36)MouseMoveEvent(45-50)
🔇 Additional comments (1)
.gitignore (1)
14-15: Good dev-environment ignores.
Ignoring.direnv/and.devenv/is sensible and avoids accidental commits.
| private handleUnitSelection(e: UnitSelectionEvent) { | ||
| if (e.unit?.type() === UnitType.Warship && e.isSelected) { | ||
| this.selectedWarship = e.unit; | ||
| this.selectedShow = true; | ||
| } else if (!e.isSelected && this.selectedWarship === e.unit) { | ||
| this.selectedWarship = null; | ||
| this.selectedShow = false; | ||
| } else if (e.isSelected && e.unit && e.unit.type() !== UnitType.Warship) { | ||
| this.selectedWarship = null; | ||
| this.selectedShow = false; | ||
| } | ||
| this.needsRedraw = true; | ||
| } |
There was a problem hiding this comment.
Fix: clear selection on deselect events where unit is null.
Right now !e.isSelected only clears when e.unit === this.selectedWarship; a UnitSelectionEvent(null, false) will leave the radius stuck on.
private handleUnitSelection(e: UnitSelectionEvent) {
if (e.unit?.type() === UnitType.Warship && e.isSelected) {
this.selectedWarship = e.unit;
this.selectedShow = true;
- } else if (!e.isSelected && this.selectedWarship === e.unit) {
+ } else if (!e.isSelected && (!e.unit || this.selectedWarship === e.unit)) {
this.selectedWarship = null;
this.selectedShow = false;
} else if (e.isSelected && e.unit && e.unit.type() !== UnitType.Warship) {
this.selectedWarship = null;
this.selectedShow = false;
}
this.needsRedraw = true;
}🤖 Prompt for AI Agents
In src/client/graphics/layers/WarshipRadiusLayer.ts around lines 108 to 120, the
deselect path only clears selection when e.unit === this.selectedWarship, so a
UnitSelectionEvent with unit === null and isSelected === false leaves the
previous selection active; update the handler so that any deselect event
(e.isSelected === false) clears this.selectedWarship and this.selectedShow (or
at minimum clear when e.unit is null), while preserving the existing logic for
selecting a warship when e.isSelected === true and non-warship selects clearing
the selection; ensure this.needsRedraw is still set to true after changes.
| private handleMouseMove(e: MouseMoveEvent) { | ||
| if (!this.selectedShow && !this.ghostShow) return; | ||
|
|
||
| const rect = this.transformHandler.boundingRect(); | ||
| if (!rect) return; | ||
|
|
||
| // Convert screen coordinates to world coordinates | ||
| const worldPos = this.transformHandler.screenToWorldCoordinates( | ||
| e.x - rect.left, | ||
| e.y - rect.top, | ||
| ); | ||
|
|
||
| this.mouseWorldPos = worldPos; | ||
| this.needsRedraw = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the TransformHandler class definition
echo "=== Finding TransformHandler class ==="
rg -n "class\s+TransformHandler" --type=ts
echo ""
# Find screenToWorldCoordinates method
echo "=== Finding screenToWorldCoordinates method ==="
rg -n "screenToWorldCoordinates\s*\(" --type=ts -B2 -A8Repository: openfrontio/OpenFrontIO
Length of output: 13857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete screenToWorldCoordinates method
echo "=== Full screenToWorldCoordinates implementation ==="
sed -n '100,120p' src/client/graphics/TransformHandler.tsRepository: openfrontio/OpenFrontIO
Length of output: 900
Remove the rect offset — screenToWorldCoordinates() already subtracts it internally.
The TransformHandler.screenToWorldCoordinates() method (lines 101–103) calls boundingRect() and subtracts canvasRect.left and canvasRect.top from the input coordinates. You are subtracting these offsets again before calling the method, which causes a double offset. Pass e.x and e.y directly instead.
🤖 Prompt for AI Agents
In src/client/graphics/layers/WarshipRadiusLayer.ts around lines 122–136, the
code subtracts the canvas bounding rect from e.x/e.y before calling
screenToWorldCoordinates, but that method already calls boundingRect() and
subtracts left/top internally; remove the subtraction and call
screenToWorldCoordinates(e.x, e.y) instead (you can keep the existing rect
existence check but do not use rect.left/rect.top when passing coordinates),
then assign the returned worldPos to this.mouseWorldPos and set needsRedraw as
before.
| it("should handle warship deselection", () => { | ||
| const unit = { | ||
| type: () => UnitType.Warship, | ||
| isActive: () => true, | ||
| patrolTile: () => ({ x: 10, y: 10 }), | ||
| }; | ||
|
|
||
| // First select | ||
| layer["handleUnitSelection"]({ | ||
| unit, | ||
| isSelected: true, | ||
| } as unknown as UnitSelectionEvent); | ||
|
|
||
| // Then deselect | ||
| layer["handleUnitSelection"]({ | ||
| unit, | ||
| isSelected: false, | ||
| } as unknown as UnitSelectionEvent); | ||
|
|
||
| expect(layer["selectedWarship"]).toBeNull(); | ||
| expect(layer["selectedShow"]).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add a test for “deselect with null unit” (common event shape).
If the app emits new UnitSelectionEvent(null, false) (instead of passing the previously selected unit), the layer should still clear selection.
+ it("should clear selection when deselect event has null unit", () => {
+ const unit = {
+ type: () => UnitType.Warship,
+ isActive: () => true,
+ patrolTile: () => ({ x: 10, y: 10 }),
+ };
+
+ layer["handleUnitSelection"]({ unit, isSelected: true } as unknown as UnitSelectionEvent);
+ layer["handleUnitSelection"]({ unit: null, isSelected: false } as unknown as UnitSelectionEvent);
+
+ expect(layer["selectedWarship"]).toBeNull();
+ expect(layer["selectedShow"]).toBe(false);
+ });Also applies to: 152-165
🤖 Prompt for AI Agents
In tests/client/graphics/WarshipRadiusLayer.test.ts around lines 129-150 (and
similarly for 152-165), add a unit-selection test that simulates the common
event shape where unit is null: call layer["handleUnitSelection"] with an object
having unit: null and isSelected: false (cast to UnitSelectionEvent), then
assert layer["selectedWarship"] is null and layer["selectedShow"] is false;
ensure the test mirrors the existing deselect flow but uses null for unit to
verify the handler tolerates null unit payloads.
Yes, the frame rate of how often the box is drawn (not the rate of how fast the lines spin)
Yeah smaller PRs make sense in this case for sure |
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
iiamlewis
moved this from Development
to Concept Review
in OpenFront Release Management
|
Never realized the patrol areas are so damn big lol
|
|
so this is an amazing feature, great idea! |
|
PR review and feedback - hello! There is a few bits we wanted to look at before this would be mergeable. The square at the moment kinda looks a little strange, I think this would look better if it moves as a single movement that's smooth rather than kinda jumpy What's the choice behind square rather than a circle as most of our info are circle based, ie Sam radiuses ect |
7 participants
Description:
Screencast_20251130_101359.webm
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
bcnelson