feat(perfoverlay): add frame time and a/b testing stats#1250
Conversation
WalkthroughThis change introduces a new modular Performance Overlay feature with comprehensive real-time performance monitoring, A/B testing capabilities, and an extensible overlay system. It removes the legacy performance overlay code, refactors feature management for overlays, adds per-shader-type frame time tracking, and introduces new utility functions for formatting, statistics, and UI rendering. The WeatherPicker feature is also integrated as an overlay. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant OverlayFeature
participant PerformanceOverlay
participant ABTestingManager
participant ABTestAggregator
participant State
User->>Menu: Press overlay toggle key / open menu
Menu->>OverlayFeature: Iterate loaded features, call DrawOverlay()
OverlayFeature-->>PerformanceOverlay: (if PerformanceOverlay enabled)
PerformanceOverlay->>State: Query per-shader metrics (draw calls, frame times)
PerformanceOverlay->>ABTestingManager: Query A/B test state
ABTestingManager->>ABTestAggregator: On variant switch, OnFrame (collect data)
PerformanceOverlay->>PerformanceOverlay: Render overlay UI (metrics, graphs, A/B results)
Menu->>WeatherPicker: DrawOverlay() if enabled
sequenceDiagram
participant User
participant ABTestingManager
participant PerformanceOverlay
participant ABTestAggregator
User->>ABTestingManager: Enable A/B testing, set interval
ABTestingManager->>PerformanceOverlay: Save/restore test config
ABTestingManager->>ABTestAggregator: OnABSwitch(Variant)
loop Every frame
PerformanceOverlay->>ABTestAggregator: OnFrame(draw call data)
ABTestingManager->>PerformanceOverlay: Update()
end
User->>ABTestingManager: Disable A/B testing
ABTestingManager->>ABTestAggregator: OnTestEnd()
PerformanceOverlay->>ABTestAggregator: GetAggregatedResults()
PerformanceOverlay->>PerformanceOverlay: Render A/B test results table
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 9
🧹 Nitpick comments (9)
src/Utils/UI.cpp (1)
608-616: Consider using else-if for clarity.The threshold logic is correct, but the code could be more readable with explicit else-if blocks.
ImVec4 GetThresholdColor(float value, float good, float warn, ImVec4 goodColor, ImVec4 warnColor, ImVec4 badColor) { - if (value < good) - return goodColor; - else if (value < warn) - return warnColor; - else - return badColor; + if (value < good) { + return goodColor; + } else if (value < warn) { + return warnColor; + } else { + return badColor; + } }src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp (1)
85-95: Consider bounds checking for the slider.While the ImGui slider should handle bounds internally, it's good practice to validate the interval value.
Add validation after the slider:
if (ImGui::SliderInt("A/B Test Interval", reinterpret_cast<int*>(&testInterval), 0, 10)) { + testInterval = std::clamp(testInterval, 0u, 10u); bool overlayWasEnabled = performanceOverlay->settings.ShowInOverlay;src/Menu.h (1)
178-179: Consider makingoverlayVisibleprivate with getter/setter methods.The public
overlayVisiblemember breaks encapsulation. Consider making it private and providing getter/setter methods to maintain consistency with the class design pattern used for other members likesettings.- bool overlayVisible = false; - // Static utility functions static const char* KeyIdToString(uint32_t key); private: + bool overlayVisible = false; Settings settings;Add public methods in the public section:
+ bool IsOverlayVisible() const { return overlayVisible; } + void SetOverlayVisible(bool visible) { overlayVisible = visible; }src/Utils/Format.cpp (1)
120-131: Consider simplifying the percentage calculation logic.The percentage calculation logic is correct but could be more readable. The current implementation calculates absolute percentage change differently based on which value is smaller, which is mathematically sound but could be clearer.
std::string FormatDeltaWithPercent(float a, float b, float threshold) { float delta = b - a; float percentDelta = 0.0f; - if (a < b && a > 0.0f) { - percentDelta = 100.0f * (b - a) / a; - } else if (b < a && b > 0.0f) { - percentDelta = 100.0f * (a - b) / b; - } - std::string percentStr = (percentDelta >= threshold) ? std::format(" ({:+.1f}%)", (b < a ? -percentDelta : percentDelta)) : ""; + if (a > 0.0f) { + percentDelta = 100.0f * delta / a; + } + std::string percentStr = (std::abs(percentDelta) >= threshold) ? std::format(" ({:+.1f}%)", percentDelta) : ""; return (delta > 0.0f ? "+" : "") + FormatMilliseconds(delta) + percentStr; }src/Menu.cpp (1)
1724-1729: Remove redundant style variable handling.The
stylevariable is captured by reference but then immediately overwritten witholdStyle, making the reference capture redundant.Apply this diff to simplify:
- ImGuiStyle& style = ImGui::GetStyle(); - style = oldStyle; + ImGui::GetStyle() = oldStyle; ImGui::Render(); ImGui_ImplDX11_RenderDrawData(ImGui::GetDrawData());src/Features/PerformanceOverlay.cpp (3)
61-91: Consider simplifying the complex template metaprogramming in MakeMetricColumn.The nested lambda with template metaprogramming using
if constexprandstd::is_same_vmakes this code hard to maintain. Consider splitting this into two separate helper functions - one for optional values and one for regular values.-auto MakeMetricColumn(const auto& theme, auto valueGetter, auto colorGetter, auto formatter, const Util::ColoredTextLines& legend, const Util::ColoredTextLines* cellLegend = nullptr) -{ - return [theme, valueGetter, colorGetter, formatter, legend, cellLegend](const DrawCallRow& row, int) { - using ValueType = decltype(valueGetter(row)); - if constexpr (std::is_same_v<ValueType, std::optional<float>>) { - if (!valueGetter(row).has_value()) { - ImGui::TextDisabled("-"); - return; - } - float value = *valueGetter(row); - ImVec4 color = colorGetter(theme, value, row); - std::string valueStr = formatter(value, row); - ImGui::PushStyleColor(ImGuiCol_Text, color); - ImGui::Text("%s", valueStr.c_str()); - ImGui::PopStyleColor(); - } else { - float value = valueGetter(row); - ImVec4 color = colorGetter(theme, value, row); - std::string valueStr = formatter(value, row); - ImGui::PushStyleColor(ImGuiCol_Text, color); - ImGui::Text("%s", valueStr.c_str()); - ImGui::PopStyleColor(); - } - if (ImGui::IsItemHovered()) { - if (auto _tt = Util::HoverTooltipWrapper()) { - const Util::ColoredTextLines& useLegend = cellLegend ? *cellLegend : legend; - Util::DrawColoredMultiLineTooltip(useLegend); - } - } - }; -} +// Helper for required values +auto MakeRequiredMetricColumn(const auto& theme, auto valueGetter, auto colorGetter, auto formatter, const Util::ColoredTextLines& legend, const Util::ColoredTextLines* cellLegend = nullptr) +{ + return [theme, valueGetter, colorGetter, formatter, legend, cellLegend](const DrawCallRow& row, int) { + float value = valueGetter(row); + ImVec4 color = colorGetter(theme, value, row); + std::string valueStr = formatter(value, row); + ImGui::PushStyleColor(ImGuiCol_Text, color); + ImGui::Text("%s", valueStr.c_str()); + ImGui::PopStyleColor(); + if (ImGui::IsItemHovered()) { + if (auto _tt = Util::HoverTooltipWrapper()) { + const Util::ColoredTextLines& useLegend = cellLegend ? *cellLegend : legend; + Util::DrawColoredMultiLineTooltip(useLegend); + } + } + }; +} + +// Helper for optional values +auto MakeOptionalMetricColumn(const auto& theme, auto valueGetter, auto colorGetter, auto formatter, const Util::ColoredTextLines& legend, const Util::ColoredTextLines* cellLegend = nullptr) +{ + return [theme, valueGetter, colorGetter, formatter, legend, cellLegend](const DrawCallRow& row, int) { + if (!valueGetter(row).has_value()) { + ImGui::TextDisabled("-"); + return; + } + float value = *valueGetter(row); + ImVec4 color = colorGetter(theme, value, row); + std::string valueStr = formatter(value, row); + ImGui::PushStyleColor(ImGuiCol_Text, color); + ImGui::Text("%s", valueStr.c_str()); + ImGui::PopStyleColor(); + if (ImGui::IsItemHovered()) { + if (auto _tt = Util::HoverTooltipWrapper()) { + const Util::ColoredTextLines& useLegend = cellLegend ? *cellLegend : legend; + Util::DrawColoredMultiLineTooltip(useLegend); + } + } + }; +}
1680-1843: Consider separating A/B test state management from UI rendering.This function handles both A/B test state management and UI rendering, which violates the single responsibility principle.
Consider splitting into:
void PerformanceOverlay::UpdateABTestState(bool showCollapsibleSections) { // Handle state tracking and aggregator updates (lines 1682-1719) } void PerformanceOverlay::RenderABTestUI(bool showCollapsibleSections) { // Handle UI rendering (lines 1721-1842) } void PerformanceOverlay::DrawABTestSection(const std::vector<DrawCallRow>& allRows, bool showCollapsibleSections) { UpdateABTestState(showCollapsibleSections); RenderABTestUI(showCollapsibleSections); }
209-211: Consider extracting magic numbers as named constants.Buffer sizes and format strings contain magic numbers that could be named constants for better maintainability.
+ static constexpr size_t kTooltipBufferSize = 128; + static constexpr const char* kFrameTimeFormat = "%.2f ms (%.1f FPS)"; + - char validStr[128], marginalStr[128]; + char validStr[kTooltipBufferSize], marginalStr[kTooltipBufferSize]; - snprintf(overlay_text, IM_ARRAYSIZE(overlay_text), - "%s%.2f ms (%.1f FPS)", + snprintf(overlay_text, IM_ARRAYSIZE(overlay_text), + "%s" kFrameTimeFormat,Also applies to: 1119-1119
src/Features/PerformanceOverlay.h (1)
16-20: Document the rationale for using negative enum values.Using negative values to distinguish special shader types from regular ones is clever but could be fragile if the regular shader type enum changes. Consider adding a comment explaining this design choice.
// Special shader type enum for summary rows +// Uses negative values to distinguish from regular shader types (which are non-negative) enum class SpecialShaderType { Total = -1, Other = -2 };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
features/Performance Overlay/Shaders/Features/PerformanceOverlay.ini(1 hunks)src/Feature.cpp(2 hunks)src/Features/OverlayFeature.h(1 hunks)src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/PerformanceOverlay.h(1 hunks)src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.cpp(1 hunks)src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.h(1 hunks)src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp(1 hunks)src/Features/PerformanceOverlay/ABTesting/ABTesting.h(1 hunks)src/Features/WeatherPicker.cpp(6 hunks)src/Features/WeatherPicker.h(3 hunks)src/Globals.cpp(3 hunks)src/Globals.h(2 hunks)src/Menu.cpp(12 hunks)src/Menu.h(1 hunks)src/State.cpp(10 hunks)src/State.h(4 hunks)src/Utils/FileSystem.cpp(2 hunks)src/Utils/FileSystem.h(2 hunks)src/Utils/Format.cpp(2 hunks)src/Utils/Format.h(1 hunks)src/Utils/PerfUtils.h(1 hunks)src/Utils/UI.cpp(3 hunks)src/Utils/UI.h(4 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 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.
features/Performance Overlay/Shaders/Features/PerformanceOverlay.ini (1)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
src/Globals.h (1)
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.
src/Feature.cpp (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
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.
src/Globals.cpp (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
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.
src/Features/WeatherPicker.h (1)
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.
src/State.cpp (5)
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
src/Features/WeatherPicker.cpp (1)
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.
src/State.h (4)
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.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
src/Features/PerformanceOverlay.cpp (2)
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.
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.
src/Menu.cpp (4)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
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#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
🧬 Code Graph Analysis (3)
src/Utils/FileSystem.cpp (1)
src/Utils/FileSystem.h (1)
LoadJsonDiff(111-111)
src/Features/WeatherPicker.cpp (4)
src/Menu.cpp (4)
KeyIdToString(2153-2178)KeyIdToString(2153-2153)DrawOverlay(1615-1729)DrawOverlay(1615-1615)src/Utils/UI.h (2)
HoverTooltipWrapper(58-58)HoverTooltipWrapper(59-59)src/Utils/UI.cpp (2)
HoverTooltipWrapper(24-31)HoverTooltipWrapper(33-39)src/Features/WeatherPicker.h (1)
open(29-29)
src/Utils/Format.cpp (1)
src/Utils/Format.h (1)
FormatMilliseconds(30-30)
⏰ 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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (50)
features/Performance Overlay/Shaders/Features/PerformanceOverlay.ini (1)
1-2: LGTM! Configuration follows established patterns.The INI file correctly follows the standard format for feature configuration files with proper version marking.
src/Feature.cpp (2)
17-17: LGTM! Header include follows established pattern.The include is properly added in alphabetical order with other feature headers.
210-210: LGTM! Feature integration follows established pattern.The PerformanceOverlay is correctly added to the feature list, enabling it to be managed alongside other features.
src/Globals.h (2)
27-27: LGTM! Forward declaration follows established pattern.The forward declaration is properly added in alphabetical order with other feature declarations.
82-82: LGTM! Global pointer declaration follows established pattern.The extern pointer declaration correctly follows the same pattern as other features in the globals::features namespace.
src/Globals.cpp (3)
26-26: LGTM! Header include follows established pattern.The include is properly added in alphabetical order with other feature headers.
81-81: LGTM! Pointer initialization follows established pattern.The pointer is correctly initialized to nullptr following the same pattern as other features.
167-167: LGTM! Singleton assignment follows established pattern.The singleton assignment in OnInit() correctly follows the same pattern as other features, ensuring proper initialization.
src/Utils/FileSystem.cpp (2)
2-2: LGTM! Required include for file I/O operations.The fstream include is necessary for the ifstream usage in the new LoadJsonDiff function.
83-128: LGTM! Well-implemented JSON diff functionality with robust error handling.The LoadJsonDiff function provides excellent support for A/B testing with:
- Proper file existence and stream validation
- Correct handling of all diff operation types (replace, add, remove)
- Safe JSON pointer access within exception handling
- Graceful error handling that logs issues without propagating exceptions
- Appropriate return of empty vector on any failure
The implementation correctly uses nlohmann::json::diff and handles the results appropriately for the A/B testing use case.
src/Features/OverlayFeature.h (1)
12-36: LGTM! Well-designed abstract interface.The
OverlayFeatureclass provides a clean, well-documented interface for modular overlay features. The use of pure virtual methods, comprehensive documentation, and sensible defaults demonstrates good design principles.src/Utils/FileSystem.h (3)
9-9: LGTM! Appropriate header inclusion for JSON functionality.The addition of the nlohmann/json header is correct for the new JSON diff functionality.
14-19: LGTM! Clean structure design for representing JSON differences.The
SettingsDiffEntrystruct provides a clear and simple way to represent differences between JSON configurations with path and both values stored as strings.
109-112: LGTM! Well-organized namespace and function signature.The
FileSystemnested namespace andLoadJsonDifffunction declaration provide a clean API for JSON comparison functionality.src/Features/WeatherPicker.h (4)
3-4: LGTM! Correct header updates for overlay integration.The updated includes properly reflect the migration to the new overlay system architecture.
6-6: LGTM! Proper inheritance change for overlay functionality.Changing inheritance from
FeaturetoOverlayFeaturecorrectly integrates WeatherPicker into the new modular overlay system.
40-46: LGTM! Well-structured settings for overlay configuration.The
WeatherDetailsWindowSettingsstruct provides clean configuration options with sensible defaults for overlay positioning and visibility.
121-123: LGTM! Proper implementation of OverlayFeature interface.The addition of
DrawOverlay()andIsOverlayVisible()methods correctly implements the required interface for the new overlay system.src/Features/PerformanceOverlay/ABTesting/ABTesting.h (1)
8-37: LGTM! Well-designed A/B testing manager with clean interface.The
ABTestingManagerclass demonstrates good design principles:
- Appropriate use of singleton pattern for a system manager
- Clear separation of concerns with distinct method groups (configuration, state, UI)
- Proper composition with
ABTestAggregator- Good use of
std::chronofor timing precision- Clean, well-documented interface
src/Utils/UI.cpp (1)
556-561: LGTM!The implementation correctly iterates through the colored text lines and renders each with its specified color using ImGui.
src/Features/WeatherPicker.cpp (2)
7-12: LGTM!Proper use of nlohmann JSON serialization macro for the WeatherDetailsWindowSettings struct.
860-871: LGTM!The DrawOverlay implementation correctly manages the overlay visibility and window state, properly integrating with the new overlay system.
src/Utils/Format.h (1)
26-84: Well-documented utility function declarations!The new formatting and calculation utilities are properly documented with clear descriptions of their purpose, parameters, and return values. These will provide good support for the performance overlay feature.
src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp (2)
9-13: LGTM!Standard singleton pattern implementation.
30-33: Good state preservation pattern!The consistent preservation of the overlay enabled state across configuration switches ensures a good user experience during A/B testing.
Also applies to: 47-49, 73-73, 94-94
src/State.cpp (3)
21-21: Good addition of thread safety!Using Lock() ensures thread-safe access to the state during the Draw operation.
83-91: Well-implemented frame time tracking!The frame time tracking properly measures elapsed time per shader type and applies the same exponential smoothing as draw calls. The timing logic correctly handles frame boundaries.
Also applies to: 102-113
859-862: LGTM!The utility method provides clean access to the total smoothed draw calls using magic_enum for type safety.
src/Menu.h (1)
171-173: LGTM! Good encapsulation with getter methods.The addition of getter methods for Theme, Settings, and DXGIAdapter3 follows good encapsulation practices and provides controlled access to internal state.
src/State.h (2)
22-30: Excellent thread safety implementation.The constructor properly initializes all arrays under mutex protection, and the
Lock()method correctly returns astd::lock_guardfor RAII-based locking. This ensures thread-safe access to the statistics data.
243-283: Well-designed template utility methods for shader type iteration.The three template methods provide a clean and type-safe way to iterate over shader types with different levels of detail. The use of
magic_enumfor enum handling and[[maybe_unused]]attributes for unused parameters demonstrates good C++ practices.src/Utils/Format.cpp (1)
80-106: Well-implemented formatting functions with proper edge case handling.The formatting functions correctly handle edge cases (values < 1e-4f) and use appropriate precision for different value ranges. The consistent pattern between
FormatMillisecondsandFormatMicrosecondsis good for maintainability.src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.cpp (1)
108-188: Well-implemented aggregation logic with proper summary row handling.The aggregation correctly calculates statistics per shader type and ensures that Total (-1) and Other (-2) summary rows are always present and properly sorted at the end of the results. The sorting logic correctly handles the special cases.
src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.h (1)
7-18: Well-defined constants for statistical validation.The constants for frame history, outlier detection, and statistical validation provide clear thresholds for test validity. The values chosen (30-frame baseline, 3x outlier multiplier, 100ms max frame time) are reasonable defaults for performance testing.
src/Utils/UI.h (6)
33-42: LGTM!The
ColoredTextLinestruct is well-designed for representing colored text in tooltips and legends.
228-236: Good documentation improvement!The added warning about tooltip context requirement is valuable for preventing misuse.
238-244: LGTM!The function signature and documentation are appropriate for the colored tooltip functionality.
371-412: Well-designed adapter implementation!The overload correctly adapts the simpler interface to the generic template version with proper bounds checking in the cell renderer adapter.
428-430: LGTM!The threshold color helper is appropriately placed and will be useful for performance metric visualization.
319-332: Add null check for cellRender callback.The
cellRendercallback is called without checking if it's null, which could cause a crash if not provided.Apply this diff to add null checking:
for (size_t rowIdx = 0; rowIdx < rows.size(); ++rowIdx) { const auto& row = rows[rowIdx]; ImGui::TableNextRow(); for (size_t col = 0; col < headers.size(); ++col) { ImGui::TableSetColumnIndex(static_cast<int>(col)); if (cellRender) { cellRender(static_cast<int>(rowIdx), static_cast<int>(col), row); - } else if constexpr (std::is_same_v<T, std::vector<std::string>>) { + } else { + // Default rendering for vector<string> if no custom renderer provided + if constexpr (std::is_same_v<T, std::vector<std::string>>) { if (col < row.size()) ImGui::TextUnformatted(row[col].c_str()); + } } } }Likely an incorrect or invalid review comment.
src/Menu.cpp (6)
26-29: LGTM!The new includes are properly organized and necessary for the performance overlay and A/B testing features.
129-139: Verify the hover alpha reduction from 20% to 10%.The change from hardcoded
0.2ftohoveredAlpha(0.1f) reduces hover visibility by half. This makes hover effects more subtle but might impact usability.Please confirm this alpha reduction is intentional and has been tested for adequate visibility across different display configurations.
575-576: Good consistency improvement!Using
globals::upscalingaligns with the pattern of accessing features through global references.
1214-1228: LGTM!The overlay toggle key UI implementation is consistent with other keybinding settings and properly integrated.
1469-1478: LGTM!The shader type iteration with index is properly implemented and maintains backward compatibility.
2188-2196: LGTM!The weather picker integration properly uses the singleton pattern consistent with the new overlay architecture.
src/Features/PerformanceOverlay.cpp (2)
1364-1418: Well-implemented statistical analysis for graph scaling.The graph update logic correctly calculates mean and standard deviation, applies appropriate bounds, and uses exponential smoothing for visual stability. The documentation is excellent.
1335-1335: Confirm the VideoMemoryInfo.Budget guard is correctPlease verify whether the
videoMemoryInfo.Budget > 0check is the right approach to handle all GPU configurations (integrated GPUs, virtualized environments, unsupported drivers, etc.) or if it should be adjusted to allow a zero‐budget state rather than falling back to “Not available.”• Location to review:
– src/Features/PerformanceOverlay.cpp around line 1335 (if (SUCCEEDED(hr) && videoMemoryInfo.Budget > 0) { … })
• Actions:
- Test on hardware/drivers that may return a zero budget (e.g., integrated GPUs, VMs).
- Consult the DXGI_QUERY_VIDEO_MEMORY_INFO documentation to confirm under what conditions
Budgetcan validly be zero.- Decide whether to:
– Keep the guard (to avoid division by zero and invalid metrics),
– PermitBudget == 0and display “0 GB” usage, or
– Add a more explicit fallback path.src/Features/PerformanceOverlay.h (2)
193-217: Well-designed settings structure with appropriate defaults.The
PerfOverlaySettingsstructure is well-organized with sensible defaults, proper use of constexpr for bounds checking, and clear naming.
101-108: Excellent documentation throughout the header file.The method documentation is comprehensive, explaining purpose, parameters, return values, and side effects clearly. This greatly improves code maintainability.
Also applies to: 161-178, 224-249
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the performance overlay into a standalone OverlayFeature, adds detailed per-shader frame time tracking, and introduces an A/B testing framework for automated shader performance comparisons.
- Moved legacy performance overlay/UI code into
PerformanceOverlayandOverlayFeaturesubmodules. - Added real-time frame timing, per-shader metrics, and smoothing logic in
StateandPerfUtils. - Implemented A/B testing manager and aggregator with UI controls and JSON diff support.
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Utils/UI.h | Added colored tooltip types, new sortable table template, and removed old PerformanceOverlay helper. |
| src/Utils/UI.cpp | Implemented colored tooltip and threshold color helpers. |
| src/Utils/PerfUtils.h | New inline frame‐time, FPS, mean, and median helper functions. |
| src/Utils/Format.h | Declared new formatting functions for time, percent, and deltas. |
| src/Utils/Format.cpp | Defined formatting implementations (ms, us, percent, time‐ago, delta). |
| src/Utils/FileSystem.h | Added JSON diff loader declaration and SettingsDiffEntry struct. |
| src/Utils/FileSystem.cpp | Implemented LoadJsonDiff using nlohmann::json diff. |
| src/State.h / src/State.cpp | Added per‐shader timing arrays, smoothing, and thread safety. |
| src/Menu.h / src/Menu.cpp | Converted overlay and weather picker to use new OverlayFeature interface; removed monolithic overlay code. |
| src/Globals.h / src/Globals.cpp | Registered PerformanceOverlay in globals list. |
| src/Features/PerformanceOverlay/** | New feature directory: core overlay, A/B testing manager, aggregator and related UI. |
| src/Features/WeatherPicker.{h,cpp} | Converted to OverlayFeature, wired ShowInOverlay settings. |
Comments suppressed due to low confidence (5)
src/Utils/FileSystem.h:14
- [nitpick] The
SettingsDiffEntrystruct is declared in the global namespace but used inUtil::FileSystem. Consider moving it intonamespace Util::FileSystemfor clearer scoping.
struct SettingsDiffEntry
src/Utils/Format.h:43
- The header uses std::chrono::steady_clock::time_point but doesn't include . Add
#include <chrono>to Format.h.
std::string TimeAgoString(std::chrono::steady_clock::time_point last);
src/Utils/Format.cpp:129
- Uses
std::formatwithout including the header. Add#include <format>or switch tofmt::formatto avoid missing symbol errors.
std::string percentStr = (percentDelta >= threshold) ? std::format(" ({:+.1f}%)", (b < a ? -percentDelta : percentDelta)) : "";
src/Utils/FileSystem.cpp:124
- The file references
logger::warnbut does not include the header that defineslogger. Add the appropriate logging include (e.g., "Globals.h" or your logger header).
logger::warn("Failed to load JSON diff: {}", e.what());
src/State.h:30
- Returning a
std::lock_guardby value is ill-formed becauselock_guardis neither copyable nor movable. Consider returning astd::unique_lock<std::mutex>or locking inside the caller scope instead.
std::lock_guard<std::mutex> Lock() { return std::lock_guard<std::mutex>(statsMutex); }
davo0411
left a comment
davo0411
left a comment
There was a problem hiding this comment.
Excellent refactor, from a brief look through everything looked good. Nice comments, very helpful
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/PerformanceOverlay.h (1)
123-123: Improve const-correctness for BuildDrawCallLegendsThe method is declared as
constbut takes a non-constPerformanceOverlay*parameter. If the overlay parameter doesn't need modification, it should beconst PerformanceOverlay*.- DrawCallLegends BuildDrawCallLegends(const Menu::ThemeSettings& theme, bool anyTestData, PerformanceOverlay* overlay) const; + DrawCallLegends BuildDrawCallLegends(const Menu::ThemeSettings& theme, bool anyTestData, const PerformanceOverlay* overlay) const;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/PerformanceOverlay.h(1 hunks)src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.cpp(1 hunks)src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.h(1 hunks)src/Menu.cpp(12 hunks)src/Utils/PerfUtils.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Utils/PerfUtils.h
- src/Features/PerformanceOverlay/ABTesting/ABTestAggregator.cpp
- src/Menu.cpp
🧰 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.
src/Features/PerformanceOverlay.cpp (1)
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: 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.
⏰ 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). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
src/Features/PerformanceOverlay.cpp (3)
511-550: Well-implemented VRAM monitoring with proper error handlingThe function correctly checks for null pointers, handles DXGI query failures gracefully, and provides appropriate fallback UI.
1951-2004: Excellent statistical implementation for graph visualizationThe function properly calculates mean, standard deviation, and applies exponential smoothing for stable graph scaling. The dynamic range adjustment based on variance provides an optimal viewing experience.
82-86: Consider thread safety for singleton accessThe singleton pattern implementation doesn't include thread synchronization. If the overlay can be accessed from multiple threads (e.g., main thread and render thread), consider adding thread-safe initialization.
static PerformanceOverlay* GetSingleton() { static std::once_flag initFlag; static PerformanceOverlay* singleton = nullptr; std::call_once(initFlag, []() { singleton = new PerformanceOverlay(); }); return singleton; }src/Features/PerformanceOverlay.h (1)
314-338: Well-designed settings structure with proper constraintsThe
PerfOverlaySettingsstructure properly uses constexpr for bounds checking and includes all necessary configuration options for the overlay. The enum for text size provides type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Utils/UI.h (1)
371-434: Fix const_cast usage that violates const-correctness.The function accepts
constrows but usesconst_castto remove const qualification when sorting is not needed. This breaks the const contract and could lead to undefined behavior.Replace the const_cast approach with a cleaner solution:
-inline void ShowSortedStringTable( +inline void ShowSortedStringTable( const char* table_id, const std::vector<std::string>& headers, - const std::vector<std::vector<std::string>>& rows, + std::vector<std::vector<std::string>> rows, size_t sortColumn = 0, bool ascending = true, const std::vector<TableSortFunc>& customSorts = {}, TableCellRenderFunc cellRender = nullptr) { - // Check if sorting is needed by looking at ImGui table sort specs - bool needsSorting = false; - if (const ImGuiTableSortSpecs* sortSpecs = ImGui::TableGetSortSpecs()) { - needsSorting = (sortSpecs->SpecsCount > 0); - } - - // Only make a copy if we need to sort, otherwise use the original data - if (needsSorting) { - std::vector<std::vector<std::string>> rowsCopy = rows; - ShowSortedStringTable<std::vector<std::string>>( - table_id, - headers, - rowsCopy, - sortColumn, - ascending, - adaptedSorts, - adaptedCellRender); - } else { - // For non-sorting case, we can use the original data directly - // We need to const_cast because the template expects non-const, but we know it won't be modified - ShowSortedStringTable<std::vector<std::string>>( - table_id, - headers, - const_cast<std::vector<std::vector<std::string>>&>(rows), - sortColumn, - ascending, - adaptedSorts, - adaptedCellRender); - } + ShowSortedStringTable<std::vector<std::string>>( + table_id, + headers, + rows, + sortColumn, + ascending, + adaptedSorts, + adaptedCellRender); }This change accepts the vector by value (utilizing move semantics when possible) and eliminates the dangerous const_cast.
src/Features/PerformanceOverlay.cpp (1)
282-283: Fix void return value passed to SetTextScale.The code attempts to pass the void return value of
SetTextScale()to itself, which shouldn't compile:float scale = this->perfOverlayState.CalculateTextScale(); -this->perfOverlayState.SetTextScale(this->perfOverlayState.SetTextScale()); +this->perfOverlayState.SetTextScale(scale);
🧹 Nitpick comments (4)
src/Utils/UI.h (1)
284-354: Consider moving complex template implementation to reduce compilation overhead.The 70+ line template implementation increases compilation time for every file that includes this header.
Consider moving this to a separate implementation header:
// In UI.h: template <typename T> void ShowSortedStringTable( const char* table_id, const std::vector<std::string>& headers, std::vector<T>& rows, size_t sortColumn, bool ascending, const std::vector<std::function<bool(const T&, const T&, bool)>>& customSorts, std::function<void(int, int, const T&)> cellRender, const std::vector<T>& footerRows = {}); // In UI_impl.h or UI.inl: #include "UI.h" template <typename T> void ShowSortedStringTable(...) { // Full implementation here } // Include the implementation header at the bottom of UI.h: #include "UI_impl.h"This keeps the main header cleaner while maintaining template functionality.
src/Features/PerformanceOverlay.h (3)
17-21: Consider using positive enum values for clarity.The
SpecialShaderTypeenum uses negative values which can be confusing and error-prone when used in contexts expecting positive indices or when interfacing with systems that expect unsigned types.Consider using positive enum values or a separate enum class:
enum class SpecialShaderType { - Total = -1, - Other = -2 + Total = 1000, // Use a high value to avoid conflicts with real shader types + Other = 1001 };
196-215: Document the derivation of performance threshold constants.The performance threshold constants are well-organized, but some would benefit from documentation explaining how these specific values were determined, especially for thresholds that might seem arbitrary.
Consider adding comments explaining the rationale:
// Performance threshold constants static constexpr float kSmoothingFactor = 0.15f; // Smoothing factor: 0.1f = slow, 0.3f = fast. static constexpr float kFrameTimeGoodThreshold = 2.0f; // ms - Good performance threshold static constexpr float kFrameTimeWarningThreshold = 5.0f; // ms - Warning performance threshold static constexpr float kCostPerCallGoodThreshold = 0.05f; // ms/call - Good cost per call threshold static constexpr float kCostPerCallWarningThreshold = 0.2f; // ms/call - Warning cost per call threshold +static constexpr float kMicrosecondThreshold = 0.01f; // ms - Below this threshold, display in microseconds for precision +static constexpr float kPercentDisplayThreshold = 0.01f; // % - Minimum meaningful difference for user display -static constexpr float kMicrosecondThreshold = 0.01f; // ms - Threshold for showing microseconds -static constexpr float kPercentDisplayThreshold = 0.01f; // Minimum percent difference to display
216-278: Review the extensive getter/setter pattern for potential over-exposure.The class provides both const getters and non-const setters for almost all private members. This extensive accessor pattern suggests that the internal state might be too tightly coupled with external code, potentially undermining the benefits of encapsulation.
Consider:
- Providing higher-level methods that encapsulate multiple state changes
- Using builder pattern for complex state updates
- Reducing the number of individual setters by grouping related operations
For example:
// Instead of individual setters for all frame time metrics void UpdateFrameMetrics(float frameTimeMs, float fps, bool isPostFG = false); // Instead of exposing raw history vectors void AddFrameTimeToHistory(float frameTime, bool isPostFG = false);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/PerformanceOverlay.h(1 hunks)src/State.cpp(10 hunks)src/Utils/UI.h(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/State.cpp
🧰 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.
src/Features/PerformanceOverlay.cpp (1)
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: 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.
🧬 Code Graph Analysis (1)
src/Features/PerformanceOverlay.cpp (8)
src/Features/PerformanceOverlay.h (22)
theme(116-116)theme(118-118)theme(119-119)theme(126-126)theme(127-127)value(256-256)value(256-256)value(257-257)value(257-257)value(258-258)value(258-258)value(259-259)value(259-259)value(260-260)value(260-260)value(261-261)value(261-261)value(262-262)value(262-262)value(263-263)value(263-263)results(117-117)src/Utils/UI.cpp (8)
HoverTooltipWrapper(24-31)HoverTooltipWrapper(33-39)DrawColoredMultiLineTooltip(556-561)DrawColoredMultiLineTooltip(556-556)DrawSectionHeader(415-476)DrawSectionHeader(415-415)GetThresholdColor(608-616)GetThresholdColor(608-608)src/Utils/Format.cpp (16)
CalculateOtherFrameTime(143-146)CalculateOtherFrameTime(143-143)CalculatePercentage(133-136)CalculatePercentage(133-133)CalculateCostPerCall(138-141)CalculateCostPerCall(138-138)FormatMilliseconds(80-90)FormatMilliseconds(80-80)FormatDeltaWithPercent(120-131)FormatDeltaWithPercent(120-120)FormatPercent(101-106)FormatPercent(101-101)FormatMicroseconds(92-99)FormatMicroseconds(92-92)TimeAgoString(108-118)TimeAgoString(108-108)src/Menu.h (2)
menu(13-17)dxgiAdapter3(173-173)src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp (2)
GetSingleton(9-13)GetSingleton(9-9)src/Utils/PerfUtils.h (4)
CalcFrameTime(15-18)CalcFrameTime(15-15)CalcFPS(25-30)CalcFPS(25-25)src/Features/PerformanceOverlay/ABTesting/ABTesting.h (3)
aggregator(29-29)interval(14-14)abTestingEnabled(16-16)src/Utils/FileSystem.cpp (2)
LoadJsonDiff(83-128)LoadJsonDiff(83-83)
⏰ 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). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Features/PerformanceOverlay.h (2)
286-304: Excellent documentation for complex algorithm.The detailed documentation for
UpdateGraphValues()is exemplary. It clearly explains the multi-step algorithm for statistical analysis and graph range calculation, making the complex logic accessible to future maintainers.
83-104: Clean implementation of the overlay feature interface.The class properly inherits from
OverlayFeatureand implements the required virtual methods. The singleton pattern is correctly implemented, and the method signatures are clear and consistent.
overlayand moved both weatherpicker and perfoverlay to use thisSummary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores