-
Notifications
You must be signed in to change notification settings - Fork 739
Update map-generator readme (output files & tile count) #2709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughUpdated 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
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this 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
📒 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.
There was a problem hiding this 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
📒 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.
map-generator/README.md
Outdated
| ## Notes | ||
|
|
||
| - Maps should be between 2 - 3 million pixels square (area) | ||
| - Maps with over 3 million land tiles are not recommended for performance reasons. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description:
I must have missed some context when first documenting this, oops!
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tidwell