From 33810e41c55e15f488d67a800c22ace7e6d75077 Mon Sep 17 00:00:00 2001 From: scamiv <6170744+scamiv@users.noreply.github.com> Date: Fri, 21 Nov 2025 23:33:08 +0100 Subject: [PATCH] Optimize edge lookup railnetwork (#2493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description: This PR optimizes how the rail network looks up railroads connecting two stations by introducing an O(1) neighbor→railroad map on `TrainStation`. It also updates `getOrientedRailroad` and railroad deletion to use this new API, avoiding repeated linear scans over all railroads attached to a station. ### What changed - **TrainStation neighbor→railroad index** - Added `railroadByNeighbor: Map` to `TrainStation` for quick edge lookup. - Kept `railroads: Set` for iteration and existing APIs. - Updated lifecycle methods to keep both data structures in sync: - `addRailroad(railRoad: Railroad)` now: - Adds to `railroads`. - Computes the neighbor station (`railRoad.from === this ? railRoad.to : railRoad.from`). - Stores the mapping in `railroadByNeighbor`. - `removeRailroad(railRoad: Railroad)` now: - Removes from `railroads`. - Removes the corresponding entry from `railroadByNeighbor`. - `clearRailroads()` now clears both `railroads` and `railroadByNeighbor`. - Added `getRailroadTo(station: TrainStation): Railroad | null` to retrieve the connecting railroad in O(1). - **Use the new API in `TrainStation` and `Railroad`** - `TrainStation.removeNeighboringRails(station)` now calls `removeRailroad(toRemove)` instead of manually deleting from the set, ensuring the map stays in sync. - `Railroad.delete(game)` now calls `from.removeRailroad(this)` and `to.removeRailroad(this)` instead of mutating the sets directly. - **Refactor `getOrientedRailroad` to use O(1) lookup** - Replaced a linear scan over `from.getRailroads()` with a direct lookup: ```ts export function getOrientedRailroad( from: TrainStation, to: TrainStation, ): OrientedRailroad | null { const railroad = from.getRailroadTo(to); if (!railroad) return null; // If tiles are stored from -> to, we go forward when railroad.to === to const forward = railroad.to === to; return new OrientedRailroad(railroad, forward); } ``` - Behavior is preserved: - `getRailroadTo` returns the same `Railroad` instance that was previously found by scanning `getRailroads()`. - Direction (`forward` vs reversed) is still derived from the `Railroad.from` / `.to` fields in the same way as before. ### Motivation - `getOrientedRailroad` and upcoming logic both need to resolve “the railroad between station A and station B” frequently. - The old pattern (`for (const railroad of from.getRailroads()) { ... }`) was: - O(degree) per lookup, - Repeated in multiple places, - Harder to maintain as more features (like fare-based costs) touch this code. - Centralizing edge lookup in a dedicated `railroadByNeighbor` map makes this: - **O(1)** per lookup, - Less error-prone (one source of truth), - Easier to reuse from new systems (e.g. train pathfinding, fare-aware logic). ### Impact / Risk - **Public behavior:** No functional change in how railroads are created, deleted, or oriented; only the lookup mechanism changed. - **Internal invariants:** Correctness relies on: - All railroad creations using `addRailroad` on both endpoints (already true via `RailNetworkImpl.connect`). - All removals (`Railroad.delete`, `TrainStation.removeNeighboringRails`, `disconnectFromNetwork`) using `removeRailroad` / `clearRailroads`, which this PR updates. - **Tests:** Existing `TrainStation` tests still pass; they exercise `addRailroad`, `removeNeighboringRails`, and `getRailroads()`, which continue to behave the same from the outside. ## 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: DISCORD_USERNAME --- src/core/game/Railroad.ts | 17 +++++++---------- src/core/game/TrainStation.ts | 17 ++++++++++++++++- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/core/game/Railroad.ts b/src/core/game/Railroad.ts index 2a271055d..8b0f3086a 100644 --- a/src/core/game/Railroad.ts +++ b/src/core/game/Railroad.ts @@ -20,8 +20,8 @@ export class Railroad { isActive: false, railTiles, }); - this.from.getRailroads().delete(this); - this.to.getRailroads().delete(this); + this.from.removeRailroad(this); + this.to.removeRailroad(this); } } @@ -29,14 +29,11 @@ export function getOrientedRailroad( from: TrainStation, to: TrainStation, ): OrientedRailroad | null { - for (const railroad of from.getRailroads()) { - if (railroad.from === to) { - return new OrientedRailroad(railroad, false); - } else if (railroad.to === to) { - return new OrientedRailroad(railroad, true); - } - } - return null; + const railroad = from.getRailroadTo(to); + if (!railroad) return null; + // If tiles are stored from -> to, we go forward when railroad.to === to + const forward = railroad.to === to; + return new OrientedRailroad(railroad, forward); } /** diff --git a/src/core/game/TrainStation.ts b/src/core/game/TrainStation.ts index 0181137c7..9318fab25 100644 --- a/src/core/game/TrainStation.ts +++ b/src/core/game/TrainStation.ts @@ -76,6 +76,8 @@ export class TrainStation { {}; private cluster: Cluster | null; private railroads: Set = new Set(); + // Quick lookup from neighboring station to connecting railroad + private railroadByNeighbor: Map = new Map(); constructor( private mg: Game, @@ -91,10 +93,19 @@ export class TrainStation { clearRailroads() { this.railroads.clear(); + this.railroadByNeighbor.clear(); } addRailroad(railRoad: Railroad) { this.railroads.add(railRoad); + const neighbor = railRoad.from === this ? railRoad.to : railRoad.from; + this.railroadByNeighbor.set(neighbor, railRoad); + } + + removeRailroad(railRoad: Railroad) { + this.railroads.delete(railRoad); + const neighbor = railRoad.from === this ? railRoad.to : railRoad.from; + this.railroadByNeighbor.delete(neighbor); } removeNeighboringRails(station: TrainStation) { @@ -111,7 +122,7 @@ export class TrainStation { isActive: false, railTiles, }); - this.railroads.delete(toRemove); + this.removeRailroad(toRemove); } } @@ -139,6 +150,10 @@ export class TrainStation { return this.railroads; } + getRailroadTo(station: TrainStation): Railroad | null { + return this.railroadByNeighbor.get(station) ?? null; + } + setCluster(cluster: Cluster | null) { this.cluster = cluster; }