refactor(perf overlay): tidy up perf overlay state update#1340
Conversation
WalkthroughThe changes refactor the performance overlay code by renaming and simplifying settings and state structures, replacing enum-based text size with a float slider, and introducing a generic circular buffer for frame time history. Timing and graph logic are unified and consolidated, with new methods for updating and drawing graph values, and redundant or duplicated code removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/PerformanceOverlay.cpp(23 hunks)src/Features/PerformanceOverlay.h(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
src/Features/PerformanceOverlay.cpp (1)
Learnt from: davo0411
PR: #1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the smoothDrawCalls array in the State class is declared as type double in src/State.h, which is the correct floating-point type for performing exponential moving average smoothing calculations.
src/Features/PerformanceOverlay.h (1)
Learnt from: davo0411
PR: #1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the smoothDrawCalls array in the State class is declared as type double in src/State.h, which is the correct floating-point type for performing exponential moving average smoothing calculations.
🔇 Additional comments (6)
src/Features/PerformanceOverlay.h (3)
136-154: Well-documented method with clear responsibilitiesThe documentation for
UpdateGraphValues()is comprehensive and clearly explains the method's purpose and steps. This improves maintainability.
240-279: Good refactoring of constants to Settings structMoving constants to a separate Settings struct improves code organization and makes configuration values more discoverable. The static constexpr approach is efficient and the values are well-documented.
97-104: Verify circular buffer resize usage and behavior
I didn’t find any calls toResize(size_t)in the repository, so it’s unclear whether this method ever runs in practice. Please manually confirm:
- How and where the buffer is resized at runtime
- Whether resetting
headIdxto 0 could disrupt ongoing frame-time trackingIf you do rely on dynamic resizing, consider preserving the buffer’s relative position instead of unconditionally zeroing
headIdx, or document why a hard reset is acceptable here.src/Features/PerformanceOverlay.cpp (3)
1910-1919: Min/max tracking implementation is correct but has O(n) complexity in edge casesThe implementation correctly handles the case where the overwritten value was the min/max, requiring a full rescan. While this is O(n) complexity, it's acceptable given the small buffer sizes (60-480 frames).
505-541: Good separation of post-FG graph drawing logicMoving the post-FG frame time graph drawing to a standalone method improves code organization and readability. The implementation is consistent with the pre-FG graph drawing pattern.
1925-1943: Statistical calculations for adaptive graph scalingThe implementation calculates mean and standard deviation to dynamically adjust the graph range, which provides a better visualization experience. The performance impact is minimal given the small buffer sizes.
| // WHAT DOES oldFrameTime EVEN MEAN??????? | ||
| // Insert latest frame time into circular buffer | ||
| float oldFrameTime = frameTimeHistory[frameTimeHistoryIndex]; | ||
| frameTimeHistory[frameTimeHistoryIndex] = frameTimeMs; | ||
| frameTimeHistoryIndex = ((frameTimeHistoryIndex + 1) % overlaySettings.FrameHistorySize); | ||
| float oldFrameTime = state.frameTimeHistory.GetData()[state.frameTimeHistory.GetHeadIdx()]; | ||
| state.frameTimeHistory.Push(state.frameTimeMs); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or clarify confusing comment
The comment "WHAT DOES oldFrameTime EVEN MEAN???????" is unprofessional and indicates unclear code. The variable represents the frame time value that will be overwritten in the circular buffer.
- // WHAT DOES oldFrameTime EVEN MEAN???????
- // Insert latest frame time into circular buffer
+ // Get the frame time that will be overwritten (used for min/max tracking)
float oldFrameTime = state.frameTimeHistory.GetData()[state.frameTimeHistory.GetHeadIdx()];📝 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.
| // WHAT DOES oldFrameTime EVEN MEAN??????? | |
| // Insert latest frame time into circular buffer | |
| float oldFrameTime = frameTimeHistory[frameTimeHistoryIndex]; | |
| frameTimeHistory[frameTimeHistoryIndex] = frameTimeMs; | |
| frameTimeHistoryIndex = ((frameTimeHistoryIndex + 1) % overlaySettings.FrameHistorySize); | |
| float oldFrameTime = state.frameTimeHistory.GetData()[state.frameTimeHistory.GetHeadIdx()]; | |
| state.frameTimeHistory.Push(state.frameTimeMs); | |
| // Get the frame time that will be overwritten (used for min/max tracking) | |
| float oldFrameTime = state.frameTimeHistory.GetData()[state.frameTimeHistory.GetHeadIdx()]; | |
| state.frameTimeHistory.Push(state.frameTimeMs); |
🤖 Prompt for AI Agents
In src/Features/PerformanceOverlay.cpp around lines 1905 to 1908, replace the
unprofessional and unclear comment "WHAT DOES oldFrameTime EVEN MEAN???????"
with a clear explanation that oldFrameTime holds the frame time value currently
at the head of the circular buffer, which will be overwritten by the new frame
time being pushed. This clarifies the purpose of the variable and improves code
readability.
| // Update smooth values with user-specified interval | ||
| state.updateTimer += deltaTime; | ||
| if (state.updateTimer >= settings.UpdateInterval) { | ||
| state.smoothFps = state.fps; // Sampling white noise won't give you smoothed noise. This is useless. | ||
| state.smoothFrameTimeMs = state.frameTimeMs; | ||
| state.updateTimer = 0.0f; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper exponential smoothing
The current implementation just samples values at intervals rather than applying continuous smoothing. The comment correctly identifies this as ineffective.
Consider implementing exponential moving average (EMA) smoothing:
- // Update smooth values with user-specified interval
- state.updateTimer += deltaTime;
- if (state.updateTimer >= settings.UpdateInterval) {
- state.smoothFps = state.fps; // Sampling white noise won't give you smoothed noise. This is useless.
- state.smoothFrameTimeMs = state.frameTimeMs;
- state.updateTimer = 0.0f;
- }
+ // Apply exponential smoothing continuously
+ float alpha = 1.0f - std::exp(-deltaTime / settings.UpdateInterval);
+ state.smoothFps = state.smoothFps + alpha * (state.fps - state.smoothFps);
+ state.smoothFrameTimeMs = state.smoothFrameTimeMs + alpha * (state.frameTimeMs - state.smoothFrameTimeMs);🤖 Prompt for AI Agents
In src/Features/PerformanceOverlay.cpp around lines 1974 to 1980, the current
code resets smoothFps and smoothFrameTimeMs directly to the latest fps and
frameTimeMs values at intervals, which does not provide true smoothing. Replace
this with an exponential moving average calculation by updating smoothFps and
smoothFrameTimeMs continuously each frame using a smoothing factor (alpha), for
example: smoothValue = alpha * currentValue + (1 - alpha) * previousSmoothValue.
This will provide proper continuous smoothing instead of discrete sampling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Features/PerformanceOverlay.cpp (2)
1870-1980: Comprehensive timing logic consolidation with minor issueThe UpdateGraphValues method effectively consolidates all frame timing calculations, smoothing, and state updates into a single location. The circular buffer usage is correct, and the min/max tracking with std::ranges algorithms is an improvement.
However, there's a confusing comment on line 1906 that should be clarified.
The unprofessional comment "what is the point of oldFrameTime?" should be replaced with a clear explanation that oldFrameTime represents the frame time value that will be overwritten in the circular buffer, which is used for efficient min/max tracking.
1973-1979: Smoothing implementation needs improvementThe current smoothing logic still uses discrete sampling rather than continuous exponential smoothing, as noted in the comment. This doesn't provide true smoothing benefits.
Consider implementing proper exponential moving average (EMA) smoothing that updates continuously each frame rather than sampling at intervals. This would provide genuine smoothing of the displayed values.
🧹 Nitpick comments (2)
src/Features/PerformanceOverlay.cpp (2)
200-202: Verify Settings constant referenceEnsure that the Settings constant reference is correct - it should be
Settings::kMaxUpdateIntervalinstead ofPerformanceOverlay::Settings::kMaxUpdateIntervalfor consistency with other usages in the file.- ImGui::SliderFloat("Update Interval", &this->settings.UpdateInterval, 0.001f, PerformanceOverlay::Settings::kMaxUpdateInterval, "%.2f seconds"); + ImGui::SliderFloat("Update Interval", &this->settings.UpdateInterval, 0.001f, Settings::kMaxUpdateInterval, "%.2f seconds");
932-932: Consistent Settings constant referencesAll references to Settings constants should use consistent naming. Some use the full
PerformanceOverlay::Settings::prefix while others may not need it within the class scope.Consider using just
Settings::for consistency within the class implementation, as done in most other locations in the file.Also applies to: 1452-1452, 1461-1461, 1494-1494
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/PerformanceOverlay.cpp(23 hunks)src/Features/PerformanceOverlay.h(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
src/Features/PerformanceOverlay.h (1)
Learnt from: davo0411
PR: #1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the smoothDrawCalls array in the State class is declared as type double in src/State.h, which is the correct floating-point type for performing exponential moving average smoothing calculations.
src/Features/PerformanceOverlay.cpp (1)
Learnt from: davo0411
PR: #1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the smoothDrawCalls array in the State class is declared as type double in src/State.h, which is the correct floating-point type for performing exponential moving average smoothing calculations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (5)
src/Features/PerformanceOverlay.h (3)
82-115: LGTM: Well-designed CircularBuffer implementationThe CircularBuffer template class is well-implemented with proper bounds checking, resize handling, and const-correct accessor methods. The design effectively replaces manual vector indexing with a clean abstraction.
198-235: Good: Simplified state structureThe State struct is properly simplified by removing methods and converting it to a pure data structure. The use of CircularBuffer for frame time histories is a clean improvement over the previous implementation.
240-279: Please manually verify frame history size limits
I wasn’t able to locate any usages ofkMinFrameHistorySize,kMaxFrameHistorySize, orFrameHistorySize(for example in any ImGui slider calls) in the codebase. Please double-check that whereverFrameHistorySizeis exposed—UI sliders, config loading, etc.—it’s correctly clamped to[kMinFrameHistorySize, kMaxFrameHistorySize]and that the default of 120 frames matches prior behavior.src/Features/PerformanceOverlay.cpp (2)
103-116: LGTM: JSON serialization updated correctlyThe JSON serialization has been properly updated to use the new Settings struct name and includes the float TextSize field.
197-197: Good: TextSize converted to continuous sliderThe change from discrete enum-based text size to a continuous float slider provides better user control and aligns with the header struct changes.
|
✅ A pre-release build is available for this PR: |
2 participants
UpdateGraphValuesmethodCircularBufferfor abstractionSummary by CodeRabbit
Refactor
New Features
Bug Fixes