Make MIRVs take fixed time to fire regardless of launch location 🚀#3770
Make MIRVs take fixed time to fire regardless of launch location 🚀#3770FloPinguin wants to merge 2 commits intomainfrom
Conversation
WalkthroughMIRVExecution defers computing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/execution/MIRVExecution.ts (2)
121-137: Measuring the path withincrement: 1works, but it is a lot of work — and a small wording fix.For every MIRV launch this builds an entire parabolic path at one-tile resolution and then walks it to sum Euclidean segments. For long-range silos that is a non-trivial amount of allocation just to derive a single number. Two cheaper options that keep the rest of the change intact:
- Compute the parabolic arc length analytically from
spawnTile→separateDst(and the apex implied by line 87). For a quadratic/parabolic curve this is a closed form and avoids any pathfinder allocation.- Or sample with a coarser
increment(e.g. 8–16); the relative error on arc length is tiny for a smooth parabola and the work drops by the same factor.Also: the comment says "~1 px apart" but the units here are map tiles, not pixels.
♻️ Small wording fix
private computeFixedTimeSpeed(): number { // Measure the actual parabolic path length, then divide by the desired // tick count. A temporary fine-grained pathfinder gives a precise - // length estimate (cached points are spaced ~1 px apart). + // length estimate (cached points are spaced ~1 tile apart). const measure = UniversalPathFinding.Parabola(this.mg, { increment: 1 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/MIRVExecution.ts` around lines 121 - 137, computeFixedTimeSpeed currently builds a dense Parabola path with UniversalPathFinding.Parabola(this.mg, { increment: 1 }) and walks every segment to compute arc length (using spawnTile, separateDst and fixedFireTicks), which is wasteful and misdocuments units; replace this with either an analytic quadratic arc-length computation for the parabola defined by spawnTile → separateDst and the apex (preferred) or, if you want a minimal change, pass a coarser increment (e.g. 8 or 16) into UniversalPathFinding.Parabola to drastically reduce allocations, then sum segments as before and keep the fallback to this.mg.config().defaultNukeSpeed(); also update the comment to say "tiles" (not "px") and mention the chosen sampling increment or the analytic formula used.
90-95: The speed-equals-increment trick is correct, but consider optimizing the path length calculation.The mechanism works as intended:
computeFixedTimeSpeed()calculatesspeed = pathLength / 75, then eachtick()call advances by that speed distance. WithDistanceBasedBezierCurve.increment()accumulating distance (not counting points), 75 iterations reach exactlypathLength, yielding the fixed 75-tick flight time.However, computing a full fine-grained path (increment: 1) every launch and summing Euclidean distances is expensive for distant targets. For a pure parabola, arc length can be derived analytically or sampled with coarser steps, reducing work significantly.
Also, the comment says "~1 px apart"—the units are tiles, not pixels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/MIRVExecution.ts` around lines 90 - 95, The current computeFixedTimeSpeed() computes pathLength by generating a fine-grained DistanceBasedBezierCurve (increment: 1) via UniversalPathFinding.Parabola and summing distances, which is expensive for distant targets; change computeFixedTimeSpeed() (or the path length calculation it calls) to avoid full 1-unit sampling—either compute the parabola arc length analytically for the parabola parameters used, or sample with a coarser step and optionally refine (e.g., Simpson/gauss integration or adaptive sampling) to approximate length much faster; keep the interface so this.speed and the Parabola path finder still use the computed speed, and update the comment that units are tiles (not pixels).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/execution/MIRVExecution.ts`:
- Around line 121-137: computeFixedTimeSpeed currently builds a dense Parabola
path with UniversalPathFinding.Parabola(this.mg, { increment: 1 }) and walks
every segment to compute arc length (using spawnTile, separateDst and
fixedFireTicks), which is wasteful and misdocuments units; replace this with
either an analytic quadratic arc-length computation for the parabola defined by
spawnTile → separateDst and the apex (preferred) or, if you want a minimal
change, pass a coarser increment (e.g. 8 or 16) into
UniversalPathFinding.Parabola to drastically reduce allocations, then sum
segments as before and keep the fallback to this.mg.config().defaultNukeSpeed();
also update the comment to say "tiles" (not "px") and mention the chosen
sampling increment or the analytic formula used.
- Around line 90-95: The current computeFixedTimeSpeed() computes pathLength by
generating a fine-grained DistanceBasedBezierCurve (increment: 1) via
UniversalPathFinding.Parabola and summing distances, which is expensive for
distant targets; change computeFixedTimeSpeed() (or the path length calculation
it calls) to avoid full 1-unit sampling—either compute the parabola arc length
analytically for the parabola parameters used, or sample with a coarser step and
optionally refine (e.g., Simpson/gauss integration or adaptive sampling) to
approximate length much faster; keep the interface so this.speed and the
Parabola path finder still use the computed speed, and update the comment that
units are tiles (not pixels).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ddd293c-4629-4e50-853c-c13e939469c0
📒 Files selected for processing (1)
src/core/execution/MIRVExecution.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/execution/MIRVExecution.ts (2)
29-29: Optional: hoistfixedFireTicksto game config.
75ticks is the new fixed flight duration. If balance tuning is expected (similar to other MIRV constants), exposing it viathis.mg.config()would make tuning easier without code changes. Inline is fine for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/MIRVExecution.ts` at line 29, Hoist the magic constant `fixedFireTicks` out of the class and into game config: replace the inline `private fixedFireTicks = 75` in `MIRVExecution` with a value read from `this.mg.config()` (e.g., `this.mg.config('mirv.fixedFireTicks')`) with a default of 75, and add that key to the config defaults/typing; update any code that references `fixedFireTicks` to use the new config-backed value so tuning can be done without code changes.
121-137: Approach is sound; consider a warn on the fallback path.Sampling the parabola at
increment: 1and summing Euclidean segment lengths is a fine, simple way to estimate arc length, andlength / fixedFireTickscorrectly turns that into a per-tick step. The?? []pluslength <= 0guard is robust against an empty/missing path.One minor point: when the fallback to
defaultNukeSpeed()triggers, the MIRV silently reverts to the old variable-time behavior, which is exactly the thing this PR is trying to remove. A shortconsole.warnthere would make any future regression visible without changing behavior.🪵 Suggested fallback log
if (length <= 0) { + console.warn( + "MIRV: failed to measure parabola path length; falling back to default speed", + ); return this.mg.config().defaultNukeSpeed(); } return length / this.fixedFireTicks;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/MIRVExecution.ts` around lines 121 - 137, In computeFixedTimeSpeed(), detect when the measured path length is <= 0 and before returning this.mg.config().defaultNukeSpeed(), emit a warning (e.g., console.warn) indicating the parabola path measurement failed and you're falling back to defaultNukeSpeed; reference the UniversalPathFinding.Parabola(...) result, the inputs this.spawnTile and this.separateDst, and this.fixedFireTicks in the message so future regressions are easier to trace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/execution/MIRVExecution.ts`:
- Line 29: Hoist the magic constant `fixedFireTicks` out of the class and into
game config: replace the inline `private fixedFireTicks = 75` in `MIRVExecution`
with a value read from `this.mg.config()` (e.g.,
`this.mg.config('mirv.fixedFireTicks')`) with a default of 75, and add that key
to the config defaults/typing; update any code that references `fixedFireTicks`
to use the new config-backed value so tuning can be done without code changes.
- Around line 121-137: In computeFixedTimeSpeed(), detect when the measured path
length is <= 0 and before returning this.mg.config().defaultNukeSpeed(), emit a
warning (e.g., console.warn) indicating the parabola path measurement failed and
you're falling back to defaultNukeSpeed; reference the
UniversalPathFinding.Parabola(...) result, the inputs this.spawnTile and
this.separateDst, and this.fixedFireTicks in the message so future regressions
are easier to trace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba5ab5cf-0eed-49ad-b1b1-77a91b890736
📒 Files selected for processing (1)
src/core/execution/MIRVExecution.ts
1 participant
Description:
Multiple people asked for that in the game-feedback DC channel, so here is a PR for it: MIRV flight time from launch to warhead separation is now a fixed 75 ticks (enough reaction time for the targeted player?) regardless of where the launching silo sits relative to the target.
(The video was taken with just 50 ticks configured)
2026-04-26.00-16-23.mp4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin