Highlight hovering railroad#3156
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors railroad handling to event-driven updates: adds RailroadView for per-tile direction and incremental drawing, splits updates into Construction/Snap/Destruction with per-tile RailTileChangedEvent, integrates overlappingRailroads into UIState/build flow, removes RailroadExecution, and updates RailroadLayer, FxLayer, and core railroad models. Changes
Sequence Diagram(s)sequenceDiagram
participant RailNetwork as RailNetworkImpl
participant Game as Game (update bus)
participant EventBus as EventBus
participant RailroadLayer as RailroadLayer
participant FxLayer as FxLayer
participant UI as UIState
RailNetwork->>Game: emit RailroadConstructionEvent / RailroadSnapEvent / RailroadDestructionEvent
Game->>EventBus: publish game update
EventBus->>RailroadLayer: deliver construction/snap/destruction update
RailroadLayer->>RailroadLayer: create/update RailroadView, start pending animation
RailroadLayer->>EventBus: emit RailTileChangedEvent (per tile painted)
EventBus->>FxLayer: deliver RailTileChangedEvent(tile)
FxLayer->>FxLayer: spawn Dust FX at tile coords
RailroadLayer->>UI: read UIState.overlappingRailroads to draw highlights
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/RailroadLayer.ts (1)
221-235:⚠️ Potential issue | 🔴 CriticalBug: highlight is painted before the railroad image, so it will be hidden underneath.
highlightOverlappingRailroadsdraws green rectangles oncontextat line 223, thendrawImagepaints the off-screen railroad canvas on top at line 224. The green overlay ends up behind the railroad sprites — users will not see it.Move the highlight call to after
drawImageso the green overlay is visible on top.Proposed fix
context.save(); context.globalAlpha = alpha; - this.highlightOverlappingRailroads(context); context.drawImage( this.canvas, srcX, srcY, srcW, srcH, dstX, dstY, visWidth, visHeight, ); + this.highlightOverlappingRailroads(context); context.restore();
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RailroadView.ts`:
- Around line 1-2: The imports in RailroadView.ts use bare "src/..." paths;
update them to project-relative imports by replacing the top-level imports for
TileRef and GameView (the lines importing TileRef from "src/core/game/GameMap"
and GameView from "src/core/game/GameView") with the correct relative paths
(e.g., "../../core/game/GameMap" and "../../core/game/GameView" or the relative
path that matches this file's directory) so the file uses relative module paths
consistent with FxLayer.ts and the rest of the project.
- Around line 18-49: computeRailTiles crashes for 0 or 1 element arrays because
it unconditionally indexes neighbors; update computeRailTiles to early-return or
guard short arrays: if tiles.length === 0 return []; if tiles.length === 1
return [{ tile: tiles[0], type: RailType.VERTICAL }] (or otherwise avoid passing
undefined into computeExtremityDirection). Also adjust the two extremity
branches that call computeExtremityDirection (the first push and the final push)
to only reference tiles[1] or tiles[tiles.length-2] when tiles.length > 1 so
computeExtremityDirection and computeDirection never receive undefined.
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 336-344: The uiState.overlappingRailroads array is left stale when
hovering unbuildable tiles or when there is no unit, so update
StructureIconsLayer to clear uiState.overlappingRailroads in both the
early-return path where no unit is found and inside the unit.canBuild === false
branch; specifically, in the method handling hover/ghost updates (referencing
unit, ghostUnit, and uiState.overlappingRailroads), set
uiState.overlappingRailroads = [] before returning when unit is null/undefined
and also assign an empty array in the canBuild === false branch alongside
applying the red OutlineFilter.
In `@src/core/game/RailNetwork.ts`:
- Line 11: The interface method overlappingRailroads lacks a return type and
should be declared to return number[]; update the signature of
overlappingRailroads(tile: TileRef) to overlappingRailroads(tile: TileRef):
number[] so implementations and callers (e.g., the assignment in PlayerImpl
where the result is stored into a number[] field) have correct static typing.
🧹 Nitpick comments (6)
tests/core/game/TrainStation.test.ts (1)
99-120: Test mock is missing theidfield onrailRoad.The
railRoadmock (line 103–107) does not include anidproperty, butremoveNeighboringRailsreadstoRemove.id(seeTrainStation.ts). The assertion only checkstype, so it passes — but the emitted update hasid: undefined. Consider addingidto the mock and asserting on it for a stronger test.Suggested improvement
const railRoad = { from: stationA, to: stationB, tiles: [{ x: 1, y: 1 }], + id: 42, } as any;expect(game.addUpdate).toHaveBeenCalledWith( expect.objectContaining({ type: GameUpdateType.RailroadDestructionEvent, + id: 42, }), );src/client/graphics/layers/RailroadView.ts (1)
74-113:computeDirectionfalls through to the warning for diagonal straight-line movement.If
prev → current → nextis a straight diagonal (e.g.,dx1=1, dy1=1, dx2=1, dy2=1), the straight-line block (line 93) is entered but neitherdx1 !== 0triggersHORIZONTALnordy1 !== 0triggersVERTICAL— it falls through, then the turn block condition (line 99) isfalse, and you hit the warning on line 111.If the pathfinder only produces cardinal moves, this is not reachable. But the fallback to
VERTICALwith aconsole.warnis a safe default.Optional: handle straight diagonals explicitly
// Straight line if (dx1 === dx2 && dy1 === dy2) { if (dx1 !== 0) return RailType.HORIZONTAL; if (dy1 !== 0) return RailType.VERTICAL; + // Diagonal straight line — shouldn't happen with cardinal pathing + return RailType.VERTICAL; }src/client/graphics/layers/FxLayer.ts (1)
1-2: Import style is inconsistent but not a build issue.Lines 1–2 use bare
src/paths while lines 3–8 use relative paths. The tsconfig hasbaseUrl: "."with bundler module resolution, so both styles resolve correctly and the codebase widely adopts thesrc/pattern (seen in MapLandTiles.ts, NukeTelegraph.ts, NavalTarget.ts, RailroadView.ts, and others). For consistency within this file, align all imports to one style—either all relative or all baresrc/.src/client/graphics/layers/RailroadLayer.ts (3)
167-187: Early return whenoverlappingRailroadsisundefined— but the type saysnumber[].Line 170 checks
this.uiState.overlappingRailroads === undefined, yet theUIStateinterface declares it asnumber[](not optional). If this field can truly be absent at runtime, the type should reflect that. Otherwise, just check.length === 0and drop theundefinedguard.Also a minor note: consider using a distinct highlight color or outline rather than a filled green rectangle, so the railroad sprites remain visible under the overlay. Right now,
rgba(0, 255, 0, 0.4)with a2.5 × 2.5fill may obscure small rail details.
238-270: Snap handler uses dummyRailType.HORIZONTALfor all tiles — fragile if these views are used beyond ID tracking.The comment explains the tiles are already painted, so direction does not matter. That holds today because
highlightOverlappingRailroadsonly reads.tileandremoveRailroadcallsclearRailroadwhich also ignores direction. But any future code that iteratesdrawnTiles()and relies on.typewill silently get wrong data.A short inline comment on lines 251–258 noting why the type is throwaway would help future readers. Low priority — just a readability note.
372-394:paintRailis public — is that intentional?All other paint methods (
paintRailTile,paintRailRects,paintBridge) areprivate.paintRailhas no access modifier, so it defaults to public in TypeScript. If nothing outside the class calls it, mark itprivatefor consistency.Proposed fix
- paintRail(railTile: RailTile) { + private paintRail(railTile: RailTile) {
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
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/RailNetworkImpl.ts (1)
138-200:⚠️ Potential issue | 🟡 MinorSnap logic looks correct — one edge case to verify at boundary index
The snap flow correctly:
- Preserves the
originalIdbefore unregistering the old rail- Creates two new segments with fresh IDs
- Re-wires stations and the spatial grid
- Emits
RailroadSnapEventwith all needed dataOne thing to double-check: when
closestRailIndex === rail.tiles.length - 1(last tile),newRailTogets a single-tile segment (rail.tiles.slice(closestRailIndex)→[lastTile]). A 1-tile railroad betweenstationandtomay cause odd rendering or train pathing. The guard on line 150 filters=== 0but not the symmetric end case.Consider also filtering the last index
- if (closestRailIndex === 0 || closestRailIndex >= rail.tiles.length) { + if ( + closestRailIndex === 0 || + closestRailIndex >= rail.tiles.length - 1 + ) {
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RailroadView.ts`:
- Around line 62-70: The branch that handles diagonal neighbors currently falls
through to returning RailType.VERTICAL when dx !== 0 && dy !== 0; update the
function (the block that inspects dx and dy and returns RailType constants) to
explicitly handle diagonals: either determine and return the correct diagonal
rail sprite if available, or at minimum emit a warning (matching the style of
computeDirection) when dx !== 0 && dy !== 0 and then return a clear default; use
the dx/dy check and the RailType enum names (e.g., RailType.VERTICAL /
RailType.HORIZONTAL) to locate the logic and add the warning/log call and/or the
explicit diagonal case so diagonal steps are not silently treated as VERTICAL.
🧹 Nitpick comments (3)
src/client/graphics/layers/RailroadView.ts (2)
90-94: Straight-line check does not return when both deltas are zero.If two consecutive tiles are identical (
dx1 === 0 && dy1 === 0), the condition on line 91 is true but neither innerifmatches, so execution falls through to the corner-case block and eventually hits the warning on line 109. This is a minor edge case (duplicate tiles should not appear in practice), but adding an explicit guard would make the intent clearer.Suggested guard
// Straight line if (dx1 === dx2 && dy1 === dy2) { + if (dx1 === 0 && dy1 === 0) { + console.warn("Duplicate tiles in rail segment"); + return RailType.VERTICAL; + } if (dx1 !== 0) return RailType.HORIZONTAL; if (dy1 !== 0) return RailType.VERTICAL; }
145-153:drawnTiles()creates new arrays on every call.
slice+concatallocates two intermediate arrays each call. If this is called frequently (every render frame), consider caching the result or using a single-pass approach. Not urgent — just noting for future optimization if profiling shows it matters.src/core/game/RailNetworkImpl.ts (1)
202-206: RenameoverlappingRailroadstonearbyRailroadsfor clarityThe method returns railroads within a 3-tile radius, not railroads that overlap the exact tile. The name
overlappingRailroadsis misleading. Since the radius (3 tiles) matches the train station connection distance and is used for UI hints during placement preview, renaming tonearbyRailroadswould accurately reflect the actual behavior.
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/client/graphics/layers/RailroadLayer.ts (1)
229-243:⚠️ Potential issue | 🟠 MajorHighlight is drawn before the railroad canvas is composited — it ends up behind the rails.
highlightOverlappingRailroadspaints directly ontocontextat line 231, thendrawImagepaints the offscreen rail canvas on top at line 232. The green overlay will be occluded by the railroad pixels. If the intent is for the highlight to be visible on top of the rails, move the call afterdrawImage.context.save(); context.globalAlpha = alpha; context.drawImage( this.canvas, srcX, srcY, srcW, srcH, dstX, dstY, visWidth, visHeight, ); + this.highlightOverlappingRailroads(context); context.restore(); - this.highlightOverlappingRailroads(context);If the "glow behind" effect is intentional, please add a short comment so future readers know.
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RailroadLayer.ts`:
- Around line 259-275: The submitted snap code creates RailroadView instances
for update.newId1/update.newId2 using placeholder RailType.HORIZONTAL for every
tile (from update.tiles1/update.tiles2), which can silently break later code
paths that read tile.type (e.g., drawnTiles() used by
highlightOverlappingRailroads or removeRailroad); change the creation to compute
real directions for each tile (call computeRailTiles or equivalent to produce
correct RailTile.type entries) before constructing new RailroadView, or if
computing directions is impossible here, add a clear doc comment on RailroadView
construction that snapped views must not have their tile.type read and ensure
removeRailroad/clearRailroad only rely on TileRef rather than type.
- Around line 316-334: The existing paintRailTile silently updates stored
direction by assigning railRef.tile = railTile without repainting, causing the
offscreen canvas to retain the old direction; in paintRailTile, before
overwriting railRef.tile compare the previous tile/direction (railRef.tile) to
the incoming railTile and if they differ call paintRail(railTile) after updating
the record (and still increment railRef.numOccurence and set lastOwnerId) so the
visual and bookkeeping stay in sync; use the existing symbols paintRailTile,
paintRail, existingRailroads, railRef, numOccurence, and lastOwnerId to locate
and implement this change.
🧹 Nitpick comments (5)
src/client/graphics/layers/RailroadLayer.ts (5)
171-195: Highlight rect size (2.5 × 2.5) does not match the offscreen canvas 2× scale.Rail tiles are painted on the offscreen canvas at
x * 2,y * 2coordinates. But the highlightfillRectat line 191 uses game-space coordinates (x + offsetX) with a 2.5 × 2.5 rect. Since the main context is in game-space (the transform handler scales it), the 2.5 size may be correct visually — but it is a magic number that is easy to get wrong. A short inline comment explaining the chosen size relative to the tile scale would help future readers.
84-104: Deleting from aSetwhile iterating — works per spec but worth a note.JavaScript guarantees safe deletion during
for...ofon aSet, so this is correct. A one-line comment would prevent a future contributor from "fixing" it.
34-36:RailTileChangedEventis a simple value event — clean and minimal.One small note: this class is exported from a layer file, which means other layers (like
FxLayer) import from a sibling layer. If more events accumulate here, consider a sharedevents.tsto avoid circular or awkward cross-layer imports.
380-402:paintRailis public (noprivatekeyword) — is that intentional?Every other paint helper in this class is
private. If nothing outside the class callspaintRail, mark itprivatefor consistency and to shrink the public surface.Proposed fix
- paintRail(railTile: RailTile) { + private paintRail(railTile: RailTile) {
29-33: Export the snappable structure list from a shared module to keep client and core in sync.The client layer defines
SNAPPABLE_STRUCTURESat lines 29–33, but the core layer inRailNetworkImpl.ts(line 212) has an equivalent list[UnitType.City, UnitType.Factory, UnitType.Port]used for train station snapping logic. These lists already show drift (different order: Port, City, Factory vs City, Factory, Port). Move the canonical list to a shared module and import it in both places.
| const directions1: RailTile[] = update.tiles1.map((tile) => ({ | ||
| tile, | ||
| type: RailType.HORIZONTAL, | ||
| })); | ||
| const directions2: RailTile[] = update.tiles2.map((tile) => ({ | ||
| tile, | ||
| type: RailType.HORIZONTAL, | ||
| })); | ||
| // The rails are already painted, consider them complete | ||
| this.railroads.set( | ||
| update.newId1, | ||
| new RailroadView(update.newId1, directions1, true), | ||
| ); | ||
| this.railroads.set( | ||
| update.newId2, | ||
| new RailroadView(update.newId2, directions2, true), | ||
| ); |
There was a problem hiding this comment.
Snap creates RailroadViews with fake RailType.HORIZONTAL for every tile.
The comment says "the rails are already painted" so directions do not matter now. But these RailroadView instances are stored in this.railroads and later used by drawnTiles() in highlightOverlappingRailroads and removeRailroad. If removeRailroad is ever called on a snapped railroad and the removal path starts reading type (e.g. to clear bridge rects of a specific orientation), the wrong direction will produce incorrect clearing.
Even today, if a snapped railroad is later destroyed, clearRailroad only uses TileRef so it works — but this is fragile. Consider computing actual directions via computeRailTiles for the two new segments, or at minimum add a clear doc comment explaining the invariant that type on snapped views must never be read.
🤖 Prompt for AI Agents
In `@src/client/graphics/layers/RailroadLayer.ts` around lines 259 - 275, The
submitted snap code creates RailroadView instances for
update.newId1/update.newId2 using placeholder RailType.HORIZONTAL for every tile
(from update.tiles1/update.tiles2), which can silently break later code paths
that read tile.type (e.g., drawnTiles() used by highlightOverlappingRailroads or
removeRailroad); change the creation to compute real directions for each tile
(call computeRailTiles or equivalent to produce correct RailTile.type entries)
before constructing new RailroadView, or if computing directions is impossible
here, add a clear doc comment on RailroadView construction that snapped views
must not have their tile.type read and ensure removeRailroad/clearRailroad only
rely on TileRef rather than type.
| private paintRailTile(railTile: RailTile) { | ||
| const currentOwner = this.game.owner(railTile.tile)?.id() ?? null; | ||
| const railRef = this.existingRailroads.get(railTile.tile); | ||
|
|
||
| if (railTile) { | ||
| railTile.numOccurence++; | ||
| railTile.tile = railRoad; | ||
| railTile.lastOwnerId = currentOwner; | ||
| if (railRef) { | ||
| railRef.numOccurence++; | ||
| railRef.tile = railTile; | ||
| railRef.lastOwnerId = currentOwner; | ||
| } else { | ||
| this.existingRailroads.set(railRoad.tile, { | ||
| tile: railRoad, | ||
| this.existingRailroads.set(railTile.tile, { | ||
| tile: railTile, | ||
| numOccurence: 1, | ||
| lastOwnerId: currentOwner, | ||
| }); | ||
| this.railTileIndex.set(railRoad.tile, this.railTileList.length); | ||
| this.railTileList.push(railRoad.tile); | ||
| this.paintRail(railRoad); | ||
| this.railTileIndex.set(railTile.tile, this.railTileList.length); | ||
| this.railTileList.push(railTile.tile); | ||
| this.paintRail(railTile); | ||
| } | ||
| } |
There was a problem hiding this comment.
Overlapping tile skips repaint — stored direction silently diverges from what is painted.
When two railroads share a tile, the second call to paintRailTile increments numOccurence and overwrites railRef.tile (new direction) but does not call paintRail. The visual on the offscreen canvas still shows the first railroad's direction while the bookkeeping now stores the second one. This mismatch can cause a wrong sprite on the next color-update repaint (line 132 calls paintRail(railRef.tile) with the silently-replaced direction).
Consider repainting when the direction changes:
Proposed fix
if (railRef) {
railRef.numOccurence++;
+ const directionChanged = railRef.tile.type !== railTile.type;
railRef.tile = railTile;
railRef.lastOwnerId = currentOwner;
+ if (directionChanged) {
+ this.paintRail(railTile);
+ }
} else {🤖 Prompt for AI Agents
In `@src/client/graphics/layers/RailroadLayer.ts` around lines 316 - 334, The
existing paintRailTile silently updates stored direction by assigning
railRef.tile = railTile without repainting, causing the offscreen canvas to
retain the old direction; in paintRailTile, before overwriting railRef.tile
compare the previous tile/direction (railRef.tile) to the incoming railTile and
if they differ call paintRail(railTile) after updating the record (and still
increment railRef.numOccurence and set lastOwnerId) so the visual and
bookkeeping stay in sync; use the existing symbols paintRailTile, paintRail,
existingRailroads, railRef, numOccurence, and lastOwnerId to locate and
implement this change.
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
## Description: Based on [this suggestion on Discord](https://discord.com/channels/1284581928254701718/1447110257196138577) and feedback gathered in [this thread](https://discord.com/channels/1359946986937258015/1469598906173227184). Supersedes #3143 This PR introduces "ghost railways": when you are going to place a city or port, previews railway connections that will be made when actually building the structure. Ghost railways are skipped if the structure is going to be snapped to existing railways (as in railway snapping functionality introduced in #3156 ). ### Video https://github.com/user-attachments/assets/ff8cf325-6501-4df8-801d-c8ae3ced3d0e ### Ghost rails color revisited black with 40% opacity <img width="695" height="430" alt="image" src="https://github.com/user-attachments/assets/272efbcc-4185-426a-921c-7fae61f6c462" /> ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: deshack_82603
3 participants
Description:
The
RailroadLayersimply displays tiles as instructed by the core worker. While it's practical for the layer to only care about the tiles, it also means it has no understanding of railroads as entities (their paths, connections, or identities).It also means that the core worker is responsible for rendering tasks such as tile orientation and construction animation, which is not expected.
To support ID-based events and better separation of concerns, the rendering layer needs to be aware of complete railroads. With this change, the core worker can send the tiles once and subsequently reference railroads only by ID for all other events.
Changes:
RailroadLayernow stores full railroad data instead of only individual tilesRailroadLayeris responsible for animating newly built railroadsAdd a new
RailroadSnapUpdatesent when a new structure is built over an existing railroad. This event is used byRailroadLayerto keep railroad ID in sync.When hovering over a railroad, the render worker is querying the core worker about overlapping railroads.
Alternatively, RailroadLayer could compute overlaps itself now that it has full railroad knowledge, but this logic would need to be duplicated and kept in sync across workers. Keeping a single source of truth in the core worker is preferred.
Edgecases:
RailroadUpdatehandled both construction and destruction. This no longer works withRailroadSnapUpdate, as event ordering is now pretty important and IDs may be lost before they are consumed.To address this, RailroadUpdate is split in two:
RailroadConstructionUpdateandRailroadDestructionUpdate.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
IngloriousTom