Sasj93/change build style#191
Open
sasj93 wants to merge 16 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough<review_stack_artifact_start --> All rangeIds placed in unassigned_ranges per validation retry.None — ranges are unassigned for automated hydration.range_976d7b933f8e range_66e34b750a39 range_de0a28d3470f range_85595692da27 range_1e90a2e31442 range_8a913ae454b7 range_6b907d75108a range_ad682b82ec15 range_98dcc30d5a86 range_067bec3e6deb range_0a0adc65010a range_adb61c7f3b89 range_2db480813e34 range_38f4ec44180e range_213775d52053 range_feca9ba2b909 range_2dd2074aff2f range_b917e7c0c6dc range_b51ad6f35782 range_1c8c5d10665d range_4fd551434bbe range_07979cf52b7f range_0c07a15ab71d range_91e0f480137b range_e6b3b792d34b range_1f4013131713 range_4af70a89fc72 range_117304e9b8b7 range_c098b3e231e8 range_7366debb67ad range_7c424fbd5888 range_e033c833da7f range_cdcc4be930b9 range_ba8a11a9015d range_06c421ac40af range_78140d499fea range_b8ebbf45964d range_054dc69257b5 range_50407af3c792 range_213775d52053 range_123456789012 range_e8085e9dd4f9 range_67f89367382a range_d6ef42212060 range_14daeb720743 range_7f0f32fb822b range_00f600e6a868 range_84e668c77827 range_8ca0e265dac5 range_8eab88b51830 range_5910b1a7b378 range_154d84f83d74 range_f9d39fcaa3f4 range_8b0ae7d27858 range_de59978a7793 range_10b9dd9cbd21 range_f2a565aeb88d range_3682303dc489 range_748f939ce216 range_3d865b989901 range_76d97a8a4c96 range_174503100c04 range_f92caf101385 range_46c86443a0c9 range_cda546696ff5 range_983a79c9298f range_5f3f1bed0902 range_bc6480ab4187 range_ab27569ec1bf range_06b2438a9ac5 range_70b8d0b943ad range_2222b77a7630 range_50cfdb63326e range_c60f347b33fa range_23c0dc76d00c range_1c1847f41972 range_cf70119f59fb range_b383386060eb range_fbac48f68cd3 range_a7b962ab8a80 range_7583556fc985 range_9a01133b44d4 range_1f881666e51d range_dfd276b1bc54 range_15f92dd3b080 range_ade16d7f334c range_6938265c4a22 range_b86f2ea4d0d1 range_b621fa97aa68 range_9372940ec3a2 range_322e68cb324f range_765a3d2e3cb9 range_992a827a38f5 range_3354f0627269 range_2f436e854ff9 range_4d1452a75cca range_f211d5f7a385 range_f8a7d272d4ab range_0d2952c8a7dd range_8dbc64e74c84 range_bd289b09d467 range_16133dca840e range_f37fcf132ad7 range_d98e1d5089c3 range_c36f8f6c6329 range_fe5078e2a8d4 range_eefeb18db30d range_139dcc54214f range_a84838b8cca0 range_02b66246ced1 range_d01a5ea4c66a range_3851daf00ea0 range_6b800e3d9a91 range_7e1ad8d5304a range_9156e2c61e23 range_695d016b2c44 range_241462236107 range_441357de5ab5 range_1ca7349bd0ed range_95f0b125bb17 range_359dbcfd96d8 range_396e5ac6ebc2 range_eeef9d9d7647 range_eccb8f1de26e range_0a7d647465fb range_12049d875e88 range_4d993b8d1be3 range_4859ef0b471d range_eab144bbcd91 range_3075b437fcf2 range_ce390e77e4b6 range_13501b8aeb58 range_49709a1445f4 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_ce390e77e4b6 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_ce390e77e4b6 range_3075b437fcf2 range_ce390e77e4b6 range_13501b8aeb58 range_3075b437fcf2 range_ce390e77e4b6 range_13501b8aeb58 range_ce390e77e4b6 range_13501b8aeb58 range_13501b8aeb58 range_ce390e77e4b6 range_13501b8aeb58 range_ce390e77e4b6 range_13501b8aeb58 range_ce390e77e4b6 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_13501b8aeb58 range_d228db543921 range_316e6607c994 range_e9ed72ee0757 range_0369b98b9404 range_c6731fa105a2 range_5e8ed4b7a464 range_6ef0ae86e884 range_9fddbd5378f8 range_99595b829e02 range_213ac58bc0df range_3f3926460ef5 range_5b6b1d827986 range_065c54fb2935 range_4905686646a7 range_c77b23547117 range_6cd66bbea17f range_a17f4d50f48a range_8187dc8f00fb range_5eb564922ce4 range_db27407f5234 range_55af993664be range_a384c0f9dd38 range_e7d973b0fad9 range_5ca9bd4af18d range_0e8e414f7a81 range_64cc2f08c8b8 range_67c2e57a796f range_a90901bd1c2e range_eeaa209e975c range_8c3ab90af33d range_1bd4e84f7ad6 range_971dbc17c86e range_f9999f239002 range_abac3c5b77b9 range_f31b67a6b8b8 range_c6731fa105a2 range_85e7a8b6bed9 range_bdc83dfa3b5f range_85a0bec87db6 range_b4c88792d68d range_d38351b3c702 range_01f0c6e98dde range_b45dae639e79 range_8902f1a88f59 range_cabcee64b841 range_bfc39b29c136 range_ccbddb390454 range_3c7591632230 range_0784f003e3bd range_4468c97b5142 range_d70c2e536e99 range_f8e7b86a89e1 range_ad321245a32e range_834f11b36c46 range_efc39d4b857f range_b260efcca18a range_f319aa013d50 range_33a9afb3360e range_0ccbb7f43499 range_d7e57e15ab17 range_8848fa9732ab range_680836d65aaf range_7c8ceea06030 range_8900a8c1d005 range_48dc612f81f6 range_b816eed9216a range_07bcc0c3af48 range_dab79774ccbf range_a8a0d9cf7cbd range_dae56cf3592d range_713ad47217ba range_a3342a97eec4 range_8ea14cecc905 range_1d39434375ee range_e5fe208628ef range_cf048dce9718 range_86e6f216c18d range_cc7fcef53fef range_b9cbdcb79339 range_f9ba19c6c2dc range_bda27b8bd3c3 range_ae913561cdda range_99bf3e9ea796 range_6023eedfba47 range_9fbddc4006ca range_5d5588eb4e39 range_bc5a11109947 range_2c4382eeafd9 range_19b5e706d9ea range_d228db543921 range_316e6607c994 range_e9ed72ee0757 range_0369b98b9404 range_c6731fa105a2 range_5e8ed4b7a464 range_6ef0ae86e884 range_9fddbd5378f8 range_99595b829e02 range_213ac58bc0df range_3f3926460ef5 range_5b6b1d827986 range_065c54fb2935 range_4905686646a7 range_c77b23547117 range_6cd66bbea17f range_b260efcca18a range_976d7b933f8e range_66e34b750a39 range_de0a28d3470f range_85595692da27 range_1e90a2e31442 range_8a913ae454b7 range_6b907d75108a range_ad682b82ec15 range_98dcc30d5a86 range_067bec3e6deb range_0a0adc65010a range_adb61c7f3b89 range_2db480813e34 range_38f4ec44180e range_213775d52053 range_feca9ba2b909 range_2dd2074aff2f range_b917e7c0c6dc range_b51ad6f35782 range_1c8c5d10665d range_4fd551434bbe range_07979cf52b7f range_0c07a15ab71d range_91e0f480137b range_e6b3b792d34b range_1f4013131713 range_4af70a89fc72 range_117304e9b8b7 range_c098b3e231e8 range_7366debb67ad range_7c424fbd5888 range_e033c833da7f range_cdcc4be930b9 range_ba8a11a9015d range_06c421ac40af range_78140d499fea range_b8ebbf45964d range_054dc69257b5 range_50407af3c792 range_213775d52053 range_e8085e9dd4f9 range_67f89367382a range_d6ef42212060 range_14daeb720743 range_7f0f32fb822b range_00f600e6a868 range_84e668c77827 range_8ca0e265dac5 range_8eab88b51830 range_5910b1a7b378 range_154d84f83d74 range_f9d39fcaa3f4 range_8b0ae7d27858 range_de59978a7793 range_10b9dd9cbd21 range_f2a565aeb88d range_3682303dc489 range_748f939ce216 range_3d865b989901 range_76d97a8a4c96 range_174503100c04 range_f92caf101385 range_46c86443a0c9 range_cda546696ff5 range_983a79c9298f range_5f3f1bed0902 range_bc6480ab4187 range_ab27569ec1bf range_06b2438a9ac5 range_70b8d0b943ad range_2222b77a7630 range_50cfdb63326e range_c60f347b33fa range_23c0dc76d00c range_1c1847f41972 range_cf70119f59fb range_b383386060eb range_fbac48f68cd3 range_a7b962ab8a80 range_7583556fc985 range_9a01133b44d4 range_1f881666e51d range_dfd276b1bc54 range_15f92dd3b080 range_ade16d7f334c range_6938265c4a22 range_b86f2ea4d0d1 range_b621fa97aa68 range_9372940ec3a2 range_322e68cb324f range_765a3d2e3cb9 range_992a827a38f5 range_3354f0627269 range_2f436e854ff9 range_4d1452a75cca range_f211d5f7a385 range_f8a7d272d4ab range_0d2952c8a7dd range_8dbc64e74c84 range_bd289b09d467 range_16133dca840e range_f37fcf132ad7 range_d98e1d5089c3 range_c36f8f6c6329 range_fe5078e2a8d4 range_eefeb18db30d range_139dcc54214f range_a84838b8cca0 range_02b66246ced1 range_d01a5ea4c66a range_3851daf00ea0 range_6b800e3d9a91 range_7e1ad8d5304a range_9156e2c61e23 range_695d016b2c44 range_241462236107 range_441357de5ab5 range_1ca7349bd0ed range_95f0b125bb17 range_359dbcfd96d8 range_396e5ac6ebc2 range_eeef9d9d7647 range_eccb8f1de26e range_0a7d647465fb range_12049d875e88 range_4d993b8d1be3 range_4859ef0b471d range_eab144bbcd91 range_3075b437fcf2 range_ce390e77e4b6 range_13501b8aeb58 range_49709a1445f4 range_d228db543921 range_316e6607c994 range_e9ed72ee0757 range_0369b98b9404 range_c6731fa105a2 range_5e8ed4b7a464 range_6ef0ae86e884 range_9fddbd5378f8 range_99595b829e02 range_213ac58bc0df range_3f3926460ef5 range_5b6b1d827986 range_065c54fb2935 range_4905686646a7 range_c77b23547117 range_6cd66bbea17f range_a17f4d50f48a range_8187dc8f00fb range_5eb564922ce4 range_db27407f5234 range_55af993664be range_a384c0f9dd38 range_e7d973b0fad9 range_5ca9bd4af18d range_0e8e414f7a81 range_64cc2f08c8b8 range_67c2e57a796f range_a90901bd1c2e range_eeaa209e975c range_8c3ab90af33d range_1bd4e84f7ad6 range_971dbc17c86e range_f9999f239002 range_abac3c5b77b9 range_f31b67a6b8b8 range_c6731fa105a2 range_85e7a8b6bed9 range_bdc83dfa3b5f range_85a0bec87db6 range_b4c88792d68d range_d38351b3c702 range_01f0c6e98dde range_b45dae639e79 range_8902f1a88f59 range_cabcee64b841 range_bfc39b29c136 range_ccbddb390454 range_3c7591632230 range_0784f003e3bd range_4468c97b5142 range_d70c2e536e99 range_f8e7b86a89e1 range_ad321245a32e range_834f11b36c46 range_efc39d4b857f range_b260efcca18a range_f319aa013d50 range_33a9afb3360e range_0ccbb7f43499 range_d7e57e15ab17 range_8848fa9732ab range_680836d65aaf range_7c8ceea06030 range_8900a8c1d005 range_48dc612f81f6 range_b816eed9216a range_07bcc0c3af48 range_dab79774ccbf range_a8a0d9cf7cbd range_dae56cf3592d range_713ad47217ba range_a3342a97eec4 range_8ea14cecc905 range_1d39434375ee range_e5fe208628ef range_cf048dce9718 range_86e6f216c18d range_cc7fcef53fef range_b9cbdcb79339 range_f9ba19c6c2dc range_bda27b8bd3c3 range_ae913561cdda range_99bf3e9ea796 range_6023eedfba47 range_9fbddc4006ca range_5d5588eb4e39 range_bc5a11109947 range_2c4382eeafd9 range_19b5e706d9ea range_d228db543921 range_316e6607c994 range_e9ed72ee0757 range_0369b98b9404 range_c6731fa105a2 range_5e8ed4b7a464 range_6ef0ae86e884 range_9fddbd5378f8 range_99595b829e02 range_213ac58bc0df range_3f3926460ef5 range_5b6b1d827986 range_065c54fb2935 range_4905686646a7 range_c77b23547117 range_6cd66bbea17f range_b260efcca18a✨ Finishing Touches🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a "Change Build Style" feature for the Canary Map Editor, allowing users to replace connected structural walls with another automagic style across multiple floors. It introduces a new service (ChangeBuildStyleService) for component discovery and compatibility checks, along with a wxWidgets dialog (ChangeBuildStyleDialog) that provides text/category filtering and a visual preview. The review feedback highlights critical issues including potential silent truncation of door IDs due to an incorrect integer type, case-sensitivity bugs in style matching, missing <algorithm> header includes, and a visual rendering bug that stretches sprites in the style list.
| Position position; | ||
| ::DoorType type; | ||
| bool open; | ||
| uint8_t doorId; |
There was a problem hiding this comment.
The doorId field is declared as uint8_t, which will cause silent truncation and corruption of door IDs greater than 255 (since door IDs, action IDs, or unique IDs in RME/Canary are typically uint32_t or uint16_t). It should be changed to uint32_t to match the rest of the codebase.
Suggested change
| uint8_t doorId; | |
| uint32_t doorId; |
|
|
||
| #include "main.h" | ||
|
|
||
| #include <queue> |
There was a problem hiding this comment.
The code uses standard algorithms like std::sort, std::min, and std::transform (in the suggested case-insensitive check) without explicitly including <algorithm>. While it might compile on some platforms due to transitive includes, it can cause compilation failures on other compilers/platforms. Explicitly including <algorithm> ensures portability.
#include <algorithm>
#include <queue>
Comment on lines
+53
to
+55
| bool isWoodStyle(const WallBrush* target) { | ||
| return target && target->getName().find("wood") != std::string::npos; | ||
| } |
There was a problem hiding this comment.
The isWoodStyle check is case-sensitive (find("wood")). Since brush names in RME are typically capitalized (e.g., "Wooden Wall", "Wood Railing"), this check will fail to match them, breaking the gate replacement logic for capitalized wooden styles. Converting the name to lowercase before checking ensures case-insensitive matching.
bool isWoodStyle(const WallBrush* target) {
if (!target) {
return false;
}
std::string name = target->getName();
std::transform(name.begin(), name.end(), name.begin(), [](unsigned char c) {
return std::tolower(c);
});
return name.find("wood") != std::string::npos;
}|
|
||
| #include "main.h" | ||
|
|
||
| #include <cctype> |
There was a problem hiding this comment.
The code uses std::transform without explicitly including <algorithm>. Explicitly including <algorithm> ensures portability across different compilers and platforms.
#include <algorithm>
#include <cctype>| dc.SetTextForeground(wxColour(0, 0, 0)); | ||
| } | ||
| if (Sprite* sprite = g_gui.gfx.getSprite(brush->getLookID())) { | ||
| sprite->DrawTo(&dc, SPRITE_SIZE_32x32, rect.GetX() + 3, rect.GetY() + 3, rect.GetWidth(), rect.GetHeight()); |
There was a problem hiding this comment.
In OnDrawItem, sprite->DrawTo is called with rect.GetWidth() and rect.GetHeight(). Since rect represents the entire list item bounds (which is much wider than 32px), this will stretch the 32x32 sprite horizontally and vertically, causing severe visual distortion. Removing these arguments will draw the sprite at its native 32x32 size.
sprite->DrawTo(&dc, SPRITE_SIZE_32x32, rect.GetX() + 3, rect.GetY() + 3);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
source/map_display.cpp (1)
2749-2749: ⚡ Quick winUnify structural-wall eligibility behind one helper.
Line 2749 + Lines 2763-2767 duplicate a structural-wall predicate that should match the handler’s
ChangeBuildStyleService::getStructuralWall(...)check. Reusing the same helper here prevents menu visibility/handler eligibility drift.♻️ Proposed refactor
- bool hasStructuralWall = false; + const bool hasStructuralWall = ChangeBuildStyleService::getStructuralWall(tile) != nullptr; @@ for (auto* item : tile->items) { if (item->isWall()) { Brush* wb = item->getWallBrush(); - if (wb && !wb->isWallDecoration()) { - hasStructuralWall = true; - if (wb->visibleInPalette()) { - hasWall = true; - } - } + if (wb && !wb->isWallDecoration() && wb->visibleInPalette()) { + hasWall = true; + } }Also applies to: 2763-2767, 2850-2852, 2902-2904
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/map_display.cpp` at line 2749, Replace the duplicated structural-wall predicate logic (variables like hasStructuralWall in map_display.cpp around the menu visibility checks) with a single call to the existing helper ChangeBuildStyleService::getStructuralWall(...) so menu visibility and handler eligibility use the same rule; find each duplicated block (around the hasStructuralWall declaration and the later checks at the ranges mentioned) and call getStructuralWall with the same parameters used by the handler, remove the ad-hoc predicate, and propagate its boolean result to the menu/visibility logic to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-26-change-build-style.md`:
- Line 13: The heading "### Task 1: Wall brush conversion API and service" is
one level too deep directly after an H1 and triggers MD001; change it to an H2
by replacing "###" with "##" (or insert an intermediate H2 heading above it) so
the heading reads "## Task 1: Wall brush conversion API and service" to satisfy
the markdown lint rule.
In `@source/change_build_style.cpp`:
- Around line 257-261: In ChangeBuildStyleService::simulate(WallBrush* target,
const std::set<int>& selectedFloors, BaseMap &working, std::set<Position>
&changed, wxString &reason) const, clear the outgoing reason at the beginning
(e.g., reason.clear() or reason = "") so callers don't see stale error text on
success; apply the same change to the other simulate overloads referenced in the
review (the other ChangeBuildStyleService::simulate methods around the same
area) so every simulate starts by resetting reason before any early returns.
- Around line 315-323: In the opening pass the code calls
getStructuralWall(tile) which can pick the wrong wall on mixed tiles; change the
lookup to restrict to the converted target wall (i.e., call the variant of
getStructuralWall that accepts the conversion target or target brush) so the
check uses the wall from target rather than any source wall — update the call at
the loop over openings (where Tile* tile = working.getTile(opening.position) is
used) to use the target-aware getStructuralWall(tile, target) (or equivalent
API) before computing doorItemId with
target->getDoorItemID(alignments[opening.position], opening.type, opening.open).
---
Nitpick comments:
In `@source/map_display.cpp`:
- Line 2749: Replace the duplicated structural-wall predicate logic (variables
like hasStructuralWall in map_display.cpp around the menu visibility checks)
with a single call to the existing helper
ChangeBuildStyleService::getStructuralWall(...) so menu visibility and handler
eligibility use the same rule; find each duplicated block (around the
hasStructuralWall declaration and the later checks at the ranges mentioned) and
call getStructuralWall with the same parameters used by the handler, remove the
ad-hoc predicate, and propagate its boolean result to the menu/visibility logic
to avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98eee607-f8e9-4972-9963-a8b7cb5e0b72
📒 Files selected for processing (15)
docs/superpowers/plans/2026-05-26-change-build-style.mddocs/superpowers/specs/2026-05-26-change-build-style-design.mdsource/CMakeLists.txtsource/action.cppsource/action.hsource/actions_history_window.cppsource/change_build_style.cppsource/change_build_style.hsource/change_build_style_window.cppsource/change_build_style_window.hsource/gui_ids.hsource/map_display.cppsource/map_display.hsource/wall_brush.cppsource/wall_brush.h
|
|
||
| --- | ||
|
|
||
| ### Task 1: Wall brush conversion API and service |
There was a problem hiding this comment.
Fix heading level increment to satisfy markdown linting.
Line 13 starts at ### directly after an H1 section. Insert an H2 level before this (or downgrade this heading to ##) to avoid MD001 violations.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-05-26-change-build-style.md` at line 13, The
heading "### Task 1: Wall brush conversion API and service" is one level too
deep directly after an H1 and triggers MD001; change it to an H2 by replacing
"###" with "##" (or insert an intermediate H2 heading above it) so the heading
reads "## Task 1: Wall brush conversion API and service" to satisfy the markdown
lint rule.
Comment on lines
+257
to
+261
| bool ChangeBuildStyleService::simulate(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &working, std::set<Position> &changed, wxString &reason) const { | ||
| if (!isValid() || !target || target->isWallDecoration() || target == sourceBrush) { | ||
| reason = "Select a different structural wall style."; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Clear reason at the start of simulation to prevent stale errors on success.
simulate() only sets reason on failure. If callers reuse the same wxString, successful calls may still expose an old error message.
Suggested fix
bool ChangeBuildStyleService::simulate(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &working, std::set<Position> &changed, wxString &reason) const {
+ reason.clear();
if (!isValid() || !target || target->isWallDecoration() || target == sourceBrush) {
reason = "Select a different structural wall style.";
return false;
}Also applies to: 365-367, 393-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_build_style.cpp` around lines 257 - 261, In
ChangeBuildStyleService::simulate(WallBrush* target, const std::set<int>&
selectedFloors, BaseMap &working, std::set<Position> &changed, wxString &reason)
const, clear the outgoing reason at the beginning (e.g., reason.clear() or
reason = "") so callers don't see stale error text on success; apply the same
change to the other simulate overloads referenced in the review (the other
ChangeBuildStyleService::simulate methods around the same area) so every
simulate starts by resetting reason before any early returns.
Comment on lines
+315
to
+323
| for (const Opening &opening : openings) { | ||
| Tile* tile = working.getTile(opening.position); | ||
| Item* wall = getStructuralWall(tile); | ||
| if (!wall) { | ||
| reason = "An opening lost its supporting wall during conversion."; | ||
| return false; | ||
| } | ||
| const uint16_t doorItemId = target->getDoorItemID(alignments[opening.position], opening.type, opening.open); | ||
| if (doorItemId == 0) { |
There was a problem hiding this comment.
Convert openings against the converted target wall explicitly.
In the opening pass, getStructuralWall(tile) can select the wrong structural wall on mixed tiles. Since the first pass already converts source walls to the target brush, restrict lookup to target here to avoid mutating unrelated walls.
Suggested fix
- Item* wall = getStructuralWall(tile);
+ Item* wall = getStructuralWall(tile, target);
if (!wall) {
reason = "An opening lost its supporting wall during conversion.";
return false;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_build_style.cpp` around lines 315 - 323, In the opening pass
the code calls getStructuralWall(tile) which can pick the wrong wall on mixed
tiles; change the lookup to restrict to the converted target wall (i.e., call
the variant of getStructuralWall that accepts the conversion target or target
brush) so the check uses the wall from target rather than any source wall —
update the call at the loop over openings (where Tile* tile =
working.getTile(opening.position) is used) to use the target-aware
getStructuralWall(tile, target) (or equivalent API) before computing doorItemId
with target->getDoorItemID(alignments[opening.position], opening.type,
opening.open).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/map_display.cpp (1)
2798-2807:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same structural-wall check as the action handler.
This gating only treats
item->isWall()as structural, while bothOnSelectWallBrush()andOnChangeBuildStyle()rely onChangeBuildStyleService::getStructuralWall(tile). That means the menu can stay hidden on tiles where the structural piece is exposed through a related opening/door/gate instead of a wall item.Suggested fix
- bool hasStructuralWall = false; + bool hasStructuralWall = ChangeBuildStyleService::getStructuralWall(tile) != nullptr; bool hasCarpet = false; bool hasTable = false; @@ - if (item->isWall()) { + if (item->isWall()) { Brush* wb = item->getWallBrush(); - if (wb && !wb->isWallDecoration()) { - hasStructuralWall = true; - if (wb->visibleInPalette()) { - hasWall = true; - } + if (wb && !wb->isWallDecoration() && wb->visibleInPalette()) { + hasWall = true; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/map_display.cpp` around lines 2798 - 2807, The loop currently treats structural walls as any item where item->isWall(), but the action handlers use ChangeBuildStyleService::getStructuralWall(tile); update the logic in the loop to call ChangeBuildStyleService::getStructuralWall(tile) for the current tile (instead of item->isWall()), and use the returned structural wall item/brush to set hasStructuralWall and hasWall (checking the Brush via getWallBrush() and visibleInPalette()) so the menu visibility matches OnSelectWallBrush()/OnChangeBuildStyle() behavior.
🧹 Nitpick comments (1)
docs/superpowers/specs/2026-05-26-change-build-style-design.md (1)
96-100: 💤 Low valueMinor Portuguese language refinements.
Static analysis identified three stylistic improvements:
- Line 96: "todas as demais posicoes" contains a pleonasm; prefer "as demais posicoes"
- Line 98: "ate" should have an acute accent: "até"
- Line 100: "contem" should match subject agreement: "contém"
These do not affect technical clarity but would polish the document.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-26-change-build-style-design.md` around lines 96 - 100, Replace the three Portuguese wording issues in the document: change "todas as demais posicoes" to "as demais posicoes" (line containing "o tile simulado, e em todas as demais posicoes"), change "ate" to "até" (the occurrence in "real nao muda ate `Apply`"), and change "contem" to "contém" (the heading or sentence starting "O painel contem:"); update the three strings accordingly to fix pleonasm, add the accent, and correct agreement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/change_build_style_window.cpp`:
- Around line 135-137: showFloor currently just calls ChangeFloor(floor) which
changes the floor but doesn't compensate for CenterViewOnPosition's
floor-dependent offset, causing the view to drift; fix by preserving the current
view center before changing floors and re-centering after: in
ChangeBuildStylePreview::showFloor capture the current center/world position
(e.g., via whatever method returns the view center), call ChangeFloor(floor),
then call CenterViewOnPosition(...) with the previously captured center so the
visual center remains anchored; alternatively implement
ChangeBuildStylePreview::OnFloorChanged() to perform this re-centering and
invoke it after ChangeFloor; apply the same change to the similar code at the
other occurrence mentioned (lines 182-183).
- Around line 451-456: When buildPreview fails you must clear the previous
preview so the UI doesn't show stale data: inside the failure branch after
status->SetLabel(reason) and before preview->Refresh(), clear the preview's data
(e.g. call preview->getPreviewMap()->Clear() or the preview's clear/Reset
method) and ensure applyButton->Enable(false) remains; update the code in the
block handling service.buildPreview(target, selectedFloors(),
preview->getPreviewMap(), reason) to explicitly clear the preview map before
refreshing.
In `@source/change_build_style.cpp`:
- Around line 345-348: The buildPreview function can return false while leaving
previewTiles partially populated because simulate mutates it; change
buildPreview to call simulate(...) into a bool result (e.g., bool ok =
simulate(target, selectedFloors, previewTiles, changed, reason);), and if ok is
false then clear previewTiles (previewTiles.clear()) before returning; keep the
rest of the signature and ensure reason is forwarded unchanged.
In `@source/lua/lua_dialog.cpp`:
- Around line 233-240: CenterViewOnPosition subtracts a floorOffset from
view_x/view_y for z < 8 but GetMapX()/GetMapY() return raw values from
view_x/view_y, causing readback mismatch; fix by making GetMapX and GetMapY
apply the inverse of the centering offset (i.e., when GetMapZ() < 8 add back
(rme::MapGroundLayer - GetMapZ()) * rme::TileSize) so GetMapX/GetMapY/from
updateValue produce coordinates that are inverse-consistent with
CenterViewOnPosition (or alternatively store the centered z and compute the same
floorOffset there) — update the implementations of GetMapX, GetMapY (and any
code using view_x/view_y for map coordinate readback like updateValue) to use
that inverse offset.
---
Outside diff comments:
In `@source/map_display.cpp`:
- Around line 2798-2807: The loop currently treats structural walls as any item
where item->isWall(), but the action handlers use
ChangeBuildStyleService::getStructuralWall(tile); update the logic in the loop
to call ChangeBuildStyleService::getStructuralWall(tile) for the current tile
(instead of item->isWall()), and use the returned structural wall item/brush to
set hasStructuralWall and hasWall (checking the Brush via getWallBrush() and
visibleInPalette()) so the menu visibility matches
OnSelectWallBrush()/OnChangeBuildStyle() behavior.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-05-26-change-build-style-design.md`:
- Around line 96-100: Replace the three Portuguese wording issues in the
document: change "todas as demais posicoes" to "as demais posicoes" (line
containing "o tile simulado, e em todas as demais posicoes"), change "ate" to
"até" (the occurrence in "real nao muda ate `Apply`"), and change "contem" to
"contém" (the heading or sentence starting "O painel contem:"); update the three
strings accordingly to fix pleonasm, add the accent, and correct agreement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52be10be-e792-408c-92f3-0b91a6f46143
📒 Files selected for processing (10)
docs/superpowers/plans/2026-05-26-change-build-style.mddocs/superpowers/specs/2026-05-26-change-build-style-design.mdsource/change_build_style.cppsource/change_build_style.hsource/change_build_style_window.cppsource/change_build_style_window.hsource/lua/lua_dialog.cppsource/map_display.cppsource/map_display.hsource/map_drawer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/superpowers/plans/2026-05-26-change-build-style.md
Comment on lines
+135
to
+137
| void ChangeBuildStylePreview::showFloor(int floor) { | ||
| ChangeFloor(floor); | ||
| } |
There was a problem hiding this comment.
Keep the preview anchored when switching floors.
CenterViewOnPosition() bakes in a floor-dependent offset, but showFloor() only changes floor and OnFloorChanged() is empty. Stepping floors will drift the building by one tile per floor instead of keeping the same center point in view.
Suggested fix
void ChangeBuildStylePreview::showFloor(int floor) {
- ChangeFloor(floor);
+ int centerX = 0;
+ int centerY = 0;
+ GetScreenCenter(¢erX, ¢erY);
+ ChangeFloor(floor);
+ CenterViewOnPosition(Position(centerX, centerY, floor));
}Also applies to: 182-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_build_style_window.cpp` around lines 135 - 137, showFloor
currently just calls ChangeFloor(floor) which changes the floor but doesn't
compensate for CenterViewOnPosition's floor-dependent offset, causing the view
to drift; fix by preserving the current view center before changing floors and
re-centering after: in ChangeBuildStylePreview::showFloor capture the current
center/world position (e.g., via whatever method returns the view center), call
ChangeFloor(floor), then call CenterViewOnPosition(...) with the previously
captured center so the visual center remains anchored; alternatively implement
ChangeBuildStylePreview::OnFloorChanged() to perform this re-centering and
invoke it after ChangeFloor; apply the same change to the similar code at the
other occurrence mentioned (lines 182-183).
Comment on lines
+451
to
+456
| wxString reason; | ||
| if (!service.buildPreview(target, selectedFloors(), preview->getPreviewMap(), reason)) { | ||
| status->SetLabel(reason); | ||
| applyButton->Enable(false); | ||
| preview->Refresh(); | ||
| return; |
There was a problem hiding this comment.
Clear the old preview when rebuilding fails.
If buildPreview(...) fails after a previous success, the last preview stays rendered even though the dialog now shows an error and disables apply. That leaves the preview out of sync with the current selection.
Suggested fix
wxString reason;
if (!service.buildPreview(target, selectedFloors(), preview->getPreviewMap(), reason)) {
+ preview->getPreviewMap().clear();
status->SetLabel(reason);
applyButton->Enable(false);
preview->Refresh();
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| wxString reason; | |
| if (!service.buildPreview(target, selectedFloors(), preview->getPreviewMap(), reason)) { | |
| status->SetLabel(reason); | |
| applyButton->Enable(false); | |
| preview->Refresh(); | |
| return; | |
| wxString reason; | |
| if (!service.buildPreview(target, selectedFloors(), preview->getPreviewMap(), reason)) { | |
| preview->getPreviewMap().clear(); | |
| status->SetLabel(reason); | |
| applyButton->Enable(false); | |
| preview->Refresh(); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_build_style_window.cpp` around lines 451 - 456, When
buildPreview fails you must clear the previous preview so the UI doesn't show
stale data: inside the failure branch after status->SetLabel(reason) and before
preview->Refresh(), clear the preview's data (e.g. call
preview->getPreviewMap()->Clear() or the preview's clear/Reset method) and
ensure applyButton->Enable(false) remains; update the code in the block handling
service.buildPreview(target, selectedFloors(), preview->getPreviewMap(), reason)
to explicitly clear the preview map before refreshing.
Comment on lines
+345
to
+348
| bool ChangeBuildStyleService::buildPreview(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &previewTiles, wxString &reason) const { | ||
| std::set<Position> changed; | ||
| previewTiles.clear(); | ||
| return simulate(target, selectedFloors, previewTiles, changed, reason); |
There was a problem hiding this comment.
Clear partial preview state when preview generation fails.
simulate() mutates previewTiles incrementally. If it fails after copying or converting some tiles, this returns false with a partially populated preview map still attached to the output parameter.
Proposed fix
bool ChangeBuildStyleService::buildPreview(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &previewTiles, wxString &reason) const {
std::set<Position> changed;
previewTiles.clear();
- return simulate(target, selectedFloors, previewTiles, changed, reason);
+ if (!simulate(target, selectedFloors, previewTiles, changed, reason)) {
+ previewTiles.clear();
+ return false;
+ }
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool ChangeBuildStyleService::buildPreview(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &previewTiles, wxString &reason) const { | |
| std::set<Position> changed; | |
| previewTiles.clear(); | |
| return simulate(target, selectedFloors, previewTiles, changed, reason); | |
| bool ChangeBuildStyleService::buildPreview(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &previewTiles, wxString &reason) const { | |
| std::set<Position> changed; | |
| previewTiles.clear(); | |
| if (!simulate(target, selectedFloors, previewTiles, changed, reason)) { | |
| previewTiles.clear(); | |
| return false; | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_build_style.cpp` around lines 345 - 348, The buildPreview
function can return false while leaving previewTiles partially populated because
simulate mutates it; change buildPreview to call simulate(...) into a bool
result (e.g., bool ok = simulate(target, selectedFloors, previewTiles, changed,
reason);), and if ok is false then clear previewTiles (previewTiles.clear())
before returning; keep the rest of the signature and ensure reason is forwarded
unchanged.
Comment on lines
+233
to
+240
| void CenterViewOnPosition(const Position &position) override { | ||
| wxSize size = GetClientSize(); | ||
| const int width = size.GetWidth() > 0 ? size.GetWidth() : 400; | ||
| const int height = size.GetHeight() > 0 ? size.GetHeight() : 300; | ||
| const int floorOffset = position.z < 8 ? (rme::MapGroundLayer - position.z) * rme::TileSize : 0; | ||
| view_x = (position.x * 32) - floorOffset - (width * GetZoom()) / 2; | ||
| view_y = (position.y * 32) - floorOffset - (height * GetZoom()) / 2; | ||
| } |
There was a problem hiding this comment.
Centering math now disagrees with map-coordinate readback on upper floors.
At Line 237-239, CenterViewOnPosition subtracts a floor offset from view_x/view_y, but GetMapX() / GetMapY() still return raw center from view_x/view_y without adding that offset back. After centering on z < 8, readback (e.g. through updateValue) returns shifted coordinates.
Suggested fix (keep centering/readback inverse-consistent)
int GetMapX() {
wxSize size = GetClientSize();
int width = size.GetWidth() > 0 ? size.GetWidth() : 400;
- return (view_x + (width * (double)GetZoom()) / 2.0) / 32;
+ int mapX = (view_x + (width * (double)GetZoom()) / 2.0) / 32;
+ if (GetFloor() <= rme::MapGroundLayer) {
+ mapX += (rme::MapGroundLayer - GetFloor());
+ }
+ return mapX;
}
int GetMapY() {
wxSize size = GetClientSize();
int height = size.GetHeight() > 0 ? size.GetHeight() : 300;
- return (view_y + (height * (double)GetZoom()) / 2.0) / 32;
+ int mapY = (view_y + (height * (double)GetZoom()) / 2.0) / 32;
+ if (GetFloor() <= rme::MapGroundLayer) {
+ mapY += (rme::MapGroundLayer - GetFloor());
+ }
+ return mapY;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/lua/lua_dialog.cpp` around lines 233 - 240, CenterViewOnPosition
subtracts a floorOffset from view_x/view_y for z < 8 but GetMapX()/GetMapY()
return raw values from view_x/view_y, causing readback mismatch; fix by making
GetMapX and GetMapY apply the inverse of the centering offset (i.e., when
GetMapZ() < 8 add back (rme::MapGroundLayer - GetMapZ()) * rme::TileSize) so
GetMapX/GetMapY/from updateValue produce coordinates that are inverse-consistent
with CenterViewOnPosition (or alternatively store the centered z and compute the
same floorOffset there) — update the implementations of GetMapX, GetMapY (and
any code using view_x/view_y for map coordinate readback like updateValue) to
use that inverse offset.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/change_build_style.cpp (1)
506-510:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the attached-gate rewrite against null items.
This loop dereferences
itemunconditionally, but the rest of the file treatstile->itemsentries as nullable. A sparse/null slot here will crash the operation.Suggested fix
for (size_t index = 0; index < tile->items.size(); ++index) { Item* item = tile->items[index]; + if (!item) { + continue; + } const uint16_t replacementId = attachedGateReplacement(item->getID(), target); if (replacementId != 0) { transformItem(item, replacementId, tile); changed.insert(position); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/change_build_style.cpp` around lines 506 - 510, The loop over tile->items dereferences Item* item without null checking; update the loop in change_build_style.cpp (the block calling attachedGateReplacement and transformItem) to guard against nullptr entries by skipping any null item (e.g., continue when item == nullptr) before calling item->getID(), attachedGateReplacement(...) or transformItem(...), so sparse/null slots in tile->items do not cause a crash.source/map_display.cpp (1)
130-131: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAvoid assuming every
MapCanvasparent is aMapWindow.
ChangeBuildStylePreviewis now created with a dialog/widget parent (newd ChangeBuildStylePreview(this, editor, origin)), and it constructsMapCanvasasChangeBuildStylePreview(wxWindow* parent, ...) : MapCanvas(parent, editor, nullptr). IfMapCanvas::GetMapWindow()still assumes/castsGetParent()toMapWindow*(seesource/map_display.cpp, ~439-443), the preview canvases violate that contract and any inheritedMapCanvascode paths that callGetMapWindow()can be crash-prone. Harden the boundary by making theMapWindow*association explicit (store it) or by makingGetMapWindow()safe and/or overridden for preview canvases.Inspect
MapCanvas::GetMapWindow()(~439-443) and check whether any sharedMapCanvashandlers that callGetMapWindow()can execute onChangeBuildStylePreviewinstances.Also applies to: 439-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/map_display.cpp` around lines 130 - 131, GetMapWindow() currently assumes GetParent() is a MapWindow and casts unsafely; change the API so MapCanvas either stores an explicit MapWindow* (add an optional MapWindow* parameter to the MapCanvas constructor used by real map windows and saved in a member like mapWindow_) or make GetMapWindow() safe by using dynamic_cast<MapWindow*>(GetParent()) and returning nullptr when not a MapWindow, and then update any callers (handlers in MapCanvas or shared code) to check for nullptr before dereferencing; reference symbols: MapCanvas::MapCanvas(...), MapCanvas::GetMapWindow(), and ChangeBuildStylePreview(...) so preview canvases constructed with a non-MapWindow parent no longer crash.
♻️ Duplicate comments (3)
source/change_build_style.cpp (3)
363-374:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
reasonat the start ofsimulate().A successful call can currently leave the caller's old error text intact because
reasonis only assigned on failure paths.Suggested fix
bool ChangeBuildStyleService::simulate( WallBrush* target, const std::set<int> &selectedFloors, BaseMap &working, std::set<Position> &changed, wxString &reason, std::vector<ChangeBuildStyleConflict>* conflicts, ChangeBuildStyleMissingOpeningAction missingOpeningAction ) const { + reason.clear(); if (conflicts) { conflicts->clear(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/change_build_style.cpp` around lines 363 - 374, The simulate() function can return success while leaving a caller's previous error text in the wxString reason param; to fix, explicitly clear reason at the start of ChangeBuildStyleService::simulate (e.g., call reason.clear() before any early returns or error assignments). Locate the simulate method (signature ChangeBuildStyleService::simulate(..., wxString &reason, ...)) and add a reason.clear() as the first action (it can be placed before or immediately after the existing conflicts-clearing block) so successful paths never leave stale text.
527-530:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear partial preview state when preview generation fails.
simulate()mutatespreviewTilesincrementally, so this wrapper can returnfalsewith a half-built preview still in the output map.Suggested fix
bool ChangeBuildStyleService::buildPreview(WallBrush* target, const std::set<int> &selectedFloors, BaseMap &previewTiles, wxString &reason, std::vector<ChangeBuildStyleConflict>* conflicts) const { std::set<Position> changed; previewTiles.clear(); - return simulate(target, selectedFloors, previewTiles, changed, reason, conflicts, ChangeBuildStyleMissingOpeningAction::KeepExisting); + if (!simulate(target, selectedFloors, previewTiles, changed, reason, conflicts, ChangeBuildStyleMissingOpeningAction::KeepExisting)) { + previewTiles.clear(); + return false; + } + return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/change_build_style.cpp` around lines 527 - 530, buildPreview may return false while leaving previewTiles partially populated because simulate() mutates previewTiles incrementally; to fix, after calling simulate(...) from ChangeBuildStyleService::buildPreview, check the boolean result and if false, clear previewTiles (e.g., previewTiles.clear()) before returning, ensuring clients never receive a half-built map; keep all existing parameters and the call to simulate (including ChangeBuildStyleMissingOpeningAction::KeepExisting) but add the post-call conditional cleanup.
481-492:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the converted target wall when restoring openings.
The non-loose branch still calls
getStructuralWall(tile)after the first pass. On mixed tiles, that can grab an unrelated structural wall and turn the wrong item into a door/window.Suggested fix
- Item* wall = opening.loose ? findItemById(tile, opening.originalItemId) : getStructuralWall(tile); + Item* wall = opening.loose ? findItemById(tile, opening.originalItemId) : getStructuralWall(tile, target);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/change_build_style.cpp` around lines 481 - 492, The restore pass is using getStructuralWall(tile) which can pick an unrelated structural wall on mixed tiles; instead, when restoring an opening use the actual converted item placed in the first pass. Replace the second-call-to-getStructuralWall flow by locating the converted item on the tile (use openingReplacementIds[opening.position] to get the converted item id and findItemById(tile, convertedItemId) to fetch it) and pass that found item into transformItem rather than re-querying getStructuralWall; keep the loose branch using findItemById(opening.originalItemId) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-27-all-town-corpus-exporter.md`:
- Line 13: The markdown has a heading-level jump where "### Task 1: Update
Export Action Contract" follows the top-level title; change that line to an H2
(prefix with "##") so it becomes "## Task 1: Update Export Action Contract" or
insert an intermediate H2 section above it to avoid skipping from H1 to H3 and
satisfy MD001.
In `@docs/superpowers/plans/2026-05-27-city-corpus-exporter.md`:
- Line 13: The heading "### Task 1: Remove The Unsafe Entry Point" is a level-3
heading immediately under the document root and triggers MD001; update that
heading to level-2 (change "### Task 1: Remove The Unsafe Entry Point" to "##
Task 1: Remove The Unsafe Entry Point") or insert an intermediate H2 section
above it so the heading levels progress correctly; make the change where the
"Task 1" heading occurs in the file.
In `@docs/superpowers/plans/2026-05-27-city-learning-extractor.md`:
- Around line 13-60: Change the task-section headings from third-level to
second-level headings to normalize the document outline: replace "### Task 1:
Establish Tested Semantic Accumulators", "### Task 2: Stream Corpus V2 Into
Artifacts", "### Task 3: Process And Inspect The Real Corpus" and "### Task 4:
Version And Verify The Feature" with "## Task 1: Establish Tested Semantic
Accumulators", "## Task 2: Stream Corpus V2 Into Artifacts", "## Task 3: Process
And Inspect The Real Corpus" and "## Task 4: Version And Verify The Feature"
respectively so the heading levels are consistent and MD001 is resolved.
In `@docs/superpowers/specs/2026-05-27-city-learning-extractor-design.md`:
- Around line 5-13: Revise todo o documento para normalizar ortografia e
acentuação PT-BR (ex.: trocar "auditaveis" → "auditáveis", "arvore" → "árvore",
"será" com acento, "evidência", "até" onde aplicável) e corrija inconsistências
nas seções mencionadas; no texto inicial verifique e padronize termos chave como
rme-city-corpus, all-towns-city-corpus.json, "streaming" e nomes de entidades
(towns, distritos, tiles, itens) garantindo acentuação correta e concordância, e
aplique a mesma revisão às faixas de linhas referidas (23-34, 75-90, 104-113)
para que todo o spec use ortografia/diacríticos consistentes.
In `@source/change_connected_ground_style_window.cpp`:
- Around line 179-184: When service.buildPreview(target,
preview->getPreviewMap(), reason) returns false you need to clear any stale
preview data before refreshing so the old overlay is removed; call
preview->getPreviewMap().clear() (or otherwise reset the preview map returned by
getPreviewMap()) before preview->Refresh(), then keep the existing
status->SetLabel(reason) and applyButton->Enable(false) logic to ensure UI
reflects the failed generation.
In `@source/change_connected_ground_style.cpp`:
- Around line 232-235: The buildPreview implementation should ensure
previewTiles is empty if simulate() fails: currently buildPreview clears
previewTiles, calls simulate(target, previewTiles, reason), and returns its
result, but simulate may leave partial data on failure; modify
ChangeConnectedGroundStyleService::buildPreview so that after calling
simulate(target, previewTiles, reason) it checks the boolean result and, if
false, clears previewTiles again before returning, ensuring no partial preview
remains (keep the existing parameters GroundBrush* target, BaseMap
&previewTiles, wxString &reason).
In `@source/main_menubar.cpp`:
- Around line 892-901: Handlers MainMenuBar::OnExportCitySample and
MainMenuBar::OnExportAllTownCorpus only check g_gui.IsEditorOpen() but must
mirror the menu-state guards; update them to obtain Editor* editor =
g_gui.GetCurrentEditor() and verify editor != nullptr and !editor->IsLive()
before proceeding, and additionally for the selection export
(OnExportCitySample) check editor->hasSelection() (or the actual selection-check
method used by the menu logic) before calling CityCorpus::ExportSelection; keep
the existing calls to CityCorpus but skip/return when any guard fails so stale
accelerators or programmatic dispatch cannot run exports in unsupported states.
In `@tools/city_learning/generate_city_blueprint.py`:
- Around line 489-503: Detect the CLI "disabled" condition before any I/O: move
the unconditional GenerationError check to the top of this block so you do not
call arguments.model.read_text(), _load_templates(), generate_blueprint, or
render_blueprint_svg when the command is disabled; if the command is disabled
raise or return the existing GenerationError (preserving its message)
immediately, otherwise proceed to load the model, templates, generate the
blueprint, write arguments.output and optionally write arguments.preview.
In `@tools/city_learning/model.py`:
- Around line 243-287: The RLE streaming in _add_layout_tile/_flush_row assumes
tiles are added in (z,y,x) order but add_tile may receive out-of-order tiles;
detect and prevent silent corruption by either buffering and sorting pending
tiles before emitting runs (e.g., collect tiles per row until finalize() and
sort by x) or by failing fast on order regression: in add_tile/_add_layout_tile
check the incoming (z,y,x) against self._row_key and last run x, and raise an
explicit exception (or log an error and skip) if z or y has regressed or x is
less than the last emitted x; ensure layout_sink is only called from _flush_row
after rows are validated/sorted and update finalize() to flush any buffered,
sorted rows.
In `@tools/city_learning/README.md`:
- Around line 3-47: Polish the Portuguese README prose by adding missing
diacritics and normalizing vocabulary throughout the document (keep artifact
names like `city-learning-model.json`, `building-templates.jsonl.gz`,
`semantic-layouts.jsonl.gz` and code blocks unchanged); specifically update
words such as “papeis”→“papéis”, “predios”→“prédios”,
“construcoes”→“construções”, “copia”→“cópia”, “compativeis”→“compatíveis”,
“revisao”→“revisão”, “limitacao”→“limitação”, “geracao”→“geração”,
“renderizacao”→“renderização”, “le”→“lê” and similar occurrences in the sections
titled “Artefatos”, “Extrair Modelo”, “Geracao Desativada”, “Renderizacao OTBM”
and “Limitacao Atual” so the Portuguese reads naturally and consistently.
---
Outside diff comments:
In `@source/change_build_style.cpp`:
- Around line 506-510: The loop over tile->items dereferences Item* item without
null checking; update the loop in change_build_style.cpp (the block calling
attachedGateReplacement and transformItem) to guard against nullptr entries by
skipping any null item (e.g., continue when item == nullptr) before calling
item->getID(), attachedGateReplacement(...) or transformItem(...), so
sparse/null slots in tile->items do not cause a crash.
In `@source/map_display.cpp`:
- Around line 130-131: GetMapWindow() currently assumes GetParent() is a
MapWindow and casts unsafely; change the API so MapCanvas either stores an
explicit MapWindow* (add an optional MapWindow* parameter to the MapCanvas
constructor used by real map windows and saved in a member like mapWindow_) or
make GetMapWindow() safe by using dynamic_cast<MapWindow*>(GetParent()) and
returning nullptr when not a MapWindow, and then update any callers (handlers in
MapCanvas or shared code) to check for nullptr before dereferencing; reference
symbols: MapCanvas::MapCanvas(...), MapCanvas::GetMapWindow(), and
ChangeBuildStylePreview(...) so preview canvases constructed with a
non-MapWindow parent no longer crash.
---
Duplicate comments:
In `@source/change_build_style.cpp`:
- Around line 363-374: The simulate() function can return success while leaving
a caller's previous error text in the wxString reason param; to fix, explicitly
clear reason at the start of ChangeBuildStyleService::simulate (e.g., call
reason.clear() before any early returns or error assignments). Locate the
simulate method (signature ChangeBuildStyleService::simulate(..., wxString
&reason, ...)) and add a reason.clear() as the first action (it can be placed
before or immediately after the existing conflicts-clearing block) so successful
paths never leave stale text.
- Around line 527-530: buildPreview may return false while leaving previewTiles
partially populated because simulate() mutates previewTiles incrementally; to
fix, after calling simulate(...) from ChangeBuildStyleService::buildPreview,
check the boolean result and if false, clear previewTiles (e.g.,
previewTiles.clear()) before returning, ensuring clients never receive a
half-built map; keep all existing parameters and the call to simulate (including
ChangeBuildStyleMissingOpeningAction::KeepExisting) but add the post-call
conditional cleanup.
- Around line 481-492: The restore pass is using getStructuralWall(tile) which
can pick an unrelated structural wall on mixed tiles; instead, when restoring an
opening use the actual converted item placed in the first pass. Replace the
second-call-to-getStructuralWall flow by locating the converted item on the tile
(use openingReplacementIds[opening.position] to get the converted item id and
findItemById(tile, convertedItemId) to fetch it) and pass that found item into
transformItem rather than re-querying getStructuralWall; keep the loose branch
using findItemById(opening.originalItemId) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c7cbdc2-1713-4671-bba4-9168254abd47
📒 Files selected for processing (42)
.gitignoredata/menubar.xmldocs/change-build-style-wall-catalog.mddocs/superpowers/plans/2026-05-27-all-town-corpus-exporter.mddocs/superpowers/plans/2026-05-27-change-connected-ground-style.mddocs/superpowers/plans/2026-05-27-city-corpus-exporter.mddocs/superpowers/plans/2026-05-27-city-learning-extractor.mddocs/superpowers/specs/2026-05-26-city-corpus-learning-design.mddocs/superpowers/specs/2026-05-27-change-connected-ground-style-design.mddocs/superpowers/specs/2026-05-27-city-learning-extractor-design.mdscripts/city_lot_generator.luasource/CMakeLists.txtsource/action.cppsource/action.hsource/actions_history_window.cppsource/change_build_style.cppsource/change_build_style.hsource/change_build_style_window.cppsource/change_build_style_window.hsource/change_connected_ground_style.cppsource/change_connected_ground_style.hsource/change_connected_ground_style_window.cppsource/change_connected_ground_style_window.hsource/city_corpus.cppsource/city_corpus.hsource/gui_ids.hsource/lua/lua_script.cppsource/main_menubar.cppsource/main_menubar.hsource/map_display.cppsource/map_display.htests/__init__.pytests/city_learning/__init__.pytests/city_learning/test_extract_city_learning.pytests/city_learning/test_model.pytools/city_learning/README.mdtools/city_learning/__init__.pytools/city_learning/export_blueprint_lua.pytools/city_learning/extract_city_learning.pytools/city_learning/generate_city_blueprint.pytools/city_learning/model.pytools/city_learning/requirements.txt
✅ Files skipped from review due to trivial changes (5)
- tools/city_learning/init.py
- tests/init.py
- tests/city_learning/init.py
- .gitignore
- docs/change-build-style-wall-catalog.md
🚧 Files skipped from review as they are similar to previous changes (4)
- source/actions_history_window.cpp
- source/action.cpp
- source/CMakeLists.txt
- source/map_display.h
|
|
||
| --- | ||
|
|
||
| ### Task 1: Update Export Action Contract |
There was a problem hiding this comment.
Fix the heading-level jump.
### Task 1 comes directly after the top-level heading, which trips MD001. Promote it to ## Task 1 or add an intermediate H2 section.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-05-27-all-town-corpus-exporter.md` at line 13,
The markdown has a heading-level jump where "### Task 1: Update Export Action
Contract" follows the top-level title; change that line to an H2 (prefix with
"##") so it becomes "## Task 1: Update Export Action Contract" or insert an
intermediate H2 section above it to avoid skipping from H1 to H3 and satisfy
MD001.
|
|
||
| --- | ||
|
|
||
| ### Task 1: Remove The Unsafe Entry Point |
There was a problem hiding this comment.
Fix the heading-level jump.
### Task 1 comes directly after the top-level heading, which trips MD001. Promote it to ## Task 1 or add an intermediate H2 section.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-05-27-city-corpus-exporter.md` at line 13, The
heading "### Task 1: Remove The Unsafe Entry Point" is a level-3 heading
immediately under the document root and triggers MD001; update that heading to
level-2 (change "### Task 1: Remove The Unsafe Entry Point" to "## Task 1:
Remove The Unsafe Entry Point") or insert an intermediate H2 section above it so
the heading levels progress correctly; make the change where the "Task 1"
heading occurs in the file.
Comment on lines
+13
to
+60
| ### Task 1: Establish Tested Semantic Accumulators | ||
|
|
||
| **Files:** | ||
| - Create: `tools/city_learning/__init__.py` | ||
| - Create: `tools/city_learning/model.py` | ||
| - Create: `tests/city_learning/test_model.py` | ||
|
|
||
| - [ ] Write failing tests for deterministic semantic signatures, row RLE and | ||
| anchor-house footprint extraction. | ||
| - [ ] Run `python -m unittest tests.city_learning.test_model -v` and observe | ||
| missing-module/test failures before implementation. | ||
| - [ ] Implement focused accumulators for tile tags, layouts, house footprints | ||
| and ranked counters. | ||
| - [ ] Run the unit tests until they pass. | ||
|
|
||
| ### Task 2: Stream Corpus V2 Into Artifacts | ||
|
|
||
| **Files:** | ||
| - Create: `tools/city_learning/extract_city_learning.py` | ||
| - Create: `tools/city_learning/requirements.txt` | ||
| - Create: `tests/city_learning/test_extract_city_learning.py` | ||
|
|
||
| - [ ] Write a small v2 corpus fixture in the integration test and assert the | ||
| model, building gzip stream and layout gzip stream outputs. | ||
| - [ ] Run `python -m unittest tests.city_learning.test_extract_city_learning -v` | ||
| and verify it fails because the CLI reader is not implemented. | ||
| - [ ] Implement an `ijson.parse` state reader that constructs only current | ||
| house/tile records and finalizes one district at a time. | ||
| - [ ] Add CLI argument parsing, deterministic output writing and dependency | ||
| error reporting. | ||
| - [ ] Install `ijson` from the declared requirements and run all tests. | ||
|
|
||
| ### Task 3: Process And Inspect The Real Corpus | ||
|
|
||
| **Files:** | ||
| - Generate outside repository: | ||
| `D:/kl/canary/data-otservbr-global/world/city-learning/city-learning-model.json` | ||
| `D:/kl/canary/data-otservbr-global/world/city-learning/building-templates.jsonl.gz` | ||
| `D:/kl/canary/data-otservbr-global/world/city-learning/semantic-layouts.jsonl.gz` | ||
|
|
||
| - [ ] Run the extractor against | ||
| `D:/kl/canary/data-otservbr-global/world/all-towns-city-corpus.json`. | ||
| - [ ] Parse the generated compact model and verify 30 cities, 58 districts | ||
| and a nonzero prototype catalog. | ||
| - [ ] Report file sizes and counts so the next generator phase is based on | ||
| evidence rather than raw corpus assumptions. | ||
|
|
||
| ### Task 4: Version And Verify The Feature |
There was a problem hiding this comment.
Normalize the task heading levels.
These task sections jump from # straight to ###, which triggers MD001 and makes the document outline harder to scan. Use ## here instead.
Proposed fix
-### Task 1: Establish Tested Semantic Accumulators
+## Task 1: Establish Tested Semantic Accumulators
@@
-### Task 2: Stream Corpus V2 Into Artifacts
+## Task 2: Stream Corpus V2 Into Artifacts
@@
-### Task 3: Process And Inspect The Real Corpus
+## Task 3: Process And Inspect The Real Corpus
@@
-### Task 4: Version And Verify The Feature
+## Task 4: Version And Verify The Feature📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Task 1: Establish Tested Semantic Accumulators | |
| **Files:** | |
| - Create: `tools/city_learning/__init__.py` | |
| - Create: `tools/city_learning/model.py` | |
| - Create: `tests/city_learning/test_model.py` | |
| - [ ] Write failing tests for deterministic semantic signatures, row RLE and | |
| anchor-house footprint extraction. | |
| - [ ] Run `python -m unittest tests.city_learning.test_model -v` and observe | |
| missing-module/test failures before implementation. | |
| - [ ] Implement focused accumulators for tile tags, layouts, house footprints | |
| and ranked counters. | |
| - [ ] Run the unit tests until they pass. | |
| ### Task 2: Stream Corpus V2 Into Artifacts | |
| **Files:** | |
| - Create: `tools/city_learning/extract_city_learning.py` | |
| - Create: `tools/city_learning/requirements.txt` | |
| - Create: `tests/city_learning/test_extract_city_learning.py` | |
| - [ ] Write a small v2 corpus fixture in the integration test and assert the | |
| model, building gzip stream and layout gzip stream outputs. | |
| - [ ] Run `python -m unittest tests.city_learning.test_extract_city_learning -v` | |
| and verify it fails because the CLI reader is not implemented. | |
| - [ ] Implement an `ijson.parse` state reader that constructs only current | |
| house/tile records and finalizes one district at a time. | |
| - [ ] Add CLI argument parsing, deterministic output writing and dependency | |
| error reporting. | |
| - [ ] Install `ijson` from the declared requirements and run all tests. | |
| ### Task 3: Process And Inspect The Real Corpus | |
| **Files:** | |
| - Generate outside repository: | |
| `D:/kl/canary/data-otservbr-global/world/city-learning/city-learning-model.json` | |
| `D:/kl/canary/data-otservbr-global/world/city-learning/building-templates.jsonl.gz` | |
| `D:/kl/canary/data-otservbr-global/world/city-learning/semantic-layouts.jsonl.gz` | |
| - [ ] Run the extractor against | |
| `D:/kl/canary/data-otservbr-global/world/all-towns-city-corpus.json`. | |
| - [ ] Parse the generated compact model and verify 30 cities, 58 districts | |
| and a nonzero prototype catalog. | |
| - [ ] Report file sizes and counts so the next generator phase is based on | |
| evidence rather than raw corpus assumptions. | |
| ### Task 4: Version And Verify The Feature | |
| ## Task 1: Establish Tested Semantic Accumulators | |
| **Files:** | |
| - Create: `tools/city_learning/__init__.py` | |
| - Create: `tools/city_learning/model.py` | |
| - Create: `tests/city_learning/test_model.py` | |
| - [ ] Write failing tests for deterministic semantic signatures, row RLE and | |
| anchor-house footprint extraction. | |
| - [ ] Run `python -m unittest tests.city_learning.test_model -v` and observe | |
| missing-module/test failures before implementation. | |
| - [ ] Implement focused accumulators for tile tags, layouts, house footprints | |
| and ranked counters. | |
| - [ ] Run the unit tests until they pass. | |
| ## Task 2: Stream Corpus V2 Into Artifacts | |
| **Files:** | |
| - Create: `tools/city_learning/extract_city_learning.py` | |
| - Create: `tools/city_learning/requirements.txt` | |
| - Create: `tests/city_learning/test_extract_city_learning.py` | |
| - [ ] Write a small v2 corpus fixture in the integration test and assert the | |
| model, building gzip stream and layout gzip stream outputs. | |
| - [ ] Run `python -m unittest tests.city_learning.test_extract_city_learning -v` | |
| and verify it fails because the CLI reader is not implemented. | |
| - [ ] Implement an `ijson.parse` state reader that constructs only current | |
| house/tile records and finalizes one district at a time. | |
| - [ ] Add CLI argument parsing, deterministic output writing and dependency | |
| error reporting. | |
| - [ ] Install `ijson` from the declared requirements and run all tests. | |
| ## Task 3: Process And Inspect The Real Corpus | |
| **Files:** | |
| - Generate outside repository: | |
| `D:/kl/canary/data-otservbr-global/world/city-learning/city-learning-model.json` | |
| `D:/kl/canary/data-otservbr-global/world/city-learning/building-templates.jsonl.gz` | |
| `D:/kl/canary/data-otservbr-global/world/city-learning/semantic-layouts.jsonl.gz` | |
| - [ ] Run the extractor against | |
| `D:/kl/canary/data-otservbr-global/world/all-towns-city-corpus.json`. | |
| - [ ] Parse the generated compact model and verify 30 cities, 58 districts | |
| and a nonzero prototype catalog. | |
| - [ ] Report file sizes and counts so the next generator phase is based on | |
| evidence rather than raw corpus assumptions. | |
| ## Task 4: Version And Verify The Feature |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-05-27-city-learning-extractor.md` around lines 13
- 60, Change the task-section headings from third-level to second-level headings
to normalize the document outline: replace "### Task 1: Establish Tested
Semantic Accumulators", "### Task 2: Stream Corpus V2 Into Artifacts", "### Task
3: Process And Inspect The Real Corpus" and "### Task 4: Version And Verify The
Feature" with "## Task 1: Establish Tested Semantic Accumulators", "## Task 2:
Stream Corpus V2 Into Artifacts", "## Task 3: Process And Inspect The Real
Corpus" and "## Task 4: Version And Verify The Feature" respectively so the
heading levels are consistent and MD001 is resolved.
Comment on lines
+5
to
+13
| Transformar o corpus bruto `rme-city-corpus` v2 exportado pelo editor em | ||
| artefatos compactos e auditaveis que preservem tanto estatisticas como a | ||
| geometria necessaria para uma futura geracao procedural de cidades. | ||
|
|
||
| O primeiro corpus real, `all-towns-city-corpus.json`, possui aproximadamente | ||
| 7,8 GB, 30 towns, 58 distritos, 7 milhoes de tiles e 9,3 milhoes de itens. | ||
| Mesmo com 32 GB de RAM, carregar integralmente sua arvore JSON em Python cria | ||
| objetos com overhead elevado e impede escalar para mais mapas. O extrator sera | ||
| streaming desde a primeira versao. |
There was a problem hiding this comment.
Normalize the PT-BR spelling/diacritics across the spec.
There are several readability typos here (auditáveis, árvore, será, evidência, até, etc.). Cleaning them up will make the design doc look finished and easier to share.
Also applies to: 23-34, 75-90, 104-113
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Quando escrita sem acento, esta palavra é um verbo. Se pretende referir-se a um substantivo ou adjetivo, deve utilizar a forma acentuada.
Context: ...om 32 GB de RAM, carregar integralmente sua arvore JSON em Python cria objetos com overhea...
(PARONYM_ARVORE_0)
[misspelling] ~12-~12: Possível erro de acentuação. Verifique se não quis dizer “será”.
Context: ...ede escalar para mais mapas. O extrator sera streaming desde a primeira versao. ## ...
(DIACRITICS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-27-city-learning-extractor-design.md` around
lines 5 - 13, Revise todo o documento para normalizar ortografia e acentuação
PT-BR (ex.: trocar "auditaveis" → "auditáveis", "arvore" → "árvore", "será" com
acento, "evidência", "até" onde aplicável) e corrija inconsistências nas seções
mencionadas; no texto inicial verifique e padronize termos chave como
rme-city-corpus, all-towns-city-corpus.json, "streaming" e nomes de entidades
(towns, distritos, tiles, itens) garantindo acentuação correta e concordância, e
aplique a mesma revisão às faixas de linhas referidas (23-34, 75-90, 104-113)
para que todo o spec use ortografia/diacríticos consistentes.
Comment on lines
+179
to
+184
| wxString reason; | ||
| if (!service.buildPreview(target, preview->getPreviewMap(), reason)) { | ||
| status->SetLabel(reason); | ||
| preview->Refresh(); | ||
| applyButton->Enable(false); | ||
| return; |
There was a problem hiding this comment.
Clear stale preview data when preview generation fails.
If buildPreview(...) fails after a previous success, the old overlay stays visible even though the current selection is invalid. Clear the preview map in this branch before refreshing.
Suggested fix
wxString reason;
if (!service.buildPreview(target, preview->getPreviewMap(), reason)) {
+ preview->getPreviewMap().clear();
status->SetLabel(reason);
preview->Refresh();
applyButton->Enable(false);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| wxString reason; | |
| if (!service.buildPreview(target, preview->getPreviewMap(), reason)) { | |
| status->SetLabel(reason); | |
| preview->Refresh(); | |
| applyButton->Enable(false); | |
| return; | |
| wxString reason; | |
| if (!service.buildPreview(target, preview->getPreviewMap(), reason)) { | |
| preview->getPreviewMap().clear(); | |
| status->SetLabel(reason); | |
| preview->Refresh(); | |
| applyButton->Enable(false); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_connected_ground_style_window.cpp` around lines 179 - 184, When
service.buildPreview(target, preview->getPreviewMap(), reason) returns false you
need to clear any stale preview data before refreshing so the old overlay is
removed; call preview->getPreviewMap().clear() (or otherwise reset the preview
map returned by getPreviewMap()) before preview->Refresh(), then keep the
existing status->SetLabel(reason) and applyButton->Enable(false) logic to ensure
UI reflects the failed generation.
Comment on lines
+232
to
+235
| bool ChangeConnectedGroundStyleService::buildPreview(GroundBrush* target, BaseMap &previewTiles, wxString &reason) const { | ||
| previewTiles.clear(); | ||
| return simulate(target, previewTiles, reason); | ||
| } |
There was a problem hiding this comment.
Clear the preview map again when simulation fails.
simulate() can fail after it has already copied context or rewritten some tiles, so previewTiles can contain a partial preview on error paths.
Proposed fix
bool ChangeConnectedGroundStyleService::buildPreview(GroundBrush* target, BaseMap &previewTiles, wxString &reason) const {
previewTiles.clear();
- return simulate(target, previewTiles, reason);
+ if (!simulate(target, previewTiles, reason)) {
+ previewTiles.clear();
+ return false;
+ }
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool ChangeConnectedGroundStyleService::buildPreview(GroundBrush* target, BaseMap &previewTiles, wxString &reason) const { | |
| previewTiles.clear(); | |
| return simulate(target, previewTiles, reason); | |
| } | |
| bool ChangeConnectedGroundStyleService::buildPreview(GroundBrush* target, BaseMap &previewTiles, wxString &reason) const { | |
| previewTiles.clear(); | |
| if (!simulate(target, previewTiles, reason)) { | |
| previewTiles.clear(); | |
| return false; | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/change_connected_ground_style.cpp` around lines 232 - 235, The
buildPreview implementation should ensure previewTiles is empty if simulate()
fails: currently buildPreview clears previewTiles, calls simulate(target,
previewTiles, reason), and returns its result, but simulate may leave partial
data on failure; modify ChangeConnectedGroundStyleService::buildPreview so that
after calling simulate(target, previewTiles, reason) it checks the boolean
result and, if false, clears previewTiles again before returning, ensuring no
partial preview remains (keep the existing parameters GroundBrush* target,
BaseMap &previewTiles, wxString &reason).
Comment on lines
+892
to
901
| void MainMenuBar::OnExportCitySample(wxCommandEvent &WXUNUSED(event)) { | ||
| if (g_gui.IsEditorOpen()) { | ||
| CityCorpus::ExportSelection(*g_gui.GetCurrentEditor(), frame); | ||
| } | ||
|
|
||
| // CipSoft cyclopedia assets are expected around 2 px/tile for satellite (scale = 1/16). | ||
| const int satellitePixelsPerSquare = 2; | ||
|
|
||
| wxString outputPath; | ||
| if (!selectAssetsOrCustomExportFolder(g_gui.root, "Export Cyclopedia Map Data", outputPath)) { | ||
| return; | ||
| } | ||
|
|
||
| IOMapOTBM mapsaver(g_gui.GetCurrentMap().getVersion()); | ||
| cyclopediaExportRunning = true; | ||
| struct CyclopediaExportGuard final { | ||
| bool &running; | ||
| explicit CyclopediaExportGuard(bool &value) : | ||
| running(value) { } | ||
| CyclopediaExportGuard(const CyclopediaExportGuard &) = delete; | ||
| CyclopediaExportGuard &operator=(const CyclopediaExportGuard &) = delete; | ||
| ~CyclopediaExportGuard() { | ||
| running = false; | ||
| } | ||
| }; | ||
| CyclopediaExportGuard exportGuard(cyclopediaExportRunning); | ||
|
|
||
| int lastCyclopediaExportStatusPercent = -1; | ||
| auto lastCyclopediaExportStatusUpdate = std::chrono::steady_clock::now(); | ||
| auto updateCyclopediaExportStatus = [&](const int32_t done, const std::string &message, const bool force = false) { | ||
| const int clampedDone = std::max<int32_t>(0, std::min<int32_t>(100, done)); | ||
| const auto now = std::chrono::steady_clock::now(); | ||
| if (!force && clampedDone == lastCyclopediaExportStatusPercent && now - lastCyclopediaExportStatusUpdate < std::chrono::milliseconds(CyclopediaExportStatusMinIntervalMs)) { | ||
| return true; | ||
| } | ||
|
|
||
| lastCyclopediaExportStatusPercent = clampedDone; | ||
| lastCyclopediaExportStatusUpdate = now; | ||
| const wxString progressMessage = message.empty() ? wxString("Exporting cyclopedia minimap/satellite...") : wxstr(message); | ||
| g_gui.SetStatusText(wxString::Format("Cyclopedia export: %d%% - %s", clampedDone, progressMessage.c_str())); | ||
| return true; | ||
| }; | ||
| updateCyclopediaExportStatus(0, "Preparing cyclopedia export...", true); | ||
|
|
||
| if (!mapsaver.saveCyclopediaMapData( | ||
| g_gui.GetCurrentMap(), makeDirectoryFileName(outputPath), [&](const int32_t done, const std::string &message) { | ||
| return updateCyclopediaExportStatus(done, message); | ||
| }, | ||
| satellitePixelsPerSquare | ||
| )) { | ||
| g_gui.SetStatusText("Cyclopedia export failed."); | ||
| g_gui.PopupDialog("Error", "Failed to export cyclopedia minimap/satellite.", wxOK); | ||
| return; | ||
| } | ||
|
|
||
| updateCyclopediaExportStatus(100, "Cyclopedia export completed.", true); | ||
| const wxString outputBasePath = resolveCyclopediaOutputDisplayPath(outputPath); | ||
| const wxString backupPath = appendDisplaySubdirectory(outputBasePath, "bkps"); | ||
| g_gui.SetStatusText(wxString::Format("Cyclopedia export completed: %s", outputBasePath.c_str())); | ||
| g_gui.PopupDialog( | ||
| "Export completed", | ||
| wxString::Format( | ||
| "Cyclopedia minimap/satellite exported successfully.\n\nOutput: %s\nBackups: %s (backup-only)", | ||
| outputBasePath.c_str(), | ||
| backupPath.c_str() | ||
| ), | ||
| wxOK | ||
| ); | ||
| } | ||
|
|
||
| void MainMenuBar::OnRevertCyclopediaAssets(wxCommandEvent &) { | ||
| wxString selectedPath; | ||
| if (!selectAssetsOrCustomRestoreFolder(g_gui.root, "Restore Client Assets Backup", selectedPath)) { | ||
| return; | ||
| } | ||
|
|
||
| const wxString outputBasePath = resolveCyclopediaOutputDisplayPath(selectedPath); | ||
| const std::filesystem::path basePath(nstr(outputBasePath)); | ||
|
|
||
| std::vector<AssetsBackupSnapshot> snapshots; | ||
| if (!collectBackupSnapshots(basePath, snapshots)) { | ||
| g_gui.PopupDialog( | ||
| "No backups found", | ||
| wxString::Format("No restorable backups were found in:\n%s", appendDisplaySubdirectory(outputBasePath, "bkps").c_str()), | ||
| wxOK | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| wxArrayString choices; | ||
| for (const AssetsBackupSnapshot &snapshot : snapshots) { | ||
| choices.Add(snapshot.label); | ||
| } | ||
|
|
||
| wxSingleChoiceDialog snapshotDialog( | ||
| g_gui.root, | ||
| "Select which backup snapshot should be restored.", | ||
| "Restore Client Assets Backup", | ||
| choices | ||
| ); | ||
| snapshotDialog.SetSelection(0); | ||
| if (snapshotDialog.ShowModal() != wxID_OK) { | ||
| return; | ||
| } | ||
|
|
||
| const int selectedSnapshotIndex = snapshotDialog.GetSelection(); | ||
| if (selectedSnapshotIndex < 0 || static_cast<size_t>(selectedSnapshotIndex) >= snapshots.size()) { | ||
| return; | ||
| } | ||
|
|
||
| const AssetsBackupSnapshot &snapshot = snapshots[static_cast<size_t>(selectedSnapshotIndex)]; | ||
| const wxString confirmMessage = wxString::Format( | ||
| "Restore this backup snapshot?\n\nAssets folder:\n%s\n\nBackup:\n%s\n\nCurrent files that would be overwritten will be moved to a new restore backup snapshot first.", | ||
| outputBasePath.c_str(), | ||
| snapshot.label.c_str() | ||
| ); | ||
| if (wxMessageBox(confirmMessage, "Confirm Restore", wxYES_NO | wxNO_DEFAULT | wxICON_WARNING, g_gui.root) != wxYES) { | ||
| return; | ||
| } | ||
|
|
||
| size_t restoredFileCount = 0; | ||
| std::filesystem::path safetySnapshotPath; | ||
| wxString errorMessage; | ||
| if (!restoreBackupSnapshot(basePath, snapshot, restoredFileCount, safetySnapshotPath, errorMessage)) { | ||
| g_gui.PopupDialog( | ||
| "Restore failed", | ||
| errorMessage.empty() ? wxString("Failed to restore selected backup snapshot.") : errorMessage, | ||
| wxOK | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| g_gui.PopupDialog( | ||
| "Restore completed", | ||
| wxString::Format( | ||
| "Restored %llu files from backup.\n\nAssets folder: %s\nSafety backup: %s", | ||
| static_cast<unsigned long long>(restoredFileCount), | ||
| outputBasePath.c_str(), | ||
| wxstr(safetySnapshotPath.string()).c_str() | ||
| ), | ||
| wxOK | ||
| ); | ||
| } | ||
|
|
||
| void MainMenuBar::OnExportTilesets(wxCommandEvent &WXUNUSED(event)) { | ||
| if (g_gui.GetCurrentEditor()) { | ||
| ExportTilesetsWindow dlg(frame, *g_gui.GetCurrentEditor()); | ||
| dlg.ShowModal(); | ||
| dlg.Destroy(); | ||
| void MainMenuBar::OnExportAllTownCorpus(wxCommandEvent &WXUNUSED(event)) { | ||
| if (g_gui.IsEditorOpen()) { | ||
| CityCorpus::ExportAllTowns(*g_gui.GetCurrentEditor(), frame); | ||
| } |
There was a problem hiding this comment.
Mirror the menu-state guards inside these handlers.
Lines 358-359 only enable these actions for local maps, and EXPORT_CITY_SAMPLE also requires a selection, but the handlers only check IsEditorOpen(). That means a stale accelerator or programmatic dispatch can still run exports in unsupported states. Guard editor != nullptr, !editor->IsLive(), and editor->hasSelection() for the selection export before calling CityCorpus.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/main_menubar.cpp` around lines 892 - 901, Handlers
MainMenuBar::OnExportCitySample and MainMenuBar::OnExportAllTownCorpus only
check g_gui.IsEditorOpen() but must mirror the menu-state guards; update them to
obtain Editor* editor = g_gui.GetCurrentEditor() and verify editor != nullptr
and !editor->IsLive() before proceeding, and additionally for the selection
export (OnExportCitySample) check editor->hasSelection() (or the actual
selection-check method used by the menu logic) before calling
CityCorpus::ExportSelection; keep the existing calls to CityCorpus but
skip/return when any guard fails so stale accelerators or programmatic dispatch
cannot run exports in unsupported states.
Comment on lines
+489
to
+503
| try: | ||
| model = json.loads(arguments.model.read_text(encoding="utf-8")) | ||
| blueprint = generate_blueprint( | ||
| model, | ||
| _load_templates(arguments.templates), | ||
| arguments.width, | ||
| arguments.height, | ||
| arguments.seed, | ||
| arguments.source_city, | ||
| ) | ||
| arguments.output.parent.mkdir(parents=True, exist_ok=True) | ||
| arguments.output.write_text(json.dumps(blueprint, ensure_ascii=True, indent=2) + "\n", encoding="utf-8") | ||
| if arguments.preview: | ||
| arguments.preview.parent.mkdir(parents=True, exist_ok=True) | ||
| arguments.preview.write_text(render_blueprint_svg(blueprint), encoding="utf-8") |
There was a problem hiding this comment.
Short-circuit the disabled CLI before loading inputs.
The command is documented as a defensive stub, but it still opens and parses the model/templates before hitting the unconditional GenerationError. That means a missing file or bad JSON can hide the intended disabled message, and large inputs are read for no reason.
💡 Suggested fix
def main(argv: list[str] | None = None) -> int:
parser = argparse.ArgumentParser(description=__doc__)
@@
parser.add_argument("--height", type=int, required=True)
parser.add_argument("--seed", type=int, default=1)
arguments = parser.parse_args(argv)
+ print(f"[city-generation] error: {DISABLED_MESSAGE}", file=sys.stderr)
+ return 1
try:
model = json.loads(arguments.model.read_text(encoding="utf-8"))
blueprint = generate_blueprint(
model,
_load_templates(arguments.templates),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/city_learning/generate_city_blueprint.py` around lines 489 - 503,
Detect the CLI "disabled" condition before any I/O: move the unconditional
GenerationError check to the top of this block so you do not call
arguments.model.read_text(), _load_templates(), generate_blueprint, or
render_blueprint_svg when the command is disabled; if the command is disabled
raise or return the existing GenerationError (preserving its message)
immediately, otherwise proceed to load the model, templates, generate the
blueprint, write arguments.output and optionally write arguments.preview.
Comment on lines
+243
to
+287
| def add_tile(self, tile: dict[str, Any]) -> None: | ||
| position = tile["position"] | ||
| signature = semantic_signature(tile) | ||
| self._add_layout_tile(position["x"], position["y"], position["z"], signature) | ||
| self.tile_count += 1 | ||
| self.floor_counts[position["z"]] += 1 | ||
| self.signature_counts[signature] += 1 | ||
| tags = tile_semantic_tags(tile) | ||
| self.tag_counts.update(tags) | ||
| if is_street_tile(tile): | ||
| self.street_positions_by_floor[position["z"]].add((position["x"], position["y"])) | ||
| ground = tile.get("ground", {}) | ||
| style = ground.get("brush") or ground.get("name") or "tagged-road" | ||
| self.street_styles_by_floor[position["z"]][str(style)] += 1 | ||
| for item in _items_including_ground(tile): | ||
| for tag in item.get("tags", ()): | ||
| if tag in SEMANTIC_TAGS: | ||
| self.item_ids_by_tag[tag][item["id"]] += 1 | ||
| house_id = tile.get("houseId") | ||
| if house_id in self.anchor_houses: | ||
| self.anchor_houses[house_id].add_tile(tile) | ||
|
|
||
| def _add_layout_tile(self, x: int, y: int, z: int, signature: str) -> None: | ||
| row_key = (z, y) | ||
| if self._row_key != row_key: | ||
| self._flush_row() | ||
| self._row_key = row_key | ||
| if self._row_runs and x == self._row_runs[-1][0] + self._row_runs[-1][1] and signature == self._row_runs[-1][2]: | ||
| self._row_runs[-1][1] += 1 | ||
| else: | ||
| self._row_runs.append([x, 1, signature]) | ||
|
|
||
| def _flush_row(self) -> None: | ||
| if self._row_key is None or not self._row_runs: | ||
| return | ||
| z, y = self._row_key | ||
| self.layout_sink({ | ||
| "source": self.source, | ||
| "townId": self.town["id"], | ||
| "districtIndex": self.district.get("index"), | ||
| "z": z, | ||
| "y": y, | ||
| "runs": self._row_runs, | ||
| }) | ||
| self._row_runs = [] |
There was a problem hiding this comment.
Reject or normalize out-of-order tiles before streaming runs.
This RLE path only works if add_tile() arrives pre-sorted by (z, y, x). A later tile for an already-flushed row, or a smaller x on the same row, will emit duplicate rows and fragmented runs with no error. Please either buffer/sort before finalize() or fail fast when input order regresses so the corpus layout cannot be silently corrupted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/city_learning/model.py` around lines 243 - 287, The RLE streaming in
_add_layout_tile/_flush_row assumes tiles are added in (z,y,x) order but
add_tile may receive out-of-order tiles; detect and prevent silent corruption by
either buffering and sorting pending tiles before emitting runs (e.g., collect
tiles per row until finalize() and sort by x) or by failing fast on order
regression: in add_tile/_add_layout_tile check the incoming (z,y,x) against
self._row_key and last run x, and raise an explicit exception (or log an error
and skip) if z or y has regressed or x is less than the last emitted x; ensure
layout_sink is only called from _flush_row after rows are validated/sorted and
update finalize() to flush any buffered, sorted rows.
Comment on lines
+3
to
+47
| Esta etapa fica limitada a aprendizado e exportacao de dados de leitura. A | ||
| geracao semantica de cidades e a escrita automatica no mapa estao desativadas. | ||
|
|
||
| ## Artefatos | ||
|
|
||
| - `city-learning-model.json`: modelo v2 com papeis de predios, grafo de ruas | ||
| por distrito e qualificacao de fonte. | ||
| - `building-templates.jsonl.gz`: footprints observados de construcoes. | ||
| - `semantic-layouts.jsonl.gz`: linhas semanticas compactadas do corpus. | ||
|
|
||
| ## Extrair Modelo | ||
|
|
||
| ```powershell | ||
| python tools\city_learning\extract_city_learning.py ` | ||
| D:\kl\canary\data-otservbr-global\world\all-towns-city-corpus.json ` | ||
| --output D:\kl\canary\data-otservbr-global\world\city-learning-v2 ` | ||
| --quiet | ||
| ``` | ||
|
|
||
| O modelo reconhece vias a partir do brush real do ground. Isso evita contar | ||
| `sandstone mountain` como rua apenas porque o item visual se chama | ||
| `sandstone`, e rejeita capturas em que uma ponte isolada seria confundida | ||
| com um centro urbano. | ||
|
|
||
| ## Geracao Desativada | ||
|
|
||
| Os comandos `generate_city_blueprint.py` e `export_blueprint_lua.py` existem | ||
| apenas como stubs defensivos e retornam erro. O script antigo | ||
| `scripts/city_lot_generator.lua` tambem aborta imediatamente. Isso impede que | ||
| uma funcao experimental escreva ruas, lotes ou construcoes no mapa. | ||
|
|
||
| ## Renderizacao OTBM | ||
|
|
||
| `otbm-render` le `.otbm`, `.dat`, `.spr` e `.otb`, mas a copia publica inclui | ||
| assets antigos do cliente 10.41. Os ids do mapa Canary requerem assets | ||
| compativeis; usar os sprites incluidos no renderer pode mostrar tiles errados. | ||
| Por isso a revisao SVG verifica topologia, enquanto a revisao visual final | ||
| deve ocorrer no editor ou em um renderer configurado com os assets do mesmo | ||
| cliente. | ||
|
|
||
| ## Limitacao Atual | ||
|
|
||
| O pipeline nao cria cidades. Ele serve apenas para entender padroes de mapas | ||
| existentes e gerar arquivos de corpus/modelo para inspecao ou ferramentas | ||
| futuras aprovadas separadamente. |
There was a problem hiding this comment.
Polish the Portuguese copy before merging.
Several user-facing words are missing diacritics, which makes the README look unfinished in Portuguese. Please normalize the prose in this block (papéis, prédios, construções, cópia, compatíveis, revisão, limitação, etc.).
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: Sinal de pontuação isolado.
Context: ... Artefatos - city-learning-model.json: modelo v2 com papeis de predios, grafo ...
(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~8-~8: Quando escrita sem acento, esta palavra é um verbo. Se pretende referir-se a um substantivo ou adjetivo, deve utilizar a forma acentuada.
Context: ...ity-learning-model.json`: modelo v2 com papeis de predios, grafo de ruas por distrit...
(DIACRITICS)
[uncategorized] ~10-~10: Sinal de pontuação isolado.
Context: ...e fonte. - building-templates.jsonl.gz: footprints observados de construcoes. -...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~11-~11: Sinal de pontuação isolado.
Context: ...nstrucoes. - semantic-layouts.jsonl.gz: linhas semanticas compactadas do corpus...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~36-~36: Quando escrita sem acento, esta palavra é um verbo. Se pretende referir-se a um substantivo ou adjetivo, deve utilizar a forma acentuada.
Context: ....otbm, .dat, .spr e .otb, mas a copia publica inclui assets antigos do client...
(LP_PARONYMS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/city_learning/README.md` around lines 3 - 47, Polish the Portuguese
README prose by adding missing diacritics and normalizing vocabulary throughout
the document (keep artifact names like `city-learning-model.json`,
`building-templates.jsonl.gz`, `semantic-layouts.jsonl.gz` and code blocks
unchanged); specifically update words such as “papeis”→“papéis”,
“predios”→“prédios”, “construcoes”→“construções”, “copia”→“cópia”,
“compativeis”→“compatíveis”, “revisao”→“revisão”, “limitacao”→“limitação”,
“geracao”→“geração”, “renderizacao”→“renderização”, “le”→“lê” and similar
occurrences in the sections titled “Artefatos”, “Extrair Modelo”, “Geracao
Desativada”, “Renderizacao OTBM” and “Limitacao Atual” so the Portuguese reads
naturally and consistently.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/main_menubar.cpp (1)
1602-1612:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the menu-state guards inside these handlers.
The export handlers check only
g_gui.IsEditorOpen(), but the menu enables them with stricter conditions (is_local && has_selectionfor EXPORT_CITY_SAMPLE,is_localfor EXPORT_ALL_TOWN_CORPUS). Stale accelerators or programmatic dispatch can bypass disabled menu items, so handlers must validateeditor != nullptr,!editor->IsLive(), and for the selection export alsoeditor->hasSelection()before proceeding.🛡️ Proposed fix to add runtime guards
void MainMenuBar::OnExportCitySample(wxCommandEvent &WXUNUSED(event)) { - if (g_gui.IsEditorOpen()) { - CityCorpus::ExportSelection(*g_gui.GetCurrentEditor(), frame); + Editor* editor = g_gui.GetCurrentEditor(); + if (editor && !editor->IsLive() && editor->hasSelection()) { + CityCorpus::ExportSelection(*editor, frame); } } void MainMenuBar::OnExportAllTownCorpus(wxCommandEvent &WXUNUSED(event)) { - if (g_gui.IsEditorOpen()) { - CityCorpus::ExportAllTowns(*g_gui.GetCurrentEditor(), frame); + Editor* editor = g_gui.GetCurrentEditor(); + if (editor && !editor->IsLive()) { + CityCorpus::ExportAllTowns(*editor, frame); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/main_menubar.cpp` around lines 1602 - 1612, The handlers MainMenuBar::OnExportCitySample and MainMenuBar::OnExportAllTownCorpus currently only check g_gui.IsEditorOpen(); update them to fetch the editor via g_gui.GetCurrentEditor(), verify editor != nullptr and !editor->IsLive() before calling CityCorpus::ExportSelection or CityCorpus::ExportAllTowns, and for OnExportCitySample also verify editor->hasSelection() (or equivalent) before proceeding; bail out early if any check fails to mirror the menu enablement logic and prevent stale-accelerator/dispatch misuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/main_menubar.cpp`:
- Around line 2532-2540: The handler MainMenuBar::OnChangeCityStyle currently
only checks for a non-null Editor from g_gui.GetCurrentEditor but not whether
the editor session is live; add the same guard used by the menu state by
returning early when editor->IsLive() is true (i.e. only proceed when
!editor->IsLive()), before constructing ChangeCityStyleDialog(frame, *editor)
and calling g_gui.RefreshView(), so stale accelerators cannot act on live
sessions.
---
Duplicate comments:
In `@source/main_menubar.cpp`:
- Around line 1602-1612: The handlers MainMenuBar::OnExportCitySample and
MainMenuBar::OnExportAllTownCorpus currently only check g_gui.IsEditorOpen();
update them to fetch the editor via g_gui.GetCurrentEditor(), verify editor !=
nullptr and !editor->IsLive() before calling CityCorpus::ExportSelection or
CityCorpus::ExportAllTowns, and for OnExportCitySample also verify
editor->hasSelection() (or equivalent) before proceeding; bail out early if any
check fails to mirror the menu enablement logic and prevent
stale-accelerator/dispatch misuse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6018402-d4f4-445e-9806-7055d65bb3a8
📒 Files selected for processing (12)
data/menubar.xmldocs/superpowers/specs/2026-05-31-change-city-style-design.mdsource/CMakeLists.txtsource/action.cppsource/action.hsource/actions_history_window.cppsource/change_city_style.cppsource/change_city_style.hsource/change_city_style_window.cppsource/change_city_style_window.hsource/main_menubar.cppsource/main_menubar.h
✅ Files skipped from review due to trivial changes (2)
- docs/superpowers/specs/2026-05-31-change-city-style-design.md
- data/menubar.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- source/action.h
- source/actions_history_window.cpp
- source/action.cpp
- source/CMakeLists.txt
Comment on lines
+2532
to
+2540
| void MainMenuBar::OnChangeCityStyle(wxCommandEvent &WXUNUSED(event)) { | ||
| Editor* editor = g_gui.GetCurrentEditor(); | ||
| if (!editor) { | ||
| return; | ||
| } | ||
| ChangeCityStyleDialog dialog(frame, *editor); | ||
| dialog.ShowModal(); | ||
| g_gui.RefreshView(); | ||
| } |
There was a problem hiding this comment.
Add IsLive() guard to mirror menu-state logic.
The menu enables CHANGE_CITY_STYLE only for is_local maps (line 925), which means has_map && !is_live. The handler checks editor != nullptr but not !editor->IsLive(). Stale accelerators can invoke the handler even when the menu item is disabled, so add an explicit live-session check.
🛡️ Proposed fix
void MainMenuBar::OnChangeCityStyle(wxCommandEvent &WXUNUSED(event)) {
Editor* editor = g_gui.GetCurrentEditor();
- if (!editor) {
+ if (!editor || editor->IsLive()) {
return;
}
ChangeCityStyleDialog dialog(frame, *editor);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void MainMenuBar::OnChangeCityStyle(wxCommandEvent &WXUNUSED(event)) { | |
| Editor* editor = g_gui.GetCurrentEditor(); | |
| if (!editor) { | |
| return; | |
| } | |
| ChangeCityStyleDialog dialog(frame, *editor); | |
| dialog.ShowModal(); | |
| g_gui.RefreshView(); | |
| } | |
| void MainMenuBar::OnChangeCityStyle(wxCommandEvent &WXUNUSED(event)) { | |
| Editor* editor = g_gui.GetCurrentEditor(); | |
| if (!editor || editor->IsLive()) { | |
| return; | |
| } | |
| ChangeCityStyleDialog dialog(frame, *editor); | |
| dialog.ShowModal(); | |
| g_gui.RefreshView(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/main_menubar.cpp` around lines 2532 - 2540, The handler
MainMenuBar::OnChangeCityStyle currently only checks for a non-null Editor from
g_gui.GetCurrentEditor but not whether the editor session is live; add the same
guard used by the menu state by returning early when editor->IsLive() is true
(i.e. only proceed when !editor->IsLive()), before constructing
ChangeCityStyleDialog(frame, *editor) and calling g_gui.RefreshView(), so stale
accelerators cannot act on live sessions.
|
Hi there! Thanks for the contribution. Could you detail the main goal of your PR and its practical impact? In a preliminary review, I noticed the code deviates from our project standards and generated over 3,000 SonarQube alerts. Since compliance with these guidelines is required for a merge, what are your plans for this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores