feat(renderdoc): add multi-frame captures#2355
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable multi-frame RenderDoc capture: UI slider, persisted frame-count setting, disk-space calculation and pre-check, multi-frame dispatch, F12/SNAPSHOT hotkey handling routed through Menu::ProcessInputEventQueue, and disables RenderDoc's internal hotkeys. ChangesRenderDoc Configurable Multi-Frame Capture
Sequence Diagram(s)sequenceDiagram
participant MenuInput as Menu::ProcessInputEventQueue
participant RenderDocHotkey as RenderDoc::HandleCaptureHotkey
participant ConfiguredCapture as RenderDoc::TriggerConfiguredCapture
participant DiskCheck as HasSufficientDiskSpaceForConfiguredCapture
participant RenderDocAPI as RenderDocAPI
MenuInput->>RenderDocHotkey: HandleCaptureHotkey(key)
RenderDocHotkey->>ConfiguredCapture: TriggerConfiguredCapture(checkDisk=true)
ConfiguredCapture->>DiskCheck: HasSufficientDiskSpaceForConfiguredCapture(&requiredBytes)
DiskCheck-->>ConfiguredCapture: sufficient / insufficient
ConfiguredCapture->>RenderDocAPI: TriggerCapture() or TriggerMultiFrameCapture(n)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.13][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/RenderDoc.cpp Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
![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 (2)
src/Menu.cpp (1)
25-25: ⚡ Quick winUse the global feature registry here instead of the singleton.
This path already lives in the menu’s global feature dispatch, so
globals::features::renderDoc.HandleCaptureHotkey(key)is a better fit than pulling inRenderDoc.hjust to callGetSingleton(). That keeps feature access consistent and removes an otherwise unnecessary direct dependency.As per coding guidelines, "Access core systems and feature registry through globals::features and globals::d3d namespaces defined in src/Globals.h".
Also applies to: 1076-1082
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Menu.cpp` at line 25, Replace direct singleton usage and include of RenderDoc.h with the global feature registry: remove the `#include` "Features/RenderDoc.h" and any calls to RenderDoc::GetSingleton() used to handle capture hotkeys; instead call globals::features::renderDoc.HandleCaptureHotkey(key) where the menu dispatch invokes the feature (also update the similar occurrences around the other instances referenced at 1076-1082). This keeps feature access consistent with the globals::features registry and removes the unnecessary direct dependency.src/Features/RenderDoc.h (1)
103-123: ⚡ Quick winKeep the new frame-count state behind the clamp.
captureFrameCountlands in the public section, so external callers can bypassSetCaptureFrameCount()and write out-of-range values directly. That defeats the invariant this PR just introduced; please move the new state/bounds below aprivate:label or expose a public accessor instead.Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/RenderDoc.h` around lines 103 - 123, The captureFrameCount field is public and can be mutated bypassing the bounds enforced by SetCaptureFrameCount(), so move captureFrameCount (and its related bounds/state) below a private: label or make it private and add a public getter/setter that enforces clamping; update any callers to use SetCaptureFrameCount() and ensure pendingCommentsMutex, enableRenderDocCapture, and captureFrameCount are placed consistently (private if they must be protected by invariants) to prevent external out-of-range writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/RenderDoc.cpp`:
- Around line 737-740: Replace the optimistic kMinCaptureSpaceBytes *
GetCaptureFrameCount() with a conservative observed-per-frame estimate: change
RenderDoc::GetRequiredCaptureSpaceBytes() to compute required bytes as
(kObservedPerFrameBytes * GetCaptureFrameCount()) plus a fixed overhead cap
(e.g., kObservedCaptureOverheadBytes), and ensure there's still a minimum floor
(retain kMinCaptureSpaceBytes if desired). Use a new constant name like
kObservedPerFrameBytes (set to the empirically observed value ~220MB per frame
or your measured value) and update any callers such as
HasSufficientDiskSpaceForConfiguredCapture() to use this new function unchanged
so capacity checks use the conservative estimate.
---
Nitpick comments:
In `@src/Features/RenderDoc.h`:
- Around line 103-123: The captureFrameCount field is public and can be mutated
bypassing the bounds enforced by SetCaptureFrameCount(), so move
captureFrameCount (and its related bounds/state) below a private: label or make
it private and add a public getter/setter that enforces clamping; update any
callers to use SetCaptureFrameCount() and ensure pendingCommentsMutex,
enableRenderDocCapture, and captureFrameCount are placed consistently (private
if they must be protected by invariants) to prevent external out-of-range
writes.
In `@src/Menu.cpp`:
- Line 25: Replace direct singleton usage and include of RenderDoc.h with the
global feature registry: remove the `#include` "Features/RenderDoc.h" and any
calls to RenderDoc::GetSingleton() used to handle capture hotkeys; instead call
globals::features::renderDoc.HandleCaptureHotkey(key) where the menu dispatch
invokes the feature (also update the similar occurrences around the other
instances referenced at 1076-1082). This keeps feature access consistent with
the globals::features registry and removes the unnecessary direct dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08182eae-2555-4f85-a008-a0a4d4416b58
📒 Files selected for processing (3)
src/Features/RenderDoc.cppsrc/Features/RenderDoc.hsrc/Menu.cpp
|
✅ A pre-release build is available for this PR: |
- Replace optimistic 100MB/frame disk space estimate with a conservative 256MB/frame observation; preserve the 100MB minimum floor so single-frame captures keep their old guard. - Route the F12/SNAPSHOT hotkey dispatch through the globals::features::renderDoc registry instead of the singleton, matching the project convention. - Bump RenderDoc feature version 1-0-1 -> 1-1-0 for the new multi-frame capture setting. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
Motivation:
When you want to troubleshoot flicker issues it is diffcult to capture the two frames with, e.g. with and without shadows for renderdoc analysis - well you could wildly press capture all the time...
The PR applies TriggerMultiFrameCapture from Renderdoc and allows to capture and save consecutive frames.
1 -> TriggerCapture()
2..120 -> TriggerMultiFrameCapture(frameCount)
Summary by CodeRabbit