feat(map-generator): cap water magnitude at 11 for better compression#3859
Open
alexjurkiewicz wants to merge 2 commits intoopenfrontio:mainfrom
Open
feat(map-generator): cap water magnitude at 11 for better compression#3859alexjurkiewicz wants to merge 2 commits intoopenfrontio:mainfrom
alexjurkiewicz wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
|
|
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 ignored due to path filters (227)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughWater tile magnitude packing is reduced from a cap of 31 to 11 in the map generator. Three game map manifests are updated to reflect adjusted land tile counts following this encoding change. ChangesWater Tile Magnitude Encoding & Map Manifest Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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.
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 `@map-generator/map_generator.go`:
- Line 578: The pack logic at packedByte |=
byte(math.Min(math.Ceil(tile.Magnitude/2), 10)) prevents any encoded magnitude
>10, so AStar.Water.ts / AStar.WaterBounded.ts branches for magnitude >10 never
run; fix by aligning the encoder with the pathfinder: remove or raise the cap so
values >10 can be encoded (e.g., drop the math.Min(...,10) or increase the cap
above the threshold used in AStar.Water.ts), or alternatively adjust the AStar
code to use >= the packed maximum; update the packing in the map generator (the
packedByte/ tile.Magnitude encoding) accordingly so deep-water penalty logic
remains reachable.
🪄 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: 59a16d8e-a8a8-4990-98f2-0338260591db
📒 Files selected for processing (1)
map-generator/map_generator.go
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
alexjurkiewicz
force-pushed
the
feat/cap-water-magnitude
branch
from
6e5fc75 to
f97fc38
Compare
alexjurkiewicz
changed the title
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 `@map-generator/map_generator.go`:
- Line 578: The code caps encoded water magnitude to 11 using packedByte |=
byte(math.Min(math.Ceil(tile.Magnitude/2), 11)), which contradicts the PR
objective to cap at 10; change the cap constant from 11 to 10 so packedByte uses
math.Min(math.Ceil(tile.Magnitude/2), 10) and ensure any pathfinding thresholds
that assume a different encoding (refer to AStar.Water.ts and its deep-water
checks) are updated in the same PR or update the PR description to state that a
deep-water (magnitude>10) distinction is intentional; verify tile.Magnitude and
packedByte encoding consistency across map_generator.go and
src/core/pathfinding/algorithms/AStar.Water.ts.
🪄 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: 23f9041a-d297-4579-83fd-467875ddd665
📒 Files selected for processing (1)
map-generator/map_generator.go
alexjurkiewicz
commented
May 6, 2026
Contributor
|
Hi @alexjurkiewicz, it's not the most important thing but would it be possible for you to apply our default PR template? It should normally be applied automatically on PR creation, maybe something went wrong there. You can find it here: https://github.com/openfrontio/OpenFrontIO/blob/main/.github/PULL_REQUEST_TEMPLATE.md. This should also fix the failing Valid Description test. In the Description template we also ask for your Discord name if you have that. Especially welcome if you'd like to join our Dev server, link here in the readme: https://github.com/openfrontio/OpenFrontIO#-contributing Btw the CI Test failed previously in this PR, because of an unrelated test that sometimes takes too long to run. Fixing that now in another PR, and re-ran the job in here which now completed within the default timout. |
Collaborator
|
Can you run the map generator to regen the map files |
Water tile magnitude encodes distance from land and was previously stored as values up to 31. The pathfinding sweet spot is magnitude 3–10 (zero penalty); values above 10 carry a slight deep-water penalty, and values above 11 have no additional effect. Capping at 11 preserves the deep-water penalty threshold while making the lower bits of distant ocean tiles more homogeneous, improving gzip/brotli compression ratios of the generated .bin files. No change to land tile output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
alexjurkiewicz
force-pushed
the
feat/cap-water-magnitude
branch
from
f97fc38 to
e58886c
Compare
Contributor
Author
|
Rebased onto main with performance improvements from #3860 and rebuilt maps 👍 |
alexjurkiewicz
force-pushed
the
feat/cap-water-magnitude
branch
from
e58886c to
bc32f3f
Compare
evanpelle
reviewed
May 7, 2026
Collaborator
There was a problem hiding this comment.
no sure why num land tiles should change? unless the generated maps were out of date
Collaborator
There was a problem hiding this comment.
update: i reran the map generator it was out of date.
Contributor
Author
There was a problem hiding this comment.
Sorry, I'm not clear what you mean. I believe the maps in this PR are all up to date. Is that what you see?
I can see there are some merge conflicts, do you want me to rebase and regenerate all maps?
Given how fast map generation is now, it might make sense to remove "compiled" maps from the repo, and instead build them in CI. But that could be a future PR.
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
4 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:
Water tile magnitude encodes distance from land and was previously stored as values up to 31. The pathfinding sweet spot is magnitude 3–10 (zero penalty); values above 10 carry a slight deep-water penalty, and values above 11 have no additional effect. Capping at 11 preserves the deep-water penalty threshold while making the lower bits of distant ocean tiles more homogeneous, improving gzip/brotli compression ratios of the generated
.binfiles.No change to land tile output.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
alexjurkiewicz