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.

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