Skip to content

Conversation

@FloPinguin
Copy link
Contributor

Description:

There was one row of white pixels at the top:

image

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • 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:

FloPinguin

@FloPinguin FloPinguin requested a review from a team as a code owner January 9, 2026 22:11
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 9, 2026

Walkthrough

The PR updates numeric values in the Amazon River map manifest file. It reduces map height and land tile counts across three map variants (base, 16x, and 4x), with no structural changes to the manifest format.

Changes

Cohort / File(s) Summary
Map manifest numeric updates
resources/maps/amazonriver/manifest.json
Adjusted map dimensions and land tile counts: base height 280β†’276 (1,172,808β†’1,150,819 tiles), 16x height 70β†’69 (71,047β†’69,690 tiles), 4x height 140β†’138 (290,101β†’284,707 tiles). Map widths unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

Suggested labels

Maps

Suggested reviewers

  • evanpelle

Poem

πŸ“ The Amazon flows with numbers precise,
Heights trimmed clean, like polished rice.
Tiles counted true, dimensions align,
A cartographer's careful, crafted design. ✨

πŸš₯ Pre-merge checks | βœ… 3
βœ… Passed checks (3 passed)
Check name Status Explanation
Title check βœ… Passed The title clearly identifies the main fix: adjusting the Amazon River map dimensions due to one extra row of pixels, which matches the manifest.json changes.
Description check βœ… Passed The description is related to the changeset, explaining the visual issue being fixed with a screenshot and confirming testing and quality checks were completed.
Docstring Coverage βœ… Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


πŸ“œ Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between cf1e67c and 6086b94.

β›” Files ignored due to path filters (4)
  • map-generator/assets/maps/amazonriver/image.png is excluded by !**/*.png
  • resources/maps/amazonriver/map.bin is excluded by !**/*.bin
  • resources/maps/amazonriver/map16x.bin is excluded by !**/*.bin
  • resources/maps/amazonriver/map4x.bin is excluded by !**/*.bin
πŸ“’ Files selected for processing (2)
  • resources/maps/amazonriver/manifest.json
  • resources/maps/amazonriver/thumbnail.webp
πŸ”‡ Additional comments (2)
resources/maps/amazonriver/manifest.json (2)

3-5: Height reductions are proportionally consistent across all map variants.

The height changes correctly maintain the scaling relationships between variants:

  • Base map: -4 pixels (280 β†’ 276)
  • map16x (4x downscaled): -1 pixel (70 β†’ 69)
  • map4x (2x downscaled): -2 pixels (140 β†’ 138)

The land tile count reductions are proportionally consistent: removing 4 rows from a 5536-width map would remove up to 22,144 tiles, and the actual reduction of 21,989 tiles suggests the removed top row was approximately 99% land pixels.

The manifest values are correctly set. However, note that this codebase stores map data in binary .bin files, not image files. The binary files (map.bin, map16x.bin, map4x.bin) should be verified to match the new dimensions, but this cannot be done in the sandbox environment.

Likely an incorrect or invalid review comment.


18-124: Verify whether rows were actually removed from the map image.

The review comment suggests that 4 rows were removed from the top of the map, but the current manifest shows height: 276 with the smallest Y coordinates at 7 and 8. If rows were removed and coordinates needed adjustment, the manifest height should reflect the reduction (272 instead of 276) and the Y coordinates would be 11 and 12 (7+4, 8+4), not 7 and 8.

The code shows no offset logic for removed rowsβ€”nation coordinates from the manifest are used directly without adjustment (except for Compact maps, which divide by 2). All coordinate values are valid for a height-276 map.

Either:

  • The map image was not actually modified, or
  • The coordinate values in the manifest are correct as-is for the current map dimensions

Check the map dimensions and image to clarify whether rows were removed. If they were, both the manifest height and all Y coordinates require adjustment. If they were not, this review comment is incorrect.


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.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@FloPinguin FloPinguin added this to the v29 milestone Jan 9, 2026
@evanpelle evanpelle merged commit 848a3a5 into openfrontio:main Jan 9, 2026
9 of 10 checks passed
ryanbarlow97 pushed a commit that referenced this pull request Jan 10, 2026
## Description:

There was one row of white pixels at the top:

<img width="796" height="693" alt="image"
src="https://github.com/user-attachments/assets/5915c629-c2ce-4b3f-913a-bfe6e974561f"
/>

## 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:

FloPinguin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants