Optimize nuke explosion #3176
Optimize nuke explosion #3176
Conversation
WalkthroughDetonation logic moved to a two-pass process: first pass accumulates affected-tile counts per player and marks fallout/relinquishes land tiles; second pass applies scaled casualties to each player's troops, outgoing attacks, and transport ships using the aggregated counts and a diminishing death factor. Changes
Sequence Diagram(s)sequenceDiagram
participant N as NukeExecution
participant T as TileGrid
participant P as PlayerRegistry
participant U as Units/Attacks/Transports
N->>T: first pass — iterate affected tiles
T-->>N: tile owner & type
N->>P: accumulate tilesPerPlayers[owner]++
N->>T: mark fallout / relinquish land tiles
N->>P: second pass — iterate impacted players
P-->>N: tilesBeforeNuke / player info
loop per impacted tile (diminishing)
N->>U: compute and apply troop reductions
U-->>N: update outgoing attacks & transports
end
N->>P: update alliances if broken
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/NukeExecution.ts (1)
258-261:⚠️ Potential issue | 🟠 MajorFor MIRV warheads, use each affected player's own
maxTroopsin damage calculations.When a MIRV nuke hits a border, all affected players' damage is calculated using
maxTroopsfrom the target player. This breaks the MIRV damage formula for non-target players—their excess troops are normalized against the wrong capacity ceiling. Each player should use their ownmaxTroopsvalue in thenukeDeathFactorcall.(Note: Standard nukes ignore
maxTroops, so this only affects MIRV warheads.)
🤖 Fix all issues with AI agents
In `@src/core/execution/NukeExecution.ts`:
- Around line 268-269: tilesPerPlayers is updated incorrectly when a player is
first seen: in NukeExecution (around the tilesPerPlayers update using owner and
numTiles) the undefined case sets the count to 0 so the first tile is lost;
change the logic so that when numTiles is undefined you set the map entry to 1
(i.e. numTiles === undefined ? 1 : numTiles + 1) so the initial tile is counted
correctly.
- Around line 277-311: The loop in NukeExecution (tilesPerPlayers processing
inside detonate) computes nukeDeathFactor after owner.relinquish has already
reduced player.numTilesOwned(), causing incorrect scaling; fix by capturing each
player's pre-relinquish tile count (e.g., record originalTiles =
player.numTilesOwned() before any owner.relinquish calls) and pass that
originalTiles into nukeDeathFactor when calling player.removeTroops and when
adjusting outgoingAttacks and TransportShip units; also remove the unnecessary
optional chaining (change this.mg?.config() to this.mg.config()) in the two
places inside that loop so style is consistent.
🧹 Nitpick comments (1)
src/core/execution/NukeExecution.ts (1)
288-311: Duplicate logic for outgoing attacks and transport ships — consider extracting a helper.The death-application block for
outgoingAttacks()(lines 288-299) andTransportShipunits (lines 300-311) is nearly identical. A small helper would reduce duplication and make future formula tweaks less error-prone.Example extraction
// Inside detonate(), before the per-player loop: const applyDeaths = (troops: number, setTroops: (t: number) => void) => { const deaths = this.mg.config().nukeDeathFactor(this.nukeType, troops, player.numTilesOwned(), maxTroops); setTroops(troops - deaths * numTiles); }; // Then in the loop body: for (const attack of player.outgoingAttacks()) { applyDeaths(attack.troops(), (t) => attack.setTroops(t)); } for (const ship of player.units(UnitType.TransportShip)) { applyDeaths(ship.troops(), (t) => ship.setTroops(t)); }
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/execution/NukeExecution.ts`:
- Around line 288-311: In NukeExecution, clamp troop subtraction so attacks and
transport ships never go negative: replace direct
attack.setTroops(attack.troops() - deaths) with a guarded assignment that
computes newTroops = Math.max(0, attack.troops() - deaths) (or call
player.removeTroops-like logic) and then set via setTroops/newTroops; also
eliminate the duplicated logic for player.outgoingAttacks() and
player.units(UnitType.TransportShip) by extracting a small helper (e.g.,
processAttackDeaths(attack)) that computes deaths via
this.mg.config().nukeDeathFactor(this.nukeType, attack.troops(),
player.numTilesOwned(), maxTroops) * numTiles and applies the clamped troop
update.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/NukeExecution.ts (1)
258-260:⚠️ Potential issue | 🟠 MajorCompute
maxTroopsper affected player, not once from the destination owner.For MIRV warheads, the
nukeDeathFactor()formula scales damage based onmaxTroops. Currently,maxTroopsis computed once fromthis.target()(destination tile owner) and applied to all affected players in the second loop. This doesn't match the per-player pattern used everywhere else in the codebase and causes incorrect MIRV damage scaling—especially problematic if the destination becomesTerraNulliuswheremaxTroopsequals 1.Regular nukes are unaffected since their formula ignores the
maxTroopsparameter. Move themaxTroopscalculation inside the second loop using each affected player's own max troops.
🤖 Fix all issues with AI agents
In `@src/core/execution/NukeExecution.ts`:
- Around line 282-284: The loop in NukeExecution.ts shadows the outer numTiles
(destructured from the map entry) by declaring a new const numTiles inside the
for loop; rename the inner variable to a distinct name like remainingTiles (or
tilesLeft) and update all uses inside the loop body (the references currently
expecting tilesBeforeNuke - i) to use remainingTiles so the outer numTiles
remains unshadowed and intent is clear (affects the for loop where
tilesBeforeNuke and numTiles are used).
🧹 Nitpick comments (1)
src/core/execution/NukeExecution.ts (1)
293-310: Inner loop re-fetchesplayer.outgoingAttacks()on every tile iteration.
player.outgoingAttacks()is callednumTilestimes per player. The list does not change during the loop (no attacks are added or removed), so cache it once — just liketransportShipsis cached on line 280.Proposed fix
const transportShips = player.units(UnitType.TransportShip); + const outgoingAttacks = player.outgoingAttacks(); // nukeDeathFactor could compute the complete fallout in a single call instead for (let i = 0; i < numTiles; i++) { ... - player.outgoingAttacks().forEach((attack) => { + outgoingAttacks.forEach((attack) => {
1 participant
Description:
When a nuke detonates, the explosion is looping over each tiles. Among other things, it filters the owner units to find the
TransportShipsunits.This operation is cpu-intensive and repeated over each tiles can make the tick noticably slower.
On this screenshot the detonation function took 100ms:
I suspect it led to a few frame freeze when I MIRVed too.
Suggestion: loop over the impacted players rather than the tiles.
With this suggestion, the same nuke takes between 4ms to 20ms:
However this changes the death formula used, as they were repeated over each tiles with ever-smaller values, and with this change the operation is done all-at-once. This will result in a different outcome.
In my opinion the performance gain is seductive enough to maybe tweak the formula to make it work with this revised strategy.
What do you think?
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
IngloriousTom