enhance: Nuke preview trajectory shows SAM targetability #2531
Conversation
|
|
WalkthroughAdds logic and rendering to NukeTrajectoryPreviewLayer to detect two indices where the bomb's targetability flips and draws the trajectory with distinct styles/colors and arc markers at those switch points. Also introduces a private field to store the untargetable segment bounds. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (4)
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (4)
18-18: Clarify what the two indices represent.The field name
targetableSwitchPointIndexdoesn't clearly indicate what each index in the tuple represents. Consider renaming tountargetableSegmentBoundsor adding a JSDoc comment explaining that[0]marks where the bomb becomes untargetable (leaves spawn range) and[1]marks where it becomes targetable again (enters target range).
215-242: Document the switch point algorithm.The logic computes two critical boundaries but lacks explanation. The algorithm handles three trajectory segments:
- Near spawn (targetable by SAMs)
- Middle flight (untargetable - invulnerable)
- Near target (targetable by SAMs)
The
breakat line 231 handles trajectories where spawn and target ranges overlap (no untargetable segment), but this isn't obvious from the code. Add comments explaining:
- What each index represents
- Why the break occurs when
tileis beyond spawn range AND within target range- That -1 means "no switch point found"
Consider adding comments like:
+ // Find two switch points where bomb transitions: + // [0]: leaves spawn range, enters untargetable zone + // [1]: enters target range, becomes targetable again this.targetableSwitchPointIndex = [-1, -1]; for (let i = 0; i < this.trajectoryPoints.length; i++) { const tile = this.trajectoryPoints[i]; if (this.targetableSwitchPointIndex[0] === -1) { if ( this.game.euclideanDistSquared(tile, this.cachedSpawnTile) > targetRangeSquared ) { if ( this.game.euclideanDistSquared(tile, targetTile) < targetRangeSquared ) { + // Spawn and target ranges overlap - trajectory stays targetable break; } else { this.targetableSwitchPointIndex[0] = i; }
264-271: Consider extracting color parameters as constants.The alpha and saturation values (0.8, 0.5) are magic numbers. If these visual parameters need tuning later, extracting them as named constants would improve maintainability:
const TRAJECTORY_ALPHA = 0.8; const UNTARGETABLE_SATURATION = 0.8; const TARGETABLE_SATURATION = 0.5;
278-316: LGTM with minor refinement suggestions.The rendering logic correctly visualizes the three trajectory segments with distinct styles. The arc markers at transition points provide clear visual feedback.
Minor refinements to consider:
- Extract magic numbers: Arc radius
4and dash patterns[8, 4],[2, 6]could be constants- Simplify state changes: The multiple
beginPath(),stroke(),setLineDash()calls work correctly but make the loop harder to followIf simplification is desired:
const ARC_RADIUS = 4; const TARGETABLE_DASH = [8, 4]; const UNTARGETABLE_DASH = [2, 6];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/NukeTrajectoryPreviewLayer.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/NukeTrajectoryPreviewLayer.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/NukeTrajectoryPreviewLayer.ts
|
I like it but for v28 I believe we are removing all bomb invincibility so I’m not sure if this will be approved If you want, I can assign it to you? |
|
What's the argument for removing bomb invincibility? My biggest gripe is that it would remove from what little influence you have on players on the other side of the map, and therefore helps players on the edge of the map even more than they already are. If it's a sure, agreed upon change, you can assign it to me (I am also working on showing which SAMs will shoot down the bomb in the preview) but I believe it's a good game mechanic and would like to have some input on the discussion if possible! |
|
|
Closing in favor for #2541 |
3 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Nuke preview trajectory now calculates when the nuke becomes invulnerable to SAM defense and displays accordingly.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
bibizu