perf: optimize render loop, input polling, and event dispatching for …#69
Open
soyfabi wants to merge 1 commit into
Open
perf: optimize render loop, input polling, and event dispatching for …#69soyfabi wants to merge 1 commit into
soyfabi wants to merge 1 commit into
Conversation
…high-refresh-rate displays Optimized the engine and game loops to allow AstraClient to run smoothly on 165Hz+ high-refresh-rate monitors, eliminating frame capping, input lagging (freeze during item dragging), and mutex contention. Summary of changes: 1. Frame Limiter & Window Polling: - Changed default `m_maxFps` from 100 to 0 (uncapped/VSync controlled). - Removed the 10ms window polling restriction (`m_windowPollTimer`) to poll input events every frame, significantly improving mouse/keyboard responsiveness. - Refined sleep times in worker and render loops, replacing heavy sleep calls with microseconds-based precision sleeps. - Removed forced 100 FPS cap and 60 FPS quality-drop downgrades from the adaptive renderer. 2. Dynamic UI Refresh & Drag Responsiveness: - Implemented dynamic UI refresh rate scaling. During active widget dragging (e.g., inventory items), the UI refresh interval is scaled down from 16.6ms (~60 FPS) to 4ms (~250 FPS) to match mouse drag speed. - Registered drag state changes directly from `UIManager` to trigger high-frequency UI repaints when dragging starts. 3. Mutex Contention & Spinlock Refactoring: - Replaced heavy `std::mutex` synchronization between the worker and rendering threads with a lightweight atomic spinlock (`std::atomic_flag`) to swap draw queues, reducing frame time variance. 4. Lock-Free Event Dispatcher (Double-Buffering): - Redesigned `EventDispatcher` to use thread-safe double-buffered incoming event and scheduled event queues guarded by a lightweight `std::mutex`. - Thread execution of events now runs completely lock-free inside `poll()`, only acquiring the lock briefly to swap incoming queues at the beginning of the frame. 5. Rendering & Batching Enhancements: - Refactored `DrawQueue::draw()` to eliminate redundant double-cache attempts and removed per-frame `glCheckError` calls which were stalling the GPU pipeline. - Cached tile 2D coordinate calculations (`transformPositionTo2D`) in `MapView::drawFloor` to avoid recomputing the same position across multiple floor render passes.
WalkthroughO PR refatora o pipeline de renderização e a sincronização de eventos: ChangesPipeline de Renderização e Concorrência
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over UIManager,GraphicalApplication: Fluxo de dragging e repintura
UIManager->>GraphicalApplication: setDragging(true)
GraphicalApplication->>GraphicalApplication: m_dragging = true, m_mustRepaint = true
end
rect rgba(255, 222, 173, 0.5)
Note over WorkerThread,GraphicalApplication: Renderização com spinlock
WorkerThread->>GraphicalApplication: lockQueues() [atomic_flag]
WorkerThread->>WorkerThread: swap drawMapQueue / drawMapForegroundQueue
WorkerThread->>GraphicalApplication: unlockQueues()
WorkerThread->>WorkerThread: uiRefreshInterval = m_dragging ? 4000 : 8000
WorkerThread->>DrawQueue: draw()
end
rect rgba(144, 238, 144, 0.5)
Note over ExternalThread,EventDispatcher: Two-phase event dispatch
ExternalThread->>EventDispatcher: addEventEx() / scheduleEventEx()
EventDispatcher->>EventDispatcher: push em m_incomingEvents (lock_guard)
EventDispatcher->>EventDispatcher: poll() → mergeEvents()
EventDispatcher->>EventDispatcher: swap incoming → filas principais
EventDispatcher->>EventDispatcher: execute eventos
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 introduces several performance optimizations to the rendering and event dispatching systems, including pre-computing tile draw positions, batching event merging with a standard mutex, replacing mutexes with an atomic spinlock in the main loop, and optimizing the draw queue. However, the review identifies several critical issues: TOCTOU race conditions in event scheduling, potential heap reallocations from vector swapping, and rendering hot-path allocations in MapView. Additionally, the spinlock lacks a backoff mechanism, the adaptive renderer may falsely detect lag under VSync, the UI refresh rate cap could cause stuttering on high-refresh-rate monitors, and a performance regression was introduced in the draw queue caching logic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/framework/core/eventdispatcher.cpp (1)
72-79: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueIncremento de
m_pollEventsSizeé redundante.O incremento na linha 75 nunca é observado porque
m_pollEventsSizeé sobrescrito porm_eventList.size()logo após cada chamada demergeEvents()(linhas 114 e 144 empoll()). Pode ser removido para evitar confusão.♻️ Correção sugerida
for(auto& incoming : incomingEvents) { if(incoming.pushFront) { m_eventList.push_front(incoming.event); - m_pollEventsSize++; } else { m_eventList.push_back(incoming.event); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/framework/core/eventdispatcher.cpp` around lines 72 - 79, The increment of m_pollEventsSize inside the mergeEvents() function is redundant because this variable is overwritten by m_eventList.size() immediately after each call to mergeEvents() in the poll() function. Remove the m_pollEventsSize++ statement from the conditional block where incoming.pushFront is true in the mergeEvents() function, as this increment never affects the final value and only adds unnecessary confusion to the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/mapview.cpp`:
- Around line 230-235: The brightness is being set twice in the rendering
pipeline - once in the pre-pass and again in the current code block. Remove the
duplicate call to setFieldBrightness in the section where m_lightView checks the
ground conditions (when checking ground, isGround(), and isTranslucent()). Since
the brightness is already applied in the pre-pass earlier in the function, this
duplicate setFieldBrightness call in the !GameMapDrawGroundFirst path should be
removed entirely to avoid redundant work in the hot rendering path per tile per
frame.
In `@src/framework/core/adaptiverenderer.cpp`:
- Around line 30-34: The hardcoded fallback value of 200 FPS in the maxFps
initialization block within the adaptive renderer is too restrictive for
displays with refresh rates exceeding 200Hz (such as 240Hz or higher). When
maxFps is zero or negative, replace the fixed fallback of 200 with a higher
default value that better accommodates modern high-refresh-rate displays, or
implement logic to detect the actual display refresh rate instead of using a
hardcoded constant. This will prevent the speed thresholds at the subsequent
comparison points from unnecessarily downgrading performance on high-refresh
displays.
---
Nitpick comments:
In `@src/framework/core/eventdispatcher.cpp`:
- Around line 72-79: The increment of m_pollEventsSize inside the mergeEvents()
function is redundant because this variable is overwritten by m_eventList.size()
immediately after each call to mergeEvents() in the poll() function. Remove the
m_pollEventsSize++ statement from the conditional block where incoming.pushFront
is true in the mergeEvents() function, as this increment never affects the final
value and only adds unnecessary confusion to the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75a4f6f4-d5d7-4c7f-b3fa-58b0d63f317b
📒 Files selected for processing (8)
src/client/mapview.cppsrc/framework/core/adaptiverenderer.cppsrc/framework/core/eventdispatcher.cppsrc/framework/core/eventdispatcher.hsrc/framework/core/graphicalapplication.cppsrc/framework/core/graphicalapplication.hsrc/framework/graphics/drawqueue.cppsrc/framework/ui/uimanager.cpp
Comment on lines
230
to
235
| if (m_lightView) { | ||
| ItemPtr ground = tile->getGround(); | ||
| ItemPtr ground = tiles[i]->getGround(); | ||
| if (ground && ground->isGround() && !ground->isTranslucent()) { | ||
| m_lightView->setFieldBrightness(tileDrawPos, lightFloorStart, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remover escrita de brilho duplicada no caminho !GameMapDrawGroundFirst.
O brilho já é aplicado no pré-passe (Lines 196-204). Repetir setFieldBrightness em Lines 230-235 duplica trabalho por tile/frame no hot path de renderização.
💡 Patch sugerido
- if (m_lightView) {
- ItemPtr ground = tiles[i]->getGround();
- if (ground && ground->isGround() && !ground->isTranslucent()) {
- m_lightView->setFieldBrightness(tileDrawPos, lightFloorStart, 0);
- }
- }
-
tiles[i]->drawGround(tileDrawPos, m_lightView.get());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/mapview.cpp` around lines 230 - 235, The brightness is being set
twice in the rendering pipeline - once in the pre-pass and again in the current
code block. Remove the duplicate call to setFieldBrightness in the section where
m_lightView checks the ground conditions (when checking ground, isGround(), and
isTranslucent()). Since the brightness is already applied in the pre-pass
earlier in the function, this duplicate setFieldBrightness call in the
!GameMapDrawGroundFirst path should be removed entirely to avoid redundant work
in the hot rendering path per tile per frame.
Comment on lines
30
to
+34
| int maxFps = g_app.getMaxFps(); | ||
| if (maxFps <= 0) { | ||
| maxFps = 100; | ||
| } | ||
| maxFps = std::min<int>(100, std::max<int>(1, maxFps)); | ||
| if (m_speed >= 2 && maxFps > 60) { // fix for forced vsync | ||
| maxFps = 60; | ||
| maxFps = 200; | ||
| } | ||
| maxFps = std::max<int>(1, maxFps); |
There was a problem hiding this comment.
Fallback fixo em 200 ainda induz downgrade em displays >200Hz.
Com m_maxFps <= 0, o render loop fica sem cap (frameDelay = 0 em GraphicalApplication), mas aqui o baseline vira 200. Em taxas reais maiores (ex.: 240Hz), os limiares de Lines 36/39 tendem a puxar m_speed para baixo, reintroduzindo o downgrade que o PR quer eliminar.
Sugestão de ajuste
int maxFps = g_app.getMaxFps();
if (maxFps <= 0) {
- maxFps = 200;
+ // Usa FPS observado na janela atual (5s) para evitar baseline artificial em modo uncapped/vsync.
+ const auto observedFps = static_cast<int>(std::ceil(m_frames.size() / 5.0f));
+ maxFps = std::max(1, observedFps);
}
maxFps = std::max<int>(1, maxFps);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/framework/core/adaptiverenderer.cpp` around lines 30 - 34, The hardcoded
fallback value of 200 FPS in the maxFps initialization block within the adaptive
renderer is too restrictive for displays with refresh rates exceeding 200Hz
(such as 240Hz or higher). When maxFps is zero or negative, replace the fixed
fallback of 200 with a higher default value that better accommodates modern
high-refresh-rate displays, or implement logic to detect the actual display
refresh rate instead of using a hardcoded constant. This will prevent the speed
thresholds at the subsequent comparison points from unnecessarily downgrading
performance on high-refresh displays.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…high-refresh-rate displays
Optimized the engine and game loops to allow AstraClient to run smoothly on 165Hz+ high-refresh-rate monitors, eliminating frame capping, input lagging (freeze during item dragging), and mutex contention. Summary of changes:
m_maxFpsfrom 100 to 0 (uncapped/VSync controlled).m_windowPollTimer) to poll input events every frame, significantly improving mouse/keyboard responsiveness.UIManagerto trigger high-frequency UI repaints when dragging starts.std::mutexsynchronization between the worker and rendering threads with a lightweight atomic spinlock (std::atomic_flag) to swap draw queues, reducing frame time variance.EventDispatcherto use thread-safe double-buffered incoming event and scheduled event queues guarded by a lightweightstd::mutex.poll(), only acquiring the lock briefly to swap incoming queues at the beginning of the frame.DrawQueue::draw()to eliminate redundant double-cache attempts and removed per-frameglCheckErrorcalls which were stalling the GPU pipeline.transformPositionTo2D) inMapView::drawFloorto avoid recomputing the same position across multiple floor render passes.Summary by CodeRabbit
Release Notes