mirror of
https://github.com/openfrontio/OpenFrontIO.git
synced 2026-06-24 23:44:36 +00:00
Fix NaN coordinates in Warship patrol logic 🚢 (#3697)
## 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 (<anonymous>) 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
This commit is contained in:
@@ -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).
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user