fix: guard division-by-zero in WinCheck when all tiles have fallout#3882
fix: guard division-by-zero in WinCheck when all tiles have fallout#3882berkelmali wants to merge 1 commit intoopenfrontio:mainfrom
Conversation
WalkthroughWin-detection guards were added: both FFA and team checks early-return when zero non-fallout tiles exist; team check uses a direct arithmetic comparison instead of an intermediate percentage. Tests verify no winner is declared when all land tiles have fallout. ChangesWin-Check Threshold Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
| const numTilesWithoutFallout = | ||
| this.mg.numLandTiles() - this.mg.numTilesWithFallout(); |
There was a problem hiding this comment.
why nut just do:
if numTilesWithoutFallout === this.mg.numLandTiles() {
return
}
When numTilesWithoutFallout === 0, the tile-percentage division produces Infinity, and Infinity > 80 is true in JS — falsely declaring a winner. Use an early return guard (numTilesWithoutFallout === 0) to avoid the division entirely. Added tests for both FFA and Team modes.
berkelmali
force-pushed
the
fix/wincheck-divzero-fallout
branch
from
82dc05d to
dd3ec83
Compare
|
Thanks for the suggestion, @evanpelle! That is a much cleaner way to handle it. I've updated the PR to use the early return guard clause ( |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/WinCheckExecution.ts`:
- Around line 72-74: In WinCheckExecution.ts adjust the guard so timer-based win
conditions still run when all tiles have fallout: in the win-checking logic
(functions checkWinner and checkWinnerTeam) remove the early "if
(numTilesWithoutFallout === 0) return;" that short-circuits the whole combined
if, and instead apply the numTilesWithoutFallout === 0 check only to the
tile-percentage term inside the combined if (i.e., wrap the tile-percentage
comparison with "&& numTilesWithoutFallout > 0" or equivalent). Keep the other
terms (maxTimerValue countdown and HARD_TIME_LIMIT_SECONDS) evaluated normally
so timer expiries can still declare a winner.
- Around line 118-119: The comparison uses floating-point division in
WinCheckExecution (the expression "(max[1] / numTilesWithoutFallout) * 100")
which violates src/core/ determinism rules; replace it with an integer
cross-multiplication to avoid floats: compare max[1] * 100 against
this.mg.config().percentageTilesOwnedToWin() * numTilesWithoutFallout (using the
same identifiers max, numTilesWithoutFallout and
this.mg.config().percentageTilesOwnedToWin()) so the logic is identical but uses
only integer arithmetic.
🪄 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: 395e9281-12ba-4f07-b9a9-2990faf2c836
📒 Files selected for processing (2)
src/core/execution/WinCheckExecution.tstests/core/executions/WinCheckExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/executions/WinCheckExecution.test.ts
| if (numTilesWithoutFallout === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Timer-based win conditions are suppressed when all tiles have fallout.
The early return before the combined if block bypasses all three win conditions — tile-percentage, maxTimerValue countdown, and HARD_TIME_LIMIT_SECONDS. The timer conditions have nothing to do with numTilesWithoutFallout, so a game where all tiles have fallout when the timer expires (or the hard limit fires) will never declare a winner. The server's hard kill then ends the game without a graceful winner, which defeats the purpose of HARD_TIME_LIMIT_SECONDS.
The guard should wrap only the tile-percentage term:
- if (numTilesWithoutFallout === 0) {
- return;
- }
-
if (
- (max.numTilesOwned() / numTilesWithoutFallout) * 100 >
- this.mg.config().percentageTilesOwnedToWin() ||
+ (numTilesWithoutFallout > 0 &&
+ (max.numTilesOwned() / numTilesWithoutFallout) * 100 >
+ this.mg.config().percentageTilesOwnedToWin()) ||
(this.mg.config().gameConfig().maxTimerValue !== undefined &&
timeElapsed - this.mg.config().gameConfig().maxTimerValue! * 60 >= 0) ||
timeElapsed >= WinCheckExecution.HARD_TIME_LIMIT_SECONDSApply the same pattern in checkWinnerTeam (lines 113-115 and 117-119).
Also applies to: 113-115
🤖 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/WinCheckExecution.ts` around lines 72 - 74, In
WinCheckExecution.ts adjust the guard so timer-based win conditions still run
when all tiles have fallout: in the win-checking logic (functions checkWinner
and checkWinnerTeam) remove the early "if (numTilesWithoutFallout === 0)
return;" that short-circuits the whole combined if, and instead apply the
numTilesWithoutFallout === 0 check only to the tile-percentage term inside the
combined if (i.e., wrap the tile-percentage comparison with "&&
numTilesWithoutFallout > 0" or equivalent). Keep the other terms (maxTimerValue
countdown and HARD_TIME_LIMIT_SECONDS) evaluated normally so timer expiries can
still declare a winner.
| (max[1] / numTilesWithoutFallout) * 100 > | ||
| this.mg.config().percentageTilesOwnedToWin() || |
There was a problem hiding this comment.
Floating-point division on a changed line violates the src/core/ determinism guideline.
Line 118 still computes (max[1] / numTilesWithoutFallout) * 100, which is floating-point division. The PR description said this would be replaced with integer cross-multiplication, but it was not. The coding guideline for src/core/ requires avoiding floating-point math to keep simulation deterministic.
Replace with an integer comparison (no division, no floats):
- (max[1] / numTilesWithoutFallout) * 100 >
- this.mg.config().percentageTilesOwnedToWin() ||
+ max[1] * 100 >
+ this.mg.config().percentageTilesOwnedToWin() * numTilesWithoutFallout ||As per coding guidelines: "Ensure deterministic behavior in src/core/ by using seeded PRNG and avoiding floating-point math."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (max[1] / numTilesWithoutFallout) * 100 > | |
| this.mg.config().percentageTilesOwnedToWin() || | |
| max[1] * 100 > | |
| this.mg.config().percentageTilesOwnedToWin() * numTilesWithoutFallout || |
🤖 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/WinCheckExecution.ts` around lines 118 - 119, The
comparison uses floating-point division in WinCheckExecution (the expression
"(max[1] / numTilesWithoutFallout) * 100") which violates src/core/ determinism
rules; replace it with an integer cross-multiplication to avoid floats: compare
max[1] * 100 against this.mg.config().percentageTilesOwnedToWin() *
numTilesWithoutFallout (using the same identifiers max, numTilesWithoutFallout
and this.mg.config().percentageTilesOwnedToWin()) so the logic is identical but
uses only integer arithmetic.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
2 participants
Description:
When
numTilesWithoutFallout === 0(all land tiles have fallout), the tile-percentage division producesInfinity, andInfinity > 80istruein JS - falsely declaring a winner.This PR:
numTilesWithoutFallout > 0before the percentage checktiles * 100 > threshold * total) to avoid floating-point division entirelyPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
barfires