optimize(RailroadLayer): throttle color scans, cull blits, and remove #2565
Conversation
O(n) tile deletions
WalkthroughOptimizations to RailroadLayer improve rendering performance through a rail tile indexing system for efficient removals, throttled color updates with frame-based rate limiting, and frustum-based visibility culling to draw only the visible viewport portion of the rail canvas. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
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: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/RailroadLayer.ts (1)
54-87: Color scan throttling + chunking may make ownership color changes feel slow on large mapsThe throttling and chunking logic is correct, but combined they can make a full pass over
railTileListquite slow:
- You run
updateRailColorsat most once everyrailColorIntervalMs = 50ms.- Each call checks at most
ceil(railTileList.length / 120)tiles.At ~60 FPS this means you only run the scan about 20 times per second, so a full pass is ~
120 * 50ms = 6sregardless of frame rate. On big maps, a rail whose owner changed might take several seconds to recolor.If that delay is intentional, this is fine. If you want a tighter bound (for example, “recolor everything within ~2 seconds”), consider making the “120” derived from a target scan duration instead of a magic frame count:
- private readonly railColorIntervalMs = 50; + private readonly railColorIntervalMs = 50; + // Aim to touch each rail at least once within this many ms. + private readonly railColorFullScanTargetMs = 2000; @@ - const maxTilesPerFrame = Math.max( - 1, - Math.ceil(this.railTileList.length / 120), - ); + const scansPerFullPass = Math.max( + 1, + Math.round(this.railColorFullScanTargetMs / this.railColorIntervalMs), + ); + const maxTilesPerFrame = Math.max( + 1, + Math.ceil(this.railTileList.length / scansPerFullPass), + );This keeps the performance win while making the “how long until all rails are refreshed?” behavior explicit and easy to tune.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/RailroadLayer.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/client/graphics/layers/RailroadLayer.ts
📚 Learning: 2025-11-26T22:27:31.844Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>
Applied to files:
src/client/graphics/layers/RailroadLayer.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/RailroadLayer.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/RailroadLayer.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🎨 Prettier
🔇 Additional comments (3)
src/client/graphics/layers/RailroadLayer.ts (3)
28-32: Rail index map and color throttling fields look goodUsing
railTileIndexfor O(1) lookup andlastRailColorUpdate/railColorIntervalMsto gate work is a clean way to support the later optimizations. No issues from a correctness point of view.
117-135: Viewport culling and partial draw are consistent; please confirm coordinate spacesThe early return for
existingRailroads.size === 0and the visible-rect culling (visLeft/visTop/visRight/visBottom) are correct and well clamped. Mapping:
vis*in game units →src*scaled by* 2for the high‑DPI canvas- Destination rectangle using
-game.width()/2 + visLeft/visTopandvisWidth/visHeightis internally consistent and should draw only the visible slice.
The only thing to double‑check is that
screenBoundingRect()returns coordinates in the same “tile space” asgame.width()/game.height(). If it were in pixels already, the* 2factor would be off. Assuming it is tile space, this looks solid.Also applies to: 141-155
184-187: O(1) tile removal and cursor maintenance look correctThe new index bookkeeping and
removeRailTilelogic are sound:
- On insert you set
railTileIndexto the currentrailTileList.lengthand thenpush, so the index always points at the added element.- On removal you:
- Look up
idxfromrailTileIndexand return early if the tile was never indexed.- Swap the last tile into
idx, update its index inrailTileIndex, thenpopand delete the removed tile’s index.- Clamp
nextRailIndexToCheckback into range and decrement it when removing an element at or before the current cursor, so iteration overrailTileListdoes not skip entries.I walked through edge cases (single element, removing last, removing before and at the cursor); they all keep the list and map in sync and keep the cursor within bounds, at the cost of some benign re-checks, which is fine.
No changes needed here.
Also applies to: 190-197, 216-238
| const now = performance.now(); | ||
| if (now - this.lastRailColorUpdate < this.railColorIntervalMs) { | ||
| return; | ||
| } | ||
| this.lastRailColorUpdate = now; |
There was a problem hiding this comment.
The rest of this function benefits from a greater framerate to draw the new railroad over time rather than all at once, so throttling it seems counter-intuitive here.
Do you have data showing it's increasing performances?
There was a problem hiding this comment.
My goal was to reduce the main thread's workload per frame when the ownership of the rails doesn't change frequently.
Also, the data I collected (in firefox) with the performance overlay is as follows:
Before:
{"timestamp":"2025-12-06T00:01:11.188Z","fps":{"current":8,"average60s":9,"frameTimeMs":122,"history":[10,10,10,10,9,10,10,10,10,9,8,8,8,8,8,8,8,8,9,10,9,8,8,8,8,8,8,9,9,8,8,8,9,9,9,9,9,8,9,9,8,9,8,8,9,9,9,8,9,8,8,8,9,9,9,9,9,9,8,8]},"ticks":{"executionAvgMs":25.92,"executionMaxMs":50,"delayAvgMs":190.2,"delayMaxMs":282,"executionSamples":[18,22,24,17,21,16,18,23,17,18,20,24,17,23,17,35,30,24,18,20,23,24,22,32,33,33,37,39,40,41,37,39,49,35,49,37,45,42,50,21,21,21,20,17,20,20,18,20,19,22,20,34,19,20,21,19,18,19,20,17],"delaySamples":[124,138,148,131,123,153,125,135,144,120,138,145,133,123,149,127,253,248,230,220,160,222,239,232,241,169,206,223,249,218,231,225,254,226,273,282,199,220,238,245,212,177,199,167,216,160,198,174,204,161,221,155,264,221,164,221,220,169,131,119]},"layers":[{"name":"RailroadLayer","avg":50.29442780702114,"max":76,"total":29503}]}After:
{"timestamp":"2025-12-06T00:04:04.363Z","fps":{"current":36,"average60s":37,"frameTimeMs":28,"history":[28,41,42,39,38,38,40,40,43,43,43,42,40,43,38,37,36,37,37,37,36,37,38,36,38,38,36,36,34,37,36,37,36,34,35,36,36,37,39,37,36,36,36,36,35,36,36,35,36,36,34,36,36,36,36,36]},"ticks":{"executionAvgMs":21.7,"executionMaxMs":93,"delayAvgMs":114.35,"delayMaxMs":148,"executionSamples":[18,22,27,21,25,20,24,17,22,22,20,29,27,20,19,17,22,23,17,19,24,17,19,28,16,93,25,24,23,21,19,27,18,20,14,18,23,21,25,14,16,35,26,19,18,23,17,17,18,17,18,23,20,24,15,15,14,16,18,13],"delaySamples":[121,122,124,111,118,118,114,114,118,113,118,109,118,109,111,110,114,111,109,113,117,117,113,114,117,110,148,102,110,111,116,107,112,116,111,110,123,109,115,109,114,111,114,109,111,112,111,110,113,113,118,118,115,110,114,109,110,112,141,114]},"layers":[{"name":"RailroadLayer","avg":2.0065826048887067e-23,"max":1,"total":8}]}There was a problem hiding this comment.
Thanks for the data.
I don't doubt those are correct but on my computer the performances are similar to your after data, even on very large rail networks.
It's worrying that the layer could have such poor performances.
| if (this.railTileList.length === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nitpick: early return could be slightly earlier
| } else if ( | ||
| idx <= this.nextRailIndexToCheck && | ||
| this.nextRailIndexToCheck > 0 | ||
| ) { | ||
| // Keep iteration stable when removing an element before the current cursor | ||
| this.nextRailIndexToCheck--; | ||
| } |
There was a problem hiding this comment.
I don't think this is necessary, as it will just recheck the last checked rail, which is the least probable to have changed.
DevelopingTom
left a comment
DevelopingTom
left a comment
There was a problem hiding this comment.
Looks good to me. If you could address the few comments / update, it will be approved.
2 participants
Description:
Render optimizations were applied for RailroadLayer: limiting color updates, skipping off-screen work, drawing only the visible region, and preventing O(n) ray tile removals.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
wraith4081