refactor(ui): implement granular map repaint reasons - #1015
Conversation
Replaced generic canvas refreshes with a flag-based repaint system to optimize rendering performance. Introduced granular reasons for updates, including viewport shifts, interaction overlays, and throttled animation ticks. - `source/game/animation_timer.cpp` — switched to `RequestAnimationRepaint()` for throttled animation updates - `source/rendering/ui/drawing_controller.cpp` — utilized `InteractionOverlayChanged` flag for drag-related refreshes - `source/rendering/ui/keyboard_handler.cpp` — mapped keyboard actions to specific hover, interaction, or viewport repaint reasons - `source/rendering/ui/map_display.cpp` — implemented `RequestRepaint` logic with animation throttling and overlay state validation - `source/rendering/ui/map_display.h` — added `RepaintReason` enum and members for tracking repaint flags and animation timing - `source/rendering/ui/navigation_controller.cpp` — replaced generic refreshes with `ViewportChanged` repaint requests - `source/rendering/ui/selection_controller.cpp` — categorized selection dragging under `InteractionOverlayChanged` repaint reason - `source/rendering/ui/zoom_controller.cpp` — transitioned zoom operations to use `ViewportChanged` flag - `source/ui/gui.cpp` — updated `RefreshView` to skip redundant refreshes for unrelated map tabs - `source/ui/map_window.cpp` — updated scroll event handlers to request `ViewportChanged` repaints
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 introduces a granular repaint mechanism for the map UI to optimize rendering performance. By categorizing update triggers into specific reasons, the system can now avoid redundant full-canvas refreshes, particularly during animations and interaction events. This change significantly improves the responsiveness of the UI by ensuring that only necessary components are re-rendered based on the context of the user's actions. 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
|
|
🤖 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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR replaces direct/unconditional canvas refreshes with a reasoned repaint-request system: view invalidation state, reason-tagged requests, centralized evaluation/throttling (including animation-rate logic), and targeted flush/queueing APIs used across controllers and UI entry points. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Controller
participant GUI as GUI
participant Canvas as MapCanvas
participant Policy as RepaintPolicy
participant Native as Native Renderer
User->>GUI: input event (drag/scroll/keypress/animation tick)
GUI->>Canvas: MarkInvalid(reason) / RequestLocalRefresh(reason, immediate)
Canvas->>Canvas: accumulate pending_reasons, update invalidation_state
Canvas->>Policy: EvaluateRepaintRequest(state, pending_reasons, immediate, now)
Policy-->>Canvas: RepaintDecision (should_refresh, allowed_reasons, interval)
alt should_refresh == true
Canvas->>Native: QueueNativeRefresh(immediate?)
Native->>Canvas: OnPaint()
Canvas->>Native: Render using allowed_reasons
Canvas->>Canvas: clear pending_reasons, update last_animation_refresh
else should_refresh == false
Canvas-->>User: throttle / defer repaint
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces a granular repaint system for the map canvas to optimize rendering performance by categorizing update reasons and throttling animation ticks. While the architectural direction is excellent, there is a critical issue where hover-dependent previews (like brushes) may stop updating, and a logic flaw in the throttling mechanism that could delay unrelated repaint requests.
🔍 General Feedback
- Animation Throttling: The implementation of throttled animation updates is a great addition for performance, especially when zoomed out.
- Tab-Aware Refreshing: The optimization in
GUI::RefreshViewcorrectly avoids repainting unrelated map tabs, which is a significant improvement for multi-tab workflows. - Refinement Needed: The
RequestRepaintlogic needs to be more robust when handling multiple flags simultaneously to ensure no critical updates are skipped due to throttling of a single reason.
|
|
||
| bool MapCanvas::IsAnimationEnabled() const { | ||
| return g_settings.getBoolean(Config::SHOW_PREVIEW); | ||
| } |
There was a problem hiding this comment.
🔴 HasHoverDependentPreview() is too restrictive. It should also return true if an interactive brush is selected, as the brush preview overlay needs to be repainted when the mouse moves. Without this, the brush preview will not update its position during hovering.
| } | |
| bool MapCanvas::HasHoverDependentPreview() const { | |
| if (isPasting() || g_gui.GetSelectedBrush()) { | |
| return true; | |
| } | |
| if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow())) { | |
| return map_tab->GetSession()->secondary_map != nullptr; | |
| } | |
| return false; | |
| } |
|
|
||
| void MapCanvas::QueueNativeRefresh(bool immediate) { | ||
| (void)immediate; | ||
| if (refresh_watch.Time() > g_settings.getInteger(Config::HARD_REFRESH_RATE)) { |
There was a problem hiding this comment.
🟢 The immediate parameter is currently unused. It should likely be used to bypass the HARD_REFRESH_RATE throttling or to decide whether to call wxGLCanvas::Update() (immediate) vs wxGLCanvas::Refresh() (queued).
| if (refresh_watch.Time() > g_settings.getInteger(Config::HARD_REFRESH_RATE)) { | |
| void MapCanvas::QueueNativeRefresh(bool immediate) { | |
| if (immediate || refresh_watch.Time() > g_settings.getInteger(Config::HARD_REFRESH_RATE)) { | |
| refresh_watch.Start(); | |
| wxGLCanvas::Update(); | |
| } | |
| wxGLCanvas::Refresh(); | |
| } |
| wxGLCanvas::Refresh(); | ||
| } | ||
|
|
||
| void MapCanvas::RequestRepaint(RepaintReason reason, bool immediate) { |
There was a problem hiding this comment.
🟠 Returning early when an AnimationTick is throttled can cause other flags in the same reason bitmask to be ignored, as QueueNativeRefresh is skipped. The check should be adjusted to only return if AnimationTick is the only reason or if all reasons are throttled.
| void MapCanvas::RequestRepaint(RepaintReason reason, bool immediate) { | |
| void MapCanvas::RequestRepaint(RepaintReason reason, bool immediate) { | |
| repaint_flags_ |= static_cast<uint32_t>(reason); | |
| bool should_refresh = !HasAnyFlag(reason, RepaintReason::AnimationTick); | |
| if (!should_refresh) { | |
| if (!IsAnimationEnabled()) { | |
| repaint_flags_ &= ~static_cast<uint32_t>(RepaintReason::AnimationTick); | |
| } else { | |
| const long long now = wxGetUTCTimeMillis().GetValue(); | |
| if (now - last_animation_refresh_ms_ >= GetAnimationRefreshIntervalMs()) { | |
| last_animation_refresh_ms_ = now; | |
| should_refresh = true; | |
| } | |
| } | |
| } | |
| if (should_refresh || (repaint_flags_ & ~static_cast<uint32_t>(RepaintReason::AnimationTick)) != 0) { | |
| QueueNativeRefresh(immediate); | |
| } | |
| } |
| class MapMenuHandler; | ||
|
|
||
| class MapCanvas : public wxGLCanvas { | ||
| public: |
There was a problem hiding this comment.
🟢 Using an enum class for bitmask flags leads to repetitive static_cast calls. Consider adding bitwise operator overloads for RepaintReason to improve readability and maintainability.
| public: | |
| enum class RepaintReason : uint32_t { | |
| None = 0, | |
| ViewportChanged = 1u << 0, | |
| MapContentChanged = 1u << 1, | |
| HoverOverlayChanged = 1u << 2, | |
| InteractionOverlayChanged = 1u << 3, | |
| AnimationTick = 1u << 4, | |
| }; | |
| friend constexpr RepaintReason operator|(RepaintReason a, RepaintReason b) { | |
| return static_cast<RepaintReason>(static_cast<uint32_t>(a) | static_cast<uint32_t>(b)); | |
| } | |
| friend constexpr RepaintReason operator&(RepaintReason a, RepaintReason b) { | |
| return static_cast<RepaintReason>(static_cast<uint32_t>(a) & static_cast<uint32_t>(b)); | |
| } |
![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 introduces a granular repaint system for MapCanvas using RepaintReason flags to optimize rendering updates. Key changes include the implementation of RequestRepaint, which allows for specific updates like viewport changes or animation ticks, and the addition of logic to throttle animation refreshes based on zoom levels. Feedback includes a correction for a dynamic_cast that would otherwise fail, a suggestion to implement the ignored immediate parameter in QueueNativeRefresh, and recommendations to use C++23's std::to_underlying for enum operations and std::chrono::steady_clock for timing to align with the project's mandatory modern C++ standards.
| return true; | ||
| } | ||
|
|
||
| if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow())) { |
There was a problem hiding this comment.
The dynamic_cast here will always fail because MapWindow is a wxPanel (as defined in map_window.cpp), not a MapTab. MapTab is the parent of the MapWindow. You should cast the parent of the window instead.
if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow()->GetParent())) {| void MapCanvas::QueueNativeRefresh(bool immediate) { | ||
| (void)immediate; | ||
| if (refresh_watch.Time() > g_settings.getInteger(Config::HARD_REFRESH_RATE)) { | ||
| refresh_watch.Start(); | ||
| wxGLCanvas::Update(); | ||
| } | ||
| wxGLCanvas::Refresh(); | ||
| } |
There was a problem hiding this comment.
The immediate parameter is currently ignored. If immediate is true, the canvas should call wxGLCanvas::Update() to force an immediate repaint and bypass the HARD_REFRESH_RATE throttle.
void MapCanvas::QueueNativeRefresh(bool immediate) {
if (immediate || refresh_watch.Time() > g_settings.getInteger(Config::HARD_REFRESH_RATE)) {
refresh_watch.Start();
wxGLCanvas::Update();
}
wxGLCanvas::Refresh();
}| bool MapCanvas::HasAnyFlag(RepaintReason value, RepaintReason flag) const { | ||
| return (static_cast<uint32_t>(value) & static_cast<uint32_t>(flag)) != 0; | ||
| } |
There was a problem hiding this comment.
To adhere to the C++20/23 mandatory standard (Rule 7 & 80), use std::to_underlying from <utility> instead of manual static_cast for enum class bitwise operations.
bool MapCanvas::HasAnyFlag(RepaintReason value, RepaintReason flag) const {
return (std::to_underlying(value) & std::to_underlying(flag)) != 0;
}References
- Every line of code you write or modify MUST use C++20/23 features. Pre-C++17 patterns are technical debt. (link)
| return; | ||
| } | ||
|
|
||
| const long long now = wxGetUTCTimeMillis().GetValue(); |
There was a problem hiding this comment.
Prefer using std::chrono::steady_clock for interval timing instead of wxGetUTCTimeMillis(). Steady clocks are monotonic and unaffected by system time adjustments, making them safer for throttling logic. Also, use auto for the type as it is obvious from the assignment (Rule 308).
const auto now = std::chrono::steady_clock::now().time_since_epoch();
const auto now_ms = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();
if (now_ms - last_animation_refresh_ms_ < GetAnimationRefreshIntervalMs()) {
return;
}
last_animation_refresh_ms_ = now_ms;References
- Use auto where type is obvious. Eliminate pre-C++17 patterns (like legacy time helpers) on contact. (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: 3
🧹 Nitpick comments (1)
source/rendering/ui/navigation_controller.cpp (1)
95-108: Optional: only request a viewport repaint when the floor actually changes.
RequestRepaint(ViewportChanged)at line 107 runs unconditionally, even whenold_floor == new_floor(e.g., mouse-wheel floor change at the bottom/top, or repeated calls from wheel accumulation). Moving it inside the existing guard avoids a redundant full-viewport repaint on no-op calls.♻️ Proposed refactor
if (old_floor != new_floor) { if (auto* map_window = dynamic_cast<MapWindow*>(canvas->GetParent())) { map_window->ResumeMinimapTrackingToCurrentView(); } canvas->UpdatePositionStatus(); g_gui.root->UpdateFloorMenu(); g_gui.UpdateMinimap(true); + canvas->RequestRepaint(MapCanvas::RepaintReason::ViewportChanged); } - canvas->RequestRepaint(MapCanvas::RepaintReason::ViewportChanged); }🤖 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 95 - 108, The RequestRepaint call is made unconditionally in NavigationController::ChangeFloor causing unnecessary full viewport repaints even when old_floor == new_floor; move the call to canvas->RequestRepaint(MapCanvas::RepaintReason::ViewportChanged) inside the existing if (old_floor != new_floor) block (alongside UpdatePositionStatus(), g_gui.root->UpdateFloorMenu(), and g_gui.UpdateMinimap(true)) so a viewport repaint only occurs when the floor actually changes.
🤖 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/ui/map_display.cpp`:
- Around line 186-196: MapCanvas::HasHoverDependentPreview currently only
returns true for pasting and a secondary_map in MapTab, causing normal
brush/autoborder previews to stop following the cursor; update
HasHoverDependentPreview to also return true when the canvas is in a
drawing/brush/autoborder preview state by querying the canvas/tool state (e.g.
check the current tool or a method like IsDrawingMode/HasBrushPreview or similar
on MapCanvas or its tool manager) so OnMouseMove() and SyncCursorHoverState()
will repaint while brush/autoborder previews are active; keep existing
isPasting() and MapTab/GetSession()->secondary_map checks and add the
drawing-preview check before returning false.
- Around line 223-240: MapCanvas::RequestRepaint currently bails out early when
RepaintReason contains AnimationTick, which drops any other simultaneous flags;
instead, only remove the AnimationTick bit when animation is disabled or
throttled and continue to call QueueNativeRefresh for the remaining reasons.
Concretely: in MapCanvas::RequestRepaint, replace the early returns inside the
IsAnimationEnabled() check and the throttling check so that you clear
repaint_flags_ &= ~RepaintReason::AnimationTick when animation is disabled or
when now - last_animation_refresh_ms_ < GetAnimationRefreshIntervalMs(), and
only return early when you intend to suppress all reasons (which you don’t
here); if the tick is allowed, update last_animation_refresh_ms_ as now; always
fall through to QueueNativeRefresh(immediate) so non-animation repaint reasons
still trigger a refresh.
- Around line 214-221: MapCanvas::QueueNativeRefresh currently ignores the
immediate parameter and calls Update() before Refresh(), which violates
wxWidgets contract; change the logic to honor immediate and ensure Refresh() is
called before any Update(): always call wxGLCanvas::Refresh(), then if
immediate==true or refresh_watch.Time() >
g_settings.getInteger(Config::HARD_REFRESH_RATE) call refresh_watch.Start() and
wxGLCanvas::Update(); remove the (void)immediate cast and reference
MapCanvas::QueueNativeRefresh and refresh_watch so the fix is applied in that
function.
---
Nitpick comments:
In `@source/rendering/ui/navigation_controller.cpp`:
- Around line 95-108: The RequestRepaint call is made unconditionally in
NavigationController::ChangeFloor causing unnecessary full viewport repaints
even when old_floor == new_floor; move the call to
canvas->RequestRepaint(MapCanvas::RepaintReason::ViewportChanged) inside the
existing if (old_floor != new_floor) block (alongside UpdatePositionStatus(),
g_gui.root->UpdateFloorMenu(), and g_gui.UpdateMinimap(true)) so a viewport
repaint only occurs when the floor actually changes.
🪄 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: d82e5daa-91cf-40e1-8a50-c2bd822d359b
📒 Files selected for processing (10)
source/game/animation_timer.cppsource/rendering/ui/drawing_controller.cppsource/rendering/ui/keyboard_handler.cppsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/navigation_controller.cppsource/rendering/ui/selection_controller.cppsource/rendering/ui/zoom_controller.cppsource/ui/gui.cppsource/ui/map_window.cpp
| bool MapCanvas::HasHoverDependentPreview() const { | ||
| if (isPasting()) { | ||
| return true; | ||
| } | ||
|
|
||
| if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow())) { | ||
| return map_tab->GetSession()->secondary_map != nullptr; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Keep normal brush previews hover-dependent.
OnMouseMove() and SyncCursorHoverState() now repaint cursor movement only when this returns true. This currently covers paste and secondary-map previews, but not ordinary drawing-mode brush/autoborder previews, so the preview can stop following the cursor when neither condition is true.
🐛 Proposed fix
bool MapCanvas::HasHoverDependentPreview() const {
if (isPasting()) {
return true;
}
+ if (g_gui.IsDrawingMode() && g_gui.GetCurrentBrush() && g_settings.getBoolean(Config::SHOW_PREVIEW)) {
+ return true;
+ }
+
if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow())) {
return map_tab->GetSession()->secondary_map != nullptr;
}
return false;
}📝 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.
| bool MapCanvas::HasHoverDependentPreview() const { | |
| if (isPasting()) { | |
| return true; | |
| } | |
| if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow())) { | |
| return map_tab->GetSession()->secondary_map != nullptr; | |
| } | |
| return false; | |
| } | |
| bool MapCanvas::HasHoverDependentPreview() const { | |
| if (isPasting()) { | |
| return true; | |
| } | |
| if (g_gui.IsDrawingMode() && g_gui.GetCurrentBrush() && g_settings.getBoolean(Config::SHOW_PREVIEW)) { | |
| return true; | |
| } | |
| if (auto* map_tab = dynamic_cast<MapTab*>(GetMapWindow())) { | |
| return map_tab->GetSession()->secondary_map != nullptr; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/rendering/ui/map_display.cpp` around lines 186 - 196,
MapCanvas::HasHoverDependentPreview currently only returns true for pasting and
a secondary_map in MapTab, causing normal brush/autoborder previews to stop
following the cursor; update HasHoverDependentPreview to also return true when
the canvas is in a drawing/brush/autoborder preview state by querying the
canvas/tool state (e.g. check the current tool or a method like
IsDrawingMode/HasBrushPreview or similar on MapCanvas or its tool manager) so
OnMouseMove() and SyncCursorHoverState() will repaint while brush/autoborder
previews are active; keep existing isPasting() and
MapTab/GetSession()->secondary_map checks and add the drawing-preview check
before returning false.
Refactored map canvas invalidation into a policy-based system to handle throttled animation ticks and state-dependent refreshes. Replaced direct refresh calls with a mark-and-flush pattern that evaluates invalidation reasons against current view state. - `source/CMakeLists.txt` — registered new repaint policy and invalidation state files in the build system - `source/rendering/ui/drawing_controller.cpp` — switched interaction overlay updates to `MarkInvalid` and `FlushRepaintRequest` pattern - `source/rendering/ui/keyboard_handler.cpp` — updated keyboard-driven view updates to use explicit invalidation marking and shared map refresh reasons - `source/rendering/ui/map_display.cpp` — implemented `invalidation_state_` tracking, switched to `steady_clock` timing, and integrated `EvaluateRepaintRequest` logic - `source/rendering/ui/map_display.h` — moved `RepaintReason` to dedicated header, modernized refresh API, and added `ViewInvalidationState` member - `source/rendering/ui/navigation_controller.cpp` — migrated viewport navigation and floor changes to use granular invalidation marking - `source/rendering/ui/repaint_policy.cpp` — implemented `EvaluateRepaintRequest` with zoom-dependent animation intervals and refresh throttling logic - `source/rendering/ui/repaint_policy.h` — defined the `RepaintDecision` interface for evaluating invalidation requests against view state - `source/rendering/ui/selection_controller.cpp` — converted selection drag triggers to use the mark-and-flush invalidation pattern - `source/rendering/ui/view_invalidation_state.h` — defined `RepaintReason` enum, `ViewInvalidationState` container, and bitwise operator overloads for flag management - `source/rendering/ui/zoom_controller.cpp` — updated zoom handlers to mark viewport changes and flush repaint requests sequentially - `source/ui/gui.cpp` — introduced `SyncCurrentMapCanvasPreviewState` to manage global hover status and updated multi-tab refresh propagation - `source/ui/gui.h` — added state synchronization declarations and enhanced `RefreshView` parameter signatures - `source/ui/gui_autoborder_ext.cpp` — integrated canvas preview state synchronization with autoborder buffer lifecycle - `source/ui/map_window.cpp` — migrated scroll event handlers to the new invalidation marking and flushing system
|
🤖 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. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
🤖 I'm sorry @karolak6612, but I was unable to process your request. Please see the logs for more details. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/ui/gui_autoborder_ext.cpp (1)
20-34:⚠️ Potential issue | 🟠 MajorEnsure
secondary_mapmutations are consistently synchronized.The autoborder and brush-change paths in
gui_autoborder_ext.cppproperly callSyncCurrentMapCanvasPreviewState()after mutations, but doodad preview updates inbrush_manager.cpp(lines 118, 197, 394) and pasting operations ineditor_manager.cpp(lines 421, 430) do not. While the GUI wrapper layer (GUI::FillDoodadPreviewBuffer()andGUI::StartPasting()/GUI::EndPasting()) currently provides synchronization, this relies on an implicit contract rather than explicit pairing at the mutation site. Centralizesecondary_mapupdates through a setter method that handles synchronization, or add sync calls consistently at each mutation point to prevent stale preview state if the code is refactored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/ui/gui_autoborder_ext.cpp` around lines 20 - 34, The code mutates session->secondary_map in several places (e.g., the autoborder path uses mapTab->GetSession()->secondary_map = g_autoborder_preview.GetBufferMap()) but not every mutation (brush_manager.cpp lines ~118/197/394 and editor_manager.cpp lines ~421/430) calls SyncCurrentMapCanvasPreviewState(), risking stale previews after refactors; fix by centralizing secondary_map updates into a single setter (e.g., Session::SetSecondaryMap or MapTab::SetSecondaryMap) that assigns secondary_map and always calls SyncCurrentMapCanvasPreviewState(), and then replace direct assignments to session->secondary_map across g_autoborder_preview.Update, brush_manager, and editor_manager with calls to that setter (or, if you prefer not to add a setter, ensure every direct assignment to session->secondary_map is immediately followed by SyncCurrentMapCanvasPreviewState()).
🤖 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/ui/map_display.cpp`:
- Around line 238-240: SetHoverPreviewActive currently only flips
invalidation_state_.hover_preview_active and doesn't request a redraw, leaving
previews stale; update MapCanvas::SetHoverPreviewActive(bool active) to also
trigger a repaint after changing the flag by calling the canvas
redraw/invalidate entry (e.g., call Invalidate() or the existing MapCanvas
redraw/schedule method) so the hover/paste preview is repainted immediately when
hover_preview_active changes.
---
Outside diff comments:
In `@source/ui/gui_autoborder_ext.cpp`:
- Around line 20-34: The code mutates session->secondary_map in several places
(e.g., the autoborder path uses mapTab->GetSession()->secondary_map =
g_autoborder_preview.GetBufferMap()) but not every mutation (brush_manager.cpp
lines ~118/197/394 and editor_manager.cpp lines ~421/430) calls
SyncCurrentMapCanvasPreviewState(), risking stale previews after refactors; fix
by centralizing secondary_map updates into a single setter (e.g.,
Session::SetSecondaryMap or MapTab::SetSecondaryMap) that assigns secondary_map
and always calls SyncCurrentMapCanvasPreviewState(), and then replace direct
assignments to session->secondary_map across g_autoborder_preview.Update,
brush_manager, and editor_manager with calls to that setter (or, if you prefer
not to add a setter, ensure every direct assignment to session->secondary_map is
immediately followed by SyncCurrentMapCanvasPreviewState()).
🪄 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: 7e82b015-2dee-42db-a447-fd6838eeb30a
📒 Files selected for processing (16)
source/CMakeLists.txtsource/game/animation_timer.cppsource/rendering/ui/drawing_controller.cppsource/rendering/ui/keyboard_handler.cppsource/rendering/ui/map_display.cppsource/rendering/ui/map_display.hsource/rendering/ui/navigation_controller.cppsource/rendering/ui/repaint_policy.cppsource/rendering/ui/repaint_policy.hsource/rendering/ui/selection_controller.cppsource/rendering/ui/view_invalidation_state.hsource/rendering/ui/zoom_controller.cppsource/ui/gui.cppsource/ui/gui.hsource/ui/gui_autoborder_ext.cppsource/ui/map_window.cpp
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a granular view invalidation system for the map canvas, replacing generic refresh calls with specific repaint reasons such as ViewportChanged and MapContentChanged. It also implements a RepaintPolicy to throttle animation updates based on zoom levels and interaction states, improving rendering efficiency. The review feedback suggests minor refinements to align with modern C++20/23 idioms, including using default-constructed time_point comparisons, removing redundant scope resolution operators for consistency, and pruning an unused helper function in the invalidation state header.
| return; | ||
| } | ||
|
|
||
| if (::HasFlag(decision.allowed_reasons, RepaintReason::AnimationTick)) { |
There was a problem hiding this comment.
The global scope resolution operator :: is redundant here as there is no name conflict. Removing it would be more consistent with other parts of the codebase, such as repaint_policy.cpp.
if (HasFlag(decision.allowed_reasons, RepaintReason::AnimationTick)) {| } | ||
|
|
||
| const auto refresh_interval = std::chrono::milliseconds(decision.animation_interval_ms); | ||
| if (state.last_animation_refresh.time_since_epoch().count() != 0 && now - state.last_animation_refresh < refresh_interval) { |
There was a problem hiding this comment.
A more idiomatic way to check if a time_point has been initialized (i.e., is not at the epoch) is to compare it directly against a default-constructed time_point instead of checking the underlying tick count.
| if (state.last_animation_refresh.time_since_epoch().count() != 0 && now - state.last_animation_refresh < refresh_interval) { | |
| if (state.last_animation_refresh != std::chrono::steady_clock::time_point{} && now - state.last_animation_refresh < refresh_interval) { |
| [[nodiscard]] constexpr bool AnyNonAnimationFlags(RepaintReason value) { | ||
| return (std::to_underlying(value) & ~std::to_underlying(RepaintReason::AnimationTick)) != 0; | ||
| } |
There was a problem hiding this comment.
The helper function AnyNonAnimationFlags appears to be unused in the current implementation. Consider removing it to keep the header clean, or mark it with [[maybe_unused]] if it is intended for future use in other modules.
Centralized secondary map (preview) assignment into a single GUI method to ensure consistent state synchronization. Fixed hover preview invalidation and refined animation throttling logic. - `source/brushes/managers/brush_manager.cpp` — replaced direct session access with `SetCurrentMapSecondaryMap()` calls - `source/editor/managers/editor_manager.cpp` — migrated pasting preview map assignment to centralized GUI setter - `source/rendering/ui/map_display.cpp` — added state guard and invalidation triggers to `SetHoverPreviewActive()` - `source/rendering/ui/repaint_policy.cpp` — switched to idiomatic `time_point` comparison for animation throttling [minor] - `source/rendering/ui/view_invalidation_state.h` — removed unused `AnyNonAnimationFlags` helper [minor] - `source/ui/gui.cpp` — implemented `SetCurrentMapSecondaryMap` to handle session updates and canvas sync - `source/ui/gui.h` — added `SetCurrentMapSecondaryMap` to public interface - `source/ui/gui_autoborder_ext.cpp` — updated autoborder preview logic to use centralized GUI map setter
|
🤖 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. |
Simplified the view invalidation and repaint logic by removing the complex `RepaintReason` system and `RepaintPolicy`. Replaced manual invalidation tracking with direct refresh requests to streamline canvas updates. - `source/CMakeLists.txt` — removed deleted repaint policy and invalidation state files from build system - `source/rendering/ui/drawing_controller.cpp` — replaced explicit invalidation and flush calls with `RequestLocalRefresh()` - `source/rendering/ui/keyboard_handler.cpp` — migrated to direct refresh calls and simplified map refresh request signature - `source/rendering/ui/map_display.cpp` — implemented direct refresh logic, moved animation throttling to class, and removed `ViewInvalidationState` dependency - `source/rendering/ui/map_display.h` — simplified public interface by removing `MarkInvalid` and `FlushRepaintRequest`, replaced state struct with local members - `source/rendering/ui/navigation_controller.cpp` — switched to `RequestLocalRefresh()` for viewport updates - `source/rendering/ui/repaint_policy.cpp` — removed complex repaint decision logic and animation interval calculations - `source/rendering/ui/repaint_policy.h` — removed repaint policy interface and `RepaintDecision` struct - `source/rendering/ui/selection_controller.cpp` — updated overlay updates to use direct refresh calls - `source/rendering/ui/view_invalidation_state.h` — removed `RepaintReason` enum, `RefreshScope` enum, and `ViewInvalidationState` struct - `source/rendering/ui/zoom_controller.cpp` — updated zoom-triggered repaints to use direct refresh - `source/ui/gui.cpp` — updated `RefreshView` implementation to use simplified canvas refresh signature - `source/ui/gui.h` — removed `RepaintReason` parameter from `RefreshView` signature - `source/ui/map_window.cpp` — updated scroll-triggered repaints to use direct refresh calls
|
🤖 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.
The Pull Request successfully implements a more granular and optimized repaint system for the map canvas, replacing many generic full-view refreshes with local refreshes and throttled animations. This should lead to noticeable performance improvements, especially during mouse movement and animations.
🔍 General Feedback
- Optimization: The introduction of the
hover_preview_active_flag is a great optimization to avoid redundant repaints on every mouse move when no preview (brush, paste, or autoborder) is active. - Throttling: The animation throttling system is a good addition to manage CPU/GPU usage, although the current zoom-based logic seems to have its naming or priority inverted.
- Consistency: Ensure that global state changes (like brush selection) are synchronized across all open map tabs, as the current implementation only updates the active tab.
- Description Mismatch: The PR description mentions specific enum-based repaint reasons that are not yet implemented in the provided code; consider updating the description to reflect the current state of the refactor.
| const bool hover_preview_active = IsPasting() | ||
| || (mapTab->GetMode() == DRAWING_MODE && GetCurrentBrush() != nullptr) | ||
| || mapTab->GetSession()->secondary_map != nullptr; | ||
| mapTab->GetCanvas()->SetHoverPreviewActive(hover_preview_active); |
There was a problem hiding this comment.
🟠 This method only updates the current tab. Since the brush selection is global, switching to another tab that was previously in Drawing mode will result in a stale hover_preview_active_ state (e.g., if the brush was changed or cleared while the other tab was backgrounded). It should iterate through all map tabs to ensure consistency.
| mapTab->GetCanvas()->SetHoverPreviewActive(hover_preview_active); | |
| void GUI::SyncCurrentMapCanvasPreviewState() { | |
| for (int i = 0; i < tabbook->GetTabCount(); ++i) { | |
| auto* map_tab = dynamic_cast<MapTab*>(tabbook->GetTab(i)); | |
| if (!map_tab) { | |
| continue; | |
| } | |
| const bool hover_preview_active = IsPasting() | |
| || (map_tab->GetMode() == DRAWING_MODE && GetCurrentBrush() != nullptr) | |
| || map_tab->GetSession()->secondary_map != nullptr; | |
| map_tab->GetCanvas()->SetHoverPreviewActive(hover_preview_active); | |
| } | |
| } |
|
|
||
| if (map_update) { | ||
| Refresh(); | ||
| if (map_update && hover_preview_active_) { |
There was a problem hiding this comment.
🟡 In Selection mode, hover_preview_active_ will be false, which skips the refresh in SyncCursorHoverState. This will disable the hover highlight (the blue box indicating the tile under the mouse) in Selection mode, which might be a regression in UX. Consider enabling it for Selection mode if a hover highlight is desired.
| if (map_update && hover_preview_active_) { | |
| if (map_update && (hover_preview_active_ || g_gui.IsSelectionMode())) { |
| constexpr int near_zoom_refresh_interval_ms = 1000 / 60; | ||
| constexpr int far_zoom_refresh_interval_ms = 1000 / 20; | ||
| return zoom <= far_zoom_threshold ? near_zoom_refresh_interval_ms : far_zoom_refresh_interval_ms; | ||
| } |
There was a problem hiding this comment.
🟠 The logic for animation throttling appears to have swapped variable names or logic. If zoom <= 2.0 is considered "zoomed out" (far), it is currently receiving the near_zoom_refresh_interval_ms (60 FPS), while zoom > 2.0 (zoomed in/near) receives far_zoom_refresh_interval_ms (20 FPS). Usually, users expect smoother animations when looking closely (zoomed in).
| } | |
| int MapCanvas::GetAnimationRefreshIntervalMs() const { | |
| constexpr double near_zoom_threshold = 2.0; | |
| constexpr int near_zoom_refresh_interval_ms = 1000 / 60; | |
| constexpr int far_zoom_refresh_interval_ms = 1000 / 20; | |
| return zoom >= near_zoom_threshold ? near_zoom_refresh_interval_ms : far_zoom_refresh_interval_ms; | |
| } |
| class SelectionController; | ||
| class DrawingController; | ||
| class ScreenshotController; | ||
| class MapMenuHandler; |
There was a problem hiding this comment.
🟡 The PR description mentions the introduction of a RepaintReason enum and a flag-based repaint system (e.g., InteractionOverlayChanged, ViewportChanged). However, these are not present in the current diff. It seems the implementation was simplified to generic local/shared refreshes, or some changes were omitted from this commit.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the rendering and refresh logic by introducing specialized methods like RequestLocalRefresh and RequestSharedMapRefresh, alongside a state-driven hover preview system. While these changes aim to optimize performance, the current implementation of the hover_preview_active_ flag causes a regression by disabling tile highlights in Selection Mode. Furthermore, the Refresh method in MapCanvas hides the base wxWindow::Refresh due to a signature mismatch, which could lead to unexpected behavior. The refactor also pushes map_display.cpp beyond the 500-line limit specified in the style guide, necessitating a plan for file decomposition. Other minor improvements regarding modern C++ attributes like [[nodiscard]] and noexcept were also noted.
| if (map_update && hover_preview_active_) { | ||
| RequestLocalRefresh(); | ||
| } |
There was a problem hiding this comment.
This optimization introduces a regression in visual feedback. In Selection Mode, the editor typically displays a hover highlight (border) around the tile under the cursor. By checking hover_preview_active_ (which is false in selection mode per SyncCurrentMapCanvasPreviewState), you are preventing the canvas from repainting when the mouse moves between tiles. This means the hover highlight will not move until a click or other event occurs.
Since map_update already ensures we only refresh when the cursor actually moves to a different tile, the hover_preview_active_ check is likely unnecessary here and should be removed to maintain basic UI responsiveness.
if (map_update) {
RequestLocalRefresh();
}| if (hover_preview_active_) { | ||
| RequestLocalRefresh(); | ||
| } |
There was a problem hiding this comment.
Similar to the issue in SyncCursorHoverState, this check prevents the hover highlight from updating in Selection Mode. The map_update flag already provides sufficient throttling by ensuring we only repaint when the mouse crosses tile boundaries. Restricting it further to only when a "preview" is active breaks standard hover feedback.
if (map_update) {
RequestLocalRefresh();
}| @@ -80,6 +79,10 @@ class MapCanvas : public wxGLCanvas { | |||
| void OnMousePropertiesRelease(wxMouseEvent& event); | |||
|
|
|||
| virtual void Refresh(); | |||
There was a problem hiding this comment.
The signature of this Refresh method does not match the base wxWindow::Refresh(bool eraseBackground = true, const wxRect* rect = nullptr). Since wxWindow::Refresh is not virtual, this creates a name-hiding situation rather than an override. If a caller uses a wxWindow* pointer to this canvas and calls Refresh(), your optimized logic will be bypassed.
To adhere to the repository's principle of using modern C++ and avoiding confusion, you should either match the signature and mark it override (if you intend to hide it consistently) or, preferably, rename this to something like RequestRefresh and use RequestLocalRefresh consistently throughout the codebase.
References
- Upgrade legacy patterns and ensure clear function responsibilities (SRP). (link)
| const bool hover_preview_active = IsPasting() | ||
| || (mapTab->GetMode() == DRAWING_MODE && GetCurrentBrush() != nullptr) | ||
| || mapTab->GetSession()->secondary_map != nullptr; |
There was a problem hiding this comment.
The logic here for hover_preview_active specifically excludes Selection Mode (unless pasting). As noted in map_display.cpp, this causes the hover highlight to stop updating during normal mouse movement. You should include a check for whether the hover highlight is enabled in settings, or simply allow it to be active if the mode is not NONE.
const bool hover_preview_active = IsPasting()
|| (mapTab->GetMode() == DRAWING_MODE && GetCurrentBrush() != nullptr)
|| mapTab->GetMode() == SELECTION_MODE
|| mapTab->GetSession()->secondary_map != nullptr;| return g_settings.getBoolean(Config::SHOW_PREVIEW); | ||
| } | ||
|
|
||
| int MapCanvas::GetAnimationRefreshIntervalMs() const { |
There was a problem hiding this comment.
This function should be marked with [[nodiscard]] to ensure the return value is not ignored, and noexcept as it only performs simple arithmetic and a comparison.
int MapCanvas::GetAnimationRefreshIntervalMs() const noexcept {| @@ -171,11 +172,58 @@ MapCanvas::~MapCanvas() { | |||
| } | |||
There was a problem hiding this comment.
The file map_display.cpp is currently 755 lines long, which violates the repository style guide limit of 500 lines. While this PR is a refactor, consider planning a split of this file (e.g., moving event handlers to a separate controller) to adhere to the SRP and file size limits.
References
- File > 500 lines → split. (link)
Removed redundant Refresh override in MapCanvas in favor of RequestLocalRefresh and refactored GUI to synchronize hover preview state across all map tabs. - `source/rendering/ui/map_display.cpp` — removed `Refresh()` override, added `noexcept` to refresh interval getter, and simplified refresh triggers - `source/rendering/ui/map_display.h` — removed `Refresh()` declaration and marked `GetAnimationRefreshIntervalMs()` as `noexcept` - `source/rendering/ui/screenshot_controller.cpp` — replaced `Refresh()` calls with `RequestLocalRefresh()` - `source/ui/gui.cpp` — updated `SyncCurrentMapCanvasPreviewState()` to iterate through all tabs and update hover state for each MapCanvas
|
🤖 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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Replaced generic canvas refreshes with a flag-based repaint system to optimize rendering performance. Introduced granular reasons for updates, including viewport shifts, interaction overlays, and throttled animation ticks.
source/game/animation_timer.cpp— switched toRequestAnimationRepaint()for throttled animation updatessource/rendering/ui/drawing_controller.cpp— utilizedInteractionOverlayChangedflag for drag-related refreshessource/rendering/ui/keyboard_handler.cpp— mapped keyboard actions to specific hover, interaction, or viewport repaint reasonssource/rendering/ui/map_display.cpp— implementedRequestRepaintlogic with animation throttling and overlay state validationsource/rendering/ui/map_display.h— addedRepaintReasonenum and members for tracking repaint flags and animation timingsource/rendering/ui/navigation_controller.cpp— replaced generic refreshes withViewportChangedrepaint requestssource/rendering/ui/selection_controller.cpp— categorized selection dragging underInteractionOverlayChangedrepaint reasonsource/rendering/ui/zoom_controller.cpp— transitioned zoom operations to useViewportChangedflagsource/ui/gui.cpp— updatedRefreshViewto skip redundant refreshes for unrelated map tabssource/ui/map_window.cpp— updated scroll event handlers to requestViewportChangedrepaintsSummary by CodeRabbit