WebGL: return of factory/defence post radii, and railroad highlighting when placing city/port right on top#3981
Merged
Merged
Conversation
… structures from build bar (unitdisplay). Show when city/port is placed directly over existing railroad, by highlighting the railroad green. Both changes emulate how it worked before the move to WebGL. OverlappingRailroads now returns TileRefs instead of a railroad ID, and it does so with less allocations than the previous code.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
WalkthroughRail-overlap APIs and types were migrated from numeric IDs to TileRef arrays across network, model, and renderer layers. Build preview radius selection was refactored to a switch and now handles Factory and DefensePost via game.config(); UIState rail preview fields were removed and tests updated. ChangesRail overlap type refinement and range radius extension
Sequence Diagram(s)sequenceDiagram
participant Client
participant BuildPreviewController
participant GameConfig as Game.config
participant Renderer
Client->>BuildPreviewController: buildGhostPreviewData(unit)
BuildPreviewController->>GameConfig: trainStationMaxRange()/defensePostRange() (for Factory/DefensePost)
GameConfig-->>BuildPreviewController: radius value
BuildPreviewController->>Renderer: updateGhostPreview / updateNukeTrajectory
Renderer-->>Client: rendered preview / trajectory
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/controllers/BuildPreviewController.ts (1)
339-341: ⚡ Quick winFactory range uses
trainStationMaxRange()on purpose (no factory-specific config exists)
src/core/configuration/Config.tsdefines onlytrainStationMinRange()/trainStationMaxRange(); nofactory*Rangemethods exist.- Core logic passes
trainStationMaxRange()forUnitType.Factory(e.g.,src/core/execution/FactoryExecution.ts,src/core/execution/PortExecution.ts,src/core/game/RailNetworkImpl.ts).- Naming is confusing—consider a clearer alias/comment that “factory uses train-station range”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/controllers/BuildPreviewController.ts` around lines 339 - 341, The switch case for UnitType.Factory sets rangeRadius from this.game.config().trainStationMaxRange() but that reuse is intentional; clarify intent by adding a concise inline comment next to the UnitType.Factory case stating that factories share the train-station range, and (optionally) create a descriptive alias in Config like factoryMaxRange() that forwards to trainStationMaxRange() and replace the direct call with this.game.config().factoryMaxRange() so callers (e.g., the case handling rangeRadius and any uses in FactoryExecution/PortExecution/RailNetworkImpl) are self-documenting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/client/controllers/BuildPreviewController.ts`:
- Around line 339-341: The switch case for UnitType.Factory sets rangeRadius
from this.game.config().trainStationMaxRange() but that reuse is intentional;
clarify intent by adding a concise inline comment next to the UnitType.Factory
case stating that factories share the train-station range, and (optionally)
create a descriptive alias in Config like factoryMaxRange() that forwards to
trainStationMaxRange() and replace the direct call with
this.game.config().factoryMaxRange() so callers (e.g., the case handling
rangeRadius and any uses in FactoryExecution/PortExecution/RailNetworkImpl) are
self-documenting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51b6725b-c2a9-480a-93c0-859abafb553e
📒 Files selected for processing (1)
src/client/controllers/BuildPreviewController.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/game/RailNetworkImpl.ts (2)
226-232:⚠️ Potential issue | 🟠 MajorMake
overlappingRailroads()ordering deterministic + add core tests
RailSpatialGrid.query()uses deterministic nested loops, butoverlappingRailroads()concatenatesrailroad.tilesin the iteration order ofSet<Railroad>returned byrailGrid.query(). That order ultimately depends on Set insertion order fromregister()(rail registration order /rail.tilesiteration order), and there’s no sorting when producing theTileRef[].Also,
tests/core/game/RailNetwork.test.tsdoesn’t coveroverlappingRailroads()(onlycomputeGhostRailPaths), and the onlyoverlappingRailroadsoccurrences in tests are UI fixtures set to[]. Add a core unit test asserting the returnedTileRef[]content (and, if order matters, order) foroverlappingRailroads().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/game/RailNetworkImpl.ts` around lines 226 - 232, overlappingRailroads() currently concatenates railroad.tiles in the iteration order of the Set returned by railGrid.query(), which makes results non-deterministic; change overlappingRailroads(tile: TileRef) to produce a deterministic, deduplicated TileRef[] by collecting tiles from each railroad returned by this.railGrid.query(tile, this.stationRadius) into a canonical holder (e.g., a Set keyed by tile coordinates), then sort the resulting TileRef array by a stable key (e.g., y then x or a single index) before returning; update references to overlappingRailroads and add a core unit test in tests/core/game/RailNetwork.test.ts that constructs a known configuration, calls RailNetworkImpl.overlappingRailroads, and asserts the returned TileRef[] contains the expected tiles (and the expected order if order is meaningful).
226-232:⚠️ Potential issue | 🟠 MajorAdd unit tests for
RailNetworkImpl.overlappingRailroads(TileRef[] contract).
overlappingRailroadsinsrc/core/game/RailNetworkImpl.tshas no direct unit test coverage; existing test hits only mock the UI state fieldoverlappingRailroads: [], not the method behavior. Add tests that assert the returnedTileRef[]flattening fromrailGrid.query(tile, stationRadius), including multiple overlapping railroads and empty-results cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/game/RailNetworkImpl.ts` around lines 226 - 232, Add unit tests for RailNetworkImpl.overlappingRailroads to verify it returns a flattened TileRef[] from railGrid.query(tile, this.stationRadius); specifically, write tests that (1) stub or mock the instance's railGrid.query to return multiple railroad objects each with a tiles array and assert overlappingRailroads(tile) concatenates them in order, and (2) stub railGrid.query to return an empty array and assert overlappingRailroads(tile) returns an empty array; reference the method overlappingRailroads(tile: TileRef) and the instance property stationRadius when setting up the RailNetworkImpl instance and mocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/game/RailNetworkImpl.ts`:
- Around line 226-232: overlappingRailroads() currently concatenates
railroad.tiles in the iteration order of the Set returned by railGrid.query(),
which makes results non-deterministic; change overlappingRailroads(tile:
TileRef) to produce a deterministic, deduplicated TileRef[] by collecting tiles
from each railroad returned by this.railGrid.query(tile, this.stationRadius)
into a canonical holder (e.g., a Set keyed by tile coordinates), then sort the
resulting TileRef array by a stable key (e.g., y then x or a single index)
before returning; update references to overlappingRailroads and add a core unit
test in tests/core/game/RailNetwork.test.ts that constructs a known
configuration, calls RailNetworkImpl.overlappingRailroads, and asserts the
returned TileRef[] contains the expected tiles (and the expected order if order
is meaningful).
- Around line 226-232: Add unit tests for RailNetworkImpl.overlappingRailroads
to verify it returns a flattened TileRef[] from railGrid.query(tile,
this.stationRadius); specifically, write tests that (1) stub or mock the
instance's railGrid.query to return multiple railroad objects each with a tiles
array and assert overlappingRailroads(tile) concatenates them in order, and (2)
stub railGrid.query to return an empty array and assert
overlappingRailroads(tile) returns an empty array; reference the method
overlappingRailroads(tile: TileRef) and the instance property stationRadius when
setting up the RailNetworkImpl instance and mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb5e10c8-516b-4a95-8fbe-586c291d2dd5
📒 Files selected for processing (1)
src/core/game/RailNetworkImpl.ts
Cleanup obsolete Canvas2D rail highlighting state that was superseded by GhostPreviewData.
…eeded, so we could revert it back to just outputting an unsorted array, but maybe in case we ever do decync checks on its outcome this is better. Fix UIState mock in InputHandler tests Add tests in RailNetwork.test.ts Remove overlappingRailroads and ghostRailPaths initialization from tests after they were removed from the UIState interface.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/game/RailNetwork.test.ts`:
- Around line 171-197: The tests currently mutate the private railGrid and use
vi.fn mocks; instead create a real game via the setup() helper and exercise
public APIs so overlappingRailroads is tested through normal simulation: call
setup() from tests/util/Setup.ts to build a game instance, obtain the
RailNetwork instance (the same object referenced as network in the test) via the
public game/core accessor, use public game actions or helper utilities to
place/define rail segments on tiles that produce overlapping railroads (instead
of injecting railGrid or stubbing query), then call
network.overlappingRailroads(tile) and assert the deterministic deduplicated
result; remove direct assignment to (network as any).railGrid and any vi.fn
usage so the test operates through public behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac669c9a-e484-416e-bdf0-a59445cab296
📒 Files selected for processing (4)
src/client/UIState.tssrc/core/game/RailNetworkImpl.tstests/InputHandler.test.tstests/core/game/RailNetwork.test.ts
💤 Files with no reviewable changes (1)
- src/client/UIState.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
approved these changes
May 22, 2026
github-project-automation
Bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
Bot
moved this from Final Review
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Show factory and defence post radius for ghost structure when placing structures from build bar (unitdisplay).
Show when city/port is placed directly over existing railroad, by highlighting the railroad green. The railroad is not highlighted when instead a city/port nearby the ghost structure will be upgraded instead of placing it on the railroad. This works with the existing code in buildableUnits in PlayerImpl: it would already return an empty array [] for overlappingRailroads and for ghostRailPaths when canUpgrade is false. So the old checks for uiState for Canvas2D in BuildPreviewController weren't even needed per se, they followed the same logic as buildableUnits in PlayerImpl already did.
Both changes emulate how it worked before the move to WebGL.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33