feat: add average FPS to performance overlay#1406
Conversation
WalkthroughAdds conditional average FPS calculation and display to PerformanceOverlay::DrawFPS using a fully-populated frameTimeHistory; increases allowed frame-history bounds and default size in PerformanceOverlay settings. No exported/public signatures changed. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant PerformanceOverlay
participant UI
Renderer->>PerformanceOverlay: provide frame time (per frame)
PerformanceOverlay->>PerformanceOverlay: append to frameTimeHistory
PerformanceOverlay->>PerformanceOverlay: check if history full (non-zero count == size)
alt history full
PerformanceOverlay->>PerformanceOverlay: compute avgFrameTime (mean)
PerformanceOverlay->>PerformanceOverlay: avgFps = 1000.0 / avgFrameTime
end
PerformanceOverlay->>UI: draw FPS/ms (include " | Avg: <avgFps>" if computed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 1
🧹 Nitpick comments (3)
src/Features/PerformanceOverlay.cpp (3)
293-299: Keep overlay size stable: account for “Avg” text in initial width calculationYou added “ | Avg: …” to the FPS line but the initial width calculation still measures only “FPS (ms)”. This can cause a small resize on first render and occasional jitter. Include the worst‑case “Avg” text in the sizing.
Apply this diff to the sizing block:
- std::string fpsText = std::format("{:.1f} ({:.2f} ms)", this->state.smoothFps, this->state.smoothFrameTimeMs); - if (this->state.isFrameGenerationActive) { - fpsText = std::format("Raw FPS: {:.1f} ({:.2f} ms)", this->state.smoothFps, this->state.smoothFrameTimeMs); - } + // Include Avg placeholder to avoid initial width jitter + std::string fpsText = std::format("{:.1f} ({:.2f} ms) | Avg: 000.0", this->state.smoothFps, this->state.smoothFrameTimeMs); + if (this->state.isFrameGenerationActive) { + fpsText = std::format("Raw FPS: {:.1f} ({:.2f} ms) | Avg: 000.0", this->state.smoothFps, this->state.smoothFrameTimeMs); + }Also applies to: 400-401
402-407: Optional: also display average Post-FG FPS for consistencyYou already track a postFGFrameTimeHistory; showing an average here improves parity with the pre‑FG line.
Apply this diff:
ImGui::TableNextColumn(); - ImGui::Text("Post-FG FPS:"); + ImGui::Text("Post-FG FPS:"); ImGui::TableNextColumn(); - ImGui::Text("%.1f (%.2f ms)", this->state.postFGSmoothFps, this->state.postFGSmoothFrameTimeMs); + // Compute average Post-FG FPS (ignore zero entries) + { + const auto& pfData = this->state.postFGFrameTimeHistory.GetData(); + float pfSum = 0.0f; + size_t pfCount = 0; + for (float ft : pfData) { + if (ft > 0.0f) { pfSum += ft; ++pfCount; } + } + float avgPostFGMs = pfCount ? (pfSum / static_cast<float>(pfCount)) : 0.0f; + float avgPostFGFps = avgPostFGMs > 0.0f ? (1000.0f / avgPostFGMs) : 0.0f; + ImGui::Text("%.1f (%.2f ms) | Avg: %.1f", this->state.postFGSmoothFps, this->state.postFGSmoothFrameTimeMs, avgPostFGFps); + }
388-391: Nit: avoid duplicate ImGui table IDs (“FrametimeTargets”)Two tables in this function share the same ID. While it often works, ID reuse can lead to odd interactions with state (e.g., column sizing). Rename one to prevent collisions.
Apply this diff:
- if (ImGui::BeginTable("FrametimeTargets", 3, ImGuiTableFlags_SizingStretchSame)) { + if (ImGui::BeginTable("FrametimeTargetsLegend", 3, ImGuiTableFlags_SizingStretchSame)) {Also applies to: 437-448
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Features/PerformanceOverlay.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/PerformanceOverlay.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/PerformanceOverlay.cpp
⏰ 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
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Features/PerformanceOverlay.h (1)
271-271: Default history size is reasonable; align field type with bounds to prevent sign/overflow issues
- 600 fits well within the new bounds and supports a meaningful avg-FPS window.
- Align the field type with the constants to avoid implicit signed→unsigned conversions at call sites.
Apply this diff:
- int FrameHistorySize = 600; // 10s @ 60fps, 2.5s @ 240fps + size_t FrameHistorySize = 600; // 10s @ 60fps, 2.5s @ 240fpsOptional:
- If you want stronger guardrails, clamp on assignment or after deserialization:
FrameHistorySize = std::clamp(FrameHistorySize, kMinFrameHistorySize, kMaxFrameHistorySize);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Features/PerformanceOverlay.cpp(1 hunks)src/Features/PerformanceOverlay.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/PerformanceOverlay.cpp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/PerformanceOverlay.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/PerformanceOverlay.h
⏰ 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 (1)
src/Features/PerformanceOverlay.h (1)
261-263: Use size_t for history bounds and verify clampingApplying the suggested diff to switch the constants to
size_tis fine. However, I wasn’t able to locate any references toFrameHistorySizeor its clamping/resizing sites automatically. Please manually confirm that every place you set or loadFrameHistorySize(UI controls, JSON deserialization, and wherever you call.Resize(...)on your history buffers) clamps the value to the[kMinFrameHistorySize, kMaxFrameHistorySize]range.
alandtse
changed the title
|
@coderabbitai does an average make sense for a smoothed fps metric? What does the average represent? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@alandtse: Great question! The average FPS and smoothed FPS serve different purposes and provide complementary information: Smoothed FPS (the existing metric):
Average FPS (the new metric):
Why both are valuable:
For example, if you're in a demanding area:
This is similar to how network monitoring tools show both "current" and "average" metrics - they answer different questions about system performance. The average becomes especially useful for benchmarking and understanding if performance changes are temporary fluctuations or sustained improvements/degradations. 🏁 Script executed: Length of output: 109 🏁 Script executed: Length of output: 105 🏁 Script executed: Length of output: 283 🏁 Script executed: Length of output: 438 @alandtse: After examining the implementation, I can explain why both metrics are valuable and serve different purposes: The Implementation Shows Two Distinct Approaches:
Why Both Are Meaningful: The smooth FPS answers: "How is performance trending right now?"
The average FPS answers: "What is my sustained performance over time?"
Real-world Example:
This gives users both immediate feedback for gameplay decisions and stable metrics for performance tuning - they complement rather than duplicate each other. 🏁 Script executed: Length of output: 224 🏁 Script executed: Length of output: 316 🏁 Script executed: Length of output: 503 🏁 Script executed: Length of output: 524 @alandtse: After examining the implementation, I can explain why having both metrics makes perfect sense: The Two Metrics Serve Complementary Purposes:
Why Both Are Valuable:
Example Scenario:
This combination gives users both immediate situational awareness and reliable performance baselines - they complement rather than duplicate each other's purpose. 🏁 Script executed: Length of output: 175 🏁 Script executed: Length of output: 370 🏁 Script executed: Length of output: 468 🏁 Script executed: Length of output: 436 🏁 Script executed: Length of output: 444 @alandtse: Based on the PR changes described in the AI summary, I can explain why both metrics are valuable and complementary: The Two FPS Metrics Serve Different Purposes:
Why Both Make Sense: The average FPS represents true sustained performance over a meaningful time window, while smooth FPS shows current performance trajectory. This is similar to how network monitoring tools show both "current" and "average" metrics. Practical Value:
Example:
The average provides a stable reference point that's less influenced by brief stutters or temporary performance dips, making it valuable for determining if performance changes are meaningful or just momentary fluctuations. So yes, the average absolutely makes sense alongside the smoothed metric - they answer different questions about system performance. 🏁 Script executed: Length of output: 1953 @alandtse: After examining the actual implementation, I can now provide a definitive answer about why both metrics are valuable: The Implementation Shows Two Distinct Temporal Aggregation Approaches: From the code, I can see:
Why Both Metrics Are Meaningful: The smooth FPS answers: "How is performance trending right now?"
The average FPS answers: "What is my actual sustained performance?"
Practical Example:
The average is absolutely meaningful - it provides a stable reference point that complements the responsive smooth FPS. They serve different analytical purposes: immediate awareness vs. reliable measurement. 🏁 Script executed: Length of output: 362 🏁 Script executed: Length of output: 443 @alandtse: Based on the detailed information from the PR changes, I can definitively explain why both metrics are valuable and serve different purposes: The Implementation Shows Two Complementary Approaches:
Why the Average Makes Perfect Sense: The average represents sustained performance over a meaningful time window, while smooth FPS shows current performance trajectory. This is a classic pattern in performance monitoring. Different Questions, Different Answers:
Practical Value:
The average provides a stable reference point that's less influenced by temporary performance fluctuations, making it invaluable for determining if performance changes are meaningful or just momentary variations. Yes, the average absolutely makes sense - it complements rather than duplicates the smooth metric by providing stable long-term context alongside responsive short-term feedback. 🏁 Script executed: Length of output: 95040 @alandtse: After examining the complete implementation, I can provide a definitive technical answer about why both metrics are valuable: The Implementation Shows Two Distinct Temporal Sampling Approaches:
Key Technical Insight:
Why Both Make Perfect Sense: The average FPS represents sustained performance reality - what your system actually delivers over 10 seconds of gameplay. The smooth FPS shows recent performance trends with discrete updates. Practical Example:
The average provides crucial context that the sampled smooth FPS cannot - it shows whether recent performance is representative of longer-term stability or just a temporary fluctuation. Yes, the average absolutely makes sense and provides valuable information that complements the discrete-sampled "smooth" metric. |
2 participants
Summary by CodeRabbit