Impassible terrain acts like map edge - #4922
Conversation
WalkthroughThe change updates impassable-terrain classification, map-edge detection, conquest validation, rail traversal, and related tests. ChangesImpassable terrain behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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: 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/pathfinding/algorithms/AStar.Rail.ts (1)
55-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject impassable destination tiles.
GameMap.isLand()now returns false for impassable tiles. Therefore,isWater(to)returns true for those tiles. WhenfromShorelineis true,isTraversable()accepts the impassable destination.The rail graph can then enter impassable terrain. Check
isImpassable(to)before applying the water rule.Suggested fix
private isTraversable(to: number, fromShoreline: boolean): boolean { + if (this.gameMap.isImpassable(to)) return false; const toWater = this.gameMap.isWater(to); if (!toWater) return true; return fromShoreline || this.gameMap.isShoreline(to); }Add a regression test with a shoreline source and an adjacent impassable destination. As per coding guidelines, every change in
src/core/must include tests.🤖 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/pathfinding/algorithms/AStar.Rail.ts` around lines 55 - 80, Update AStar.Rail’s isTraversable method to reject gameMap.isImpassable(to) before applying the water and shoreline rules, including when fromShoreline is true. Add a regression test covering a shoreline source with an adjacent impassable destination, and ensure the rail graph does not include that destination.Source: Coding guidelines
🤖 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 `@src/core/game/GameMap.ts`:
- Around line 210-217: Update terrainType() to evaluate isImpassable() before
isLand(), ensuring magnitude-31 tiles classify as impassable rather than Ocean.
In updateTile(), use the same passable-land predicate as isLand() when adjusting
numLandTiles_, including passable-to-impassable transitions. Add regression
tests covering both terrain classification and land-count updates.
---
Outside diff comments:
In `@src/core/pathfinding/algorithms/AStar.Rail.ts`:
- Around line 55-80: Update AStar.Rail’s isTraversable method to reject
gameMap.isImpassable(to) before applying the water and shoreline rules,
including when fromShoreline is true. Add a regression test covering a shoreline
source with an adjacent impassable destination, and ensure the rail graph does
not include that destination.
🪄 Autofix
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 Plus
Run ID: d820b799-2068-4b04-b199-9f80cf8a9f17
📒 Files selected for processing (2)
src/core/game/GameMap.tssrc/core/pathfinding/algorithms/AStar.Rail.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
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 (1)
src/core/game/GameMap.ts (1)
210-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
numLandTiles_aligned withisLand().
isLand()now excludes magnitude-31 tiles, butupdateTile()still calculatesisNowLandfrom onlyIS_LAND_BITat Lines [547]-[551]. A passable-land to impassable transition does not decrementnumLandTiles_. The reverse transition increments it incorrectly.Use
this.isLand(tile)after assigningterrainByte, and add tests for both transitions.🤖 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/GameMap.ts` around lines 210 - 223, Update updateTile() so isNowLand is computed with this.isLand(tile) after terrainByte is assigned, keeping numLandTiles_ consistent with isLand() for passable-to-impassable and impassable-to-passable transitions. Add tests covering both transitions and verifying the land-tile count.
🤖 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/GameMap.ts`:
- Around line 210-223: Update updateTile() so isNowLand is computed with
this.isLand(tile) after terrainByte is assigned, keeping numLandTiles_
consistent with isLand() for passable-to-impassable and impassable-to-passable
transitions. Add tests covering both transitions and verifying the land-tile
count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c635906e-4a41-470f-a66d-55a9d153e20a
📒 Files selected for processing (3)
src/core/game/GameImpl.tssrc/core/game/GameMap.tstests/ImpassableTerrain.test.ts
FloPinguin
left a comment
FloPinguin
left a comment
There was a problem hiding this comment.
Little AI Check
🔴 Critical issue — rails can now cross impassable terrain (reintroduces #4902)
The AStar.Rail.ts hunk removes the source-node isImpassable check from isTraversable(). That check was added by PR #4903 (same author, merged 2 days ago) specifically to stop rails from crossing impassable walls.
I proved this empirically with a throwaway test (wall map from the existing test file, rail path from one side to the other):
| Branch | Path crosses impassable? |
|---|---|
main (with #4903 fix) |
✅ No — 0 impassable tiles in path |
| PR #4922 | ❌ Yes — 2 impassable tiles in path |
The new isLand/isWater semantics don't compensate: impassable is not water, so isTraversable() returns true for impassable destinations, and with the source check gone impassable nodes are fully expandable by A*. The result is railroad tracks get built over impassable terrain again — exactly the bug #4903 fixed. This needs to be fixed before merge (re-add the source check, or add a destination-side isImpassable(to) guard) and needs a regression test.
🟠 Secondary issues
-
Misleading test name. The new test is named "isLand returns false for impassable (can't pathfind trains through it)" — but after this PR, trains can pathfind through it. The parenthetical is factually wrong and there's no actual rail test anywhere, which is how this regression slipped through 2810 tests.
-
isShorelinechange silently breaksWaterManagercleanup.WaterManager.updateWater()has a defensive branch that clears stale shoreline bits on impassable tiles:if (map.isImpassable(tile)) { if (map.isShoreline(tile)) { map.clearShorelineBit(tile); ... } }. BecauseisShorelinenow always returnsfalsefor impassable, that clearing never runs. The raw bit is still serialized viaterrainByte()and uploaded to the GPU, so a stale bit could render a sand/water outline around impassable tiles. Low likelihood, but the defensive code is now dead — better to read the raw bit there. -
isOnEdgeOfMapbounds safety is fragile.ref(x, y)throws on invalid coordinates, and the newref(x±1, y)calls are only safe because of short-circuit OR ordering (they never run when the tile is on the physical edge). It works, but any future reordering of the condition would throw at runtime. Worth a comment or a bounds guard.
2 participants
Add approved & assigned issue number here:
Resolves #4907
Description:
Impassible terrain acts like map edge
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tktk1234567