fix: remove double x() dereference in MIRV separation point calc#3940
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR fixes a calculation error in MIRVExecution.tick: the separation midpoint X was computed with a duplicate map-grid transform; the extra ChangesMIRV Separation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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.
Actionable comments posted: 1
🤖 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/MIRVExecution.ts`:
- Line 85: Add a deterministic unit test for MirvExecution.tick() that
constructs a MirvExecution with a controllable map/manager mock (mock mg.x to
return known integers for the dst and nuke.tile()), set dst and nuke.tile()
values so the midpoint is non-trivial, call MirvExecution.tick(), and assert the
computed separation midpoint X equals floor((x(dst) + x(nuke.tile())) / 2)
(i.e., Math.floor((dstX + nukeTileX) / 2)). Target the MirvExecution class and
its tick() behavior and assert the exact numeric midpoint to prevent regressions
in the single-dereference midpoint calculation.
🪄 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
Run ID: 6170a842-6079-4a44-b274-a96da8c63f33
📒 Files selected for processing (1)
src/core/execution/MIRVExecution.ts
| this.mg.stats().bombLaunch(this.player, this.targetPlayer, UnitType.MIRV); | ||
| const x = Math.floor( | ||
| (this.mg.x(this.dst) + this.mg.x(this.mg.x(this.nuke.tile()))) / 2, | ||
| (this.mg.x(this.dst) + this.mg.x(this.nuke.tile())) / 2, |
There was a problem hiding this comment.
Add a regression test for the midpoint X fix.
Line 85 fixes the bug, but this src/core/ logic change ships without a test in the provided diff. Please add a deterministic test that exercises MirvExecution.tick() and asserts the separation midpoint uses a single dereference (x = floor((x(dst)+x(nukeTile))/2)), so this regression does not return.
As per coding guidelines, “All changes to src/core/ must include tests”.
🤖 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/MIRVExecution.ts` at line 85, Add a deterministic unit
test for MirvExecution.tick() that constructs a MirvExecution with a
controllable map/manager mock (mock mg.x to return known integers for the dst
and nuke.tile()), set dst and nuke.tile() values so the midpoint is non-trivial,
call MirvExecution.tick(), and assert the computed separation midpoint X equals
floor((x(dst) + x(nuke.tile())) / 2) (i.e., Math.floor((dstX + nukeTileX) / 2)).
Target the MirvExecution class and its tick() behavior and assert the exact
numeric midpoint to prevent regressions in the single-dereference midpoint
calculation.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
mg.x() takes a TileRef and returns a number (x coordinate). The code was calling mg.x(mg.x(tile)), feeding the numeric result back into x() which expects a TileRef. This produces an incorrect midpoint for MIRV warhead separation, causing warheads to spread from a wrong position on the map.
berkelmali
force-pushed
the
fix/mirv-double-x-dereference
branch
from
e31b2e3 to
5b98b27
Compare
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
2 participants
mg.x() takes a TileRef and returns a number (x coordinate). The code was calling mg.x(mg.x(tile)), feeding the numeric result back into x() which expects a TileRef. This produces an incorrect midpoint for MIRV warhead separation, causing warheads to spread from a wrong position on the map.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires