Implement Impact Timer for Nukes #2401
Conversation
|
Resolves #2394 @TheGiraffe3 |
|
Warning Rate limit exceeded@ryanbarlow97 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds inbound/outbound alert state, trajectory-aware timers, and flashing/timer rendering to nuke target FX. Propagates Changes
Sequence Diagram(s)sequenceDiagram
participant FxLayer
participant GameView
participant NukeAreaFx
rect rgb(240,250,255)
Note over FxLayer: Per-tick FX timing refresh
FxLayer->>FxLayer: tick()
FxLayer->>FxLayer: updateNukeTargetFxRemainingTime()
loop each nuke FX
FxLayer->>GameView: query unit ownership & timing (remainingSeconds, trajectoryIndex, trajectoryLength)
FxLayer->>NukeAreaFx: updateTimer(remainingSeconds, isOutbound, trajectoryIndex, trajectoryLength)
end
end
rect rgb(255,245,235)
Note over NukeAreaFx: Render decision & alert
NukeAreaFx->>NukeAreaFx: calculateAlertIntensity() → 0..1
alt alertIntensity > 0
NukeAreaFx->>NukeAreaFx: apply flashing alpha (faster as intensity ↑)
end
alt remainingSeconds present
NukeAreaFx->>NukeAreaFx: render timer block (red if inbound, amber if outbound)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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
🧹 Nitpick comments (1)
src/client/graphics/fx/NukeAreaFx.ts (1)
58-71: Remove duplicate comment.Line 68 duplicates the comment from line 67. Keep only one.
// Flash speed increases as alertIntensity increases (0 = slow, 1 = fast) if (this.isInbound && this.alertIntensity > 0 && !this.ended) { // Flash faster as intensity increases: 0.5s per flash at intensity 0, 0.2s per flash at intensity 1 const maxFlashPeriod = 500; // ms at intensity 0 const minFlashPeriod = 200; // ms at intensity 1 const flashPeriod = maxFlashPeriod - (maxFlashPeriod - minFlashPeriod) * this.alertIntensity; const flashPhase = (this.lifeTime % flashPeriod) / flashPeriod; - // Flash between 0.5 (50%) and 1.0 (100%) alpha in sinusoidal pattern // Flash between 0.6 (60%) and 1.0 (100%) alpha in sinusoidal pattern const flashAlpha = 0.6 + 0.4 * (0.5 + 0.5 * Math.sin(flashPhase * Math.PI * 2)); alpha = flashAlpha; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
resources/lang/en.json(1 hunks)src/client/graphics/GameRenderer.ts(2 hunks)src/client/graphics/fx/NukeAreaFx.ts(2 hunks)src/client/graphics/layers/BombTimerLayer.ts(1 hunks)src/client/graphics/layers/FxLayer.ts(2 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 (6)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-08-24T11:13:08.495Z
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.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/FxLayer.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/core/game/UnitImpl.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/GameRenderer.tssrc/client/graphics/layers/BombTimerLayer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/GameRenderer.ts
🧬 Code graph analysis (4)
src/client/graphics/layers/FxLayer.ts (2)
src/core/game/GameView.ts (7)
unit(666-668)targetTile(130-132)trajectoryIndex(134-136)trajectoryLength(138-140)UnitView(44-187)x(679-681)y(682-684)src/client/graphics/fx/NukeAreaFx.ts (1)
NukeAreaFx(4-103)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/BombTimerLayer.ts (1)
BombTimerLayer(7-144)
src/client/graphics/layers/BombTimerLayer.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (4)
GameView(456-782)myPlayer(600-602)unit(666-668)trajectoryLength(138-140)
src/client/graphics/fx/NukeAreaFx.ts (1)
src/core/configuration/Config.ts (1)
NukeMagnitude(69-72)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/FxLayer.ts
[warning] 1-1: Code style issues found by Prettier. Run 'npx prettier --write' to fix this file.
src/client/graphics/layers/BombTimerLayer.ts
[warning] 1-1: Code style issues found by Prettier. Run 'npx prettier --write' to fix this file.
src/client/graphics/fx/NukeAreaFx.ts
[warning] 1-1: Code style issues found by Prettier. Run 'npx prettier --write' to fix this file.
🪛 GitHub Check: 🔍 ESLint
src/client/graphics/layers/BombTimerLayer.ts
[failure] 2-2:
'UnitView' is defined but never used.
🔇 Additional comments (11)
resources/lang/en.json (1)
769-772: LGTM! Translation strings properly added.The new
bomb_timerkeys follow the existing translation structure and provide clear labels for the timer feature.src/core/game/GameUpdates.ts (1)
137-138: LGTM! Optional trajectory fields properly added.Making these fields optional ensures backwards compatibility while supporting the new timer feature for nukes.
src/client/graphics/GameRenderer.ts (2)
10-10: LGTM! Layer import added.
246-246: LGTM! BombTimerLayer properly integrated.The layer is correctly instantiated with required dependencies and positioned appropriately in the render sequence.
src/core/game/GameView.ts (1)
134-140: LGTM! Trajectory accessors properly implemented.The accessors follow the established pattern in
UnitViewand correctly expose the trajectory data.src/core/game/UnitImpl.ts (1)
142-143: LGTM! Trajectory data properly serialized.The trajectory information is correctly included in the unit update payload.
src/client/graphics/layers/FxLayer.ts (1)
46-46: LGTM! Timer update correctly integrated into tick cycle.src/client/graphics/fx/NukeAreaFx.ts (2)
18-35: LGTM! Inbound alert state properly added.The constructor maintains backwards compatibility with default parameters, and the new state is well-documented.
37-39: LGTM! Update method correctly bounds the intensity.The clamping to [0, 1] range is appropriate.
src/client/graphics/layers/BombTimerLayer.ts (2)
1-5: Note: Static analysis false positive.ESLint flags
UnitViewas unused, but it's actually used in the type system within the filter predicate (lines 63-64 checkunit.type()). This is a false positive.
7-144: LGTM! BombTimerLayer implementation is clean and correct.The layer properly:
- Filters for active AtomBomb and HydrogenBomb units with trajectory data
- Calculates remaining time correctly (trajectoryLength - trajectoryIndex) / 10
- Distinguishes inbound (red) from outbound (yellow/orange) timers
- Uses localized translation strings
- Follows the Layer interface pattern
The tick-based redraw is appropriate for this dynamic timer display.
Just so you know, if you put |
|
Ohh didn't know about that. thanks!
…On Thu, Nov 6, 2025, 1:56 AM Loymdayddaud ***@***.***> wrote:
*TheGiraffe3* left a comment (openfrontio/OpenFrontIO#2401)
<#2401 (comment)>
Resolves #2394 <#2394>
@TheGiraffe3 <https://github.com/TheGiraffe3>
Just so you know, if you put Resolves #issue number in the main body of
the pull request, it will automatically link.
—
Reply to this email directly, view it on GitHub
<#2401 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVEPAME54ZHC5D65UGGSV4T33LWJBAVCNFSM6AAAAACLJLHIXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIOJVGM4DKMBVGI>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Should we have that in the PR template? |
… and FxLayer. Adjusted formatting for clarity and consistency in calculations and conditionals.
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 (3)
src/client/graphics/fx/NukeAreaFx.ts(2 hunks)src/client/graphics/layers/BombTimerLayer.ts(1 hunks)src/client/graphics/layers/FxLayer.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-08-24T11:13:08.495Z
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.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/FxLayer.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/BombTimerLayer.ts
🧬 Code graph analysis (3)
src/client/graphics/layers/FxLayer.ts (2)
src/core/game/GameView.ts (7)
unit(666-668)targetTile(130-132)trajectoryIndex(134-136)trajectoryLength(138-140)UnitView(44-187)x(679-681)y(682-684)src/client/graphics/fx/NukeAreaFx.ts (1)
NukeAreaFx(4-105)
src/client/graphics/layers/BombTimerLayer.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (3)
GameView(456-782)myPlayer(600-602)unit(666-668)
src/client/graphics/fx/NukeAreaFx.ts (1)
src/core/configuration/Config.ts (1)
NukeMagnitude(69-72)
…it for alert intensity calculations. This improves code clarity by removing redundant logic related to inbound bomb checks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/graphics/layers/FxLayer.ts (2)
124-179: Outbound and inbound detection logic is correct.The method properly identifies bombs owned by the player/team (outbound) and bombs targeting the player's territory (inbound). The FX creation with the new parameters looks good.
However, the alert intensity calculation on lines 161-164 duplicates the logic from
updateNukeTargetFxRemainingTime()(lines 112-115). As noted in the previous review, extract this into a helper method:private calculateAlertIntensity( trajectoryIndex: number | undefined, trajectoryLength: number | undefined, ): number { if ( trajectoryIndex === undefined || trajectoryLength === undefined || trajectoryLength === 0 ) { return 0; } return Math.max(0, Math.min(1, trajectoryIndex / trajectoryLength)); }Then replace both occurrences:
// In updateNukeTargetFxRemainingTime (line 112-115) const alertIntensity = this.calculateAlertIntensity( unit.trajectoryIndex(), unit.trajectoryLength(), ); // In createNukeTargetFxIfOwned (line 161-164) alertIntensity = this.calculateAlertIntensity( unit.trajectoryIndex(), unit.trajectoryLength(), );Based on previous review comments.
124-179: Inbound flag is never updated after creation.The previous review identified that if target tile ownership changes during flight (e.g., player recaptures a city), the FX keeps its initial inbound/outbound state and won't show the flashing warning.
This requires:
- Adding a
setInbound()method toNukeAreaFx(flagged in the other file)- Updating the inbound state in
updateNukeTargetFxRemainingTime()before adjusting intensityAfter adding the setter to
NukeAreaFx, update the method to recalculate and set the inbound flag:private updateNukeTargetFxRemainingTime() { for (const [unitId, fx] of Array.from( this.nukeTargetFxByUnitId.entries(), )) { const unit = this.game.unit(unitId); if (!unit || !unit.isActive()) continue; // Recalculate inbound state each tick const targetTile = unit.targetTile(); let isInbound = false; if (targetTile !== undefined) { const targetOwner = this.game.owner(targetTile); const my = this.game.myPlayer(); if (my) { isInbound = targetOwner.isPlayer() && targetOwner.id() === my.id(); } } fx.setInbound(isInbound); // Update alert intensity for inbound bombs if (fx.isInboundBomb()) { // ... existing intensity update logic } } }Based on previous review comments.
🧹 Nitpick comments (1)
src/client/graphics/fx/NukeAreaFx.ts (1)
64-77: Flashing logic is correct.The flash period interpolation and sinusoidal alpha calculation work as intended. The effect will speed up as impact approaches, oscillating between 60% and 100% opacity.
Minor: remove the duplicate comment on line 72.
- // Flash between 0.5 (50%) and 1.0 (100%) alpha in sinusoidal pattern // Flash between 0.6 (60%) and 1.0 (100%) alpha in sinusoidal pattern
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/fx/NukeAreaFx.ts(2 hunks)src/client/graphics/layers/FxLayer.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-08-24T11:13:08.495Z
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.
Applied to files:
src/client/graphics/layers/FxLayer.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/FxLayer.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/FxLayer.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/FxLayer.ts
🧬 Code graph analysis (2)
src/client/graphics/layers/FxLayer.ts (2)
src/core/game/GameView.ts (7)
unit(666-668)trajectoryIndex(134-136)trajectoryLength(138-140)UnitView(44-187)targetTile(130-132)x(679-681)y(682-684)src/client/graphics/fx/NukeAreaFx.ts (1)
NukeAreaFx(4-109)
src/client/graphics/fx/NukeAreaFx.ts (1)
src/core/configuration/Config.ts (1)
NukeMagnitude(69-72)
🔇 Additional comments (2)
src/client/graphics/fx/NukeAreaFx.ts (1)
18-35: Constructor and fields look good.The new
isInboundandalertIntensityfields are initialized properly with sensible defaults. The constructor signature maintains backward compatibility.src/client/graphics/layers/FxLayer.ts (1)
93-120: Alert intensity update logic is correct.The method properly filters for inbound bombs and calculates alert intensity based on trajectory progress. The validation checks for undefined values and zero-length trajectories are appropriate.
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 (2)
src/client/graphics/fx/NukeAreaFx.ts(2 hunks)src/client/graphics/layers/FxLayer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/FxLayer.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/fx/NukeAreaFx.ts
🧬 Code graph analysis (1)
src/client/graphics/fx/NukeAreaFx.ts (1)
src/core/configuration/Config.ts (1)
NukeMagnitude(69-72)
🔇 Additional comments (4)
src/client/graphics/fx/NukeAreaFx.ts (4)
18-20: LGTM!Field additions are clean with sensible defaults. The comment clearly explains the alert intensity feature.
22-35: LGTM!Constructor properly initializes the new inbound/alert state with sensible defaults. Backwards compatibility maintained.
37-47: LGTM!The
setInbound()method addresses the past review concern about dynamic ownership changes.updateAlertIntensity()properly clamps values to the valid [0, 1] range.
64-81: Verify: Flash effect ignores fade animations.Line 80 completely replaces the fade alpha from line 64, meaning inbound bombs skip fade-in (appear at full flash intensity immediately) and when
endedbecomes true, the bomb abruptly stops flashing and starts fading out. This might create a jarring transition.Consider whether:
- Fade-in should blend with the flash:
alpha = flashAlpha * t- Fade-out should continue flashing: remove
&& !this.endedfrom line 68Is the current immediate-flash/abrupt-stop behavior intentional?
|
I had a go with this and it seems pretty nice |
…e ownership, improving inbound and outbound bomb detection.
…mproved clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/BombTimerLayer.ts (1)
113-145: Consider extracting magic numbers to constants.The rendering code uses several hard-coded values (font size
10px, padding3, offset25, colors) that could be extracted to class-level or file-level constants. This would improve maintainability if you need to adjust styling later.Example refactor:
export class BombTimerLayer implements Layer { private canvas: HTMLCanvasElement; private context: CanvasRenderingContext2D | null; // Timer display constants private static readonly TIMER_FONT = "bold 10px sans-serif"; private static readonly TIMER_PADDING = 3; private static readonly TIMER_BG_HEIGHT = 12; private static readonly TIMER_OFFSET_Y = 25; private static readonly INBOUND_COLOR = "#ff6666"; private static readonly OUTBOUND_COLOR = "#ffcc66"; private static readonly BG_COLOR = "rgba(0, 0, 0, 0.4)"; private static readonly STROKE_COLOR = "rgba(0, 0, 0, 0.5)"; // ... then use these constants in redraw() this.context.font = BombTimerLayer.TIMER_FONT; const padding = BombTimerLayer.TIMER_PADDING; const bgHeight = BombTimerLayer.TIMER_BG_HEIGHT + padding * 2; const offsetY = BombTimerLayer.TIMER_OFFSET_Y; // etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/BombTimerLayer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/BombTimerLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/BombTimerLayer.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (4)
GameView(456-782)myPlayer(600-602)unit(666-668)trajectoryLength(138-140)
🔇 Additional comments (3)
src/client/graphics/layers/BombTimerLayer.ts (3)
11-30: Constructor and lifecycle methods look good.The initialization is straightforward and follows the Layer interface pattern. Calling
redraw()every tick ensures timers stay current.
59-97: Bomb filtering and direction logic is correct.The outbound/inbound classification properly handles team games, including edge cases where a bomb targets the same team that launched it. The filtering correctly excludes enemy-to-enemy bombs.
104-110: Translation usage is correct.The timer text construction properly uses
translateText()for all user-facing strings and handles the optional incoming label cleanly.
| const worldX = this.game.x(targetTile); | ||
| const worldY = this.game.y(targetTile); |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Coordinate system mismatch—timers likely render at wrong positions.
The redraw() method draws directly at worldX, worldY on the offscreen canvas (lines 124, 144), but renderLayer positions the canvas at (-width/2, -height/2) in world space (line 35). This means:
- Canvas pixel
(worldX, worldY)appears at world(-width/2 + worldX, -height/2 + worldY) - To draw at world
(worldX, worldY), you need canvas position(worldX + width/2, worldY + height/2)
Apply this diff to translate world coordinates to canvas coordinates:
// Get world coordinates of target tile
const worldX = this.game.x(targetTile);
const worldY = this.game.y(targetTile);
+
+ // Translate world coords to canvas coords
+ const canvasX = worldX + this.game.width() / 2;
+ const canvasY = worldY + this.game.height() / 2;
// Draw timer text
const incomingText = isInbound
? translateText("bomb_timer.incoming")
: "";
const impactText = translateText("bomb_timer.til_impact", {
seconds: remainingSeconds,
});
const text = incomingText ? `${incomingText} ${impactText}` : impactText;
// Set text style
this.context.font = "bold 10px sans-serif";
this.context.textAlign = "center";
this.context.textBaseline = "middle";
// Measure text for background sizing
const textMetrics = this.context.measureText(text);
const padding = 3;
const bgWidth = textMetrics.width + padding * 2;
const bgHeight = 12 + padding * 2;
const offsetY = 25; // Offset further up to avoid blocking
- const bgX = worldX - bgWidth / 2;
- const bgY = worldY - offsetY - bgHeight / 2;
+ const bgX = canvasX - bgWidth / 2;
+ const bgY = canvasY - offsetY - bgHeight / 2;
// Draw transparent background
this.context.fillStyle = "rgba(0, 0, 0, 0.4)";
this.context.fillRect(bgX, bgY, bgWidth, bgHeight);
// Set text color based on direction
if (isInbound) {
// Red for inbound
this.context.fillStyle = "#ff6666";
this.context.strokeStyle = "rgba(0, 0, 0, 0.5)";
} else {
// Yellow/orange for outbound
this.context.fillStyle = "#ffcc66";
this.context.strokeStyle = "rgba(0, 0, 0, 0.5)";
}
// Draw text with lighter stroke for less blocking
this.context.lineWidth = 1;
- this.context.strokeText(text, worldX, worldY - offsetY);
- this.context.fillText(text, worldX, worldY - offsetY);
+ this.context.strokeText(text, canvasX, canvasY - offsetY);
+ this.context.fillText(text, canvasX, canvasY - offsetY);Verify the coordinate system by checking how other layers (like UnitLayer or StructureIconsLayer) handle world-to-canvas coordinate translation when using offscreen canvases:
Also applies to: 124-125, 144-145
🏁 Script executed:
#!/bin/bash
# Check how other layers translate world coordinates to canvas coordinates
rg -n -A 5 -B 5 'game\.width\(\) / 2|game\.height\(\) / 2' --type=ts -g 'src/client/graphics/layers/*Layer.ts'Length of output: 9254
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full BombTimerLayer redraw() method to verify the coordinate issue
rg -n -A 50 'redraw\(' src/client/graphics/layers/BombTimerLayer.ts
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Get the full BombTimerLayer redraw() method to verify the coordinate issue
rg -n -A 50 'redraw\(' src/client/graphics/layers/BombTimerLayer.tsLength of output: 2305
🏁 Script executed:
#!/bin/bash
# Get the coordinate calculations and drawing code from BombTimerLayer
rg -n -B 3 -A 3 'worldX|worldY|bgX|bgY' src/client/graphics/layers/BombTimerLayer.tsLength of output: 966
Coordinate system mismatch confirmed—bomb timers render at wrong positions.
The redraw() method draws bomb timers directly at world coordinates on an offscreen canvas that renderLayer positions at (-width/2, -height/2) in world space. This causes timers to render offset by half the game viewport.
Canvas pixel (worldX, worldY) appears at world position (-width/2 + worldX, -height/2 + worldY). To draw at world position (worldX, worldY), translate to canvas coordinates: (worldX + width/2, worldY + height/2).
The NukeTrajectoryPreviewLayer shows the correct pattern with explicit offset calculation (lines 237-238).
Apply translation at lines 100-101, then update lines 124-125 and 144-145 to use canvas coordinates:
// Get world coordinates of target tile
const worldX = this.game.x(targetTile);
const worldY = this.game.y(targetTile);
+
+ // Translate to canvas coordinates
+ const canvasX = worldX + this.game.width() / 2;
+ const canvasY = worldY + this.game.height() / 2;
// Draw timer text
const incomingText = isInbound
? translateText("bomb_timer.incoming")
: "";
const impactText = translateText("bomb_timer.til_impact", {
seconds: remainingSeconds,
});
const text = incomingText ? `${incomingText} ${impactText}` : impactText;
// Set text style
this.context.font = "bold 10px sans-serif";
this.context.textAlign = "center";
this.context.textBaseline = "middle";
// Measure text for background sizing
const textMetrics = this.context.measureText(text);
const padding = 3;
const bgWidth = textMetrics.width + padding * 2;
const bgHeight = 12 + padding * 2;
const offsetY = 25; // Offset further up to avoid blocking
- const bgX = worldX - bgWidth / 2;
- const bgY = worldY - offsetY - bgHeight / 2;
+ const bgX = canvasX - bgWidth / 2;
+ const bgY = canvasY - offsetY - bgHeight / 2;
// Draw transparent background
this.context.fillStyle = "rgba(0, 0, 0, 0.4)";
this.context.fillRect(bgX, bgY, bgWidth, bgHeight);
// Set text color based on direction
if (isInbound) {
// Red for inbound
this.context.fillStyle = "#ff6666";
this.context.strokeStyle = "rgba(0, 0, 0, 0.5)";
} else {
// Yellow/orange for outbound
this.context.fillStyle = "#ffcc66";
this.context.strokeStyle = "rgba(0, 0, 0, 0.5)";
}
// Draw text with lighter stroke for less blocking
this.context.lineWidth = 1;
- this.context.strokeText(text, worldX, worldY - offsetY);
- this.context.fillText(text, worldX, worldY - offsetY);
+ this.context.strokeText(text, canvasX, canvasY - offsetY);
+ this.context.fillText(text, canvasX, canvasY - offsetY);🤖 Prompt for AI Agents
In src/client/graphics/layers/BombTimerLayer.ts around lines 100-101, the code
uses world coordinates directly for canvas drawing which is offset because the
offscreen canvas is positioned at (-width/2, -height/2); change the computation
to convert world -> canvas coords by adding (width/2, height/2): set canvasX =
worldX + this.width/2 and canvasY = worldY + this.height/2 at those lines, then
update the draw calls referenced at lines 124-125 and 144-145 to use canvasX and
canvasY instead of worldX/worldY so drawn timers appear at the correct world
positions.
There was a problem hiding this comment.
A completely new layer is overkill just to display labels over the nukes, specially since the nukes already have UI elements displayed independently.
The NukeAreaFx could be simply extended to display the remaining time, and that would also reduce your code by a great extent.
For example:
## Description: Adds a section to the pull request template about linking issues, as suggested in #2401 ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] 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: loymdayddaud
…ajectory data and simplify FxLayer updates
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 (3)
src/client/graphics/fx/NukeAreaFx.ts(3 hunks)src/client/graphics/layers/FxLayer.ts(2 hunks)src/core/game/UnitImpl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/UnitImpl.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/fx/NukeAreaFx.tssrc/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/graphics/layers/FxLayer.ts
📚 Learning: 2025-08-24T11:13:08.495Z
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.
Applied to files:
src/client/graphics/layers/FxLayer.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/FxLayer.ts
🧬 Code graph analysis (2)
src/client/graphics/fx/NukeAreaFx.ts (3)
src/core/game/GameView.ts (4)
x(679-681)y(682-684)trajectoryIndex(134-136)trajectoryLength(138-140)src/core/configuration/Config.ts (1)
NukeMagnitude(69-72)src/core/game/UnitImpl.ts (1)
trajectoryIndex(374-376)
src/client/graphics/layers/FxLayer.ts (2)
src/core/game/GameView.ts (7)
unit(666-668)targetTile(130-132)trajectoryIndex(134-136)trajectoryLength(138-140)UnitView(44-187)x(679-681)y(682-684)src/client/graphics/fx/NukeAreaFx.ts (1)
NukeAreaFx(4-193)
| if (isOutbound || isInbound) { | ||
| fx.updateTimer( | ||
| remainingSeconds, | ||
| isOutbound, | ||
| trajectoryIndex, | ||
| trajectoryLength, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Timer sticks after ownership change
Because we only call fx.updateTimer() when either flag is true, the FX never learns that isOutbound flipped to false. When a missile stops belonging to us (for example, an alliance break mid-flight), NukeAreaFx keeps its stale isOutbound = true and keeps rendering the countdown even though we no longer own or target that tile. Please push the updated flags every tick and send null when we want the timer hidden so the FX clears its state immediately.
- if (isOutbound || isInbound) {
- fx.updateTimer(
- remainingSeconds,
- isOutbound,
- trajectoryIndex,
- trajectoryLength,
- );
- }
+ const timerSeconds =
+ isOutbound || isInbound ? remainingSeconds : null;
+ fx.updateTimer(
+ timerSeconds,
+ isOutbound,
+ trajectoryIndex,
+ trajectoryLength,
+ );🤖 Prompt for AI Agents
In src/client/graphics/layers/FxLayer.ts around lines 137 to 144, the code only
calls fx.updateTimer(...) when isOutbound or isInbound are true, so FX instances
never learn when ownership flips to false and keep stale timers; change logic to
call fx.updateTimer(...) every tick with the current remainingSeconds,
isOutbound, trajectoryIndex and trajectoryLength so the FX state is always
updated, and when the timer should be hidden call fx.updateTimer(null, false,
null, null) (or equivalent null/false sentinel values) to clear the FX timer
immediately.
|
Do you have any images of what it looks like now? |
6 participants
Description:
Added impact timer to incoming and outgoing nukes.
Note: As part of this change, the nuke impact highlighter effect now applies to nukes targeting the current player's territory, in addition to outgoing ones. Incoming nukes flash, with the flashing intensifying as impact approaches to distinguish them from outgoing nukes.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
kerverse