From 0801cad0b52756df98f8bc9676e660868df7d5ae Mon Sep 17 00:00:00 2001 From: FloPinguin <25036848+FloPinguin@users.noreply.github.com> Date: Sat, 18 Apr 2026 02:34:12 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20NaN=20coordinates=20in=20Warship=20patrol?= =?UTF-8?q?=20logic=20=F0=9F=9A=A2=20(#3697)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description: This PR fixes the `Invalid coordinates: NaN,NaN` crash during Warship patrol execution. ### Root cause `WarshipExecution.randomTile` picks a patrol destination inside `warshipPatrolRange / 2` of the current patrol tile. When a search fails to find a valid tile, the range expands by 50% per retry (`100 → 150 → 225 → 337`) and becomes odd. Once odd, `warshipPatrolRange / 2` is a float (e.g. `112.5`), which is handed straight to `PseudoRandom.nextInt`: ```ts Math.floor(this.rng() * (max - min)) + min; ``` With a float `min`, this returns `integer + float` - a float. Despite its name, `nextInt` was silently returning a non-integer. From there: - `x = mg.x(patrolTile) + floatOffset` → float - `mg.isValidCoord(floatX, floatY)` → `true` (only bounds were checked) - `mg.ref(floatX, floatY)` → `yToRef[floatY] + floatX` → `undefined + float` → `NaN` - `hasWaterComponent(NaN, …)` → `miniMap.ref(NaN, NaN)` → **throw** ### Why this only started crashing recently The float‑leaking `nextInt` bug has been latent since at least the pathfinding refactor (#2866, January), which introduced the `hasWaterComponent` check. It was invisible because the guard directly above it short‑circuited on `NaN`: ```ts if (!this.mg.isOcean(tile) || (!allowShoreline && this.mg.isShoreline(tile))) continue; ``` For a `NaN` tile ref, `terrain[NaN]` is `undefined`, so: - `isOcean(NaN)` → `Boolean(undefined & OCEAN_BIT)` → **`false`** - `isLand(NaN)` → **`false`** - `isWater(NaN)` → `!isLand(NaN)` → **`true`** Before: `!isOcean(NaN)` was `true`, execution hit `continue`, and the poisoned ref never reached `hasWaterComponent`. The "Trading in lakes" PR (#3653) relaxed that single line to allow patrol on lakes: ```diff - if (!this.mg.isOcean(tile) || ...) continue; + if (!this.mg.isWater(tile) || ...) continue; ``` Because `isWater(NaN)` is `true`, `!isWater(NaN)` is now `false` - execution falls through to `hasWaterComponent(NaN, …)` and crashes. #3653 didn't introduce the bug; it just happened to remove the accidental NaN filter that was hiding it. ### Changes - **`PseudoRandom.nextInt`** - root‑cause fix. Floors both `min` and `max` so `nextInt` always returns an integer regardless of what callers pass. Future callers can't re‑trip this trap. - **`WarshipExecution.randomTile`** - replaced the unsafe `this.warship.patrolTile()!` non‑null assertion with a proper `undefined` guard that returns early. - **`GameMap.isValidCoord`** - defense in depth: also requires `Number.isInteger(x)` and `Number.isInteger(y)`. Non‑integer coords can still be produced outside `nextInt` (trig, arithmetic); this makes `ref()` fail loudly at the boundary instead of silently producing `NaN` refs. ### Original stacktrace Please paste the following in your bug report in Discord: Game crashed! game id: gGicMpDh client id: wXE5SpT2 Error: Invalid coordinates: NaN,NaN Message: Error: Invalid coordinates: NaN,NaN at at.ref (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:64853) at r_.hasWaterComponent (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:243326) at l_.hasWaterComponent (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:260740) at b1.randomTile (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:92634) at b1.patrol (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:91728) at b1.tick (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:89996) at https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:251463 at Array.forEach () at l_.executeNextTick (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:251383) at p_.executeNextTick (https://nightly.openfront.dev/assets/Worker.worker-DL_guV2P.js:31:271256) Discord: https://discord.com/channels/1284581928254701718/1494336024740888667 ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: FloPinguin --- src/core/PseudoRandom.ts | 4 +++- src/core/execution/WarshipExecution.ts | 9 +++++++-- src/core/game/GameMap.ts | 9 ++++++++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/core/PseudoRandom.ts b/src/core/PseudoRandom.ts index 3a88c7a16..be1410870 100644 --- a/src/core/PseudoRandom.ts +++ b/src/core/PseudoRandom.ts @@ -16,7 +16,9 @@ export class PseudoRandom { // Generates a random integer between min (inclusive) and max (exclusive). nextInt(min: number, max: number): number { - return Math.floor(this.rng() * (max - min)) + min; + const lo = Math.floor(min); + const hi = Math.floor(max); + return Math.floor(this.rng() * (hi - lo)) + lo; } // Generates a random float between min (inclusive) and max (exclusive). diff --git a/src/core/execution/WarshipExecution.ts b/src/core/execution/WarshipExecution.ts index 527056df6..121e7f2d7 100644 --- a/src/core/execution/WarshipExecution.ts +++ b/src/core/execution/WarshipExecution.ts @@ -242,12 +242,17 @@ export class WarshipExecution implements Execution { // Get warship's water component for connectivity check const warshipComponent = this.mg.getWaterComponent(this.warship.tile()); + const patrolTile = this.warship.patrolTile(); + if (patrolTile === undefined) { + return undefined; + } + while (expandCount < 3) { const x = - this.mg.x(this.warship.patrolTile()!) + + this.mg.x(patrolTile) + this.random.nextInt(-warshipPatrolRange / 2, warshipPatrolRange / 2); const y = - this.mg.y(this.warship.patrolTile()!) + + this.mg.y(patrolTile) + this.random.nextInt(-warshipPatrolRange / 2, warshipPatrolRange / 2); if (!this.mg.isValidCoord(x, y)) { continue; diff --git a/src/core/game/GameMap.ts b/src/core/game/GameMap.ts index b885a9403..592d02ca4 100644 --- a/src/core/game/GameMap.ts +++ b/src/core/game/GameMap.ts @@ -167,7 +167,14 @@ export class GameMapImpl implements GameMap { } isValidCoord(x: number, y: number): boolean { - return x >= 0 && x < this.width_ && y >= 0 && y < this.height_; + return ( + Number.isInteger(x) && + Number.isInteger(y) && + x >= 0 && + x < this.width_ && + y >= 0 && + y < this.height_ + ); } // Terrain getters (immutable)