Move attack ratio calculations inside of execution #2564
Conversation
WalkthroughThis PR fixes a race condition where queued percentage-based attacks calculated troop amounts using stale player state. The fix shifts troop computation from send-time to execution-time by storing attack ratios throughout the event and execution pipeline, ensuring the current troop count is used when the attack actually executes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/ClientGameRunner.ts (1)
645-659: Bug:sendBoatAttackIntentstill multiplies troops by ratio.This method still computes
this.myPlayer.troops() * this.renderer.uiState.attackRatioon line 654, butSendBoatAttackIntentEventnow expects just the ratio (perTransport.tschanges). This defeats the purpose of the fix for boat attacks initiated via this path.Apply this diff to fix:
this.myPlayer.bestTransportShipSpawn(tile).then((spawn: number | false) => { if (this.myPlayer === null) throw new Error("not initialized"); this.eventBus.emit( new SendBoatAttackIntentEvent( this.gameView.owner(tile).id(), tile, - this.myPlayer.troops() * this.renderer.uiState.attackRatio, + this.renderer.uiState.attackRatio, spawn === false ? null : spawn, ), ); });
🧹 Nitpick comments (3)
src/core/Schemas.ts (1)
243-249: Consider adding upper bound validation.
attackRatiousesz.number().nonnegative()which allows any value ≥ 0, including values > 1.0 (like 1.5 = 150%). The execution code clamps to available troops anyway, but adding.max(1)here would catch invalid client input early.export const BoatAttackIntentSchema = BaseIntentSchema.extend({ type: z.literal("boat"), targetID: ID.nullable(), - attackRatio: z.number().nonnegative(), + attackRatio: z.number().nonnegative().max(1), dst: z.number(), src: z.number().nullable(), });Same could apply to
AttackIntentSchemaif desired.src/core/execution/TransportShipExecution.ts (1)
98-106: Minor: falsy check onattackRatiotreats 0 as "not provided".The condition
if (this.attackRatio)is falsy, soattackRatio = 0triggers the fallback toboatAttackAmount(). This is probably fine since sending 0% troops is meaningless, but if you want stricter handling:- if (this.attackRatio) { + if (this.attackRatio !== null && this.attackRatio > 0) { this.startTroops = this.attacker.troops() * this.attackRatio; } else {Or just document that 0 uses the default amount.
src/core/execution/AttackExecution.ts (1)
108-123: Consider adding inline documentation for the three initialization paths.The three-way branching logic (attackRatio → extraTroops → default) would benefit from brief comments explaining:
- When
attackRatiois used (normal queued attacks)- When
extraTroopsis used (boat landing attacks that preserve departure-time counts)- When the default path is used (backward compatibility or fallback)
This helps future maintainers understand the design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/client/ClientGameRunner.ts(2 hunks)src/client/Transport.ts(3 hunks)src/client/graphics/layers/PlayerActionHandler.ts(2 hunks)src/core/Schemas.ts(2 hunks)src/core/execution/AttackExecution.ts(2 hunks)src/core/execution/ExecutionManager.ts(2 hunks)src/core/execution/FakeHumanExecution.ts(2 hunks)src/core/execution/TransportShipExecution.ts(4 hunks)src/core/execution/utils/BotBehavior.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/utils/BotBehavior.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/ExecutionManager.tssrc/client/ClientGameRunner.tssrc/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/utils/BotBehavior.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/ExecutionManager.tssrc/client/ClientGameRunner.tssrc/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/utils/BotBehavior.tssrc/core/Schemas.tssrc/core/execution/FakeHumanExecution.tssrc/client/ClientGameRunner.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/Transport.tssrc/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/utils/BotBehavior.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/ExecutionManager.tssrc/client/ClientGameRunner.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/utils/BotBehavior.tssrc/client/ClientGameRunner.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/Transport.tssrc/core/execution/AttackExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/utils/BotBehavior.tssrc/client/ClientGameRunner.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/core/execution/utils/BotBehavior.tssrc/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/Schemas.tssrc/client/ClientGameRunner.tssrc/client/Transport.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/execution/FakeHumanExecution.tssrc/client/graphics/layers/PlayerActionHandler.tssrc/client/Transport.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-08-23T08:03:44.914Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.
Applied to files:
src/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.ts
🧬 Code graph analysis (5)
src/core/execution/utils/BotBehavior.ts (3)
src/core/game/GameView.ts (2)
troops(97-99)troops(379-381)src/core/game/PlayerImpl.ts (1)
troops(829-831)src/core/execution/AttackExecution.ts (1)
AttackExecution(18-388)
src/core/execution/FakeHumanExecution.ts (1)
src/core/execution/TransportShipExecution.ts (1)
TransportShipExecution(19-296)
src/client/graphics/layers/PlayerActionHandler.ts (1)
src/client/Transport.ts (1)
SendAttackIntentEvent(72-77)
src/client/Transport.ts (4)
src/core/EventBus.ts (1)
GameEvent(1-1)src/core/execution/AttackExecution.ts (1)
targetID(41-43)src/core/game/Game.ts (1)
PlayerID(22-22)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/AttackExecution.ts (2)
src/core/game/Game.ts (5)
Player(525-661)TerraNullius(512-517)Game(663-749)Attack(357-376)PlayerID(22-22)src/core/game/PlayerImpl.ts (1)
removeTroops(840-847)
🔇 Additional comments (17)
src/client/graphics/layers/PlayerActionHandler.ts (2)
33-37: LGTM!Clean change - now sends the raw
attackRatiofrom UI state instead of pre-computing troop counts. This moves the calculation to the server where it belongs, fixing the race condition.
39-53: LGTM!Same pattern as
handleAttack- passesattackRatiodirectly. Consistent with the PR's goal.src/client/ClientGameRunner.ts (2)
490-496: LGTM!Now sends
attackRatioinstead of pre-computed troops. Consistent with the fix.
604-615: LGTM!Same pattern - sends
attackRatiodirectly. Good.src/core/execution/ExecutionManager.ts (2)
57-64: LGTM!Passes
intent.attackRatiotoAttackExecution. Aligns with schema and constructor changes.
73-80: LGTM!Passes
intent.attackRatiotoTransportShipExecution. Consistent with the new signature.src/core/execution/FakeHumanExecution.ts (2)
415-418: LGTM!Changed from
this.player.troops() / 5to0.2(equivalent to 20%). Now passes a ratio toTransportShipExecutionwhich computes actual troops at execution time.
615-624: LGTM!Same pattern -
0.2ratio instead of pre-computed troops. Consistent with the PR's approach.src/core/execution/utils/BotBehavior.ts (2)
378-386: LGTM!Uses
0.5ratio instead ofthis.player.troops() / 2. Clean and consistent.
388-407: LGTM!Computes a dynamic ratio based on reserve logic. Since bots run server-side, this calculation happens at execution time - no race condition risk. The math preserves original behavior: send troops minus reserve, expressed as a ratio.
src/core/Schemas.ts (1)
232-236: LGTM!Renamed from
troopstoattackRatio. Thenullable()allowsnullfor boat-landing attacks that pass explicit troop counts instead.src/core/execution/TransportShipExecution.ts (2)
41-49: LGTM!Constructor now takes
attackRatioinstead of pre-computed troops. Clean change.
255-264: LGTM!Good approach for boat landing - passes
nullfor ratio and provides explicit troop count via the last parameter. This correctly preserves the boat's current troops rather than re-calculating from a ratio at that moment.src/core/execution/AttackExecution.ts (2)
32-39: Constructor signature updated to accept attackRatio instead of startTroops.The change from accepting
startTroopstoattackRatiois the core of this fix. TheextraTroopsparameter provides a fallback for boat attacks that need to preserve departure-time troop counts.
108-117: Verify that attackRatio is validated and clamped to [0, 1] in the schema layer.The computation
this.startTroops = this.attackRatio * currentTroopswill produce incorrect results ifattackRatiois outside the expected range (e.g.,attackRatio = 1.5sends 150% of troops; negative values could produce negative troops). Additionally, floating-point multiplication may produce non-integer results that require truncation or rounding. Confirm validation in AttackIntent and BoatAttackIntent schemas, and verify that startTroops is properly rounded if needed.src/client/Transport.ts (2)
72-86: LGTM! Event classes updated consistently.Both
SendAttackIntentEventandSendBoatAttackIntentEventnow useattackRatioinstead oftroops, which aligns with the execution-layer changes. The parameter naming is clear and consistent.
454-472: LGTM! Serialization updated to match event classes.The payload serialization correctly uses
attackRatioinstead oftroopsfor both attack and boat attack intents. This ensures the ratio is transmitted to the server where it will be multiplied by current troops at execution time.
1 participant
Resolves #1985
Description:
Fixes possible race condition caused by internet lag where troop calculation from ratio was done on the client, and therefore events like badly timed hydros could cause the player to lose all their troops. See original issue for clip.
This PR moves all of the calculation of troops for attacks and transport ships from the user to the server, specifically the executions.
Instead of the intent sending troop counts directly, they simply send the ratio of troops to attack with.
For attacks created by boats landing, the old behaviour is kept (with an optional parameter in the attack execution used only by boat landing attacks), as it doesn't make sense for them to use the current attack ratio, and indeed their point is a delayed attack.
Additionally, bots now also send attacks in this new format.
I have tested as much as I can, and I believe the behaviour my implementation gives for players and NPCs is identical. Despite this, I would ask multiple people to test the game and review the code before it is merged, as this system is critical for gameplay and a small flaw in its implementation could be quite bad.
I was not able to test the hydro situation as in the original issue, as it's highly reliant on internet speed, which I was not able to recreate.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Lavodan