Conversation
|
⏱️ Benchmark run finished in 0m 33s 📈 Compared against baseline: 30s 🟢 Generation time is unchanged. 📅 Last benchmark: 2026-02-08 15:41:38 UTC You can retrigger the benchmark by commenting |
There was a problem hiding this comment.
Pull request overview
This PR expands map feature coverage and substantially refactors building generation to produce more varied, tag-driven building styles and roof shapes, plus adds support for rendering place=* areas.
Changes:
- Extend Overpass retrieval + world processing to include and render
place=*ways as ground blocks. - Major refactor of
buildings.rs: introduce building categories/presets, deterministic style resolution, new roof generators, and new special building-type generators. - Extend block definitions with additional block IDs/properties needed for new building/roof/door features.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/retrieve_data.rs |
Adds place to the Overpass query filters. |
src/data_processing.rs |
Routes place=* ways to a new generator. |
src/element_processing/landuse.rs |
Adds generate_place to fill place=* areas with block types by tag. |
src/element_processing/buildings.rs |
Large building-generation overhaul: categories/presets/styles, roof system changes, and new special generators (shelter, parking, roof-only, etc.). |
src/block_definitions.rs |
Adds new block IDs and properties (doors, slabs, pane, terracotta) and introduces deterministic window/floor selection helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
retrigger-benchmark |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config: &BuildingConfig, | ||
| args: &Args, | ||
| generate_non_flat_roof: bool, | ||
| ) -> HashSet<(i32, i32)> { |
There was a problem hiding this comment.
The generate_non_flat_roof parameter name doesn’t match how it’s used: the logic treats it as “will any roof be generated” (to decide whether to place the top ceiling), not specifically “non-flat roof”. Renaming it (and updating call sites) would make the control flow much easier to follow and reduce the risk of future regressions.
| // Set top ceiling (only if flat roof or no roof generation) | ||
| // Use the resolved style flag, not just the OSM tag, since auto-gabled roofs | ||
| // may be generated for residential buildings without a roof:shape tag | ||
| let has_flat_roof = !args.roof || !generate_non_flat_roof; | ||
|
|
||
| if has_flat_roof { |
There was a problem hiding this comment.
has_flat_roof is computed as !args.roof || !generate_non_flat_roof, which is effectively “should place a flat ceiling because no roof will be generated”, not “flat roof”. The current variable name and preceding comment are misleading; consider renaming to something like should_place_top_ceiling and adjusting the comment accordingly.
| // Set top ceiling (only if flat roof or no roof generation) | |
| // Use the resolved style flag, not just the OSM tag, since auto-gabled roofs | |
| // may be generated for residential buildings without a roof:shape tag | |
| let has_flat_roof = !args.roof || !generate_non_flat_roof; | |
| if has_flat_roof { | |
| // Set top ceiling when no non-flat roof will be generated | |
| // Use the resolved style flag, not just the OSM tag, since auto-gabled roofs | |
| // may be generated for residential buildings without a roof:shape tag | |
| let should_place_top_ceiling = !args.roof || !generate_non_flat_roof; | |
| if should_place_top_ceiling { |
| if !states.contains_key("direction") { | ||
| states.insert("direction".to_string(), BedrockBlockStateValue::Int(0)); | ||
| } |
There was a problem hiding this comment.
Door conversion defaults direction to 0 when facing is missing. Since many of the project’s door blocks only set half (and omit facing), this default will be applied broadly and may not match Java’s default door facing, leading to inconsistent orientation in Bedrock exports. Consider defaulting direction to the Bedrock value that corresponds to Java’s default facing (commonly north) when facing isn’t provided.
| /// Convert Java door block to Bedrock format with upper_block_bit. | ||
| /// | ||
| /// Java doors use `half=upper/lower`, Bedrock uses `upper_block_bit` (bool). | ||
| /// Also maps door names: `oak_door` → `wooden_door`, others keep their names. | ||
| fn convert_door( | ||
| java_name: &str, | ||
| props: Option<&std::collections::HashMap<String, fastnbt::Value>>, | ||
| ) -> BedrockBlock { | ||
| let bedrock_name = match java_name { |
There was a problem hiding this comment.
New Bedrock property conversions were added for doors/trapdoors, but there are no unit tests covering these mappings (e.g., half→upper_block_bit, facing→direction, and defaults when facing/open/hinge are absent). Adding a few focused tests in the existing #[cfg(test)] module would help prevent subtle export regressions.
No description provided.