Skip to content

Conversation

@Tidwell
Copy link
Contributor

@Tidwell Tidwell commented Dec 27, 2025

Description:

  • clarify the map generator readme size recommendation is for land tile count, not px size (and that it is for performance, not a hard limit). Add note about average map land tile count to provide more context.
  • Add output files list to provide reference for where to find land tile count (for some reason the script doesn't output it, will probably open a new PR to add it to the output of the generator)

I must have missed some context when first documenting this, oops!

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:

tidwell

@Tidwell Tidwell requested a review from a team as a code owner December 27, 2025 08:25
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 27, 2025

Walkthrough

Updated map-generator/README.md to add an "Output Files" section listing five generated artifacts and reworked the "Notes" section: replaced the single pixel-area constraint with guidance recommending maps avoid >3,000,000 land tiles and noting typical maps have ~1–2 million land tiles. No code changes.

Changes

Cohort / File(s) Summary
Documentation
map-generator/README.md
Added an "Output Files" section enumerating artifacts: ../resources/maps/<map_name>/manifest.json, ../resources/maps/<map_name>/map.bin, ../resources/maps/<map_name>/map4x.bin, ../resources/maps/<map_name>/map16x.bin, ../resources/maps/<map_name>/thumbnail.webp. Replaced single pixel-area constraint with two bullets: recommend maps be ~2–3 million pixels area, advise against maps with >3,000,000 land tiles for performance, and note typical land tile counts are ~1–2 million. No logic or API changes.

Sequence Diagram(s)

(omitted — changes are documentation-only and do not introduce new runtime interactions)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

Suggested reviewers

  • Duwibi
  • evanpelle

Poem

A map lists files in tidy rows,
Manifest, bins, and thumbnails show,
Three million tiles — a gentle guard,
One-to-two million play the yard,
Small doc glow, maps ready to grow. 🗺️

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Update map-generator readme (output files & tile count)' accurately summarizes the main changes: documentation updates covering output files and tile count information.
Description check ✅ Passed The description clearly relates to the changeset by explaining the clarifications made to tile count recommendations, the addition of output files documentation, and context around the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 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 582d0dc and 41bb41f.

📒 Files selected for processing (1)
  • map-generator/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • map-generator/README.md

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
map-generator/README.md (1)

108-109: Optional: Consider defining "land tiles" for newcomers.

The term "land tiles" appears here for the first time in the document. Earlier sections explain pixel-to-terrain conversion (lines 5–6, 44–50), but don't explicitly define what constitutes a "land tile" or how pixels map to tiles. A brief parenthetical clarification (e.g., "land tiles (non-water pixels after processing)") could help map creators unfamiliar with the codebase quickly understand the constraint.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9d9a32 and b84d813.

📒 Files selected for processing (1)
  • map-generator/README.md
🔇 Additional comments (1)
map-generator/README.md (1)

108-109: Clear documentation improvement that aligns with PR objectives.

The new notes successfully clarify that the recommendation concerns land tile count (not pixel dimensions) and explicitly frames it as a performance consideration rather than a hard constraint. The wording is simple and direct, making it accessible to non-native English speakers. The addition of average land tile count context (1–2 million) is helpful for map creators.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 27, 2025
@Tidwell Tidwell changed the title Update map-generator readme (tile count) Update map-generator readme (output files & tile count) Dec 27, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b84d813 and ca5cb3e.

📒 Files selected for processing (1)
  • map-generator/README.md
🧰 Additional context used
🪛 LanguageTool
map-generator/README.md

[grammar] ~39-~39: Use a hyphen to join words.
Context: ...esources/maps/<map_name>/map.bin` - Full scale binary map data packed with terrai...

(QB_NEW_EN_HYPHEN)

🔇 Additional comments (1)
map-generator/README.md (1)

116-117: Great clarification on land tile recommendations.

The updated "Notes" section effectively addresses the documentation gap by:

  • Clarifying that the recommendation concerns land tile count, not pixel dimensions
  • Explicitly framing it as a performance consideration (not a hard limit)
  • Providing helpful context on typical map sizes

This aligns well with the PR objectives and removes ambiguity for map creators.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 27, 2025
@iiamlewis iiamlewis added the Maps A new map, or adjustments to an existing map itself, its json, etc, label Dec 27, 2025
@iiamlewis iiamlewis moved this from Triage to Complete in OpenFront Release Management Dec 27, 2025
@iiamlewis iiamlewis added this to the v29 milestone Dec 27, 2025
## Notes

- Maps should be between 2 - 3 million pixels square (area)
- Maps with over 3 million land tiles are not recommended for performance reasons.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep it to 2-3 million pixels total for now (maybe when ship pathfinding performance improves this can be changed)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to keep the pixel suggestion and moved all 3 performance-related recommendations into a grouping separate from the notes about the map-generator functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #2718 as a follow-up to add additional logging options to the generator to surface warnings for these recommendations, and for a number of other logs that I've been using when working on maps that I think would be nice QOL upgrades.

Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@github-project-automation github-project-automation bot moved this from Complete to Final Review in OpenFront Release Management Dec 28, 2025
@evanpelle evanpelle merged commit 63acbf1 into openfrontio:main Dec 28, 2025
7 checks passed
@github-project-automation github-project-automation bot moved this from Final Review to Complete in OpenFront Release Management Dec 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maps A new map, or adjustments to an existing map itself, its json, etc,

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants