Refactor attacker troop loss calculation#3227
Conversation
|
|
WalkthroughThe Player-vs-Player combat calculation for attacker troop loss is refactored from an inline compound expression into explicit intermediate variables and a weighted average formula. Defender troop loss is now separately computed and returned alongside other combat results. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (1)
src/core/configuration/DefaultConfig.ts (1)
664-665:altAttackerLossdoes not includelargeDefenderAttackDebufforlargeAttackBonus.The original
currentAttackerLossapplies bothlargeDefenderAttackDebuffandlargeAttackBonus, butaltAttackerLossskips them. Since the final value is a 50/50 blend, these balance modifiers are now only half as effective for large-player combat. If this is intentional (transitional step), a short comment explaining why would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 664 - 665, altAttackerLoss is missing the large-player balance modifiers (largeDefenderAttackDebuff and largeAttackBonus) that currentAttackerLoss applies, so update the altAttackerLoss calculation to include the same modifiers (i.e., apply largeDefenderAttackDebuff and largeAttackBonus in the multiplication chain just like currentAttackerLoss) so the final 50/50 blend preserves those effects; if omission was intentional, instead add a short clarifying comment near altAttackerLoss explaining why it intentionally omits largeDefenderAttackDebuff/largeAttackBonus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 655-671: The returned value named defenderTroopLoss is actually
troop density (defender.troops() / defender.numTilesOwned()); rename the local
variable and the returned property to defenderTroopDensity (instead of
defenderTroopLoss) in DefaultConfig.ts (the same scope that defines
attackerTroopLoss, currentAttackerLoss, altAttackerLoss) and update all
callers/uses to read defenderTroopDensity; if you need backward compatibility,
return both defenderTroopDensity and keep defenderTroopLoss as an alias
(documenting it) and update tests/docs accordingly.
---
Nitpick comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 664-665: altAttackerLoss is missing the large-player balance
modifiers (largeDefenderAttackDebuff and largeAttackBonus) that
currentAttackerLoss applies, so update the altAttackerLoss calculation to
include the same modifiers (i.e., apply largeDefenderAttackDebuff and
largeAttackBonus in the multiplication chain just like currentAttackerLoss) so
the final 50/50 blend preserves those effects; if omission was intentional,
instead add a short clarifying comment near altAttackerLoss explaining why it
intentionally omits largeDefenderAttackDebuff/largeAttackBonus.
| const defenderTroopLoss = defender.troops() / defender.numTilesOwned(); | ||
| const traitorMod = defender.isTraitor() ? this.traitorDefenseDebuff() : 1; | ||
| const currentAttackerLoss = | ||
| within(defender.troops() / attackTroops, 0.6, 2) * | ||
| mag * | ||
| 0.8 * | ||
| largeDefenderAttackDebuff * | ||
| largeAttackBonus * | ||
| traitorMod; | ||
| const altAttackerLoss = | ||
| 1.3 * defenderTroopLoss * (mag / 100) * traitorMod; | ||
| const attackerTroopLoss = | ||
| 0.5 * currentAttackerLoss + 0.5 * altAttackerLoss; | ||
|
|
||
| return { | ||
| attackerTroopLoss: | ||
| within(defender.troops() / attackTroops, 0.6, 2) * | ||
| mag * | ||
| 0.8 * | ||
| largeDefenderAttackDebuff * | ||
| largeAttackBonus * | ||
| (defender.isTraitor() ? this.traitorDefenseDebuff() : 1), | ||
| defenderTroopLoss: defender.troops() / defender.numTilesOwned(), | ||
| attackerTroopLoss, | ||
| defenderTroopLoss, |
There was a problem hiding this comment.
defenderTroopLoss is troop density, not a loss — naming is misleading.
The value defender.troops() / defender.numTilesOwned() is a troops-per-tile density metric, but the variable and return field are named defenderTroopLoss. Callers that see defenderTroopLoss in the return type will reasonably expect it to be an amount of troops the defender actually loses per tick.
If the intent is to expose defender troop density for use elsewhere, consider naming it defenderTroopDensity (or similar) to avoid confusion. If it truly represents the defender's troop loss, the formula likely needs adjustment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/DefaultConfig.ts` around lines 655 - 671, The returned
value named defenderTroopLoss is actually troop density (defender.troops() /
defender.numTilesOwned()); rename the local variable and the returned property
to defenderTroopDensity (instead of defenderTroopLoss) in DefaultConfig.ts (the
same scope that defines attackerTroopLoss, currentAttackerLoss, altAttackerLoss)
and update all callers/uses to read defenderTroopDensity; if you need backward
compatibility, return both defenderTroopDensity and keep defenderTroopLoss as an
alias (documenting it) and update tests/docs accordingly.
There was a problem hiding this comment.
Hi coderabbit. It is defined as the loss per tile. Density is fine here :)
There was a problem hiding this comment.
@1brucben, thank you for the clarification! I understand now — it represents the defender's troop loss per tile conquered. Appreciate you taking the time to explain the intended semantics.
🧠 Learnings used
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.
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.
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.
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.
Learnt from: bijx
Repo: openfrontio/OpenFrontIO PR: 2773
File: src/client/graphics/layers/PlayerInfoOverlay.ts:443-478
Timestamp: 2026-01-03T00:01:09.721Z
Learning: In OpenFrontIO, when an attack starts, the attacker's troops are removed from their pool before the `Attack` object is created. This means `player.troops()` does NOT include troops in outgoing attacks—those troops are tracked separately via `player.outgoingAttacks()`. The troop count and attacking troop count represent distinct, non-overlapping groups.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
3 participants
Description:
This PR fixes (at least partially) the attack loss imbalance. Would recommend adjusting the weighting towards 100% in the long run and then removing redundant code. I haven't tested it at 100% yet, but it might actually be fine to change it now. I don't notice any substantial differences in how the game feels at 50%.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
1brucben