Fix Water Nukes Pathfinding 🚢 - #4975
Merged
Merged
Conversation
FloPinguin
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughWater nuke finalization now propagates ocean status through connected minimap water and recomputes affected water magnitudes with bounded BFS operations. Integration tests validate coastal and interior-water results. ChangesWater minimap updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WaterNuke
participant WaterManager
participant MinimapWater
participant BFSBuffers
WaterNuke->>WaterManager: finalize converted tiles
WaterManager->>MinimapWater: propagate ocean status
WaterManager->>BFSBuffers: recompute affected magnitudes
BFSBuffers->>MinimapWater: update bounded water distances
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/game/WaterManager.ts (1)
604-678: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: reuse one minimap neighbour helper.
The block unrolls the four cardinal neighbours by hand three times (ocean BFS, coast seeding, magnitude BFS). The full-map path already uses the local
pushNeighborshelper at Lines 222-233. A matchingpushMiniNeighborshelper keeps the allocation-free style and removes many repeated branches, which makes the clipping conditions easier to check.🤖 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/WaterManager.ts` around lines 604 - 678, Optionally add a local allocation-free pushMiniNeighbors helper near the minimap processing code, modeled on pushNeighbors, and use it for coast seeding and both minimap BFS neighbor traversals. Preserve the existing water, coast, stamp, distance, and seed-box clipping behavior while removing the repeated cardinal-neighbor branches.
🤖 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/WaterManager.ts`:
- Around line 566-599: Update the minimap bounds in the water conversion method
around MINI_MAX_MAG so box radii use hop units: define or use MINI_MAX_MAG_DIST
as MINI_MAX_MAG * 2 for the dirty box, and use MINI_MAX_MAG_DIST * 2 for the
seed box. Preserve the existing clamping, and update the related comment near
the magnitude calculation to reference MINI_MAX_MAG_DIST * 2.
In `@tests/nukes/WaterNukes.test.ts`:
- Around line 169-196: Update tests/nukes/WaterNukes.test.ts:169-196 to assert
miniMap.isWater(mt) directly, then assert the exact expected magnitude rather
than only the 0–31 range; retain the interior-water validation. At
tests/nukes/WaterNukes.test.ts:230-270, replace guarded branches with direct
assertions for oceanMiniX >= 0, fullMap.isLand(nukeTarget), and
miniMap.isWater(nukeMini), and use the fixed target tile guaranteed by the map
data so both tests always verify the WaterManager BFS result.
---
Nitpick comments:
In `@src/core/game/WaterManager.ts`:
- Around line 604-678: Optionally add a local allocation-free pushMiniNeighbors
helper near the minimap processing code, modeled on pushNeighbors, and use it
for coast seeding and both minimap BFS neighbor traversals. Preserve the
existing water, coast, stamp, distance, and seed-box clipping behavior while
removing the repeated cardinal-neighbor branches.
🪄 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: 762343eb-b861-4d64-98aa-416b8f919595
📒 Files selected for processing (2)
src/core/game/WaterManager.tstests/nukes/WaterNukes.test.ts
github-project-automation
Bot
moved this to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
This was referenced Aug 12, 2026
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:
Fixes #4760. Check the images there. Pathfinding in water-nuked areas was sometimes weird.
Performance seems to be similar.
World map, Canada to Madagascar, Africa is waternuked
PROD situation:
This PR fixes the path:
Technical
After water nukes convert minimap tiles to water,
setWater()zeros the terrain byte, leaving magnitude at 0 and clearing the ocean bit. The existing full-map magnitude recomputation (step 2 offinalizeWaterChanges) never runs on the minimap, so all nuked minimap tiles get stuck with magnitude 0 permanently.This causes two pathfinding issues:
AStarWaterBoundedapplies a 3x cost penalty per step through magnitude < 3 tiles (cost 400 vs 100), making the pathfinder strongly avoid routing through nuked waterSmoothingWaterTransformerrejects LOS shortcuts through magnitude < 2 tiles (pass 1) and < 3 tiles (pass 2), preventing path smoothing through crater areasAdditionally, existing minimap water tiles at the edge of a nuke crater retain stale pre-computed magnitudes from the terrain file. When a nuke creates a new coastline nearby, these tiles should have lower magnitude (closer to the new coast) but keep their old high values.
The fix adds step 4b in
finalizeWaterChanges():Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin