Implement local train routing with discovery and greedy path selection #2468
Conversation
WalkthroughTrainExecution moved to per-hop, station-centric routing with journey tracking and loop prevention; TrainStation gained BATMAN-like routing, edge metrics, traffic and journey-processing; stations now tick each turn; RailNetworkImpl calls per-station cleanup on removal; Game API and GameImpl record and print train arrival/hop-limit stats. Changes
Sequence Diagram(s)sequenceDiagram
participant Train as TrainExecution
participant Station as TrainStation
participant Rail as RailroadSegment
participant Game as GameImpl
Note over Station: Each tick Station.tick() may broadcast/update routes
Train->>Station: shareJourneyInfo() / chooseNextStation(destination, recentStations, owner)
Station-->>Train: nextHop (or null)
alt no nextHop
Train->>Train: enter idle / abort
else nextHop found
Train->>Station: onTrainDepartureFromStation(this)
Train->>Rail: traverse tiles toward nextHop
Rail-->>Train: arrive at nextHop tile
Train->>Station: onTrainArrival(this)
Station->>Station: processJourneyInformation(from Train)
Station->>Station: maybe broadcastOriginatorMessage()
opt arrived at destination
Train->>Game: recordTrainArrival(steps)
end
opt hop limit exceeded
Train->>Game: recordTrainRemovedDueToHopLimit(steps)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TrainStation.ts (1)
255-255: Pipeline failure: Test mock missing unit.tile().The test environment mock doesn't implement
unit.tile(). This is a test infrastructure issue, not a code defect. Update test mocks to provideunit.tile()returning a valid TileRef.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/execution/TrainExecution.ts(3 hunks)src/core/execution/TrainStationExecution.ts(1 hunks)src/core/game/TrainStation.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/execution/TrainExecution.tssrc/core/game/TrainStation.ts
🧬 Code graph analysis (2)
src/core/execution/TrainExecution.ts (3)
src/core/game/TrainStation.ts (1)
TrainStation(135-774)src/core/game/RailNetwork.ts (1)
RailNetwork(4-8)src/core/game/Railroad.ts (1)
getOrientedRailroad(28-40)
src/core/game/TrainStation.ts (5)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/UnitImpl.ts (2)
tile(175-177)type(145-147)src/core/game/Game.ts (3)
Game(663-747)Unit(432-510)Player(525-661)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/TrainExecution.ts (1)
TrainExecution(14-310)
🪛 GitHub Actions: 🧪 CI
src/core/game/TrainStation.ts
[error] 718-718: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 718-718: Function shareJourneyInfo is missing or not defined on trainExecution object during tests.
[error] 255-255: TypeError: this.unit.tile is not a function
[error] 255-255: this.unit.tile is expected to be a function but is not a function during test executions.
[error] 479-479: TypeError: this.mg.x is not a function
[error] 479-479: this.mg.x is expected to be a function but is not a function during test execution.
🔇 Additional comments (19)
src/core/execution/TrainStationExecution.ts (1)
48-50: LGTM! Clean integration of station tick.The addition of
this.station.tick()enables periodic routing maintenance (broadcasts and traffic updates) before train spawning. The placement is correct and the change is minimal.src/core/execution/TrainExecution.ts (6)
24-34: Journey tracking state added for organic routing.The new state fields support per-hop routing decisions and loop prevention. The
maxHops: 50andrecentMemorySize: 5are reasonable starting points but may need tuning based on network size and testing.
42-46: Constructor initialization looks good.Journey source and previous station correctly initialized for new routing model.
52-65: New public API for organic route discovery.The
shareJourneyInfo()method exposes minimal journey state to stations for learning routes. Clean interface design.
70-82: Improved validation and initialization.The explicit validation of source/destination activity and handling of the edge case where source equals destination strengthens the init flow. Setting
currentStation = this.sourcecorrectly establishes the starting point.
211-220: Refactored station detection works correctly.The new
isAtStation()correctly checks if the train is at thecurrentStationtile using coordinate comparison.
296-301: Station reached handler updated correctly.The reference to
currentStationaligns with the new per-hop routing model.src/core/game/TrainStation.ts (12)
28-68: Well-defined routing data structures.The interfaces are clean and appropriately typed for the routing system. Using
RoutingEntrywith tile IDs (numbers) instead of station references is a good memory optimization.
141-172: BATMAN broadcasts disabled by default as documented.The warning comment (Lines 163-170) clearly explains the implications. The 10x increased broadcast interval (1000 ticks) reduces overhead when broadcasts are eventually enabled.
174-201: Constructor initialization is sound.Station registration, traffic initialization, and self-route setup establish the routing foundation correctly.
274-292: BATMAN route lookup with staleness checking.The
getNextHop()method correctly validates hop count and staleness before returning routes. Returning null for unknown routes allows organic discovery.
294-394: BATMAN broadcast protocol implementation.The broadcast/receive cycle correctly implements incremental updates (changed routes only) and sequence number validation. The hop count and staleness checks prevent routing loops and stale information.
396-434: Lazy cleanup is a smart scalability optimization.Checking only 3 routes per tick (round-robin) bounds the per-tick work and prevents performance spikes on large networks.
436-456: Tick method correctly gates broadcasts.The conditional broadcast (only when
enableBroadcastsis true and routes changed) respects the configuration. Traffic updates run regardless, which is correct.
463-482: Edge metric initialization and distance calculation are solid.The Euclidean distance calculation and duration estimation provide reasonable metrics for the greedy scoring function.
484-500: Profit calculation uses actual game config.Using
this.mg.config().trainGold(relationship)correctly applies the game's relationship bonuses. The BigInt-to-number conversion (Line 499) is necessary for scoring math and should be safe for game gold values.Based on learnings.
615-660: Lifecycle cleanup methods handle references correctly.Both
onStationRemoved()andonOtherStationRemoved()properly clean up routing tables and edge metrics. The StationLookup unregistration at Line 622 is critical for preventing memory leaks.
662-687: Traffic tracking with decay is well-designed.The heat decay (5% every 50 ticks) and periodic reset of recent arrivals provide reasonable congestion metrics that naturally cool down over time.
692-731: Journey information processing integrates organic learning.The
onTrainStop()flow correctly combines traffic tracking, organic route discovery, and normal station rewards. The arrival/departure hooks are properly placed.
| * Calculate physical distance to another station | ||
| */ | ||
| private calculateDistance(other: TrainStation): number { | ||
| const dx = Math.abs(this.mg.x(this.tile()) - this.mg.x(other.tile())); |
There was a problem hiding this comment.
Pipeline failure: Test mock missing mg.x().
The test environment mock doesn't implement mg.x() (and likely mg.y()). Update test mocks to provide the GameMap coordinate methods.
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 479-479: TypeError: this.mg.x is not a function
[error] 479-479: this.mg.x is expected to be a function but is not a function during test execution.
| * Process journey information from a train to update routing tables organically | ||
| */ | ||
| private processJourneyInformation(trainExecution: TrainExecution): void { | ||
| const journeyInfo = trainExecution.shareJourneyInfo(); |
There was a problem hiding this comment.
Pipeline failure: Test mock missing trainExecution.shareJourneyInfo().
The test environment mock doesn't implement the new shareJourneyInfo() method. Update test mocks to provide this method returning journey state.
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 718-718: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 718-718: Function shareJourneyInfo is missing or not defined on trainExecution object during tests.
🤖 Prompt for AI Agents
In src/core/game/TrainStation.ts around line 718, tests are failing because the
test mock for trainExecution doesn't implement the new shareJourneyInfo()
method; update the test mocks used in unit/integration tests to add a
shareJourneyInfo() function that returns the expected journey state object
(matching the shape used by TrainStation), or adjust existing mock factory to
include this method and default return values so tests can run; ensure the mock
returns all fields TrainStation reads from the journey info to avoid undefined
property errors.
|
first commit message got lost during rebase, most of that is still the case, but currently im not using broadcasting / "batman" ... need to remove that reference |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/game/TrainStation.ts (3)
147-147: Consider using tile IDs in changedRoutes for consistency.The
routingTableuses tile IDs (Map<number, RoutingEntry>), butchangedRoutesstoresTrainStationobjects. This mixed approach works but complicates reasoning. For memory efficiency and consistency with the "IDs for memory efficiency" design (line 141 comment), consider usingSet<number>instead.Also applies to: 200-200
400-434: Optimize cleanupStaleRoutes to avoid repeated allocations.Line 404 creates a new array from the routing table on every tick. For large routing tables, this allocation overhead accumulates. Consider using the Map's iterator directly or caching the entries array, updating it only when routes change.
Example using iterator:
- private cleanupStaleRoutes(): void { - const currentTime = this.mg.ticks(); - - // Convert map to array for indexed access - const routeEntries = Array.from(this.routingTable.entries()); - - if (routeEntries.length === 0) { + private cleanupStaleRoutes(): void { + const currentTime = this.mg.ticks(); + + if (this.routingTable.size === 0) { this.cleanupIndex = 0; return; } - // Check only a few routes per tick (round-robin) - const routesChecked = Math.min( - this.routesToCheckPerTick, - routeEntries.length, - ); + const routesChecked = Math.min( + this.routesToCheckPerTick, + this.routingTable.size, + ); + + let checked = 0; + let skipped = 0; + for (const [destId, route] of this.routingTable) { + if (skipped < this.cleanupIndex) { + skipped++; + continue; + } + if (checked >= routesChecked) break; - for (let i = 0; i < routesChecked; i++) { - const index = (this.cleanupIndex + i) % routeEntries.length; - const [destId, route] = routeEntries[index]; - if (currentTime - route.lastUpdate > this.routeStaleThreshold) { this.routingTable.delete(destId); - // Mark destination station as changed for potential rebroadcast const destStation = StationLookup.getStation(destId); if (destStation) { this.changedRoutes.add(destStation); } } + checked++; } - // Update index for next cleanup cycle - this.cleanupIndex = - (this.cleanupIndex + routesChecked) % routeEntries.length; + this.cleanupIndex = (this.cleanupIndex + routesChecked) % this.routingTable.size; }
630-653: Simplify cleanup: skip adding removed station to changedRoutes.Line 641 adds
thistochangedRoutes, but line 649 immediately removes it. Since the station is being destroyed, there's no need to mark it for broadcast. Remove line 641 (and the similar pattern on line 666).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/TrainStation.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/game/TrainStation.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/TrainStation.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/TrainStation.ts
🧬 Code graph analysis (1)
src/core/game/TrainStation.ts (5)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/UnitImpl.ts (2)
tile(175-177)type(145-147)src/core/game/Game.ts (3)
Game(663-747)Unit(432-510)Player(525-661)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/TrainExecution.ts (1)
TrainExecution(14-310)
🪛 GitHub Actions: 🧪 CI
src/core/game/TrainStation.ts
[error] 730-730: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 730-730: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 255-255: TypeError: this.unit.tile is not a function
[error] 255-255: TypeError: this.unit.tile is not a function
[error] 479-479: TypeError: this.mg.x is not a function
[error] 479-479: TypeError: this.mg.x is not a function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/TrainStation.ts (1)
163-172: Move broadcast implications to configuration documentation.The detailed comment about
enableBroadcastsimplications (lines 163-171) reads like configuration documentation rather than inline code comments. Consider moving this to a README, config documentation, or inline JSDoc on the field itself, leaving just a brief note here.Apply this refactor to shorten the inline comment:
- // Disabling broadcasts turns routing into local-only mode! - // Implications: - // - Stations only know routes their own trains discovered - // - No network-wide knowledge sharing (BATMAN protocol disabled) - // - Trains get stuck in loops more easily - // - Route discovery becomes slower and less efficient - // - System becomes more like individual A* pathfinding - // - Lower memory usage but higher train congestion - private enableBroadcasts: boolean = false; // Enable/disable BATMAN broadcast protocol + // When false, routing is local-only (trains learn routes organically, no broadcasts). See docs for full implications. + private enableBroadcasts: boolean = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/execution/TrainExecution.ts(3 hunks)src/core/game/RailNetworkImpl.ts(1 hunks)src/core/game/TrainStation.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/game/TrainStation.tssrc/core/execution/TrainExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/TrainStation.tssrc/core/execution/TrainExecution.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/TrainStation.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/TrainExecution.ts
🧬 Code graph analysis (2)
src/core/game/TrainStation.ts (5)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/UnitImpl.ts (2)
tile(175-177)type(145-147)src/core/game/Game.ts (3)
Game(663-747)Unit(432-510)Player(525-661)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/TrainExecution.ts (1)
TrainExecution(14-310)
src/core/execution/TrainExecution.ts (3)
src/core/game/TrainStation.ts (1)
TrainStation(135-793)src/core/game/RailNetwork.ts (1)
RailNetwork(4-8)src/core/game/Railroad.ts (1)
getOrientedRailroad(28-40)
🪛 GitHub Actions: 🧪 CI
src/core/game/RailNetworkImpl.ts
[error] 110-110: TypeError: station.onStationRemoved is not a function
[error] 110-110: TypeError: station.onStationRemoved is not a function
src/core/game/TrainStation.ts
[error] 737-737: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 737-737: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 255-255: TypeError: this.unit.tile is not a function
[error] 255-255: TypeError: this.unit.tile is not a function
[error] 479-479: TypeError: this.mg.x is not a function
[error] 479-479: TypeError: this.mg.x is not a function
🔇 Additional comments (11)
src/core/game/RailNetworkImpl.ts (1)
110-110: Correct fix for StationLookup memory leak.This call ensures
StationLookup.unregister()is invoked during station removal, preventing the static map from accumulating stale entries. The pipeline failure is expected—test mocks need updating to implementonStationRemoved(), which is already flagged in the TrainStation.ts review comments.src/core/execution/TrainExecution.ts (6)
24-34: Journey tracking fields are well-structured.The fields are clearly documented, and the defaults (maxHops=50, recentMemorySize=5) are reasonable for preventing infinite loops while allowing meaningful exploration. The separation between journey tracking (source/previous/hop count) and loop prevention (recent stations) is clean.
42-46: Constructor initialization is correct.Journey tracking is properly initialized without changing the constructor signature, maintaining API compatibility. Starting with
journeyPreviousStation = nullis appropriate for the initial station.
52-65: Public API for journey sharing is well-designed.The method returns a simple, immutable snapshot of journey state. This is a clean interface for stations to consume journey information without exposing internal train state.
70-82: Initialization validation is solid.The checks for active stations and the source-equals-destination edge case prevent unnecessary train spawning. Setting
currentStationto the source prepares the train for per-hop routing.
222-294: Per-hop routing logic is correctly implemented.The refactored flow properly handles:
- Destination detection when the train is actually at the station (lines 226-228)
- Max hop limits to prevent infinite wandering (lines 231-236)
- Journey tracking updates on each hop transition (lines 263-270)
- Railroad-following between stations (lines 277-291)
The previous concern about early destination detection has been resolved—trains now detect arrival while at the station, not one hop early.
296-301: Station arrival handling updated correctly.The method now uses
currentStationinstead of the removed stations array, which aligns with the per-hop routing model. The logic is straightforward and correct.src/core/game/TrainStation.ts (4)
174-201: Constructor initialization is correct.The registration side effect in
StationLookup.register(this)is acceptable for a lookup registry pattern. The traffic and routing table initialization properly sets up the station for both local greedy routing and BATMAN routing. The self-route entry (lines 191-200) correctly usesstationTileas both destination and next hop.
400-456: Lazy cleanup and tick logic are well-implemented.The round-robin cleanup strategy (checking only 3 routes per tick) is a good scalability optimization. The
tick()method correctly handles both event-driven (route changes) and periodic (timeout) broadcasting triggers. SinceenableBroadcasts = false, the BATMAN protocol is dormant, and only traffic updates run per tick.
488-519: Profit calculation and traffic tracking are correct.The
calculateActualProfit()method properly uses the game's config and relationship system, aligning with the design that relationship bonuses affect route profitability. Traffic updates (arrival/departure) are straightforward and correctly maintain the heat metric.Based on learnings
637-706: Lifecycle cleanup and traffic management are well-implemented.The
onStationRemoved()method thoroughly cleans up:
- StationLookup registration
- Routing table entries referencing this station
- Edge metrics
- Changed routes tracking
The
updateTraffic()decay rates (5% every 50 ticks) and recent arrival resets (every 200 ticks) provide reasonable congestion tracking.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/core/game/TrainStation.ts (1)
480-484: Test mocks need GameMap coordinate methods.The code correctly uses
this.mg.x()andthis.mg.y()sinceGameextendsGameMap. However, the test mock forgame(line 15-26 of TrainStation.test.ts) is missing these methods. WhencalculateDistance()executes at line 466, it will fail withTypeError: this.mg.x is not a function.Update the
gamemock in thebeforeEach()setup to includex()andy()methods that accept a tile reference and return coordinates:game = { ticks: jest.fn().mockReturnValue(123), config: jest.fn().mockReturnValue({...}), addUpdate: jest.fn(), addExecution: jest.fn(), x: jest.fn().mockReturnValue(0), y: jest.fn().mockReturnValue(0), } as any;
🧹 Nitpick comments (4)
src/core/game/TrainStation.ts (4)
465-475: EdgeMetrics never refresh after initialization.
lastUpdatedis set during initialization but never checked afterward. Metrics (distance, baseDuration) remain static even as traffic patterns and network conditions change. If periodic recalculation isn't needed, remove the unusedlastUpdatedfield; otherwise, implement metric refresh logic intick()or when edges are used.
519-521: Redundant method naming: merge onTrainDeparture and onTrainDepartureFromStation.Two methods with confusing names:
onTrainDeparture(private) andonTrainDepartureFromStation(public wrapper). The wrapper adds no logic—just calls the private method. Simplify by makingonTrainDeparturepublic and removing the wrapper, or rename for clarity (e.g.,updateTrafficOnDeparturefor the private helper).Also applies to: 732-734
571-635: Clarify the compounded exploration randomness.The method has two random exploration points:
- Line 581: 10% chance to ignore known routes and explore
- Line 617: 10% chance to pick random neighbor instead of best
If a known route exists, there's a 90% × 10% = 9% additional chance of random selection beyond the initial exploration decision. This compounds the effective randomness. If intentional, consider adding a comment explaining the two-stage exploration strategy; otherwise, consolidate to a single randomness check.
694-709: Extract magic numbers to named constants.Lines 699, 701, and 705 use hardcoded values (50 ticks, 0.95 decay rate, 200 ticks) for heat decay and traffic reset. Extract these as private readonly properties (e.g.,
heatDecayInterval,heatDecayRate,trafficResetInterval) to make tuning easier and document their purpose.+ private readonly heatDecayInterval = 50; // ticks between heat decay + private readonly heatDecayRate = 0.95; // 5% decay per interval + private readonly trafficResetInterval = 200; // ticks before resetting recent arrivals + private updateTraffic(): void { const currentTime = this.mg.ticks(); const timeSinceUpdate = currentTime - this.traffic.lastHeatUpdate; // Decay heat over time - if (timeSinceUpdate > 50) { + if (timeSinceUpdate > this.heatDecayInterval) { - this.traffic.heat *= 0.95; // Decay heat by 5% + this.traffic.heat *= this.heatDecayRate; this.traffic.lastHeatUpdate = currentTime; // Reset recent arrivals periodically - if (timeSinceUpdate > 200) { + if (timeSinceUpdate > this.trafficResetInterval) { this.traffic.recentArrivals = 0; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/execution/TrainExecution.ts(3 hunks)src/core/game/TrainStation.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/execution/TrainExecution.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/game/TrainStation.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/TrainStation.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/TrainStation.ts
🧬 Code graph analysis (1)
src/core/game/TrainStation.ts (5)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/UnitImpl.ts (2)
tile(175-177)type(145-147)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/game/Railroad.ts (1)
Railroad(6-26)src/core/execution/TrainExecution.ts (1)
TrainExecution(14-323)
🪛 GitHub Actions: 🧪 CI
src/core/game/TrainStation.ts
[error] 740-740: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 740-740: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 257-257: TypeError: this.unit.tile is not a function
[error] 257-257: TypeError: this.unit.tile is not a function
[error] 481-481: TypeError: this.mg.x is not a function
[error] 481-481: TypeError: this.mg.x is not a function
🔇 Additional comments (1)
src/core/game/TrainStation.ts (1)
256-258: The test mock already includes thetile()method — this review comment is incorrect.The
unitmock inTrainStation.test.tsline 43 already hastile: jest.fn().mockReturnValue({ x: 0, y: 0 })defined. The tests at lines 54 and 62 callunit.tile()without issue because the mock is present and working.Likely an incorrect or invalid review comment.
src/core/game/TrainStation.ts
Outdated
| // Batman routing properties - now using IDs for memory efficiency | ||
| private routingTable: Map<number, RoutingEntry> = new Map(); | ||
| private sequenceNumber: number = 0; | ||
| private originatorInterval: number = 1000; // ticks between broadcasts (increased 10x) | ||
| private lastOriginatorBroadcast: number = 0; | ||
| private routesChanged: boolean = false; | ||
| private changedRoutes: Set<TrainStation> = new Set(); | ||
| private maxHops: number = 20; | ||
| private routeStaleThreshold: number = 500; // ticks | ||
|
|
||
| // Lazy cleanup optimization | ||
| private cleanupIndex: number = 0; | ||
| private readonly routesToCheckPerTick = 3; // Check only 3 routes per tick | ||
|
|
||
| // Local greedy routing properties | ||
| private edgeMetrics: Map<TrainStation, EdgeMetrics> = new Map(); | ||
| private traffic: StationTraffic; | ||
| private profitSensitivity: number = 0.3; // How much traffic reduces profit scores | ||
| private distanceSensitivity: number = 0.2; // How much traffic increases duration penalties | ||
| private stationHeatSensitivity: number = 0.2; // How much station heat reduces scores | ||
| private recencyDecayFactor: number = 0.7; // Exponential decay rate for recency penalties | ||
| private maxRecencyPenalty: number = 0.9; // Maximum penalty for immediate revisits (0.5 = 50%) | ||
| // Disabling broadcasts turns routing into local-only mode! | ||
| // Implications: | ||
| // - Stations only know routes their own trains discovered | ||
| // - No network-wide knowledge sharing (BATMAN protocol disabled) | ||
| // - Trains get stuck in loops more easily | ||
| // - Route discovery becomes slower and less efficient | ||
| // - System becomes more like individual A* pathfinding | ||
| // - Lower memory usage but higher train congestion | ||
| private enableBroadcasts: boolean = false; // Enable/disable BATMAN broadcast protocol | ||
| private randomChoiceProbability: number = 0.1; // Probability of making random choice instead of best (0.1 = 10%) |
There was a problem hiding this comment.
Inconsistency with PR objectives—BATMAN code removal pending.
The PR comment states "references to BATMAN should be removed" since broadcasts are disabled, but the codebase retains the full BATMAN implementation (routing tables, broadcast methods, sequence numbers, cleanup logic). Since this is a draft PR, consider either:
- Removing the unused BATMAN infrastructure entirely to reduce complexity and memory footprint, or
- Updating the PR description to clarify the BATMAN code is being kept for future experimentation
With broadcasts disabled by default (line 172), all the BATMAN-related fields and methods add maintenance burden without providing value.
[scratchpad_end] -->
| sequenceNumber: this.sequenceNumber, | ||
| lastUpdate: mg.ticks(), | ||
| }); | ||
| this.changedRoutes.add(this); |
There was a problem hiding this comment.
Memory concern: changedRoutes accumulates when broadcasts disabled.
Line 202 adds this to changedRoutes during construction. With enableBroadcasts = false (line 172), broadcastOriginatorMessage() never runs, so this.changedRoutes.clear() (line 314) never executes. As routes are learned organically (lines 355, 386, 651, 676, 793), changedRoutes grows unbounded.
Consider clearing changedRoutes periodically even when broadcasts are disabled, or only populate it when broadcasts are enabled.
constructor(
private mg: Game,
public unit: Unit,
) {
this.stopHandlers = createTrainStopHandlers(new PseudoRandom(mg.ticks()));
this.random = new PseudoRandom(mg.ticks() + this.tile());
// Register station for lookup
StationLookup.register(this);
// Initialize traffic tracking
this.traffic = {
trainCount: 0,
recentArrivals: 0,
heat: 0,
lastHeatUpdate: mg.ticks(),
};
// Initialize self-route using tile as ID
const stationTile = this.tile();
this.routingTable.set(stationTile, {
destinationId: stationTile,
nextHopId: stationTile,
hopCount: 0,
sequenceNumber: this.sequenceNumber,
lastUpdate: mg.ticks(),
});
- this.changedRoutes.add(this);
+ // Only track changed routes if broadcasts enabled
+ if (this.enableBroadcasts) {
+ this.changedRoutes.add(this);
+ }
}📝 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.
| this.changedRoutes.add(this); | |
| // Only track changed routes if broadcasts enabled | |
| if (this.enableBroadcasts) { | |
| this.changedRoutes.add(this); | |
| } |
🤖 Prompt for AI Agents
In src/core/game/TrainStation.ts around line 202, the constructor
unconditionally does `this.changedRoutes.add(this)` which lets changedRoutes
grow when enableBroadcasts is false; modify the constructor to only add to
changedRoutes when broadcasts are enabled (guard the add with the station's
enableBroadcasts flag) or alternatively ensure changedRoutes is cleared
periodically when broadcasts are disabled (e.g., clear immediately in
constructor or after learning routines); implement the guard solution for
minimal change: wrap the add in an if-check against the broadcasts-enabled
boolean so changedRoutes is only populated when broadcasts are active.
| private processJourneyInformation(trainExecution: TrainExecution): void { | ||
| const journeyInfo = trainExecution.shareJourneyInfo(); | ||
|
|
||
| // Only process journey information if the train has established a journey source (visited a city/port) | ||
| if (!journeyInfo.source) { | ||
| // Train hasn't visited a city/port yet, skip journey processing | ||
| return; | ||
| } | ||
|
|
||
| // This station has been visited by a train coming from journeyInfo.source | ||
| // Learn reverse routes: if a train got here from source S via previous station P, | ||
| // then to get back to S from here, go to P first | ||
|
|
||
| // Store reverse route: to get to source, go to previous station (if we have one) | ||
| if (journeyInfo.previousStation && journeyInfo.previousStation !== this) { | ||
| this.updateReverseRouteFromJourney( | ||
| journeyInfo.source, | ||
| journeyInfo.previousStation, | ||
| journeyInfo.hopCount, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Pipeline failure: Test mocks missing trainExecution.shareJourneyInfo().
Line 740 calls trainExecution.shareJourneyInfo() which doesn't exist in test mocks. Based on the relevant code snippets, TrainExecution.shareJourneyInfo() returns { source: TrainStation | null; previousStation: TrainStation | null; hopCount: number }. Update test fixtures to provide this method with appropriate mock return values.
🏁 Script executed:
#!/bin/bash
# Find TrainExecution test mocks/factories to add shareJourneyInfo() method
rg -nP -C5 'TrainExecution.*mock|mock.*TrainExecution|createTrainExecution' --type=ts -g '*test*' -g '*spec*'Length of output: 901
| private updateReverseRouteFromJourney( | ||
| source: TrainStation, | ||
| previousStation: TrainStation, | ||
| hopCount: number, | ||
| ): void { | ||
| if (source === this) return; // Don't store route to self | ||
|
|
||
| const currentTime = this.mg.ticks(); | ||
| const sourceTile = source.tile(); | ||
| const existingRoute = this.routingTable.get(sourceTile); | ||
|
|
||
| // Only update if this is a better route or we don't have one | ||
| const shouldUpdate = | ||
| !existingRoute || | ||
| hopCount < existingRoute.hopCount || | ||
| (hopCount === existingRoute.hopCount && | ||
| currentTime - existingRoute.lastUpdate > this.routeStaleThreshold / 2); | ||
|
|
||
| if (shouldUpdate) { | ||
| this.routingTable.set(sourceTile, { | ||
| destinationId: sourceTile, | ||
| nextHopId: previousStation.tile(), | ||
| hopCount: hopCount, | ||
| sequenceNumber: this.sequenceNumber, | ||
| lastUpdate: currentTime, | ||
| }); | ||
|
|
||
| this.changedRoutes.add(source); | ||
| this.routesChanged = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
sequenceNumber not incremented for journey-learned routes.
Line 789 uses this.sequenceNumber when storing routes learned from train journeys, but never increments it. All journey-learned routes will have the same sequence number, which could cause issues if broadcasts are enabled later—the BATMAN protocol uses sequence numbers to distinguish newer information from stale updates. Either increment this.sequenceNumber when learning routes, or document why journey-learned routes don't need distinct sequence numbers.
private updateReverseRouteFromJourney(
source: TrainStation,
previousStation: TrainStation,
hopCount: number,
): void {
if (source === this) return; // Don't store route to self
const currentTime = this.mg.ticks();
const sourceTile = source.tile();
const existingRoute = this.routingTable.get(sourceTile);
// Only update if this is a better route or we don't have one
const shouldUpdate =
!existingRoute ||
hopCount < existingRoute.hopCount ||
(hopCount === existingRoute.hopCount &&
currentTime - existingRoute.lastUpdate > this.routeStaleThreshold / 2);
if (shouldUpdate) {
+ this.sequenceNumber++; // Increment for new route learned
this.routingTable.set(sourceTile, {
destinationId: sourceTile,
nextHopId: previousStation.tile(),
hopCount: hopCount,
sequenceNumber: this.sequenceNumber,
lastUpdate: currentTime,
});
this.changedRoutes.add(source);
this.routesChanged = true;
}
}🤖 Prompt for AI Agents
In src/core/game/TrainStation.ts around lines 766 to 796, the code stores
journey-learned routes using this.sequenceNumber but never increments it, so all
such routes share the same sequence number; update the code to increment
this.sequenceNumber when persisting journey-learned routes (e.g.,
this.sequenceNumber = (this.sequenceNumber + 1) & 0xFFFFFFFF or similar wrap if
needed) and use the new value as the stored sequenceNumber, or alternatively add
a clear comment explaining why journey-learned routes intentionally do not
advance the sequence number; ensure the increment happens atomically before
writing to routingTable and that tests/broadcast logic consider the updated
behavior.
…lection Replace fixed pathfinding with dynamic routing system featuring: - Local greedy routing: Trains evaluate neighbors based on profit potential, traffic congestion, distance, and recent history - Exploration capability: 10% randomness prevents suboptimal but discovers new routes - Congestion avoidance: Trains naturally spread to less busy stations - Loop prevention: Memory of recent visits prevents getting stuck - Adaptive behavior: System responds to changing network conditions - Enhanced journey tracking: Share complete route information instead of just start position Includes BATMAN-style routing protocol (currently disabled) for future network-wide knowledge distribution.
scamiv
force-pushed
the
feature/Local-Train-Routing--BatMan-style
branch
from
fa1fc92 to
51c603a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/execution/TrainExecution.ts (1)
157-171: recordTrainArrival conflates successful and aborted journeys
targetReached()always callsthis.mg.recordTrainArrival(this.journeyHopCount). HowevertargetReached()is also invoked when the train times out (max hops) or has no valid next hop (see comment below ongetNextTile), so aborted trains will be counted as “arrivals” and skew stats.Consider guarding the stats update so it only runs when the train actually reaches
this.destination(for example by checkingthis.currentStation === this.destinationor train tile vs destination tile), and keepsetReachedTarget()for UI/cleanup if you still want that to fire in all end states.src/core/game/RailNetworkImpl.ts (1)
104-125: AddonStationRemovedmock to test station objectsThe mock station factory
createMockStation()attests/core/game/RailNetwork.test.ts:10–26is missing theonStationRemovedmethod. When the code callsstation.onStationRemoved()atsrc/core/game/RailNetworkImpl.ts:110, the mock throws a TypeError.Add
onStationRemoved: jest.fn()to the mock object increateMockStation():const createMockStation = (unitId: number): any => { // ... existing properties ... clearRailroads: jest.fn(), onStationRemoved: jest.fn(), // ← Add this line };
♻️ Duplicate comments (4)
src/core/game/TrainStation.ts (3)
175-203: Constructor: random seeding and changedRoutes behaviorA couple of details here:
this.random = new PseudoRandom(mg.ticks() + this.tile());is fine, but consider using a dedicated seed from config or station ID to keep determinism stable across refactors to tick ordering.
this.changedRoutes.add(this);runs even whenenableBroadcastsisfalse, and with broadcasts disabled this set will never be cleared. It’s only aSet<TrainStation>, so growth is bounded, but you could avoid unnecessary references by only adding when broadcasts are enabled:if (this.enableBroadcasts) { this.changedRoutes.add(this); }This keeps routing‑change tracking aligned with the current mode.
296-396: BATMAN broadcast logic: align with PR goal or mark as experimentalThe BATMAN protocol implementation (broadcastOriginatorMessage / receiveOriginatorMessage / cleanupStaleRoutes / tick) is quite complete, but
enableBroadcastsis hard‑coded tofalse.Given the PR description says network‑wide routing is currently disabled and references “should be removed”, you might want to:
- Either remove or clearly feature‑flag the BATMAN methods and state, or
- Add a short comment near
enableBroadcastsdescribing that this block is experimental and not used in production, so future readers know they can ignore it.Right now it adds complexity without being active.
720-763: processJourneyInformation: CI failure due to missing shareJourneyInfo on mocksThe integration with
TrainExecution.shareJourneyInfo()is sound and matches the shape defined there.The CI errors (
trainExecution.shareJourneyInfo is not a function) point to tests using aTrainExecutionmock without this method. Please update those mocks to provideshareJourneyInfo()returning at least:{ journeySource: null, routeInformation: [], }or a fuller object if you want to test routing learning.
The runtime code path itself looks fine.
#!/bin/bash # Find TrainExecution mocks to extend with shareJourneyInfo() rg -nP 'mock.*TrainExecution|as TrainExecution' --type=ts tests -C3src/core/execution/TrainExecution.ts (1)
263-342: DoubletargetReached()call and ambiguous null semantics in getNextTileIn the
isAtStationbranch:
- When
this.currentStation === this.destination,getNextTile()callsthis.targetReached()and returnsnull.- For other failure cases (max hops,
nextHopmissing, railroad missing) it just returnsnull(sometimes after settingthis.active = false).Then in
tick()anynullreturn triggers a secondtargetReached()plusdeleteTrain():const tile = this.getNextTile(); if (tile) { this.updateCarsPositions(tile); } else { this.targetReached(); this.deleteTrain(); }This leads to:
- Successful arrivals calling
targetReached()twice (and recording stats twice).- Aborted journeys (max hops reached, no neighbor, broken railroad) also calling
targetReached()once, which suggests “destination reached” even though the train failed.I’d recommend:
- Remove the direct
targetReached()call fromgetNextTile()and lettick()be the single caller, and- Introduce an explicit end‑state flag (e.g.
this.reachedDestinationvsthis.aborted) sotick()can decide whether to calltargetReached()and/ordeleteTrain()and whether to record stats.This keeps the routing code focused on movement and lets high‑level lifecycle logic live in one place.
🧹 Nitpick comments (19)
src/core/execution/TrainExecution.ts (4)
24-51: InitializejourneySourceandcurrentStationmore explicitlyConstructor and
init()together setjourneySourcebased on source type and thencurrentStation = this.source. This is fine, but the split responsibility is a bit hard to follow.Consider moving all journey initialization (including
currentStation,journeySource,journeyPreviousStation,journeyHopCount) into a single private helper called frominit()so the start-of-life state is defined in one place instead of split across constructor andinit. This will also make future refactors easier and keep the class more compositional.
57-100: shareJourneyInfo: confirm route semantics and limit memoryThe reverse‑route logic (using
recentStationsand a singleimmediatePreviousasnextHop) matches the intended “to reach any earlier station from here, first go back through the previous station” behavior.Two small suggestions:
- Clamp
routeInformationsize: ifrecentStationscan grow large (config changes), you might want to cap how many hops are shared to avoid building huge route arrays for long‑running trains.- Document the semantics in the type: e.g. rename
distancetohopCountFromCurrentto make it clear this is “steps from current station” rather than physical distance.These are cosmetic but improve readability for future routing consumers.
102-125: Source/destination validation: consider user feedbackDeactivating the execution when source or destination is inactive or identical is correct, but nothing is surfaced to the player or logs besides the generic
console.warn("cannot build train")path.If this path can happen in normal play, consider emitting a clearer message via
displayMessageor a more specific log so debugging “train did not spawn” reports is easier. Otherwise this silent failure may be confusing.
344-358: stationReached: guard error path more gracefullyThe guard:
if (this.mg === null || this.player === null || !this.currentStation) { throw new Error("Not initialized"); }is fine for programmer errors but will crash the game if routing state becomes inconsistent at runtime.
Given this is called from
getNextTile()during normal ticks, consider:
- Logging and deleting the train instead of throwing, or
- At least including more context in the error message (e.g. train id, ticks) so debugging is easier.
Also,
this.playeris never re‑assigned, so thethis.player === nullcheck can be dropped.src/core/game/Game.ts (1)
713-715: ExposerecordTrainArrivalclearly in Game documentationThe new
recordTrainArrival(steps: number)hook is fine, but its intent is not obvious from the interface alone.Consider adding a short JSDoc above it, e.g. “Record a completed train journey with the given hop count for stats/telemetry”, so other
Gameimplementations (tests, tools) know how to implement or stub it.src/core/game/GameImpl.ts (5)
81-86: Stats fields: consider grouping into a dedicated metrics objectThe four train stats fields (
trainArrivalTimes,completedTrainSteps,activeTrainSteps,lastStatsPrint) live directly onGameImpl.For clarity and composition, you could wrap them in a small dedicated helper (e.g.
TrainStatsTrackerwith methodsrecordArrival,onTick,GameImpl. This keeps the core game object slimmer and makes it easier to test the stats logic in isolation.
400-405: Train stats print cadence and noisePrinting stats every 60 ticks with
console.logis okay for debugging but might be noisy in larger games or production.Consider:
- Guarding it behind a config flag or log level, or
- Printing less frequently or only when a certain number of trains exists.
Also, the printed string uses an emoji; ensure server environments you care about can handle this, or make it opt‑in.
465-481: recordTrainArrival: ensure arrays stay aligned and boundedThe sliding‑window logic is good, but two small points:
- When you slice
completedTrainStepsto matchtrainArrivalTimes.length, you assume arrivals array is always the shorter one. Given both arrays are only mutated here, that’s true today; if future code ever manipulatescompletedTrainStepsdirectly, this could desync.- You might want an explicit hard cap (e.g. max N entries) to avoid worst‑case memory if tick rate changes or arrival rate is extreme.
A simple alternative is to store a single array of
{ tick, steps }objects and filter/slice that in one place.
483-502: getActiveTrainCount / getAverageActiveTrainSteps rely on executions() snapshot
getActiveTrainCount()rebuilds the executions list every call and filters byconstructor.name.getAverageActiveTrainSteps()then calls it again, leading to repeated scans.Not critical now, but for large games this adds overhead. You could:
- Cache the active train count during
executeNextTick()as you already touch all executions, and- Use type guards instead of
constructor.name.This will keep the stats collection cheap.
504-515: printTrainStats: dependency on renderNumber vs plain numbers
avgCompletedStepsandavgActiveStepsare rounded manually and printed as plain numbers. This is fine, but elsewhere the game often uses helpers likerenderNumberfor consistent formatting.If you want consistent UX for debug metrics, consider using the same formatting utilities here or at least fixing the decimal precision via
toFixed(2)for simpler reading.src/core/game/TrainStation.ts (9)
9-26: StationLookup: lifecycle depends on onStationRemoved being calledThe static
StationLookupkeyed byTileRefis simple and fits the routing needs.Given it holds strong references to all registered stations, the correctness of cleanup depends on:
StationLookup.register(this)being called exactly once per real station (constructor does this), andStationLookup.unregister(this)being called whenever a station is removed (onStationRemoved, which is now invoked fromRailNetworkImpl.removeStation).That path looks correct. Just make sure any alternative station removal flows in tests or tools also call
onStationRemoved, or use a realTrainStationin those flows. Otherwise this map can leak stations over long games.
31-48: Deprecated RoutingEntryFull likely no longer needed
RoutingEntryFullis marked as legacy but still exported. If it’s truly unused now that routing uses ID‑basedRoutingEntry, consider removing it to keep the API smaller.If you keep it for compatibility, document which code still consumes it and when it can be deleted.
135-173: Routing state: turn BATMAN block into a dedicated routing componentThe class now mixes station responsibilities (gold, trade, cluster membership) with routing table management, greedy edge metrics, traffic heat, and random exploration flags.
To keep things simple and compositional, consider extracting the BATMAN+greedy routing state into a small helper (e.g.
StationRoutingState) held by composition insideTrainStation. That would reduce the size of this class and make routing experiments easier without touching core station behavior.
214-223: addRailroad: also consider initializing metrics for both endpointsEdge metrics are initialized only for the neighbor on
thisside. That’s correct from this station’s perspective, but the other station only gets metrics if its ownaddRailroadis called elsewhere.If that isn’t guaranteed (e.g. direct construction of
Railroadin tests), consider clearly documenting that both endpoints must calladdRailroador add a small helper onRailNetworkImplto attach the same metrics on both sides.
276-295: getNextHop: safe but may return null for stale StationLookup entries
getNextHop()usesStationLookup.getStation(route.nextHopId). If a station was removed but some routing entries weren’t cleaned, this can returnnulland you fall back to exploration.That’s acceptable behavior, but it would be good to log or count such cases somewhere (even as a TODO) so you can detect if routing cleanup is falling behind.
486-502: calculateActualProfit: BigInt to number cast can overflow silently
trainGoldreturns abigintand you cast it withNumber(goldValue). For very large values this may lose precision or overflow toInfinity.Probably fine here since it’s only for scoring, but consider:
- Documenting that the score is approximate, or
- Capping to a max numeric value when casting.
This keeps the routing math stable if trainGold values are tuned upwards later.
574-641: chooseNextStation: destination is unused in greedy phaseThe hybrid routing logic is clear:
- Prefer
getNextHop(destination)when available.- Otherwise evaluate neighbors based on profit, recency, and congestion, with a 10% random exploration factor.
In the greedy phase,
destinationis not used at all, so trains may wander in profitable loops that never approach the actual target. This may be intentional for exploration, but the code comment only mentions “exploration/unknown routes”.If you want some directional bias, you might include a small term based on geometric distance to
destinationincalculateEdgeScore(still keeping profit as the main factor). If you intend pure local exploration, consider adding a short comment saying “destination is ignored here by design” so future readers don’t assume it’s a bug.
643-669: onStationRemoved: cluster/routing cleanup is good; also clear changedRoutes for others
onStationRemoved()correctly:
- Unregisters from
StationLookup,- Removes routes that use this station as a next hop,
- Clears local edge metrics and routingTable.
One minor improvement: other stations may still have this station in their
changedRoutessets.onOtherStationRemoved()clears edge metrics and routes, but notchangedRoutes. To avoid holding references to removed stations, consider also deletingremovedStationfromchangedRoutesinonOtherStationRemoved.
766-800: updateReverseRouteFromJourney: consider sequenceNumber behaviorRoutes learned from journeys are stored with
sequenceNumber: this.sequenceNumber, butsequenceNumberis only incremented inbroadcastOriginatorMessage(). If you later enable BATMAN broadcasts, all journey‑learned entries will look like they share the same sequence.If that matters, increment
sequenceNumberhere when you update a route, or explicitly document that journey‑learned routes do not participate in sequence‑based freshness checks.Also,
this.changedRoutes.add(destination);is fine now, but when broadcasts are disabled it just accumulates references; if you adopt the earlier suggestion to guardchangedRoutesonenableBroadcasts, apply the same here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/execution/TrainExecution.ts(4 hunks)src/core/execution/TrainStationExecution.ts(1 hunks)src/core/game/Game.ts(1 hunks)src/core/game/GameImpl.ts(4 hunks)src/core/game/RailNetworkImpl.ts(1 hunks)src/core/game/TrainStation.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/execution/TrainStationExecution.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/game/RailNetworkImpl.tssrc/core/execution/TrainExecution.tssrc/core/game/GameImpl.tssrc/core/game/TrainStation.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/TrainExecution.tssrc/core/game/TrainStation.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/TrainExecution.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/GameImpl.tssrc/core/game/TrainStation.ts
🧬 Code graph analysis (2)
src/core/execution/TrainExecution.ts (4)
src/core/game/TrainStation.ts (1)
TrainStation(135-801)src/core/game/RailNetwork.ts (1)
RailNetwork(4-8)src/core/game/Game.ts (2)
Player(525-661)Game(663-748)src/core/game/Railroad.ts (1)
getOrientedRailroad(28-40)
src/core/game/TrainStation.ts (5)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/UnitImpl.ts (2)
tile(175-177)type(145-147)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/game/Railroad.ts (1)
Railroad(6-26)src/core/execution/TrainExecution.ts (1)
TrainExecution(14-367)
🪛 GitHub Actions: 🧪 CI
src/core/game/RailNetworkImpl.ts
[error] 108-110: TypeError: station.onStationRemoved is not a function. This indicates a missing or incorrectly mocked method 'onStationRemoved' for station objects during test.
[error] 108-110: TypeError: station.onStationRemoved is not a function. Same as above, failing in disconnect/remove station logic in tests.
src/core/game/TrainStation.ts
[error] 744-746: TypeError: trainExecution.shareJourneyInfo is not a function. The method 'shareJourneyInfo' is missing or not mocked, leading to failure in processing journey info.
[error] 744-746: TypeError: trainExecution.shareJourneyInfo is not a function. Same as above, affecting handling of train journey info.
[error] 257-257: TypeError: this.unit.tile is not a function. The property 'tile' is expected to be a function but is not defined or incorrectly mocked, causing failures in trade availability check.
[error] 481-481: TypeError: this.mg.x is not a function. The property 'mg.x' is expected to be a function but is not defined or mocked, leading to errors in neighbor/link calculations.
[error] 479-479: TypeError: this.mg.x is not a function. Same as above, affecting neighbor and distance calculations.
🔇 Additional comments (4)
src/core/game/TrainStation.ts (3)
504-521: Traffic counters are simple and look correctIncrement/decrement of
trainCount,recentArrivals, and heat updates on arrival/departure are consistent and theMath.max(0, trainCount - 1)guard is good.No changes requested here.
690-715: getTraffic / updateTraffic: behavior looks goodThe copy‑on‑return in
getTraffic()avoids external mutation, andupdateTraffic()’s decay and reset logic is straightforward.No issues here.
256-258: No issues found. Tests are properly mocked and passing.The
unitmock is correctly configured withtile: jest.fn().mockReturnValue({ x: 0, y: 0 })in the test setup. The test at line 124-128 explicitly callsstation.tile()and passes. All usages oftile()in tests use the mainunitmock which has the method properly mocked. TheTrainStation.tile()implementation correctly forwards tothis.unit.tile(), and no CI failures related to this method are evident in the test file.src/core/execution/TrainExecution.ts (1)
252-261: Ensure Game mocks used in TrainExecution tests providex(ref)andy(ref)methodsThe
isAtStation()method correctly usesthis.mg.x()andthis.mg.y()to check tile coordinates. TheGameinterface extendsGameMap, which defines these methods, so the production code is sound.However, test mocks of
Gamemust fully implement theGameMapinterface contract. If you write tests that instantiateTrainExecutionor call code paths usingisAtStation(), verify that yourGamemock includesx(ref: TileRef): numberandy(ref: TileRef): numbermethods (matching the signature insrc/core/game/GameMap.ts).Likely an incorrect or invalid review comment.
| private activeSourceOrDestination(): boolean { | ||
| return ( | ||
| this.stations.length > 1 && | ||
| this.stations[1].isActive() && | ||
| this.stations[0].isActive() | ||
| ); | ||
| return this.source.isActive() && this.destination.isActive(); | ||
| } |
There was a problem hiding this comment.
activeSourceOrDestination: clarify semantics
activeSourceOrDestination returns this.source.isActive() && this.destination.isActive(). This means if either endpoint dies mid‑journey, the train is deleted.
If that’s intentional, consider renaming to bothEndpointsActive() or adding a short comment. If instead the intent is “source or destination still exists”, you probably want || instead of &&.
🤖 Prompt for AI Agents
In src/core/execution/TrainExecution.ts around lines 208 to 210, the method
activeSourceOrDestination() currently uses && which requires both endpoints to
be active; decide the intended semantics and fix accordingly: if you mean "both
endpoints must be active" rename the method to bothEndpointsActive() and add a
one-line comment clarifying that deletion occurs when either endpoint is
inactive; if you mean "either endpoint still exists" change the operator to ||,
update the method name to reflect that (e.g., sourceOrDestinationActive()), and
add/adjust a unit test to cover the case where one endpoint is inactive.
| executeNextTick(): GameUpdates { | ||
| this.updates = createGameUpdatesMap(); | ||
|
|
||
| // Reset active train steps counter for this tick | ||
| this.activeTrainSteps = 0; | ||
|
|
||
| this.execs.forEach((e) => { | ||
| if ( | ||
| (!this.inSpawnPhase() || e.activeDuringSpawnPhase()) && | ||
| e.isActive() | ||
| ) { | ||
| e.tick(this._ticks); | ||
|
|
||
| // Track steps for active trains | ||
| if (e.constructor.name === "TrainExecution") { | ||
| this.activeTrainSteps += (e as any).journeyHopCount ?? 0; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
executeNextTick: fragile reliance on constructor.name and private fields
Inside the tick loop:
if (e.constructor.name === "TrainExecution") {
this.activeTrainSteps += (e as any).journeyHopCount ?? 0;
}This has a few problems:
- Using
constructor.nameis brittle (minification, refactors, subclasses). - It reaches into
journeyHopCountviaany, couplingGameImplto a private field ofTrainExecution. - It runs on every tick over every execution, adding overhead.
A cleaner, more compositional approach:
-
Add a small “stats reporter” interface for executions, e.g.
interface TrainLikeExecution { isTrain(): boolean; getJourneyHopCount(): number; }
and implement it in
TrainExecution, or -
Prefer an explicit hook where
TrainExecutionreports its “active steps” to the game (similar torecordTrainArrival) instead of the game introspecting execution internals.
This keeps type safety and avoids using any and string name checks.
🏁 Script executed:
#!/bin/bash
# Check all Execution implementations; see if we can introduce a shared interface
rg -nP 'implements Execution' src/core/execution --type=ts -C2Length of output: 15669
| private calculateEdgeScore( | ||
| edge: EdgeMetrics, | ||
| stationsAgo: number, // -1 = never visited, 1 = immediate previous, 2 = 2 ago, etc. | ||
| actualProfit: number, | ||
| neighborTrafficHeat: number, // Heat factor of the neighbor station | ||
| ): number { | ||
| // Base score: profit per time unit, boosted by profit-per-distance | ||
| const profitPerDistance = actualProfit / edge.distance; | ||
| let score = | ||
| (actualProfit / | ||
| (edge.baseDuration * (1 + this.distanceSensitivity * edge.distance))) * | ||
| (1 + this.profitSensitivity * profitPerDistance); | ||
|
|
||
| // Apply graduated recency penalty based on stations ago | ||
| if (stationsAgo > 0) { | ||
| const penaltyStrength = | ||
| Math.pow(this.recencyDecayFactor, stationsAgo - 1) * | ||
| this.maxRecencyPenalty; | ||
| const recencyPenalty = 1.0 - penaltyStrength; | ||
| score *= recencyPenalty; | ||
| } | ||
|
|
||
| // Apply station heat avoidance | ||
| score *= 1 - this.stationHeatSensitivity * neighborTrafficHeat; | ||
|
|
||
| // Ensure unvisited stations get a minimum exploration score | ||
| // This prevents zero-profit unvisited stations(facttories) from being ignored | ||
| if (stationsAgo < 0 && score <= 0) { | ||
| score = 0.2; // Small positive score to encourage exploration | ||
| } | ||
|
|
||
| return score; | ||
| } |
There was a problem hiding this comment.
calculateEdgeScore: guard against zero distance
profitPerDistance = actualProfit / edge.distance will blow up if edge.distance is zero (e.g. if two stations share the same tile due to a bug or future refactor).
You clamp baseDuration with Math.max(1, ...) but not distance. Consider:
const distance = Math.max(1e-3, edge.distance);
const profitPerDistance = actualProfit / distance;
// and use distance instead of edge.distance belowThis avoids division by zero and extreme scores.
🤖 Prompt for AI Agents
In src/core/game/TrainStation.ts around lines 526-558, profitPerDistance divides
by edge.distance which can be zero and cause infinite/extreme scores; clamp
distance to a small epsilon first (e.g. const distance = Math.max(1e-3,
edge.distance)) and then use that distance variable everywhere edge.distance is
currently used in this function (profitPerDistance calculation and any
distance-based multipliers) so division-by-zero is prevented and scores remain
bounded.
…urney transmission, journeySource wasnt loop proof
- Added search radius - Updated several properties in TrainStation class to be readonly for better immutability and clarity. - Introduced heat decay interval and factor for more flexible heat management. - pre-computed decay factors avoiding Math.pow in critical paths. - Enhance logging - Refined routing logic - removed journeyPreviousStation property - removed RecentArrivals - unbounded StationTraffic.heat -> score can now be negative
|
reopening squashed #2473 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/TrainExecution.ts (1)
130-146: Destination and hop-limit handling mix up “arrival” and “aborted” trains (double-counting stats).There are two intertwined issues:
Destination reached is counted twice.
In
getNextTile():if (this.currentStation === this.destination) { this.targetReached(); return null; }In
tick():const tile = this.getNextTile(); if (tile) { this.updateCarsPositions(tile); } else { this.targetReached(); this.deleteTrain(); }When the destination station is reached,
targetReached()is called once ingetNextTile()and again intick(). This double-incrementsrecordTrainArrival, and repeatssetReachedTarget()on units.Hop-limit removals are incorrectly treated as arrivals.
Hop-limit branch in
getNextTile():if (this.journeyHopCount >= this.maxHops) { if (this.mg) { this.mg.recordTrainRemovedDueToHopLimit(this.journeyHopCount); } this.active = false; return null; }
tick()still treats the resultingnullas a successful arrival and callstargetReached(), which recordsrecordTrainArrival()and flags the train as having reached its target, even though it was removed due to exceeding the hop limit.The same applies to other “no route” cases (
!nextHop,!railroad): they are currently treated as successful arrivals in stats.Suggested fix (keep logic simple and clear):
Make
getNextTile()purely about movement and local state changes. It should not calltargetReached(). Instead, it should:
- Return
nullwhen there is no movement (destination reached, hop limit exceeded, no route, etc.).- Keep the hop-limit removal accounting (
recordTrainRemovedDueToHopLimit) andthis.active = falseas-is.Let
tick()decide whether anullresult means “arrived” or “aborted”, based oncurrentStationandactive:- private getNextTile(): TileRef | null { + private getNextTile(): TileRef | null { // If we're at a station, decide where to go next if (this.isAtStation()) { // Process arrival if we haven't already for this station visit if (!this.hasProcessedArrival) { this.stationReached(); // Handle arrival at current station this.hasProcessedArrival = true; } // Check if we've reached the destination - if (this.currentStation === this.destination) { - this.targetReached(); - return null; - } + if (this.currentStation === this.destination) { + // Destination station reached – stop moving; tick() will handle arrival + return null; + } // Check if we've exceeded max hops if (this.journeyHopCount >= this.maxHops) { // Give up - we've wandered too long if (this.mg) { this.mg.recordTrainRemovedDueToHopLimit(this.journeyHopCount); } this.active = false; return null; } ... } // Follow current railroad ... return null; }- tick(ticks: number): void { + tick(ticks: number): void { if (this.train === null) { throw new Error("Not initialized"); } if (!this.train.isActive() || !this.activeSourceOrDestination()) { this.deleteTrain(); return; } const tile = this.getNextTile(); - if (tile) { - this.updateCarsPositions(tile); - } else { - this.targetReached(); - this.deleteTrain(); - } + if (tile) { + this.updateCarsPositions(tile); + } else { + // Only count as an arrival if we actually reached the destination + if (this.currentStation === this.destination) { + this.targetReached(); + } + this.deleteTrain(); + } }With this:
- Real arrivals at the destination station call
targetReached()exactly once.- Hop-limit removals and “no route” cases are deleted without being counted as arrivals, while still incrementing
recordTrainRemovedDueToHopLimit.- The code’s control flow stays straightforward and easy to read.
Also applies to: 159-173, 265-346
♻️ Duplicate comments (3)
src/core/game/GameImpl.ts (1)
81-87: Train stats tracking relies on brittle type checks and may overcount trains.The overall stats idea is good, but a few details are risky:
executeNextTickandgetActiveTrainCountboth depend on
e.constructor.name === "TrainExecution"and(e as any).journeyHopCount.
This is brittle (refactors/minifiers) and reaches into a private field. The earlier review already flagged this pattern; here it is now used in two places. Consider a typed hook onExecutioninstead, e.g.:// in Execution getTrainHopCount?(): number; // in TrainExecution getTrainHopCount() { return this.journeyHopCount; } // in GameImpl this.activeTrainSteps += e.getTrainHopCount?.() ?? 0;and likewise use
e.getTrainHopCountto detect train executions ingetActiveTrainCount()instead ofconstructor.name/any.
getActiveTrainCount()callsthis.executions(), which includesunInitExecs. ForTrainExecution,isActive()istrueuntil initialization, so uninitialized trains will be counted as “active”. You probably want to restrict this tothis.execs(initialized executions) or add an explicit “initialized” flag.
activeTrainStepsis summed immediately aftere.tick(...)without checking whether the execution is still active. Trains that finish this tick will still contribute to “active” steps. If you want “currently active trains only”, add ane.isActive()check aftertick()before accumulating.Also applies to: 355-373, 401-405, 466-511
src/core/execution/TrainExecution.ts (1)
210-212: ClarifyactiveSourceOrDestinationsemantics or adjust logic.
activeSourceOrDestination()currently returns:return this.source.isActive() && this.destination.isActive();So the train is deleted when either endpoint becomes inactive, even though the name reads like an “OR”.
If “both endpoints must remain alive” is the intended rule, consider renaming to something like
bothEndpointsActive()and adding a short comment. If instead you meant “as long as at least one endpoint is alive”, switch to||.src/core/game/TrainStation.ts (1)
477-499: Guard edge scoring against zero-distance routes (numerical safety).
calculateEdgeScore()assumesedge.distance > 0:const profitPerDistance = actualProfit / edge.distance; let score = (actualProfit / (edge.baseDuration * (1 + this.distanceSensitivity * edge.distance))) * (1 + this.profitSensitivity * profitPerDistance);
edge.distancecomes fromcalculateDistance()(sqrt of dx² + dy²). If two stations ever end up on the same tile (bug, mod, or future change),distancebecomes 0 and this will produceInfinity/NaNscores.A small clamp keeps things robust without changing normal behavior:
private calculateEdgeScore( edge: EdgeMetrics, stationsAgo: number, actualProfit: number, neighborTrafficHeat: number, ): number { - // Base score: profit per time unit, boosted by profit-per-distance - const profitPerDistance = actualProfit / edge.distance; + // Base score: profit per time unit, boosted by profit-per-distance + const distance = Math.max(1e-3, edge.distance); + const profitPerDistance = actualProfit / distance; let score = - (actualProfit / - (edge.baseDuration * (1 + this.distanceSensitivity * edge.distance))) * + (actualProfit / + (edge.baseDuration * (1 + this.distanceSensitivity * distance))) * (1 + this.profitSensitivity * profitPerDistance);This keeps scores finite even in degenerate layouts.
Also applies to: 540-575
🧹 Nitpick comments (1)
src/core/execution/TrainExecution.ts (1)
26-34: Journey memory never includes the source station, limiting learned routes (optional tweak).The journey-tracking setup is close, but two small tweaks would make route learning more consistent:
recentStationsis only updated ingetNextTile()when departing a station:this.recentStations.push(nextHop);It is never seeded with
sourceininit(). On the first legsource → A,recentStationsis[A]on arrival atA, soshareJourneyInfo()atAyields no routes involving the source station.
shareJourneyInfo()assumes there is at least one station inrecentStations, but for very short journeys it’s effectively a no-op. That’s fine, but you can make the intent explicit.If you want stations to learn about routes back to the origin even for the first hop, consider:
init(mg: Game, ticks: number): void { this.mg = mg; // Validate that source and destination are active ... if (this.source === this.destination) { this.active = false; return; } - this.currentStation = this.source; + this.currentStation = this.source; + // Seed journey memory with the starting station + this.recentStations = [this.source]; ... }And, for clarity and safety:
public shareJourneyInfo(): { routeInformation: Array<...> } { - const routeInformation: Array<...> = []; + const routeInformation: Array<...> = []; + + if (this.recentStations.length < 2) { + return { routeInformation }; // Not enough history to learn from + }This keeps the implementation simple but ensures early trips contribute useful information to station routing.
Also applies to: 47-102, 104-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/execution/TrainExecution.ts(5 hunks)src/core/game/Game.ts(1 hunks)src/core/game/GameImpl.ts(4 hunks)src/core/game/TrainStation.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/Game.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/GameImpl.tssrc/core/game/TrainStation.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/GameImpl.tssrc/core/game/TrainStation.tssrc/core/execution/TrainExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/game/GameImpl.tssrc/core/game/TrainStation.tssrc/core/execution/TrainExecution.ts
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/game/TrainStation.tssrc/core/execution/TrainExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/game/TrainStation.ts
🧬 Code graph analysis (2)
src/core/game/TrainStation.ts (4)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/game/Railroad.ts (1)
Railroad(6-26)src/core/execution/TrainExecution.ts (1)
TrainExecution(14-363)
src/core/execution/TrainExecution.ts (3)
src/core/game/TrainStation.ts (1)
TrainStation(134-869)src/core/game/Game.ts (1)
Game(663-749)src/core/game/Railroad.ts (1)
getOrientedRailroad(28-40)
🪛 GitHub Actions: 🧪 CI
src/core/game/TrainStation.ts
[error] 807-807: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 807-807: TypeError: trainExecution.shareJourneyInfo is not a function
[error] 272-272: TypeError: this.unit.tile is not a function
[error] 272-272: TypeError: this.unit.tile is not a function
[error] 496-496: TypeError: this.mg.x is not a function
[error] 496-496: TypeError: this.mg.x is not a function
| /** | ||
| * Simple station lookup by tile ID for routing | ||
| */ | ||
| class StationLookup { | ||
| private static stations = new Map<TileRef, TrainStation>(); | ||
|
|
||
| static register(station: TrainStation): void { | ||
| this.stations.set(station.tile(), station); | ||
| } | ||
|
|
||
| static getStation(tile: TileRef): TrainStation | null { | ||
| return this.stations.get(tile) ?? null; | ||
| } | ||
|
|
||
| static unregister(station: TrainStation): void { | ||
| this.stations.delete(station.tile()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
StationLookup + cleanup look sound; ensure unit/game mocks provide tile/x/y for tests.
The new StationLookup and the cleanup hooks are wired correctly:
StationLookup.register(this)in the constructor andStationLookup.unregister(this)inonStationRemoved()keep the global map in sync.onStationRemoved()andonOtherStationRemoved()also clear routingTable entries and edgeMetrics that go through removed stations, which avoids stale routing pointers.
At runtime, this relies on:
this.unit.tile()returning aTileRef(constructor/tile()), andthis.mg.x(...)/this.mg.y(...)being implemented (used bycalculateDistanceandTrainStationMapAdapter.position).
GameImpl implements these via Game extends GameMap, but the CI failures show the test doubles don’t. When updating tests, make sure your game and unit mocks define:
unit.tile(): TileRefgame.x(ref: TileRef): numbergame.y(ref: TileRef): number
so that distance and position helpers don’t crash in tests.
Also applies to: 182-218, 712-738
🏁 Script executed:
#!/bin/bash
# Quick check: where TrainStation is constructed in tests and what the mocks look like
rg -nP 'new TrainStation\(' tests --type=ts -C5 || trueLength of output: 4800
🏁 Script executed:
#!/bin/bash
# Look at the test file setup: beforeEach, mock definitions for `game` and `unit`
head -n 100 tests/core/game/TrainStation.test.tsLength of output: 95
🏁 Script executed:
#!/bin/bash
# Get the full test setup (beforeEach and first mock definitions)
rg -nP 'beforeEach|describe|const (game|unit|trainExecution|player)' tests/core/game/TrainStation.test.ts -A2 | head -n 80Length of output: 193
🏁 Script executed:
#!/bin/bash
# Get the complete beforeEach block with all mock definitions
rg -nP 'beforeEach' tests/core/game/TrainStation.test.ts -A50 | head -n 60Length of output: 1628
game.x() and game.y() mocks are missing; add them to prevent calculateDistance failures.
Your test setup correctly mocks unit.tile() (line 36 returns { x: 0, y: 0 }), but the game mock lacks the coordinate methods the code needs.
When calculateDistance() or TrainStationMapAdapter.position() call this.mg.x(ref) or this.mg.y(ref), those methods don't exist on the mock and tests will error. The as any cast lets the code compile but fails at runtime.
Update your game mock in beforeEach() to include:
game = {
ticks: jest.fn().mockReturnValue(123),
config: jest.fn().mockReturnValue({...}),
addUpdate: jest.fn(),
addExecution: jest.fn(),
x: jest.fn().mockReturnValue(0), // Add this
y: jest.fn().mockReturnValue(0), // Add this
} as any;This applies to all tests that construct TrainStation or call distance/position helpers.
🤖 Prompt for AI Agents
In src/core/game/TrainStation.ts around lines 9 to 26, the test mocks for game
are missing the coordinate accessors used by
calculateDistance()/TrainStationMapAdapter.position() (mg.x/mg.y), causing
runtime errors despite the as any cast; update the beforeEach() game mock to
include x and y methods (e.g., jest.fn().mockReturnValue(0)) so calls to
game.x(ref)/game.y(ref) succeed, ensuring all tests that create TrainStation or
call distance/position helpers have those methods on the mocked game.
| /** | ||
| * Update traffic when a train arrives | ||
| */ | ||
| onTrainArrival(trainExecution: TrainExecution): void { | ||
| this.traffic.trainCount++; | ||
|
|
||
| // Increase station heat (unbounded) | ||
| this.traffic.heat += 0.1; | ||
| this.traffic.lastHeatUpdate = this.mg.ticks(); | ||
| } | ||
|
|
||
| /** | ||
| * Update traffic when a train departs | ||
| */ | ||
| onTrainDeparture(trainExecution: TrainExecution): void { | ||
| this.traffic.trainCount = Math.max(0, this.traffic.trainCount - 1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Journey-based routing depends on new shareJourneyInfo() shape; update tests/mocks accordingly.
The journey-processing pipeline is:
-
TrainStation.onTrainStop()→this.processJourneyInformation(trainExecution); -
processJourneyInformation()callstrainExecution.shareJourneyInfo()and expects:{ routeInformation: Array<{ destination: TrainStation; nextHop: TrainStation | null; distance: number; }>; }
-
It then inspects
routeInfo.destination.unit.type()and passes{destination, nextHop, distance}intoupdateReverseRouteFromJourney().
The CI failure TypeError: trainExecution.shareJourneyInfo is not a function shows that at least one test double for TrainExecution is missing this method (or still returns the old shape).
To keep tests passing and aligned with the new API:
-
Update any TrainExecution mocks used with
TrainStation.onTrainStop()to defineshareJourneyInfo()with the shape above. -
For simple tests that don’t care about routing, you can return an empty route list:
shareJourneyInfo: jest.fn(() => ({ routeInformation: [] })),
-
For routing tests, return meaningful
destination/nextHop/distancevalues that exerciseupdateReverseRouteFromJourney().
This will fix the current CI error and make the journey-based routing observable in tests.
Also applies to: 781-794, 796-868
🏁 Script executed:
#!/bin/bash
# Find TrainExecution mocks that need shareJourneyInfo() added
rg -nP 'TrainExecution' tests --type=ts -C5 || trueLength of output: 1389
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full test file setup to see mock definitions
cat tests/core/game/TrainStation.test.ts | head -100
Length of output: 3011
🏁 Script executed:
#!/bin/bash
# Find processJourneyInformation implementation
rg -nP 'processJourneyInformation' src --type=ts -A15 || trueLength of output: 2203
🏁 Script executed:
#!/bin/bash
# Find all test files with TrainExecution mock references
fd --name '*.test.ts' tests --exec grep -l 'TrainExecution' {} \;Length of output: 242
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all test files with TrainExecution references (fix fd command)
find tests -name '*.test.ts' -exec grep -l 'TrainExecution' {} \;
Length of output: 36
🏁 Script executed:
#!/bin/bash
# Get full processJourneyInformation implementation to see complete expected shape
rg -nP 'private processJourneyInformation' src --type=ts -A50 || trueLength of output: 3691
🏁 Script executed:
#!/bin/bash
# Check for updateReverseRouteFromJourney to understand full interface
rg -nP 'updateReverseRouteFromJourney' src --type=ts -B5 -A10 || trueLength of output: 1695
Add shareJourneyInfo() mock to TrainExecution mock definition in tests/core/game/TrainStation.test.ts
The issue is confirmed. The trainExecution mock (lines 40-43) is missing the shareJourneyInfo() method. When tests call onTrainStop(), it immediately invokes processJourneyInformation() (line 786), which calls trainExecution.shareJourneyInfo() at line 807, causing the tests to fail.
Update the mock definition in the beforeEach block to include:
shareJourneyInfo: jest.fn(() => ({ routeInformation: [] }))This matches the expected interface shape and allows existing tests (which don't validate routing logic) to pass cleanly.
🤖 Prompt for AI Agents
In src/core/game/TrainStation.test.ts around lines 40-43, the TrainExecution
mock used in beforeEach is missing the shareJourneyInfo() method; add a mock
implementation named shareJourneyInfo that returns an object with
routeInformation as an empty array (e.g. a jest.fn that returns {
routeInformation: [] }) so calls from processJourneyInformation() /
onTrainStop() succeed and tests no longer fail.
1 participant
This is not yet a pr but a idea to tackle multiple train related issues. Feedback very welcome! Code needs cleanup!
Description:
Before: Trains followed fixed, pre-calculated paths between stations using a pathfinding algorithm
After: After: Trains now use a local cost based routing system.
Local Greedy Routing (Smart decision-making at each station)
At each station, trains evaluate neighboring stations based on multiple factors: Profit potential: How much gold they can earn based on diplomatic relationships Traffic congestion: Avoids overcrowded stations
Travel distance: Considers physical distance and travel time Recent history: Avoids recently visited stations to prevent loops Some randomness: 10% chance of making a non-optimal choice for exploration
Key Benefits
Organic route discovery: Routes improve as trains explore the network Congestion avoidance: Trains naturally spread out to less busy areas Adaptive behavior: System responds to changing network conditions Loop prevention: Memory of recent visits prevents trains from getting stuck
Currenlty disabled:
BATMAN Routing Protocol (Network-wide knowledge)
Stations broadcast routing information to neighbors like the BATMAN wireless mesh protocol Builds a distributed routing table that learns about the entire network over time Handles network topology changes automatically
Uses lazy cleanup to manage memory efficiently
Settings need to be tweaked:
Performance impact is not yet measured, but could be positive.
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME