From 22d5aba5ae822e5ba0a7f17b9747aeb166459e2a Mon Sep 17 00:00:00 2001 From: Evan Date: Fri, 3 Jul 2026 12:42:22 -0700 Subject: [PATCH] refactor: standardize cardinal-neighbor iteration on neighbors() N,S,W,E order (#4495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Follow-up to #4494. That PR added `forEachNeighborNSWE` as a third neighbor iterator because the existing allocation-free helpers (`forEachNeighbor`, `neighbors4`) visit in W,E,N,S order while `neighbors()` visits N,S,W,E — and substituting one for the other changes simulation behavior at order-sensitive call sites. This PR removes that duplication by standardizing on **one order everywhere**: `forEachNeighbor` and `neighbors4` now visit in the same N,S,W,E order as `neighbors()`, and `forEachNeighborNSWE` is deleted. ## ⚠️ Intentional behavior change Callers of the flipped helpers that are order-sensitive now make different (equally valid) decisions: - `AttackExecution.addNeighbors` — PRNG values are drawn per neighbor while building the conquest frontier, so attack expansion patterns differ - `AttackExecution.handleDeadDefender` — a dead defender's tiles go to the *first-visited* adjacent player - `WarshipExecution.bestNeighborToward` — distance ties break by visit order - `PlayerExecution` surrounded-cluster flood fill — set insertion order propagates to conquer order Game outcomes for a given seed differ from previous builds (verified: the 12k-tick reference run ends with 31 players alive vs 24 before). Determinism across clients *within* a build is unaffected — all clients run the same code, so there is no desync risk. Replays/verification pinned to old hashes will not match this build. New reference hashes for the headless perf harness (seed `perf-default`): | Run | Final hash | |---|---| | giantworldmap, 12,000 ticks | `57830793797434300` | | giantworldmap, 2,000 ticks | `55125379638382860` | | world, 1,800 ticks | `32337437717390864` | ## Verification - [x] Full suite green (1,901 tests), including new exact-order contract tests: `forEachNeighbor` and `neighbors4` must match `neighbors()` contents **and order** for every tile - [x] 20-game-minute Giant World Map benchmark: no perf regression (73 ticks/sec, GC 1.2% of wall, allocation profile unchanged) - [x] Order-sensitivity audit of every `forEachNeighbor`/`neighbors4` call site (sensitive ones listed above; the rest are booleans, counts, or min/max accumulations) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 --- src/client/view/GameView.ts | 6 --- src/core/execution/utils/AiAttackBehavior.ts | 2 +- src/core/game/Game.ts | 13 +++--- src/core/game/GameImpl.ts | 6 --- src/core/game/GameMap.ts | 35 ++++------------ src/core/game/PlayerImpl.ts | 2 +- tests/NeighborIteration.test.ts | 42 ++++++++++---------- 7 files changed, 35 insertions(+), 71 deletions(-) diff --git a/src/client/view/GameView.ts b/src/client/view/GameView.ts index 3e4eb7244..3e02af3cb 100644 --- a/src/client/view/GameView.ts +++ b/src/client/view/GameView.ts @@ -1172,12 +1172,6 @@ export class GameView implements GameMap { forEachNeighbor(ref: TileRef, callback: (neighbor: TileRef) => void): void { this._map.forEachNeighbor(ref, callback); } - forEachNeighborNSWE( - ref: TileRef, - callback: (neighbor: TileRef) => void, - ): void { - this._map.forEachNeighborNSWE(ref, callback); - } neighbors4(ref: TileRef, out: TileRef[]): number { return this._map.neighbors4(ref, out); } diff --git a/src/core/execution/utils/AiAttackBehavior.ts b/src/core/execution/utils/AiAttackBehavior.ts index 9589046e7..3b2ffe2b9 100644 --- a/src/core/execution/utils/AiAttackBehavior.ts +++ b/src/core/execution/utils/AiAttackBehavior.ts @@ -70,7 +70,7 @@ export class AiAttackBehavior { } }; for (const t of this.player.borderTiles()) { - this.game.forEachNeighborNSWE(t, visit); + this.game.forEachNeighbor(t, visit); } const playerNeighbors = this.player.nearby(); for (const n of playerNeighbors) { diff --git a/src/core/game/Game.ts b/src/core/game/Game.ts index f2b18c3b2..e09f780bf 100644 --- a/src/core/game/Game.ts +++ b/src/core/game/Game.ts @@ -703,15 +703,12 @@ export interface Game extends GameMap { map(): GameMap; miniMap(): GameMap; forEachTile(fn: (tile: TileRef) => void): void; - // Zero-allocation neighbor iteration (cardinal only) to avoid creating arrays + // Zero-allocation neighbor iteration (cardinal only), in the same N, S, W, E + // order as neighbors(). forEachNeighbor(tile: TileRef, callback: (neighbor: TileRef) => void): void; - // Same, but in neighbors() N, S, W, E order — for order-sensitive code. - forEachNeighborNSWE( - tile: TileRef, - callback: (neighbor: TileRef) => void, - ): void; - // Writes the cardinal neighbors of ref into out (W, E, N, S order) and - // returns the count. Reuse out across calls to avoid allocation. + // Writes the cardinal neighbors of ref into out (same N, S, W, E order as + // neighbors()) and returns the count. Reuse out across calls to avoid + // allocation. neighbors4(ref: TileRef, out: TileRef[]): number; // Zero-allocation neighbor iteration for performance-critical cluster calculation // Alternative to neighborsWithDiag() that returns arrays diff --git a/src/core/game/GameImpl.ts b/src/core/game/GameImpl.ts index 98e70de5c..db3a9e57c 100644 --- a/src/core/game/GameImpl.ts +++ b/src/core/game/GameImpl.ts @@ -1134,12 +1134,6 @@ export class GameImpl implements Game { forEachNeighbor(tile: TileRef, callback: (neighbor: TileRef) => void): void { this._map.forEachNeighbor(tile, callback); } - forEachNeighborNSWE( - tile: TileRef, - callback: (neighbor: TileRef) => void, - ): void { - this._map.forEachNeighborNSWE(tile, callback); - } neighbors4(ref: TileRef, out: TileRef[]): number { return this._map.neighbors4(ref, out); } diff --git a/src/core/game/GameMap.ts b/src/core/game/GameMap.ts index 3bc0013ac..92689275b 100644 --- a/src/core/game/GameMap.ts +++ b/src/core/game/GameMap.ts @@ -37,19 +37,13 @@ export interface GameMap { isOnEdgeOfMap(ref: TileRef): boolean; isBorder(ref: TileRef): boolean; neighbors(ref: TileRef): TileRef[]; - // Zero-allocation neighbor iteration (cardinal only), in W, E, N, S order. + // Zero-allocation neighbor iteration (cardinal only), in the same N, S, W, E + // order as neighbors(). All cardinal-neighbor helpers share this order so + // they are interchangeable even in order-sensitive simulation code. forEachNeighbor(ref: TileRef, callback: (neighbor: TileRef) => void): void; - // Zero-allocation neighbor iteration (cardinal only) in the same N, S, W, E - // order as neighbors(). Use this in order-sensitive code — anything feeding - // sets/arrays whose iteration order affects the simulation — where - // forEachNeighbor's W, E, N, S order would change behavior. - forEachNeighborNSWE( - ref: TileRef, - callback: (neighbor: TileRef) => void, - ): void; - // Writes the cardinal neighbors of ref into out (W, E, N, S order) and - // returns the count. out must have length >= 4; reuse it across calls to - // avoid allocation in hot loops. + // Writes the cardinal neighbors of ref into out (same N, S, W, E order as + // neighbors()) and returns the count. out must have length >= 4; reuse it + // across calls to avoid allocation in hot loops. neighbors4(ref: TileRef, out: TileRef[]): number; // Zero-allocation neighbor iteration including diagonals, in dx-major // order: (-1,-1),(-1,0),(-1,1),(0,-1),(0,1),(1,-1),(1,0),(1,1). @@ -397,19 +391,6 @@ export class GameMapImpl implements GameMap { const w = this.width_; const x = this.refToX[ref]; - if (x !== 0) callback(ref - 1); - if (x !== w - 1) callback(ref + 1); - if (ref >= w) callback(ref - w); - if (ref < (this.height_ - 1) * w) callback(ref + w); - } - - forEachNeighborNSWE( - ref: TileRef, - callback: (neighbor: TileRef) => void, - ): void { - const w = this.width_; - const x = this.refToX[ref]; - if (ref >= w) callback(ref - w); if (ref < (this.height_ - 1) * w) callback(ref + w); if (x !== 0) callback(ref - 1); @@ -421,10 +402,10 @@ export class GameMapImpl implements GameMap { const x = this.refToX[ref]; let n = 0; - if (x !== 0) out[n++] = ref - 1; - if (x !== w - 1) out[n++] = ref + 1; if (ref >= w) out[n++] = ref - w; if (ref < (this.height_ - 1) * w) out[n++] = ref + w; + if (x !== 0) out[n++] = ref - 1; + if (x !== w - 1) out[n++] = ref + 1; return n; } diff --git a/src/core/game/PlayerImpl.ts b/src/core/game/PlayerImpl.ts index 41aba0a35..d2fe820aa 100644 --- a/src/core/game/PlayerImpl.ts +++ b/src/core/game/PlayerImpl.ts @@ -493,7 +493,7 @@ export class PlayerImpl implements Player { } }; for (const border of this.borderTiles()) { - map.forEachNeighborNSWE(border, visit); + map.forEachNeighbor(border, visit); } for (const n of this.shoreReachableNeighbors()) { ns.add(n); diff --git a/tests/NeighborIteration.test.ts b/tests/NeighborIteration.test.ts index 3c95f5160..e34ef81cc 100644 --- a/tests/NeighborIteration.test.ts +++ b/tests/NeighborIteration.test.ts @@ -25,59 +25,57 @@ describe("Neighbor iteration", () => { game = await setup("ocean_and_land"); // 16x16 }); - test("forEachNeighbor visits W, E, N, S in that exact order for interior tiles", () => { + test("forEachNeighbor visits N, S, W, E in that exact order for interior tiles", () => { const tile = game.ref(5, 7); expect(collectNeighbors(tile)).toEqual([ - game.ref(4, 7), - game.ref(6, 7), game.ref(5, 6), game.ref(5, 8), + game.ref(4, 7), + game.ref(6, 7), ]); }); test("forEachNeighbor clips at corners and edges", () => { const w = game.width(); const h = game.height(); - // top-left corner: E, S only + // top-left corner: S, E only expect(collectNeighbors(game.ref(0, 0))).toEqual([ - game.ref(1, 0), game.ref(0, 1), + game.ref(1, 0), ]); - // bottom-right corner: W, N only + // bottom-right corner: N, W only expect(collectNeighbors(game.ref(w - 1, h - 1))).toEqual([ - game.ref(w - 2, h - 1), game.ref(w - 1, h - 2), + game.ref(w - 2, h - 1), ]); - // left edge: E, N, S + // left edge: N, S, E expect(collectNeighbors(game.ref(0, 5))).toEqual([ - game.ref(1, 5), game.ref(0, 4), game.ref(0, 6), + game.ref(1, 5), ]); - // bottom edge: W, E, N + // bottom edge: N, W, E expect(collectNeighbors(game.ref(5, h - 1))).toEqual([ + game.ref(5, h - 2), game.ref(4, h - 1), game.ref(6, h - 1), - game.ref(5, h - 2), ]); }); - test("forEachNeighbor matches map.neighbors() as a set for every tile", () => { + // All cardinal-neighbor helpers share neighbors()'s exact N, S, W, E order, + // including at edges and corners, so they are interchangeable even in + // order-sensitive simulation code. + test("forEachNeighbor matches map.neighbors() exactly (contents and order) for every tile", () => { game.forEachTile((tile) => { - const a = [...collectNeighbors(tile)].sort((x, y) => x - y); - const b = [...game.map().neighbors(tile)].sort((x, y) => x - y); - expect(a).toEqual(b); + expect(collectNeighbors(tile)).toEqual(game.map().neighbors(tile)); }); }); - // forEachNeighborNSWE's contract is exact order equality with neighbors(), - // including at edges and corners, so order-sensitive code can use the two - // interchangeably. - test("forEachNeighborNSWE matches map.neighbors() exactly (contents and order) for every tile", () => { + test("neighbors4 matches map.neighbors() exactly (contents and order) for every tile", () => { + const nbuf: TileRef[] = [0, 0, 0, 0]; game.forEachTile((tile) => { - const out: TileRef[] = []; - game.forEachNeighborNSWE(tile, (n) => out.push(n)); - expect(out).toEqual(game.map().neighbors(tile)); + const n = game.map().neighbors4(tile, nbuf); + expect(nbuf.slice(0, n)).toEqual(game.map().neighbors(tile)); }); });