fix(nukes): pick closest silo with a clear path around impassable terrain - #4815
Conversation
…rain Silo selection for nuke launches previously picked the closest silo by Manhattan distance with no path awareness, so a launch whose parabola crossed impassable terrain was accepted and then silently aborted even when another silo had a clear shot. - nukeSpawn now returns the closest ready silo whose trajectory avoids impassable terrain on the up OR down curve; canBuild returns false when no silo qualifies (MIRVs keep plain closest-silo selection — warheads are exempt from impassable checks) - NukeExecution flies the requested curve direction if clear, flips to the opposite curve if not, and aborts only when both are blocked - blocked checks sample the curve at a fixed fine increment instead of the nuke's speed, so a fast nuke can no longer tunnel through a thin impassable strip that a slow one would hit - shared clearParabolaDirection/isParabolaBlocked helpers keep the sim, nation AI, and build-preview arc consistent; the preview now draws from the silo and curve direction the sim would actually use Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughNuke trajectory handling now samples both parabola directions for impassable terrain. Silo selection prefers the nearest valid silo. Preview and execution reuse the resolved direction. MIRV launches retain their terrain bypass. ChangesNuke trajectory fallback
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant BuildPreviewController
participant PlayerImpl
participant PathFinder
participant NukeExecution
Player->>BuildPreviewController: choose nuke target
BuildPreviewController->>PlayerImpl: find valid silo
PlayerImpl->>PathFinder: check preferred parabola
PathFinder-->>PlayerImpl: clear or blocked
PlayerImpl->>PathFinder: check opposite parabola when needed
PathFinder-->>PlayerImpl: resolved direction or no path
PlayerImpl-->>BuildPreviewController: silo and direction
BuildPreviewController-->>Player: render trajectory
Player->>NukeExecution: launch nuke
NukeExecution->>PathFinder: validate and rebuild path when direction changes
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-project-automation
Bot
moved this from Triage
to Complete
in OpenFront Release Management
![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 (2)
src/core/pathfinding/PathFinder.Parabola.ts (1)
14-41: 🚀 Performance & Scalability | 🔵 TrivialFine-grained sampling adds cost to hot paths.
BLOCKED_CHECK_INCREMENT = 1makesisParabolaBlockedsample every unit of curve length. This is correct for the tunneling fix, but it runs inside several hot loops:
PlayerImpl.nukeSpawncallsclearParabolaDirection(up to 2 samples) for each ready silo, in distance order, until one is clear.NationNukeBehavior.maybeSendNukeevaluates up to ~40 candidate tiles per AI decision; each candidate goes throughcanBuild→nukeSpawn.NationNukeBehavior.maybeDestroyEnemySamcallsisTrajectoryBlockedByImpassableonce per silo per candidate SAM target.For nations with several silos and terrain-heavy maps, this can multiply into hundreds of fine-grained curve samples per AI turn. Consider watching for this in profiling, or caching a resolved direction per
(silo, target)pair within a single AI evaluation pass if it shows up as a bottleneck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pathfinding/PathFinder.Parabola.ts` around lines 14 - 41, Profile the hot paths invoking isParabolaBlocked, especially PlayerImpl.nukeSpawn and NationNukeBehavior.maybeSendNuke/maybeDestroyEnemySam, and reduce repeated fine-grained sampling if it is a bottleneck. Prefer caching the resolved parabola direction or blocked result per silo-target pair within a single AI evaluation pass, while preserving the tunneling fix and existing behavior.src/client/controllers/BuildPreviewController.ts (1)
337-343: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReuse
this.game.manhattanDistfor the silo sort.
BuildPreviewController.gameis aGameView,GameView.manhattanDist()delegates to the sharedGameMapImpl.manhattanDist()implementation, andPlayerImpl.nukeSpawnuses that same API for the source silo. Usethis.game.manhattanDist(a.tile(), tileRef)andthis.game.manhattanDist(b.tile(), tileRef)instead.♻️ Proposed refactor
silos.sort( (a, b) => - Math.abs(this.game.x(a.tile()) - dstX) + - Math.abs(this.game.y(a.tile()) - dstY) - - (Math.abs(this.game.x(b.tile()) - dstX) + - Math.abs(this.game.y(b.tile()) - dstY)), + this.game.manhattanDist(a.tile(), tileRef) - + this.game.manhattanDist(b.tile(), tileRef), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/controllers/BuildPreviewController.ts` around lines 337 - 343, Update the silo comparator in BuildPreviewController to use this.game.manhattanDist(a.tile(), tileRef) and this.game.manhattanDist(b.tile(), tileRef) instead of manually calculating coordinate differences, preserving the existing ascending distance sort.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/execution/nation/NationNukeBehavior.ts`:
- Around line 631-644: Update the trajectory handling between
isTrajectoryBlockedByImpassable and isTrajectoryInterceptableBySam to resolve
the flyable parabola direction with clearParabolaDirection, using the fallback
direction selected by NukeExecution when the upward curve is blocked. Pass that
resolved direction into the SAM pathfinder instead of hardcoding directionUp:
true, while preserving interception checks for unreachable trajectories.
---
Nitpick comments:
In `@src/client/controllers/BuildPreviewController.ts`:
- Around line 337-343: Update the silo comparator in BuildPreviewController to
use this.game.manhattanDist(a.tile(), tileRef) and
this.game.manhattanDist(b.tile(), tileRef) instead of manually calculating
coordinate differences, preserving the existing ascending distance sort.
In `@src/core/pathfinding/PathFinder.Parabola.ts`:
- Around line 14-41: Profile the hot paths invoking isParabolaBlocked,
especially PlayerImpl.nukeSpawn and
NationNukeBehavior.maybeSendNuke/maybeDestroyEnemySam, and reduce repeated
fine-grained sampling if it is a bottleneck. Prefer caching the resolved
parabola direction or blocked result per silo-target pair within a single AI
evaluation pass, while preserving the tunneling fix and existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 797a32a8-f2ae-4bd2-a287-58e8ed29fe1b
📒 Files selected for processing (6)
src/client/controllers/BuildPreviewController.tssrc/core/execution/NukeExecution.tssrc/core/execution/nation/NationNukeBehavior.tssrc/core/game/PlayerImpl.tssrc/core/pathfinding/PathFinder.Parabola.tstests/ImpassableTerrain.test.ts
| /** | ||
| * Check if the parabolic nuke trajectory from spawnTile to targetTile | ||
| * crosses any impassable terrain. Mirrors the check in NukeExecution that | ||
| * aborts such launches | ||
| * crosses impassable terrain on BOTH curve directions. Mirrors | ||
| * NukeExecution, which flips to the opposite curve when the requested one | ||
| * is blocked and aborts only when both are. | ||
| */ | ||
| private isTrajectoryBlockedByImpassable( | ||
| spawnTile: TileRef, | ||
| targetTile: TileRef, | ||
| ): boolean { | ||
| const pathFinder = UniversalPathFinding.Parabola(this.game, { | ||
| increment: this.game.config().nukeSpeed(UnitType.AtomBomb), | ||
| distanceBasedHeight: true, | ||
| directionUp: true, | ||
| }); | ||
| const path = pathFinder.findPath(spawnTile, targetTile) ?? []; | ||
| for (const tile of path) { | ||
| if (this.game.isImpassable(tile)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| return ( | ||
| clearParabolaDirection(this.game, spawnTile, targetTile, true) === null | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
isTrajectoryInterceptableBySam still assumes the nuke always flies "up," which is no longer guaranteed.
isTrajectoryBlockedByImpassable now correctly treats a target as reachable when either curve direction is clear (matching NukeExecution's fallback). Because of that, a candidate tile can now reach nukeTileScore/salvo planning purely because the DOWN curve is clear while the UP curve is blocked by terrain.
isTrajectoryInterceptableBySam (unchanged, lines 548-629) always builds its SAM-coverage check with directionUp: true ("AI nukes always go 'up' for now"). For a target where only the down curve is actually flyable, this function evaluates SAM coverage along a curve the nuke will never fly (the up curve, blocked by terrain), instead of the curve NukeExecution will actually use. This can make the AI:
- On Hard/Impossible, wrongly treat a safe (down-curve) launch as SAM-interceptable, or vice versa.
- In
maybeDestroyEnemySam's salvo planning, compute wrong flight times/interception odds for silos whose actual flight path is the down curve.
Before this PR, the removed upward-only terrain filter guaranteed every candidate that reached this check had a clear up curve, so the hardcoded assumption held. Now that guarantee is gone. Consider resolving the actual direction with clearParabolaDirection before computing SAM interception, and passing that resolved direction into isTrajectoryInterceptableBySam's pathfinder instead of hardcoding true.
#!/bin/bash
# Inspect all call sites and the hardcoded direction assumption.
rg -n "isTrajectoryInterceptableBySam|directionUp: true" src/core/execution/nation/NationNukeBehavior.ts -B3 -A3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/execution/nation/NationNukeBehavior.ts` around lines 631 - 644,
Update the trajectory handling between isTrajectoryBlockedByImpassable and
isTrajectoryInterceptableBySam to resolve the flyable parabola direction with
clearParabolaDirection, using the fallback direction selected by NukeExecution
when the upward curve is blocked. Pass that resolved direction into the SAM
pathfinder instead of hardcoding directionUp: true, while preserving
interception checks for unreachable trajectories.
1 participant
Problem
Silo selection for nuke launches picked the closest silo by Manhattan distance with no path awareness. When that silo's parabola crossed impassable terrain, the launch was accepted (
canBuildsucceeded) and then silently aborted inNukeExecution— even when a slightly farther silo had a clear shot, or when the other curve direction (up vs down) from the same silo was clear.Changes
Silo selection (
PlayerImpl.nukeSpawn)canBuildreturnsfalse, so the UI shows the target as unbuildable up front instead of accepting a launch that would abort.Curve direction (
NukeExecution)Speed-independent blocked checks (
PathFinder.Parabola)isParabolaBlocked/clearParabolaDirectionhelpers sample the curve at a fixed fine increment instead of the nuke's speed. Previously a fast nuke could "tunnel" through a thin impassable strip between samples that a slow nuke would hit.Consumers kept in sync
isTrajectoryBlockedByImpassablenow means "blocked on both curves" (matching the sim's flip behavior); the per-target check inmaybeSendNukewas removed sincecanBuildnow guarantees it.Tests
Five new tests in
ImpassableTerrain.test.ts(terrain builder now supports partial-height walls so the two curves can be blocked independently):reachedTarget())canBuildreturnsfalseand the execution never builds a unitFull suite passes (2842 tests).
🤖 Generated with Claude Code