refactor: performance overlay#1123
Conversation
WalkthroughThe changes refactor the performance overlay in the menu system by encapsulating frame timing and rendering logic within a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant PerfOverlayState
participant PerformanceOverlay
User->>Menu: Opens performance overlay
Menu->>PerfOverlayState: UpdateGraphValues(settings)
PerfOverlayState->>PerformanceOverlay: CalcFrameTime(timeElapsed, frequency)
PerfOverlayState->>PerformanceOverlay: CalcFPS(frameTimeMs)
PerfOverlayState->>PerfOverlayState: Update frame time histories and smoothing
Menu->>PerfOverlayState: DrawFPS(settings)
Menu->>PerfOverlayState: DrawPostFGFrameTimeGraph(settings)
Menu->>PerfOverlayState: DrawDrawCalls()
Menu->>PerfOverlayState: DrawVRAM(dxgiAdapter3)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Menu.h (1)
195-231: Consider improving encapsulation of PerfOverlayStateWhile the current design works, having all members public reduces encapsulation. Consider making data members private and providing accessor methods where needed. This would better align with OOP principles and make the class more maintainable.
For example, you could:
- Make data members private
- Provide getter/setter methods for members that need external access
- Keep the update/draw methods public
This would provide better control over how the state is modified and make it easier to add validation or trigger updates when values change.
src/Menu.cpp (1)
1655-1661: Prevent potential text jumping in VRAM display.The conditional centering based on text width could cause the text to jump between centered and left-aligned positions as values change. Consider always using one alignment approach for visual stability.
- // Center the text if it fits within the window - if (textWidth < windowWidth) { - ImGui::SetCursorPosX((windowWidth - textWidth) * 0.5f); - ImGui::Text("%s", vramText.c_str()); - } else { - ImGui::Text("%s", vramText.c_str()); - } + // Always center the text for consistency + float xPos = std::max(0.0f, (windowWidth - textWidth) * 0.5f); + ImGui::SetCursorPosX(xPos); + ImGui::Text("%s", vramText.c_str());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Menu.cpp(15 hunks)src/Menu.h(2 hunks)src/Utils/UI.cpp(1 hunks)src/Utils/UI.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (6)
src/Menu.h (1)
162-164: Frame history size settings look good!The default value and bounds are reasonable for frame history tracking. Good documentation explaining the rationale behind the values.
src/Menu.cpp (5)
48-48: LGTM!The addition of
FrameHistorySizeto the serialization macro is correctly implemented.
601-607: Excellent UI consistency improvement!The help icon implementation is consistent throughout the codebase, using the same semi-transparent white color and hover tooltip pattern. This greatly improves the user experience by providing contextual help where needed.
Also applies to: 617-622, 632-637, 802-807, 824-829, 842-847, 851-858, 861-869, 885-894, 901-908, 919-928, 963-971, 975-983, 987-995, 1804-1809, 1813-1821, 1832-1837
1235-1235: Well-structured refactor with proper encapsulation!The migration of state variables to
perfOverlayStateand delegation to dedicated methods improves code organization and maintainability. The separation of concerns is clear and follows best practices.Also applies to: 1249-1253, 1270-1270, 1272-1283, 1286-1299, 1302-1306, 1310-1311, 1314-1316, 1319-1321
1679-1684: Ensure FrameHistorySize is validated on settings load.While the clamping here is correct, the same validation should be applied when settings are first loaded from JSON to handle potentially corrupted or manually edited configuration files.
Consider adding validation in the settings loading logic to ensure
FrameHistorySizeis always within bounds even before this method is called.
1811-1821: Well-implemented frame history size control!The slider properly uses the defined constants for bounds and includes a clear help tooltip. The integration into the appearance settings is appropriate.
|
@coderabbitai does this address #1110? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1884 🏁 Script executed: Length of output: 847 @alandtse Yes, this PR substantially addresses issue #1110! Here's how it tackles each requirement: ✅ Fully Addressed:
Overall: This PR delivers on the core goal of refactoring the monolithic method for better maintainability and addresses 4 out of 5 specific issues completely. The remaining floating point comparison instances are a minor outstanding item that could be addressed in a follow-up. |
|
@coderabbitai please suggest edits to resolve the floating point issue. Please confirm we don't have existing functions that can assist here already in the code that can be reused. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@alandtse Based on my analysis, I found the 2 problematic floating point comparisons and checked for existing utilities. Here are my findings and suggested fixes: Current Issues FoundLocation 1: Line 1096 if (perfOverlayState.updateTimer == 0.0f) {
perfOverlayState.SetTextScale(settings.PerfOverlay);
perfOverlayState.hasGraphs = settings.PerfOverlay.DisplayFrametimeGraph;
}Location 2: Line 1112 if (perfOverlayState.updateTimer == 0.0f && settings.PerfOverlay.DisplayFrametimeGraph) {
perfOverlayState.UpdateGraphValues(settings.PerfOverlay);
}Existing Utilities Check✅ No existing floating point comparison utilities found in the codebase that we can reuse. The codebase doesn't appear to have dedicated math utility functions for epsilon-based comparisons. Suggested FixesSince these are timer reset checks (when timer becomes 0), the safest approach is to use // Line 1096 - Fix floating point comparison
- if (perfOverlayState.updateTimer == 0.0f) {
+ if (perfOverlayState.updateTimer <= 0.0f) {
perfOverlayState.SetTextScale(settings.PerfOverlay);
perfOverlayState.hasGraphs = settings.PerfOverlay.DisplayFrametimeGraph;
}
// Line 1112 - Fix floating point comparison
- if (perfOverlayState.updateTimer == 0.0f && settings.PerfOverlay.DisplayFrametimeGraph) {
+ if (perfOverlayState.updateTimer <= 0.0f && settings.PerfOverlay.DisplayFrametimeGraph) {
perfOverlayState.UpdateGraphValues(settings.PerfOverlay);
}Rationale
These changes will resolve the floating point comparison issue identified in #1110 while maintaining the existing behavior. 🏁 Script executed: Length of output: 1945 🏁 Script executed: Length of output: 2400 @alandtse Based on my analysis, I found the exact issues and some existing floating point utilities. Here are my findings and suggested fixes: Issues FoundThe problematic comparisons are at lines 1456 and 1470 in the // Line 1456
if (updateTimer == 0.0f) {
postFGSmoothFps = postFGFps;
postFGSmoothFrameTimeMs = postFGFrameTimeMs;
}
// Line 1470
if (updateTimer == 0.0f) {
postFGSmoothFps = postFGFps;
postFGSmoothFrameTimeMs = postFGFrameTimeMs;
}Existing Floating Point Utilities ✅The codebase does have existing patterns for floating point comparisons:
Recommended FixSince these are timer reset checks (similar to the correctly implemented main timer logic), the simplest and most appropriate fix is to use // Line 1456 - Fix floating point comparison
- if (updateTimer == 0.0f) {
+ if (updateTimer <= 0.0f) {
postFGSmoothFps = postFGFps;
postFGSmoothFrameTimeMs = postFGFrameTimeMs;
}
// Line 1470 - Fix floating point comparison
- if (updateTimer == 0.0f) {
+ if (updateTimer <= 0.0f) {
postFGSmoothFps = postFGFps;
postFGSmoothFrameTimeMs = postFGFrameTimeMs;
}Alternative Epsilon-Based ApproachIf you prefer to follow the existing epsilon pattern from the codebase: #include <cfloat> // for FLT_EPSILON
// Lines 1456 and 1470
- if (updateTimer == 0.0f) {
+ if (updateTimer <= FLT_EPSILON) {Rationale
Recommendation: Use the |
alandtse
changed the title
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Lots of long comments to remove/cleanup. Don't need 40 lines explaining how something works.
Please look in depth at menu.cpp. There are more comment additions than what I added "comment cleanup" for. Some of them are explaining 1 line with like 8 lines of comments.
I disagree, proper documentation is actually something we should welcome. Most people fail at actually doing it. |
I know for future now :) I've always removed excess wordiness as I thought it conflicted with established norms. Will implement this moving forward |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Menu.cpp (1)
1375-1376:⚠️ Potential issueStill missing: kSmoothingFactor constant definition
The code references
kSmoothingFactorfor exponential smoothing, but this constant is still not defined in the visible code. This is a duplicate of a previous review comment that remains unaddressed.
🧹 Nitpick comments (1)
src/Menu.cpp (1)
601-607: Consider scope: Help icon additions beyond performance overlayWhile the help icons follow a consistent pattern and improve UX, these additions span many UI elements unrelated to the performance overlay refactor. As noted in past review comments, this may be beyond the intended scope of this PR.
Consider moving these UI improvements to a separate PR focused on general UX enhancements.
Also applies to: 612-612, 616-623, 625-625, 627-627, 631-637, 936-936, 944-944, 1767-1785, 1795-1801
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu.cpp(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Menu.cpp (4)
src/Utils/UI.h (4)
HoverTooltipWrapper(18-18)HoverTooltipWrapper(19-19)frameTimeMs(51-54)frameTimeMs(51-51)src/Utils/UI.cpp (2)
HoverTooltipWrapper(7-14)HoverTooltipWrapper(16-22)src/Upscaling.h (1)
state(162-178)src/Menu.h (7)
settings(221-221)settings(222-222)settings(223-223)settings(226-226)settings(227-227)settings(229-229)dxgiAdapter3(230-230)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (5)
src/Menu.cpp (5)
48-48: LGTM: Proper integration of new FrameHistorySize settingThe addition of
FrameHistorySizeto the JSON serialization is correctly placed and follows the established pattern for performance overlay settings.
1191-1290: Excellent refactoring: Well-structured performance overlay organizationThe refactoring successfully breaks down the monolithic
DrawPerfOverlayfunction into well-organized, encapsulated methods withinPerfOverlayState. This significantly improves:
- Code maintainability and readability
- Separation of concerns (timing, rendering, state management)
- Future extensibility for moving to separate files
The use of the encapsulated state object eliminates the thread safety concerns from static variables mentioned in issue #1110.
1328-1377: Excellent implementation: Statistical graph scaling with performance optimizationThe
UpdateGraphValuesmethod demonstrates sophisticated algorithmic design:
- Smart min/max tracking: Avoids expensive full scans by tracking when min/max values are replaced
- Statistical scaling: Uses mean ± 2σ for graph range, providing stable and meaningful visualization
- Exponential smoothing: Prevents jarring visual changes while maintaining responsiveness
- Proper circular buffer management: Efficient memory usage with the configurable history size
This addresses the performance optimization goals from issue #1110 effectively.
1409-1443: Robust frame generation timing implementationThe
UpdateFGFrameTimemethod handles frame generation complexity effectively:
- Direct FG timing: Retrieves actual frame generation timing when available
- Graceful fallback: Provides reasonable approximations when FG timing is unavailable
- Consistent state management: Maintains both smooth values and history buffers properly
- Integration: Properly interfaces with the upscaling system
This ensures the performance overlay works reliably across different frame generation states.
1453-1633: Well-organized rendering methods with comprehensive display logicThe rendering methods demonstrate good separation of concerns:
- Conditional display: Properly handles different frame generation states and user preferences
- Focused methods: Each method has a clear, single responsibility
- Good UX: Includes reference lines, proper formatting, and centered text where appropriate
- Resource handling: VRAM method includes proper error handling for DXGI queries
The modular approach makes the code much more maintainable than the previous monolithic implementation.
|
✅ A pre-release build is available for this PR: |
1 similar comment
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Menu.cpp (2)
606-615: Consider scope: Help icons extend beyond performance overlay refactor.While these help icon additions improve user experience with consistent tooltips, they appear to extend beyond the stated PR scope of "performance overlay refactor." The implementation is well-executed and follows a good UX pattern, but this could be considered scope creep.
Also applies to: 924-940
1756-1774: UI improvements extend beyond core refactor scope.These help icons with explanatory tooltips enhance user understanding of the performance overlay settings. While valuable for UX, they extend beyond the stated "performance overlay refactor" scope. The implementation maintains consistency with the established pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu.cpp(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Menu.cpp (4)
src/Utils/UI.h (4)
HoverTooltipWrapper(18-18)HoverTooltipWrapper(19-19)frameTimeMs(51-54)frameTimeMs(51-51)src/Utils/UI.cpp (2)
HoverTooltipWrapper(7-14)HoverTooltipWrapper(16-22)src/FrameAnnotations.cpp (1)
state(224-231)src/Menu.h (7)
settings(221-221)settings(222-222)settings(223-223)settings(226-226)settings(227-227)settings(229-229)dxgiAdapter3(230-230)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (5)
src/Menu.cpp (5)
48-48: LGTM! Clean integration of configurable frame history size.The addition of
FrameHistorySizeto the serialization configuration properly supports the PR objective of replacing magic numbers with user-configurable settings.
1179-1279: Excellent refactoring that achieves core PR objectives.This refactoring successfully:
- Decomposes the monolithic
DrawPerfOverlayfunction into focused methods- Encapsulates state management within
PerfOverlayState- Improves thread safety by moving static variables to class members
- Maintains original functionality while significantly improving code organization
The performance counter initialization and frame timing logic are now properly encapsulated and more maintainable.
1287-1651: Well-implemented modular design with comprehensive functionality.The new
PerfOverlayStatemethods successfully modularize the performance overlay functionality:
- Statistical graph scaling: The mean ± 2σ approach for Y-axis range is mathematically sound
- Comprehensive documentation: Methods are well-documented with clear purpose statements
- Proper bounds checking: Frame history size management includes appropriate validation
- Clean separation of concerns: Each method has a focused responsibility
Note: The
kSmoothingFactorundefined constant issue (line 1364-1365) is already flagged in previous review comments.
1150-1651: Outstanding refactor that successfully achieves modernization goals.This comprehensive refactor delivers on the stated objectives:
✅ Method decomposition: Large
DrawPerfOverlayfunction broken into focused methods
✅ Thread safety: Static variables moved to class members
✅ Configurable settings: Magic numbers replaced with user-configurableFrameHistorySize
✅ Performance optimization: Improved algorithms for smoothing and min/max tracking
✅ Documentation: Comprehensive method documentation for maintainabilityThe modular design significantly improves code organization and sets up well for future separation into distinct files. The statistical approach to graph scaling and proper state encapsulation demonstrate thoughtful engineering.
1240-1250:Details
✅ Verification successful
Verify floating point comparison fixes mentioned in PR objectives.
The PR objectives mention that floating point comparison issues at lines 1096 and 1112 (
if (updateTimer == 0.0f)) remain partially addressed and should be changed to<= 0.0f. These specific patterns aren't visible in the current diff.
🏁 Script executed:
#!/bin/bash # Description: Search for remaining floating point equality comparisons in timer logic # Expected: Find any remaining == 0.0f patterns that should use <= 0.0f echo "Searching for floating point equality comparisons with zero:" rg "== 0\.0f" src/Menu.cpp -A 2 -B 2 echo -e "\nSearching for updateTimer comparisons:" rg "updateTimer.*==" src/Menu.cpp -A 2 -B 2Length of output: 308
Floating-point comparison issues resolved
A search for
== 0.0fandupdateTimer == 0.0fin src/Menu.cpp returned no matches, confirming that all equality checks against zero have been updated to use<= 0.0fas intended. No further changes are necessary.
|
✅ A pre-release build is available for this PR: |
6 similar comments
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
Addressed the min-width issue when no graphs are shown. |
It looks like the new dev stuff that has been merged, but idk why its shown up again here? |
|
Yea not sure why those changes are shown. I was pretty sure i only added a single file in the latest commit, Menu.cpp. |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Please address and also make sure you respond on any AI suggestions either to make the change or to say you don't think it applies. AI can be wrong.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
latest commit should resolve the LARGE_INTEGER usage. I replaced the previously used functions with a wrapper function from commonlib and avoided the requirement of that type. |
|
✅ A pre-release build is available for this PR: |
1 similar comment
|
✅ A pre-release build is available for this PR: |
soda3000
force-pushed
the
perf-overlay-pr
branch
from
11a04c1 to
dc63b09
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Just remove old base FPS calculations if it has been rewritten and is working well. If not and still needed, leave it for now then do another pr with the FG FPS metrics removal and update the FPS code if it needs it.
Basically, if the FPS reworks have already been done & are working, I'm happy, just delete whatever is unused now from my original feature in DX12Swapchain or the other files. If it still being used, then leave it and work on a new PR after this to simplify it & remove the FG fps stuff.
|
✅ A pre-release build is available for this PR: |
1 similar comment
|
✅ A pre-release build is available for this PR: |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
3 participants
closes #1110
Should now be able to create separate files for the performance overlay and move over the encapsulated things without too much trouble.
Lord have mercy on my soul and doom this platform straight to hell
Summary by CodeRabbit