refactor(minimap): extract caching, viewport math, and decouple UI, new controls - #1003
Conversation
Extracted minimap rendering into a paged GL texture cache (minimap_cache) to replace the monolithic updateRegion approach, enabling dirty-rect-based incremental uploads. Moved ScreenToMap/MapToScreen coordinate math into map_view_math module with explicit viewport structs. Split MinimapWindow into a container panel (MinimapWindow) with header controls and a GL rendering surface (MinimapCanvas), adding zoom/floor stepper buttons and a help overlay. Introduced MinimapViewportState per MapWindow to track center, zoom step, and floor independently of the main view, with sync hooks on scroll/zoom/floor-change. Added Tile::modify() implementation that marks tiles dirty in the minimap manager on mutation. Wired dirty tracking through Action commit/undo and replacement_engine to trigger minimap cache invalidations. Added IsLoading() guard to LoadingManager and GUI to suppress minimap updates during load. Changed Position forward declaration from class to struct in shared headers. [minor] CMakeLists.txt — registered new source files
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the minimap system to improve performance and maintainability. By introducing a paged texture cache and decoupling the coordinate math and UI components, the system now supports efficient incremental updates and provides a more robust foundation for future UI enhancements. The changes also include better integration with the editor's action system to ensure the minimap remains synchronized with map modifications. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughRefactors minimap rendering into a cache-driven system, adds MainMapViewMath for coordinate conversions, introduces MinimapViewportState and per-map pending invalidation, moves GL rendering into a MinimapCanvas, and wires tile/editor changes to mark minimap dirtiness and trigger updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MapWindow
participant MainMapViewMath
participant MinimapWindow
participant MinimapCanvas
User->>MapWindow: Scroll / Zoom / Change floor
MapWindow->>MainMapViewMath: WorldTileToScrollPixel / ScreenToMap
MainMapViewMath-->>MapWindow: Scroll/map coordinates
MapWindow->>MinimapWindow: Resume/Sync tracked viewport
MinimapWindow->>MinimapCanvas: RefreshMinimap(immediate)
MinimapCanvas->>MinimapCanvas: Build visible rect, request flush/render
sequenceDiagram
actor Editor
participant Tile
participant MinimapManager
participant MinimapRenderer
participant MinimapCache
participant GPU
Editor->>Tile: modify()/tile changes
Tile->>MinimapManager: MarkTileDirty(position)
MinimapManager->>MinimapManager: Queue per-layer dirty rects
MinimapRenderer->>MinimapManager: TakePendingInvalidation()
MinimapManager-->>MinimapRenderer: Pending invalidation
MinimapRenderer->>MinimapCache: markDirty(floor, rect)
MinimapCache->>MinimapCache: Merge per-page dirty rects
MinimapRenderer->>MinimapCache: flushVisible(map, floor, visible_rect)
MinimapCache->>GPU: glTextureSubImage2D uploads per-page regions
MinimapRenderer->>GPU: renderVisible binds page textures and draws quad
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 overhauls the minimap system by implementing a paged caching mechanism and refactoring the minimap window into a more robust UI component with zoom and floor controls. It introduces a refined invalidation system that tracks dirty tiles per map generation and centralizes map view math. Review feedback focuses on optimizing memory usage by merging dirty rectangles instead of storing them in vectors, reducing draw call overhead through batching, and ensuring compliance with project style guides regarding High DPI scaling and modern C++ string formatting.
| int page_x = 0; | ||
| int page_y = 0; | ||
| std::unique_ptr<GLTextureResource> texture; | ||
| std::vector<MinimapDirtyRect> dirty_rects; |
There was a problem hiding this comment.
Storing a std::vector<MinimapDirtyRect> per page can lead to unbounded memory growth if a page is modified many times while off-screen, as dirty_rects is only cleared in flushVisible. Since flushVisible merges all rects into a single bounding box anyway (via mergeRects), it would be more efficient to store a single MinimapDirtyRect per page and update it using a union operation in markDirty.
|
|
||
| glBindTextureUnit(0, page.texture_id); | ||
| shader_->SetVec4("uDestRect", dest_rect); | ||
| glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, nullptr); |
There was a problem hiding this comment.
This loop issues a raw glDrawElements call for every visible minimap page. The previous implementation used instanced rendering (glDrawElementsInstanced), which is more efficient and aligns better with the project's rendering architecture. While the move to a paged cache is beneficial, consider using a texture array for the cache pages to restore batching and avoid per-page draw calls and uniform updates.
| pending.floor_rects[position.z].push_back({ | ||
| .x = position.x, | ||
| .y = position.y, | ||
| .width = 1, | ||
| .height = 1, | ||
| }); |
There was a problem hiding this comment.
Pushing a 1x1 rect for every single tile modification into a vector can lead to significant overhead during large operations (e.g., large brush strokes or replacements). Consider merging these into larger dirty regions or using a more efficient representation (like a dirty flag per map node) before passing them to the renderer.
| } | ||
|
|
||
| const size_t buffer_size = static_cast<size_t>(clamped_rect.width) * clamped_rect.height; | ||
| upload_buffer_.assign(buffer_size, 0); |
There was a problem hiding this comment.
The upload_buffer_.assign(buffer_size, 0); call is redundant because the buffer is fully overwritten in the subsequent nested loops (lines 162-174), including the out-of-bounds cases which are explicitly set to 0. Using resize(buffer_size) would be more efficient as it avoids re-initializing the entire buffer when the size remains the same or decreases.
| upload_buffer_.assign(buffer_size, 0); | |
| upload_buffer_.resize(buffer_size); |
| } | ||
|
|
||
| wxButton* createHeaderButton(wxWindow* parent, const wxBitmap& bitmap, const wxString& label = wxString()) { | ||
| auto* button = new wxButton(parent, wxID_ANY, label, wxDefaultPosition, wxSize(24, 22), wxBU_EXACTFIT); |
There was a problem hiding this comment.
Hardcoded pixel values should be wrapped in FromDIP() to ensure proper scaling on High DPI displays, as required by the project style guide.
References
- USE: FromDIP() for any pixel values. BANNED: Hardcoded wxPoint/wxSize pixels. (link)
| floor_down_button_->Enable(false); | ||
| } else if (auto* state = GetActiveViewportState()) { | ||
| zoom_label_->SetLabel(wxString::FromUTF8(MinimapViewport::GetZoomLabel(state->zoom_step).data())); | ||
| floor_label_->SetLabel(wxString::Format("F: %d", state->floor)); |
There was a problem hiding this comment.
The project style guide mandates the use of std::format over wxString::Format for string formatting.
References
- ✅ std::format over sprintf/wxString::Format (link)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/rendering/drawers/minimap_drawer.cpp (1)
106-177:⚠️ Potential issue | 🟡 Minor
canvaspointer dereference without null check after early validation.The function validates
canvasindirectly viaGetActiveCanvas()at the call site (see context snippet 4), but the signature accepts a raw pointer. If a caller passesnullptr, line 176 will crash.Consider either:
- Changing the parameter to a reference (
MapCanvas& canvas) to express non-null intent- Adding a null guard before the
DrawMainCameraBoxcallOption 1: Change signature to reference (preferred)
-void MinimapDrawer::Draw(wxDC& pdc, const wxSize& size, Editor& editor, MapCanvas* canvas, const MinimapViewportState& viewport_state) { +void MinimapDrawer::Draw(wxDC& pdc, const wxSize& size, Editor& editor, MapCanvas& canvas, const MinimapViewportState& viewport_state) {And update line 176:
- DrawMainCameraBox(projection, size, *canvas, visible_rect); + DrawMainCameraBox(projection, size, canvas, visible_rect);Option 2: Add null guard
renderer->flushVisible(editor.map, floor, visible_rect_pixels); renderer->renderVisible(projection, 0, 0, window_width, window_height, floor, visible_rect_pixels); + if (canvas) { DrawMainCameraBox(projection, size, *canvas, visible_rect); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/minimap_drawer.cpp` around lines 106 - 177, MinimapDrawer::Draw dereferences the raw MapCanvas* parameter "canvas" when calling DrawMainCameraBox, which can crash if nullptr; fix by either changing the parameter to a non-null reference (MapCanvas& canvas) across the declaration/definitions and all call sites (preferred) so null cannot be passed, or add an explicit null check before the DrawMainCameraBox call (e.g., if (canvas) DrawMainCameraBox(...)) to guard against nullptr; update any call sites or tests accordingly and ensure signatures like MinimapDrawer::Draw and the DrawMainCameraBox invocation are adjusted consistently.
🧹 Nitpick comments (3)
source/rendering/core/map_view_math.cpp (1)
20-33: Consider documenting the+1.0padding inGetVisibleRect.The addition of
1.0to bothwidthandheightat lines 24-25 appears to be a safety margin to ensure tiles at the edge of the viewport are included. A brief comment explaining this would improve maintainability.Proposed documentation
MainMapVisibleRect MainMapViewMath::GetVisibleRect(const MainMapViewport& viewport) { const double tile_size = static_cast<double>(TILE_SIZE) / std::max(0.0001, viewport.zoom); const double start_x = static_cast<double>(viewport.view_scroll_x) / TILE_SIZE + GetFloorTileOffset(viewport.floor); const double start_y = static_cast<double>(viewport.view_scroll_y) / TILE_SIZE + GetFloorTileOffset(viewport.floor); + // Add 1.0 tile padding to ensure partial tiles at viewport edges are included const double width = viewport.pixel_width / tile_size + 1.0; const double height = viewport.pixel_height / tile_size + 1.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/map_view_math.cpp` around lines 20 - 33, Add a short explanatory comment in MainMapViewMath::GetVisibleRect describing why +1.0 is added to width and height (safety padding to include partially visible edge tiles and avoid off-by-one clipping at tile boundaries), e.g. place the comment above the calculations for width/height or inline next to the expressions to clarify the intent for future maintainers and reference that this affects the width/height variables used to compute the returned MainMapVisibleRect.source/rendering/ui/navigation_controller.cpp (1)
93-95: Consider usingdynamic_castfor safety.The
if (auto* map_window = static_cast<...>)pattern doesn't provide null-safety sincestatic_castdoesn't returnnullptron type mismatch—it results in undefined behavior. The condition only checks if the pointer itself is non-null.While this is likely safe in practice (since
MapCanvasparent is alwaysMapWindow), usingdynamic_castwould be more defensive:♻️ Safer cast option
- if (auto* map_window = static_cast<MapWindow*>(canvas->GetParent())) { + if (auto* map_window = dynamic_cast<MapWindow*>(canvas->GetParent())) { map_window->SyncTrackedMinimapViewportToCurrentView(); }That said, this pattern is consistent with other casts in the file (e.g., lines 17, 28-34, 43), so keeping
static_castfor consistency is also acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/navigation_controller.cpp` around lines 93 - 95, The cast using static_cast<MapWindow*> on canvas->GetParent() is unsafe on type mismatch; change it to use dynamic_cast<MapWindow*> and keep the null check before calling MapWindow::SyncTrackedMinimapViewportToCurrentView() so the cast returns nullptr on failure instead of causing undefined behavior; update the line that currently reads (auto* map_window = static_cast<MapWindow*>(canvas->GetParent())) to use dynamic_cast and leave the subsequent call to map_window->SyncTrackedMinimapViewportToCurrentView() guarded by the if.source/rendering/drawers/minimap_cache.cpp (1)
77-114: Consider creating pages for newly-dirtied regions.
markDirtyonly accumulates dirty rects for existing pages (line 96-98 skips if page not found). This means if a tile becomes dirty before its page is created viaflushVisible/collectVisiblePages, the dirty rect is lost.This may be intentional (pages are created on-demand when they become visible, and new pages get a full-page dirty rect in
getOrCreatePage), but it could cause a subtle issue if:
- A tile is modified
markDirtyis called but the page doesn't exist yet- The page is created later by navigation
- The new page gets a full dirty rect, so the specific tile change is uploaded anyway
If full-page upload on creation is the intended behavior, this is fine. Otherwise, consider creating the page in
markDirtyto preserve granular dirty tracking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/minimap_cache.cpp` around lines 77 - 114, markDirty currently skips non-existent pages (pages.find(...)->end) and thus drops dirty rects; either preserve finer-grained dirtiness by creating the page here or make the full-page-on-creation behavior explicit. Modify markDirty to call the page-creation path (e.g., getOrCreatePage or the equivalent factory used by flushVisible/collectVisiblePages) when pages.find(...) returns end, then compute local_rect and push it into floors_[floor].pages[pageKey].dirty_rects (instead of skipping), ensuring the same local_rect logic and isValidRect check are used; alternatively, if full-page dirtying on creation is desired, add a comment clarifying that behavior and leave the skip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/app/rme_forward_declarations.h`:
- Line 35: The forward declaration for Position is using the wrong class-key;
update the declaration in rme_forward_declarations.h from "struct Position;" to
"class Position;" so it matches the real definition of the type (Position) and
avoids ODR/class-key mismatches with the actual definition.
In `@source/editor/persistence/editor_persistence.h`:
- Line 15: The forward declaration "struct Position;" in editor_persistence.h is
incorrect (Position is defined as a class) and redundant because
rme_forward_declarations.h (already included) provides the correct forward
declaration; fix by removing the "struct Position;" line entirely (or if you
prefer to keep a local declaration, change it to "class Position;") and ensure
there are no duplicate/conflicting forward declarations for Position in
editor_persistence.h or related headers.
In `@source/rendering/ui/minimap_window.cpp`:
- Around line 71-86: The help panel is currently added to root_sizer alongside
canvas_, causing MinimapCanvas (canvas_) to be resized when help_panel_ is
shown/hidden; instead make the help UI an overlay so canvas_ size remains
stable. Remove help_panel_ from root_sizer; create help_panel_ as a child
positioned over canvas_ (or parented to the same window but not inserted into
root_sizer), set it to use wxTRANSPARENT_WINDOW or proper style and a
layout-independent position, keep help_sizer/help_text_ for its contents, and
show/hide help_panel_ without calling root_sizer->Layout() or modifying
canvas_—ensure any existing calls that adjust the GL viewport on resize (the
code around re-normalize/viewport) are not triggered by toggling help_panel_.
- Around line 483-486: MinimapCanvas::OnKey currently checks
g_gui.GetCurrentTab() but then unconditionally calls
g_gui.GetCurrentMapTab()->GetEventHandler()->AddPendingEvent(event), which can
null-deref if the current tab isn’t a MapTab; fix by obtaining a local MapTab*
tab = g_gui.GetCurrentMapTab() and guarding it (if (tab) {
tab->GetEventHandler()->AddPendingEvent(event); } else { event.Skip(); }) so you
only dereference when non-null and otherwise propagate the key event.
---
Outside diff comments:
In `@source/rendering/drawers/minimap_drawer.cpp`:
- Around line 106-177: MinimapDrawer::Draw dereferences the raw MapCanvas*
parameter "canvas" when calling DrawMainCameraBox, which can crash if nullptr;
fix by either changing the parameter to a non-null reference (MapCanvas& canvas)
across the declaration/definitions and all call sites (preferred) so null cannot
be passed, or add an explicit null check before the DrawMainCameraBox call
(e.g., if (canvas) DrawMainCameraBox(...)) to guard against nullptr; update any
call sites or tests accordingly and ensure signatures like MinimapDrawer::Draw
and the DrawMainCameraBox invocation are adjusted consistently.
---
Nitpick comments:
In `@source/rendering/core/map_view_math.cpp`:
- Around line 20-33: Add a short explanatory comment in
MainMapViewMath::GetVisibleRect describing why +1.0 is added to width and height
(safety padding to include partially visible edge tiles and avoid off-by-one
clipping at tile boundaries), e.g. place the comment above the calculations for
width/height or inline next to the expressions to clarify the intent for future
maintainers and reference that this affects the width/height variables used to
compute the returned MainMapVisibleRect.
In `@source/rendering/drawers/minimap_cache.cpp`:
- Around line 77-114: markDirty currently skips non-existent pages
(pages.find(...)->end) and thus drops dirty rects; either preserve finer-grained
dirtiness by creating the page here or make the full-page-on-creation behavior
explicit. Modify markDirty to call the page-creation path (e.g., getOrCreatePage
or the equivalent factory used by flushVisible/collectVisiblePages) when
pages.find(...) returns end, then compute local_rect and push it into
floors_[floor].pages[pageKey].dirty_rects (instead of skipping), ensuring the
same local_rect logic and isValidRect check are used; alternatively, if
full-page dirtying on creation is desired, add a comment clarifying that
behavior and leave the skip.
In `@source/rendering/ui/navigation_controller.cpp`:
- Around line 93-95: The cast using static_cast<MapWindow*> on
canvas->GetParent() is unsafe on type mismatch; change it to use
dynamic_cast<MapWindow*> and keep the null check before calling
MapWindow::SyncTrackedMinimapViewportToCurrentView() so the cast returns nullptr
on failure instead of causing undefined behavior; update the line that currently
reads (auto* map_window = static_cast<MapWindow*>(canvas->GetParent())) to use
dynamic_cast and leave the subsequent call to
map_window->SyncTrackedMinimapViewportToCurrentView() guarded by the if.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce6f9611-535e-4b8a-9a01-a65042cebcd9
📒 Files selected for processing (33)
source/CMakeLists.txtsource/app/rme_forward_declarations.hsource/editor/action.cppsource/editor/dirty_list.cppsource/editor/dirty_list.hsource/editor/managers/editor_manager.cppsource/editor/persistence/editor_persistence.hsource/map/tile.cppsource/map/tile.hsource/rendering/core/coordinate_mapper.cppsource/rendering/core/map_view_math.cppsource/rendering/core/map_view_math.hsource/rendering/drawers/minimap_cache.cppsource/rendering/drawers/minimap_cache.hsource/rendering/drawers/minimap_drawer.cppsource/rendering/drawers/minimap_drawer.hsource/rendering/drawers/minimap_renderer.cppsource/rendering/drawers/minimap_renderer.hsource/rendering/ui/keyboard_handler.cppsource/rendering/ui/map_display.cppsource/rendering/ui/minimap_viewport.hsource/rendering/ui/minimap_window.cppsource/rendering/ui/minimap_window.hsource/rendering/ui/navigation_controller.cppsource/rendering/ui/zoom_controller.cppsource/ui/gui.hsource/ui/managers/loading_manager.hsource/ui/managers/minimap_manager.cppsource/ui/managers/minimap_manager.hsource/ui/map_tab.cppsource/ui/map_window.cppsource/ui/map_window.hsource/ui/replace_tool/replacement_engine.cpp
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
- Replaced per-floor `std::vector<MinimapDirtyRect>` with `std::optional<MinimapDirtyRect>` across cache and manager, merging rects incrementally via `unionRects` instead of batch merging - Rewrote `uploadRect` to use `Map::visitLeaves` for tile iteration instead of nested loops, with direct buffer indexing instead of sequential writes - Added DPI-aware sizing helpers (`dipSize`, `dip`) and converted all hardcoded pixel values in `MinimapWindow` - Implemented zoom-to-cursor: wheel zoom now anchors to mouse position instead of center - Repositioned help panel to overlay canvas with `PositionHelpPanel()` instead of layout-based placement - Removed `clampViewportStart` and simplified `BuildVisibleWorldRect` to use unclamped center coordinates - Changed `MapCanvas` parameter from pointer to reference in `MinimapDrawer::Draw` - Changed `Position` forward declaration from `struct` to `class` in `rme_forward_declarations.h` and `minimap_manager.h` - Changed `static_cast` to `dynamic_cast` for `MapWindow` parent lookup in `navigation_controller.cpp` - Added one-tile padding comment in `map_view_math.cpp` [minor]
- Added multi-floor minimap rendering with semi-transparent shade overlay between floors in minimap_drawer.cpp - Introduced MinimapFloorRenderRange struct and getFloorRenderRange() to compute floor iteration bounds based on show_all_floors flag - Added DrawFloorShade() to render 45% black overlay when upper floors are drawn - Expanded zoom steps from 9 to 16 with intermediate values and switched labels from ratio format to numbered 1-16 in minimap_viewport.h - Added show_all_floors bool to MinimapViewportState with default true - Added 'All' wxToggleButton to minimap header with SetShowAllFloors() and OnShowAllFloorsToggle() handlers in minimimap_window.cpp/h - Fixed Ctrl+wheel floor step direction reversal in minimap mouse wheel handler - Introduced ResumeMinimapTrackingToCurrentView() in map_window.cpp/h to re-enable minimap tracking and sync to main camera - Replaced SyncTrackedMinimapViewportToCurrentView() calls with ResumeMinimapTrackingToCurrentView() across navigation_controller.cpp and zoom_controller.cpp - Added ResumeMinimapTrackingToCurrentView() calls on mouse action/camera/properties clicks and arrow key/drag navigation in map_display.cpp and navigation_controller.cpp
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @karolak6612, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
source/rendering/core/map_view_math.cpp (2)
57-74: Same asymmetric pattern inMapToScreenmirrorsScreenToMap.The inverse conversion also applies different divisors for negative vs. non-negative raw deltas. This maintains consistency with
ScreenToMap, but consider documenting why negative coordinates receive different treatment, as this is non-obvious behavior for coordinate transforms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/map_view_math.cpp` around lines 57 - 74, MapToScreen contains an asymmetric conversion for negative vs non-negative raw_x/raw_y that mirrors ScreenToMap; add a clear explanatory comment inside MainMapViewMath::MapToScreen (near the raw_x/raw_y branches) describing why negative deltas use only viewport.scale_factor while non-negative deltas use viewport.zoom * viewport.scale_factor (i.e., to match the floor/ceil behavior and keep the ScreenToMap/MapToScreen pair consistent across sign boundaries), and ensure the comment references the paired function ScreenToMap so future maintainers understand the intentional symmetry.
37-55: Clarify the purpose of asymmetric zoom handling for negative coordinates or simplify the logic.The
ScreenToMapfunction handles negative and positive scaled coordinates differently—negative values skip zoom application while positive values apply it. While this matches the inverse logic inMapToScreen, the negative branch appears unreachable sincescreen_xis always non-negative andscale_factordefaults to 1.0, makingscaled_xnon-negative. Either remove this unreachable branch if it's genuinely dead code, or add a comment explaining when/why negative coordinates might occur and why the asymmetric zoom handling is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/map_view_math.cpp` around lines 37 - 55, The ScreenToMap function contains asymmetric handling for negative scaled_x/scaled_y in MainMapViewMath::ScreenToMap (using viewport.scale_factor and viewport.zoom) that appears unreachable because screen_x/screen_y are non-negative and scale_factor defaults to 1.0; either remove the negative-branch code paths (the branches that compute *map_x/*map_y without applying viewport.zoom) to simplify logic and keep only the positive-path that uses viewport.zoom and TILE_SIZE, or retain them but add a clear comment above the conditional explaining the exact scenario where scaled_x/scaled_y can be negative (e.g., when screen coordinates can be negative due to off-screen rendering or non-standard scale_factor) and why zoom must be skipped for negatives; ensure references to MainMapViewport.scale_factor, MainMapViewport.zoom, TILE_SIZE, and GetFloorTileOffset(viewport.floor) remain correct after the change.source/rendering/ui/zoom_controller.cpp (1)
64-68: RedundantUpdateMinimapcalls inApplyRelativeZoom.
ScrollRelative()(line 65) internally callsg_gui.UpdateMinimap()(as shown in context snippet 2, line 249). The subsequentg_gui.UpdateMinimap(true)on line 67 triggers a second minimap update. If the intent is to force an immediate refresh (vs. the non-immediate call insideScrollRelative), consider either:
- Passing an
immediateflag toScrollRelativeto avoid the double update, or- Adding a brief comment explaining why the second call with
immediate=trueis necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/zoom_controller.cpp` around lines 64 - 68, ApplyRelativeZoom calls ScrollRelative which already invokes g_gui.UpdateMinimap(), so the subsequent g_gui.UpdateMinimap(true) causes a redundant second update; fix by either adding an immediate flag to ScrollRelative (e.g., change MapWindow::ScrollRelative to accept a bool immediate and forward that to g_gui.UpdateMinimap(immediate)) and update the call in ApplyRelativeZoom accordingly, or if immediate behavior is truly required here, add a concise comment above the second g_gui.UpdateMinimap(true) explaining why an explicit immediate refresh is necessary (referencing ResumeMinimapTrackingToCurrentView, MapWindow::ScrollRelative, and g_gui.UpdateMinimap).source/rendering/drawers/minimap_drawer.cpp (1)
129-131: Consider removing unusedwxDC& pdcparameter.The
pdcparameter is immediately marked unused withwxUnusedVar(pdc). If this parameter is no longer needed after the refactor to GL-based rendering, consider removing it from the function signature to clarify the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/drawers/minimap_drawer.cpp` around lines 129 - 131, The Draw function currently takes an unused parameter wxDC& pdc (MinimapDrawer::Draw) which should be removed: update the function signature in the class declaration and definition to drop the wxDC& pdc parameter, remove the wxUnusedVar(pdc) line, and update any overrides/implementations and all call sites to match the new signature (also update any virtual/base class declarations if this is an overridden method); rebuild and run tests to ensure no remaining references to the old signature remain.source/ui/managers/minimap_manager.cpp (1)
15-37: DuplicateunionRectsimplementation—extract to shared header.This
unionRectsfunction is nearly identical to the one insource/rendering/drawers/minimap_cache.cpp(lines 37-55). Both perform the same rect union logic. Consider extracting to a shared utility (e.g., inminimap_cache.hor a newminimap_types.h) to avoid duplication.♻️ Suggested approach
Move
unionRectstosource/rendering/drawers/minimap_cache.h(or a shared header) and make it a free function or static member:// In minimap_cache.h [[nodiscard]] inline MinimapDirtyRect unionRects(const MinimapDirtyRect& lhs, const MinimapDirtyRect& rhs) { // ... implementation }Then include and use it from both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/managers/minimap_manager.cpp` around lines 15 - 37, The duplicate free function unionRects operating on MinimapDirtyRect should be extracted to a single shared header (e.g., a minimap utility header) and reused: move the implementation into an inline/free function unionRects(const MinimapDirtyRect& lhs, const MinimapDirtyRect& rhs) in that header, include that header from both places, mark it inline/[[nodiscard]] as before, and remove the duplicate implementations from minimap_manager.cpp and the other file so both translation units call the shared unionRects.source/rendering/ui/minimap_window.cpp (1)
599-601: Consider clamping center coordinates to prevent out-of-bounds viewport.
ClampViewportStateonly clamps the floor but notcenter_x/center_y. During drag operations (lines 526-527) and zoom (lines 571-572), the center can potentially drift outside valid map bounds. While the renderer may handle this gracefully, consider adding bounds checks to prevent viewport center from exceeding map dimensions.♻️ Suggested enhancement
void MinimapCanvas::ClampViewportState(MinimapViewportState& state) const { state.floor = MinimapViewport::ClampFloor(state.floor); + if (Editor* editor = owner_->GetActiveEditor()) { + const int map_width = editor->map.getWidth(); + const int map_height = editor->map.getHeight(); + state.center_x = std::clamp(state.center_x, 0.0, static_cast<double>(map_width)); + state.center_y = std::clamp(state.center_y, 0.0, static_cast<double>(map_height)); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/minimap_window.cpp` around lines 599 - 601, ClampViewportState currently only calls MinimapViewport::ClampFloor on state.floor; extend it to also clamp state.center_x and state.center_y to valid map bounds so the viewport cannot drift off-map during drags/zooms. Inside MinimapCanvas::ClampViewportState, obtain the map dimensions and the viewport half-extents (or compute visible tile radius from MinimapViewportState/MinimapViewport), compute min/max allowed center_x/center_y and clamp them (e.g., with std::clamp) before returning; reference MinimapCanvas::ClampViewportState, MinimapViewportState (center_x/center_y/floor) and MinimapViewport::ClampFloor when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/rendering/drawers/minimap_cache.cpp`:
- Around line 161-191: The const_cast in uploadRect indicates visitLeaves is not
declared const; make visitLeaves a const method on the owning types and remove
the cast: add a const overload / mark visitLeaves(...) const in SpatialHashGrid
(and corresponding declaration/implementation) and in BaseMap where visitLeaves
is declared (basemap.h: update signature near the existing declaration), ensure
the implementation only uses const operations (binary search and callback
invocation) so the compiler accepts the const qualifier, then remove
const_cast<Map&>(map) in drawerminigap cache's uploadRect and call
map.visitLeaves(...) directly.
In `@source/rendering/ui/minimap_window.cpp`:
- Around line 533-578: The mouse wheel floor direction in
MinimapCanvas::OnMouseWheel is inverted compared to keyboard PageUp/PageDown:
change the call to owner_->StepFloor(...) so positive wheel rotation decrements
the floor (match ChangeFloor behavior). Specifically, update the
owner_->StepFloor call that uses event.GetWheelRotation() to use the opposite
sign (i.e., use -1 when GetWheelRotation() > 0 and +1 otherwise) so StepFloor
receives the swapped operand ordering and mouse Ctrl+Scroll matches keyboard
navigation.
---
Nitpick comments:
In `@source/rendering/core/map_view_math.cpp`:
- Around line 57-74: MapToScreen contains an asymmetric conversion for negative
vs non-negative raw_x/raw_y that mirrors ScreenToMap; add a clear explanatory
comment inside MainMapViewMath::MapToScreen (near the raw_x/raw_y branches)
describing why negative deltas use only viewport.scale_factor while non-negative
deltas use viewport.zoom * viewport.scale_factor (i.e., to match the floor/ceil
behavior and keep the ScreenToMap/MapToScreen pair consistent across sign
boundaries), and ensure the comment references the paired function ScreenToMap
so future maintainers understand the intentional symmetry.
- Around line 37-55: The ScreenToMap function contains asymmetric handling for
negative scaled_x/scaled_y in MainMapViewMath::ScreenToMap (using
viewport.scale_factor and viewport.zoom) that appears unreachable because
screen_x/screen_y are non-negative and scale_factor defaults to 1.0; either
remove the negative-branch code paths (the branches that compute *map_x/*map_y
without applying viewport.zoom) to simplify logic and keep only the
positive-path that uses viewport.zoom and TILE_SIZE, or retain them but add a
clear comment above the conditional explaining the exact scenario where
scaled_x/scaled_y can be negative (e.g., when screen coordinates can be negative
due to off-screen rendering or non-standard scale_factor) and why zoom must be
skipped for negatives; ensure references to MainMapViewport.scale_factor,
MainMapViewport.zoom, TILE_SIZE, and GetFloorTileOffset(viewport.floor) remain
correct after the change.
In `@source/rendering/drawers/minimap_drawer.cpp`:
- Around line 129-131: The Draw function currently takes an unused parameter
wxDC& pdc (MinimapDrawer::Draw) which should be removed: update the function
signature in the class declaration and definition to drop the wxDC& pdc
parameter, remove the wxUnusedVar(pdc) line, and update any
overrides/implementations and all call sites to match the new signature (also
update any virtual/base class declarations if this is an overridden method);
rebuild and run tests to ensure no remaining references to the old signature
remain.
In `@source/rendering/ui/minimap_window.cpp`:
- Around line 599-601: ClampViewportState currently only calls
MinimapViewport::ClampFloor on state.floor; extend it to also clamp
state.center_x and state.center_y to valid map bounds so the viewport cannot
drift off-map during drags/zooms. Inside MinimapCanvas::ClampViewportState,
obtain the map dimensions and the viewport half-extents (or compute visible tile
radius from MinimapViewportState/MinimapViewport), compute min/max allowed
center_x/center_y and clamp them (e.g., with std::clamp) before returning;
reference MinimapCanvas::ClampViewportState, MinimapViewportState
(center_x/center_y/floor) and MinimapViewport::ClampFloor when making the
change.
In `@source/rendering/ui/zoom_controller.cpp`:
- Around line 64-68: ApplyRelativeZoom calls ScrollRelative which already
invokes g_gui.UpdateMinimap(), so the subsequent g_gui.UpdateMinimap(true)
causes a redundant second update; fix by either adding an immediate flag to
ScrollRelative (e.g., change MapWindow::ScrollRelative to accept a bool
immediate and forward that to g_gui.UpdateMinimap(immediate)) and update the
call in ApplyRelativeZoom accordingly, or if immediate behavior is truly
required here, add a concise comment above the second g_gui.UpdateMinimap(true)
explaining why an explicit immediate refresh is necessary (referencing
ResumeMinimapTrackingToCurrentView, MapWindow::ScrollRelative, and
g_gui.UpdateMinimap).
In `@source/ui/managers/minimap_manager.cpp`:
- Around line 15-37: The duplicate free function unionRects operating on
MinimapDirtyRect should be extracted to a single shared header (e.g., a minimap
utility header) and reused: move the implementation into an inline/free function
unionRects(const MinimapDirtyRect& lhs, const MinimapDirtyRect& rhs) in that
header, include that header from both places, mark it inline/[[nodiscard]] as
before, and remove the duplicate implementations from minimap_manager.cpp and
the other file so both translation units call the shared unionRects.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87331745-7e20-4c9b-8b17-d2f588109843
📒 Files selected for processing (16)
source/editor/persistence/editor_persistence.hsource/rendering/core/map_view_math.cppsource/rendering/drawers/minimap_cache.cppsource/rendering/drawers/minimap_cache.hsource/rendering/drawers/minimap_drawer.cppsource/rendering/drawers/minimap_drawer.hsource/rendering/ui/map_display.cppsource/rendering/ui/minimap_viewport.hsource/rendering/ui/minimap_window.cppsource/rendering/ui/minimap_window.hsource/rendering/ui/navigation_controller.cppsource/rendering/ui/zoom_controller.cppsource/ui/managers/minimap_manager.cppsource/ui/managers/minimap_manager.hsource/ui/map_window.cppsource/ui/map_window.h
💤 Files with no reviewable changes (1)
- source/editor/persistence/editor_persistence.h
🚧 Files skipped from review as they are similar to previous changes (4)
- source/rendering/ui/navigation_controller.cpp
- source/rendering/ui/map_display.cpp
- source/ui/managers/minimap_manager.h
- source/ui/map_window.h
| const_cast<Map&>(map).visitLeaves(origin_x, origin_y, rect_end_x, rect_end_y, [&](MapNode* node, int node_map_x, int node_map_y) { | ||
| const Floor* node_floor = node->getFloor(floor); | ||
| if (!node_floor) { | ||
| return; | ||
| } | ||
|
|
||
| for (int local_node_x = 0; local_node_x < 4; ++local_node_x) { | ||
| const int map_x = node_map_x + local_node_x; | ||
| if (map_x < origin_x || map_x > rect_end_x) { | ||
| continue; | ||
| } | ||
|
|
||
| for (int local_node_y = 0; local_node_y < 4; ++local_node_y) { | ||
| const int map_y = node_map_y + local_node_y; | ||
| if (map_y < origin_y || map_y > rect_end_y) { | ||
| continue; | ||
| } | ||
|
|
||
| const int floor_index = local_node_x * 4 + local_node_y; | ||
| const TileLocation& location = node_floor->locs[static_cast<size_t>(floor_index)]; | ||
| const Tile* tile = location.get(); | ||
| if (!tile) { | ||
| continue; | ||
| } | ||
|
|
||
| const int buffer_x = map_x - origin_x; | ||
| const int buffer_y = map_y - origin_y; | ||
| upload_buffer_[static_cast<size_t>(buffer_y) * clamped_rect.width + buffer_x] = tile->getMiniMapColor(); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if visitLeaves is const-qualified and verify threading model
ast-grep --pattern 'void visitLeaves($$$)'
echo "---"
rg -n "visitLeaves" --type=cpp -C 2 | head -60Repository: karolak6612/remeres-map-editor-redux
Length of output: 3740
🏁 Script executed:
#!/bin/bash
# Find the uploadRect function to see its signature
rg -n "uploadRect" source/rendering/drawers/minimap_cache.cpp -B 3 -A 15 | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 1540
🏁 Script executed:
#!/bin/bash
# Check the visitLeaves implementation in spatial_hash_grid.h
sed -n '67,83p' source/map/spatial_hash_grid.h
echo "---"
sed -n '136,180p' source/map/spatial_hash_grid.hRepository: karolak6612/remeres-map-editor-redux
Length of output: 2442
🏁 Script executed:
#!/bin/bash
# Check if Map has const visitLeaves method or any thread-safety mechanisms
rg -n "class Map" source/map/ --type=cpp --type=h -A 20 | head -100Repository: karolak6612/remeres-map-editor-redux
Length of output: 5140
🏁 Script executed:
#!/bin/bash
# Check the actual Map class definition
rg -n "class Map\b" source/map/ --type=h -A 50 | head -150Repository: karolak6612/remeres-map-editor-redux
Length of output: 7253
🏁 Script executed:
#!/bin/bash
# Check if there's a const visitLeaves overload on Map
rg -n "visitLeaves" source/map/basemap.h -B 5 -A 5Repository: karolak6612/remeres-map-editor-redux
Length of output: 697
🏁 Script executed:
#!/bin/bash
# Check if Map modifies state during visitLeaves by looking at actual Map class
fd -e h "map\.h|basemap\.h" source/map/ | xargs grep -l "class Map"Repository: karolak6612/remeres-map-editor-redux
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check if there are any multi-threading constructs in the codebase
rg -n "std::thread|pthread|std::mutex|std::lock_guard|std::async" source/ | head -30Repository: karolak6612/remeres-map-editor-redux
Length of output: 2701
🏁 Script executed:
#!/bin/bash
# Check the complete visitLeaves implementation in basemap.h
sed -n '120,125p' source/map/basemap.hRepository: karolak6612/remeres-map-editor-redux
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Verify if visitLeaves actually modifies any state in grid
rg -n "void visitLeaves" source/map/spatial_hash_grid.h -A 20Repository: karolak6612/remeres-map-editor-redux
Length of output: 1769
🏁 Script executed:
#!/bin/bash
# Check where flushVisible/uploadRect is called from
rg -n "flushVisible\|uploadRect" source/ --type=cpp -B 3 | head -80Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check if uploadRect/flushVisible uses any thread synchronization
rg -n "flushVisible\|uploadRect\|MinimapCache" source/rendering/drawers/minimap_cache.cpp | head -40Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check Tile::getMiniMapColor implementation
rg -n "getMiniMapColor" source/map/tile.cpp -A 10Repository: karolak6612/remeres-map-editor-redux
Length of output: 746
🏁 Script executed:
#!/bin/bash
# Find all calls to MinimapCache methods like flushVisible
rg -n "flushVisible\|MinimapCache::" source/ --type=cpp -B 2 -A 2Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check the Map class to see if visitLeaves could be const
sed -n '34,200p' source/map/map.h | head -100Repository: karolak6612/remeres-map-editor-redux
Length of output: 2565
🏁 Script executed:
#!/bin/bash
# Check if the grid itself needs to be modified during visitLeaves
sed -n '16,90p' source/map/spatial_hash_grid.hRepository: karolak6612/remeres-map-editor-redux
Length of output: 2457
🏁 Script executed:
#!/bin/bash
# Search for where MinimapCache is instantiated and used
rg -n "MinimapCache" source/ --type=cpp --type=h | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 2212
🏁 Script executed:
#!/bin/bash
# Check if there are any background tile modifications that could race with minimap rendering
rg -n "sprite.*preloader\|background.*thread" source/ --type=cpp | head -20Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Look for the flushVisible method to understand when it's called
fd -e cpp "minimap_cache.cpp" -x grep -n "flushVisible\|void.*flush" {}Repository: karolak6612/remeres-map-editor-redux
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Check the full flushVisible method to see its calling context
sed -n '207,233p' source/rendering/drawers/minimap_cache.cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Check where flushVisible is called from (minimap renderer context)
rg -n "flushVisible" source/rendering/drawers/minimap_renderer.cpp -B 3 -A 3Repository: karolak6612/remeres-map-editor-redux
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Verify that visitLeaves doesn't modify any state
sed -n '136,200p' source/map/spatial_hash_grid.h | grep -E "cells_|grid|modify"Repository: karolak6612/remeres-map-editor-redux
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Check if visitLeaves is called from any background thread context
rg -n "visitLeaves" source/ --type=cpp -B 5 | grep -E "thread|async|background" -A 2 -B 2Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Verify visitLeaves doesn't modify Map/grid state by checking full visitLeavesImpl
sed -n '136,200p' source/map/spatial_hash_grid.hRepository: karolak6612/remeres-map-editor-redux
Length of output: 2431
🏁 Script executed:
#!/bin/bash
# Check if there's a const version of visitLeaves anywhere
rg -n "const.*visitLeaves|visitLeaves.*const" source/ --type=h --type=cppRepository: karolak6612/remeres-map-editor-redux
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Check who calls MinimapRenderer::flushVisible and from what context
rg -n "flushVisible\|MinimapRenderer" source/ --type=cpp --type=h | grep -E "flushVisible|new MinimapRenderer" -B 2 -A 2Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check the rendering pipeline to understand threading model
rg -n "render\|Render" source/ui/gui.cpp -A 2 | head -60Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Find all calls to flushVisible to understand when it's called
rg -n "\.flushVisible" source/ --type=cpp -B 5 -A 2Repository: karolak6612/remeres-map-editor-redux
Length of output: 734
🏁 Script executed:
#!/bin/bash
# Check if BaseMap or visitLeaves could legitimately be const
rg -n "class BaseMap" source/map/basemap.h -A 30 | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 1436
🏁 Script executed:
#!/bin/bash
# Verify that wxWidgets is single-threaded for the UI
rg -n "wxWidgets\|wxApp\|wxThread" source/ --type=cpp --type=h | head -20Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Check the main rendering entry point to confirm single-threaded rendering
rg -n "OnPaint\|render\(" source/ui/gui.cpp -B 2 -A 5 | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
visitLeaves should be const-qualified to match the const Map parameter.
The const_cast<Map&>(map) on line 161 is unnecessary. The visitLeaves method only reads data from the spatial hash grid—it performs a binary search and invokes the callback function without modifying any state. Since uploadRect receives const Map& map, and visitLeaves doesn't mutate the map, the method should be marked const in both SpatialHashGrid and BaseMap (basemap.h:121). This would eliminate the const_cast and improve API clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/drawers/minimap_cache.cpp` around lines 161 - 191, The
const_cast in uploadRect indicates visitLeaves is not declared const; make
visitLeaves a const method on the owning types and remove the cast: add a const
overload / mark visitLeaves(...) const in SpatialHashGrid (and corresponding
declaration/implementation) and in BaseMap where visitLeaves is declared
(basemap.h: update signature near the existing declaration), ensure the
implementation only uses const operations (binary search and callback
invocation) so the compiler accepts the const qualifier, then remove
const_cast<Map&>(map) in drawerminigap cache's uploadRect and call
map.visitLeaves(...) directly.
| void MinimapCanvas::OnMouseWheel(wxMouseEvent& event) { | ||
| if (g_gui.IsLoading() || !g_gui.IsEditorOpen()) { | ||
| return; | ||
| } | ||
|
|
||
| if (event.ControlDown()) { | ||
| owner_->StepFloor(event.GetWheelRotation() > 0 ? 1 : -1); | ||
| } else { | ||
| auto* state = owner_->GetActiveViewportState(); | ||
| Editor* editor = owner_->GetActiveEditor(); | ||
| if (!state || !editor) { | ||
| return; | ||
| } | ||
|
|
||
| const int next_step = MinimapViewport::ClampZoomStep(state->zoom_step + (event.GetWheelRotation() > 0 ? 1 : -1)); | ||
| if (next_step == state->zoom_step) { | ||
| return; | ||
| } | ||
|
|
||
| const wxSize size = GetClientSize(); | ||
| const int safe_width = std::max(1, size.GetWidth()); | ||
| const int safe_height = std::max(1, size.GetHeight()); | ||
| const double normalized_x = std::clamp(event.GetX() / static_cast<double>(safe_width), 0.0, 1.0); | ||
| const double normalized_y = std::clamp(event.GetY() / static_cast<double>(safe_height), 0.0, 1.0); | ||
|
|
||
| const double old_zoom_factor = MinimapViewport::GetZoomFactor(state->zoom_step); | ||
| const double old_visible_width = std::max(1.0, safe_width * old_zoom_factor); | ||
| const double old_visible_height = std::max(1.0, safe_height * old_zoom_factor); | ||
| const double old_start_x = state->center_x - old_visible_width / 2.0; | ||
| const double old_start_y = state->center_y - old_visible_height / 2.0; | ||
| const double anchor_x = old_start_x + normalized_x * old_visible_width; | ||
| const double anchor_y = old_start_y + normalized_y * old_visible_height; | ||
|
|
||
| state->zoom_step = next_step; | ||
|
|
||
| const double new_zoom_factor = MinimapViewport::GetZoomFactor(state->zoom_step); | ||
| const double new_visible_width = std::max(1.0, safe_width * new_zoom_factor); | ||
| const double new_visible_height = std::max(1.0, safe_height * new_zoom_factor); | ||
| state->center_x = anchor_x + (0.5 - normalized_x) * new_visible_width; | ||
| state->center_y = anchor_y + (0.5 - normalized_y) * new_visible_height; | ||
|
|
||
| ClampViewportState(*state); | ||
| owner_->SyncHeaderState(); | ||
| Refresh(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a configuration option or documented convention for floor navigation direction
rg -n -i "floor.*scroll\|wheel.*floor\|floor.*direction" --type=cpp --type=h -C2Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
# Search for OnFloorUp and OnFloorDown implementations
rg -n "OnFloorUp|OnFloorDown" --type=cpp --type=h -B2 -A8Repository: karolak6612/remeres-map-editor-redux
Length of output: 3153
🏁 Script executed:
# Search for StepFloor implementation to understand sign convention
rg -n "void StepFloor\|StepFloor\(" --type=cpp --type=h -B2 -A10Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
# Look for floor numbering convention or documentation comments
rg -n -i "floor.*0.*top\|floor.*underground\|floor.*level.*convention" --type=cpp --type=h -C2Repository: karolak6612/remeres-map-editor-redux
Length of output: 62
🏁 Script executed:
# Check navigation_controller.cpp for HandleFloorChange mentioned in scratchpad
rg -n "HandleFloorChange" --type=cpp --type=h -B2 -A15Repository: karolak6612/remeres-map-editor-redux
Length of output: 3563
🏁 Script executed:
# Search for StepFloor definition in owner_ class (likely in gui or similar)
rg -n "StepFloor\(" --type=cpp --type=h -B2 -A5 | head -50Repository: karolak6612/remeres-map-editor-redux
Length of output: 2655
🏁 Script executed:
# Find the owner_ class to understand StepFloor implementation
rg -n "void StepFloor|StepFloor.*{" --type=cpp --type=h -B1 -A8Repository: karolak6612/remeres-map-editor-redux
Length of output: 197
🏁 Script executed:
# Check if there are any comments or TODOs about this behavior
rg -n -i "mouse.*wheel.*floor|ctrl.*wheel|wheel.*direction" --type=cpp -C3Repository: karolak6612/remeres-map-editor-redux
Length of output: 601
Fix inverted mouse wheel floor direction to match keyboard behavior.
Ctrl+ScrollUp (positive rotation) currently calls StepFloor(1) which increases the floor number, but the keyboard shortcut PageUp calls ChangeFloor(canvas->floor - 1) which decreases it. These directions are inverted. Change line 539 to swap the operands: StepFloor(event.GetWheelRotation() > 0 ? -1 : 1) to make mouse wheel behavior consistent with keyboard navigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/minimap_window.cpp` around lines 533 - 578, The mouse
wheel floor direction in MinimapCanvas::OnMouseWheel is inverted compared to
keyboard PageUp/PageDown: change the call to owner_->StepFloor(...) so positive
wheel rotation decrements the floor (match ChangeFloor behavior). Specifically,
update the owner_->StepFloor call that uses event.GetWheelRotation() to use the
opposite sign (i.e., use -1 when GetWheelRotation() > 0 and +1 otherwise) so
StepFloor receives the swapped operand ordering and mouse Ctrl+Scroll matches
keyboard navigation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive overhaul of the minimap system, transitioning to a tiled caching architecture for improved performance and introducing a dedicated math utility for coordinate mapping. The user interface for the minimap has been completely rewritten to include interactive controls for zoom and floor levels, while the underlying management logic now ensures consistent synchronization with the main editor view through a new invalidation tracking system. Review feedback identifies opportunities to enhance code quality by removing a const_cast, adopting modern C++ return types for math functions, and eliminating duplicate utility code to satisfy the DRY principle.
| const int origin_y = page.page_y * PageSize + clamped_rect.y; | ||
| const int rect_end_x = origin_x + clamped_rect.width - 1; | ||
| const int rect_end_y = origin_y + clamped_rect.height - 1; | ||
| const_cast<Map&>(map).visitLeaves(origin_x, origin_y, rect_end_x, rect_end_y, [&](MapNode* node, int node_map_x, int node_map_y) { |
There was a problem hiding this comment.
The use of const_cast here breaks const-correctness and can hide potential design issues. The uploadRect function takes a const Map&, implying it won't modify the map. However, visitLeaves is apparently a non-const method.
If visitLeaves does not actually modify the map's state, it should be declared const. If it does modify the map (e.g., lazy loading of nodes), then uploadRect should accept a non-const Map& to correctly reflect its behavior. Using const_cast should be a last resort.
| void MainMapViewMath::ScreenToMap(int screen_x, int screen_y, const MainMapViewport& viewport, int* map_x, int* map_y) { | ||
| const int scaled_x = static_cast<int>(screen_x * viewport.scale_factor); | ||
| const int scaled_y = static_cast<int>(screen_y * viewport.scale_factor); | ||
|
|
||
| if (scaled_x < 0) { | ||
| *map_x = (viewport.view_scroll_x + scaled_x) / TILE_SIZE; | ||
| } else { | ||
| *map_x = static_cast<int>(viewport.view_scroll_x + (scaled_x * viewport.zoom)) / TILE_SIZE; | ||
| } | ||
|
|
||
| if (scaled_y < 0) { | ||
| *map_y = (viewport.view_scroll_y + scaled_y) / TILE_SIZE; | ||
| } else { | ||
| *map_y = static_cast<int>(viewport.view_scroll_y + (scaled_y * viewport.zoom)) / TILE_SIZE; | ||
| } | ||
|
|
||
| *map_x += GetFloorTileOffset(viewport.floor); | ||
| *map_y += GetFloorTileOffset(viewport.floor); | ||
| } | ||
|
|
||
| void MainMapViewMath::MapToScreen(int map_x, int map_y, int map_z, const MainMapViewport& viewport, int* screen_x, int* screen_y) { | ||
| map_x -= GetFloorTileOffset(map_z); | ||
| map_y -= GetFloorTileOffset(map_z); | ||
|
|
||
| const double raw_x = static_cast<double>(map_x * TILE_SIZE - viewport.view_scroll_x); | ||
| const double raw_y = static_cast<double>(map_y * TILE_SIZE - viewport.view_scroll_y); | ||
|
|
||
| if (raw_x < 0.0) { | ||
| *screen_x = static_cast<int>(std::ceil(raw_x / viewport.scale_factor)); | ||
| } else { | ||
| *screen_x = static_cast<int>(std::ceil(raw_x / (viewport.zoom * viewport.scale_factor))); | ||
| } | ||
|
|
||
| if (raw_y < 0.0) { | ||
| *screen_y = static_cast<int>(std::ceil(raw_y / viewport.scale_factor)); | ||
| } else { | ||
| *screen_y = static_cast<int>(std::ceil(raw_y / (viewport.zoom * viewport.scale_factor))); | ||
| } | ||
| } |
There was a problem hiding this comment.
To adhere more closely to modern C++ practices (as per style guide line 77) and improve type safety, consider refactoring ScreenToMap and MapToScreen to return a struct (e.g., struct Point { int x; int y; };) instead of using C-style output pointers. This avoids potential null pointer issues and makes the call sites cleaner.
For example, ScreenToMap could be changed to:
[[nodiscard]] static Point MainMapViewMath::ScreenToMap(int screen_x, int screen_y, const MainMapViewport& viewport) {
// ... logic ...
return {calculated_x, calculated_y};
}A similar change would apply to MapToScreen.
| [[nodiscard]] MinimapDirtyRect unionRects(const MinimapDirtyRect& lhs, const MinimapDirtyRect& rhs) { | ||
| if (lhs.width <= 0 || lhs.height <= 0) { | ||
| return rhs; | ||
| } | ||
| if (rhs.width <= 0 || rhs.height <= 0) { | ||
| return lhs; | ||
| } | ||
|
|
||
| const int min_x = std::min(lhs.x, rhs.x); | ||
| const int min_y = std::min(lhs.y, rhs.y); | ||
| const int max_x = std::max(lhs.x + lhs.width, rhs.x + rhs.width); | ||
| const int max_y = std::max(lhs.y + lhs.height, rhs.y + rhs.height); | ||
| return { | ||
| .x = min_x, | ||
| .y = min_y, | ||
| .width = max_x - min_x, | ||
| .height = max_y - min_y, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This unionRects function is a duplicate of the one found in source/rendering/drawers/minimap_cache.cpp. To adhere to the DRY (Don't Repeat Yourself) principle (style guide line 11), this helper function should be extracted into a common utility file (e.g., in util/ or a new rendering/util header) and included where needed.
Extracted MinimapDirtyRect struct and rectangle utility functions into dedicated minimap_rect.h header to eliminate duplication across minimap_cache, minimap_drawer, and minimap_manager modules. - source/map/basemap.h — added const-qualified visitLeaves template overload forwarding to grid - source/map/spatial_hash_grid.h — added const visitLeaves template with visitLeavesConstImpl and ConstRowCellInfo support struct - source/rendering/core/map_view_math.cpp — added symmetry comments explaining negative coordinate handling in ScreenToMap/MapToScreen conversions - source/rendering/drawers/minimap_cache.cpp — moved isValidRect and unionRects to shared minimap_rect.h, switched map.visitLeaves to const overload - source/rendering/drawers/minimap_cache.h — removed MinimapDirtyRect struct and unionRects declaration, added minimap_rect.h include - source/rendering/drawers/minimap_drawer.cpp — removed unused wxDC parameter and wxUnusedVar call from Draw method - source/rendering/drawers/minimap_drawer.h — removed wxDC& parameter from Draw declaration - source/rendering/drawers/minimap_rect.h — new file with MinimapDirtyRect struct, IsValidMinimapRect, and UnionMinimapRects inline functions - source/rendering/ui/minimap_window.cpp — removed dc argument from drawer->Draw() call - source/rendering/ui/zoom_controller.cpp — passed true to ScrollRelative to trigger immediate minimap update, removed separate g_gui.UpdateMinimap call - source/ui/managers/minimap_manager.cpp — removed local unionRects and minimap_cache.h include, using shared UnionMinimapRects - source/ui/map_window.cpp — added immediate_minimap_update parameter to ScrollRelative, forwarded to g_gui.UpdateMinimap - source/ui/map_window.h — added immediate_minimap_update parameter with false default to ScrollRelative declaration
|
🤖 Hi @karolak6612, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/rendering/ui/minimap_window.cpp (1)
299-307:⚠️ Potential issue | 🟠 MajorMake Ctrl+Wheel floor stepping match the button direction.
Line 301 defines “Floor Up” as
StepFloor(-1), but Line 539 maps wheel-up toStepFloor(1). This inverts floor navigation between controls.Proposed fix
void MinimapCanvas::OnMouseWheel(wxMouseEvent& event) { if (g_gui.IsLoading() || !g_gui.IsEditorOpen()) { return; } if (event.ControlDown()) { - owner_->StepFloor(event.GetWheelRotation() > 0 ? 1 : -1); + owner_->StepFloor(event.GetWheelRotation() > 0 ? -1 : 1); } else {Also applies to: 533-540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/ui/minimap_window.cpp` around lines 299 - 307, The wheel handler currently uses the opposite sign of the button handlers: OnFloorUp calls StepFloor(-1) and OnFloorDown calls StepFloor(1) but the Ctrl+wheel code (mouse wheel handler around the OnMouseWheel/wheel handling block) calls StepFloor(1) for wheel-up; change the wheel-up case to call StepFloor(-1) and wheel-down to call StepFloor(1) so the Ctrl+wheel behavior matches OnFloorUp/OnFloorDown (keep references to StepFloor, OnFloorUp, OnFloorDown and the mouse wheel handler when making the change).
🧹 Nitpick comments (2)
source/rendering/core/map_view_math.cpp (1)
37-58: Potential precision loss in integer conversion before division.At lines 47 and 53, the expression
static_cast<int>(viewport.view_scroll_x + (scaled_x * viewport.zoom))casts the floating-point result tointbefore dividing byTILE_SIZE. For large scroll values or zoom factors close to 1.0, this could truncate precision before the division.Consider performing the division in floating-point before truncating:
♻️ Suggested improvement
if (scaled_x < 0) { *map_x = (viewport.view_scroll_x + scaled_x) / TILE_SIZE; } else { - *map_x = static_cast<int>(viewport.view_scroll_x + (scaled_x * viewport.zoom)) / TILE_SIZE; + *map_x = static_cast<int>((viewport.view_scroll_x + scaled_x * viewport.zoom) / TILE_SIZE); } if (scaled_y < 0) { *map_y = (viewport.view_scroll_y + scaled_y) / TILE_SIZE; } else { - *map_y = static_cast<int>(viewport.view_scroll_y + (scaled_y * viewport.zoom)) / TILE_SIZE; + *map_y = static_cast<int>((viewport.view_scroll_y + scaled_y * viewport.zoom) / TILE_SIZE); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/rendering/core/map_view_math.cpp` around lines 37 - 58, In MainMapViewMath::ScreenToMap the else-branches compute map_x/map_y by casting the summed float to int before dividing by TILE_SIZE, which truncates precision; change both expressions in the else branches (the ones using viewport.view_scroll_x + (scaled_x * viewport.zoom) and viewport.view_scroll_y + (scaled_y * viewport.zoom)) to perform the division in floating point first and then cast the final result to int (e.g., static_cast<int>((viewport.view_scroll_x + scaled_x * viewport.zoom) / TILE_SIZE)), preserving the negative-branch behavior and keeping GetFloorTileOffset(viewport.floor) adjustments the same.source/ui/managers/minimap_manager.cpp (1)
35-40: Clear stale pending invalidations when maps are closed.
pending_invalidations_entries are keyed by(map pointer, generation)and only removed viaTakePendingInvalidation()when the map is rendered. If a map is closed before rendering, its pending invalidation entry remains in the map indefinitely, causing minor memory accumulation over multiple map close/open cycles. Add a call to clear entries for a givenMap*inDestroy()or create a cleanup method to be called on map unload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/managers/minimap_manager.cpp` around lines 35 - 40, Pending invalidations keyed by MinimapManager::InvalidationKey (created by MinimapManager::makeKey) are never removed if a Map is closed before rendering, leaving entries in pending_invalidations_; update MinimapManager to clear any pending_invalidations_ entries whose .map matches the closed Map pointer. Add the cleanup in Destroy(Map*), or implement a new ClearPendingInvalidationsForMap(Map*) and call it on unload; ensure it iterates pending_invalidations_ and erases keys where key.map == map, and leave TakePendingInvalidation() behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/map/spatial_hash_grid.h`:
- Around line 126-131: ConstRowCellInfo is defined at class scope but
re-declared inside visitLeavesConstImpl, shadowing the class member; remove the
local struct declaration inside visitLeavesConstImpl so the implementation uses
the class-level ConstRowCellInfo definition (update any local variable usages
there to rely on the existing class-scoped struct).
---
Duplicate comments:
In `@source/rendering/ui/minimap_window.cpp`:
- Around line 299-307: The wheel handler currently uses the opposite sign of the
button handlers: OnFloorUp calls StepFloor(-1) and OnFloorDown calls
StepFloor(1) but the Ctrl+wheel code (mouse wheel handler around the
OnMouseWheel/wheel handling block) calls StepFloor(1) for wheel-up; change the
wheel-up case to call StepFloor(-1) and wheel-down to call StepFloor(1) so the
Ctrl+wheel behavior matches OnFloorUp/OnFloorDown (keep references to StepFloor,
OnFloorUp, OnFloorDown and the mouse wheel handler when making the change).
---
Nitpick comments:
In `@source/rendering/core/map_view_math.cpp`:
- Around line 37-58: In MainMapViewMath::ScreenToMap the else-branches compute
map_x/map_y by casting the summed float to int before dividing by TILE_SIZE,
which truncates precision; change both expressions in the else branches (the
ones using viewport.view_scroll_x + (scaled_x * viewport.zoom) and
viewport.view_scroll_y + (scaled_y * viewport.zoom)) to perform the division in
floating point first and then cast the final result to int (e.g.,
static_cast<int>((viewport.view_scroll_x + scaled_x * viewport.zoom) /
TILE_SIZE)), preserving the negative-branch behavior and keeping
GetFloorTileOffset(viewport.floor) adjustments the same.
In `@source/ui/managers/minimap_manager.cpp`:
- Around line 35-40: Pending invalidations keyed by
MinimapManager::InvalidationKey (created by MinimapManager::makeKey) are never
removed if a Map is closed before rendering, leaving entries in
pending_invalidations_; update MinimapManager to clear any
pending_invalidations_ entries whose .map matches the closed Map pointer. Add
the cleanup in Destroy(Map*), or implement a new
ClearPendingInvalidationsForMap(Map*) and call it on unload; ensure it iterates
pending_invalidations_ and erases keys where key.map == map, and leave
TakePendingInvalidation() behavior unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f755332-169d-4589-8706-29c8673fb98b
📒 Files selected for processing (13)
source/map/basemap.hsource/map/spatial_hash_grid.hsource/rendering/core/map_view_math.cppsource/rendering/drawers/minimap_cache.cppsource/rendering/drawers/minimap_cache.hsource/rendering/drawers/minimap_drawer.cppsource/rendering/drawers/minimap_drawer.hsource/rendering/drawers/minimap_rect.hsource/rendering/ui/minimap_window.cppsource/rendering/ui/zoom_controller.cppsource/ui/managers/minimap_manager.cppsource/ui/map_window.cppsource/ui/map_window.h
✅ Files skipped from review due to trivial changes (1)
- source/rendering/drawers/minimap_rect.h
🚧 Files skipped from review as they are similar to previous changes (3)
- source/rendering/ui/zoom_controller.cpp
- source/ui/map_window.h
- source/rendering/drawers/minimap_cache.h
| struct ConstRowCellInfo { | ||
| const GridCell* cell; | ||
| int cell_start_nx; | ||
| int local_start_nx; | ||
| int local_end_nx; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate struct definition - local definition shadows class member.
ConstRowCellInfo is defined at class scope (lines 126-131), but then redefined locally inside visitLeavesConstImpl (lines 221-226). The local definition shadows the class member and makes the class-level definition dead code.
♻️ Remove the duplicate local definition
template <typename Func>
void visitLeavesConstImpl(int start_nx, int start_ny, int end_nx, int end_ny, int start_cx, int start_cy, int end_cx, int end_cy, Func&& func) const {
if (cells_.empty()) {
return;
}
- struct ConstRowCellInfo {
- const GridCell* cell;
- int cell_start_nx;
- int local_start_nx;
- int local_end_nx;
- };
-
static thread_local std::vector<ConstRowCellInfo> row_cells;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/map/spatial_hash_grid.h` around lines 126 - 131, ConstRowCellInfo is
defined at class scope but re-declared inside visitLeavesConstImpl, shadowing
the class member; remove the local struct declaration inside
visitLeavesConstImpl so the implementation uses the class-level ConstRowCellInfo
definition (update any local variable usages there to rely on the existing
class-scoped struct).
1 participant
Work in progress
Summary by CodeRabbit
New Features
Improvements
Bug Fixes