fix(screenshot): wait for end-of-frame before composited capture#1132
Conversation
ScreenshotUtility.CaptureComposited called ScreenCapture.CaptureScreenshotAsTexture inline, before the next frame had been rendered and presented. UI Toolkit overlays are composited at end-of-frame, so the captured texture contained an unwritten backbuffer and the saved PNG was blank. Route the play-mode editor path through a transient MonoBehaviour that yields WaitForEndOfFrame before calling ScreenCapture, then advance the player loop with EditorApplication.Step() until the coroutine completes. The single-call API is preserved; total cost is bounded (5 steps, ~80ms at 60fps). Complements CoplayDev#1040, which switched to the correct API but kept the synchronous invocation that caused the empty capture.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an editor-only after-frame capture helper that schedules CaptureScreenshotAsTexture at end-of-frame via a transient MonoBehaviour, and updates CaptureComposited and ManageUI to use it in Editor Play Mode so UI Toolkit overlays are included in screenshots. ChangesEditor composited-frame screenshot capture
Sequence Diagram(s)sequenceDiagram
participant ManageUI
participant CaptureComposited
participant CaptureCompositedAfterFrame
participant ScreenshotCapturer
participant WaitForEndOfFrame
participant ScreenCapture
participant EditorApplication
ManageUI->>CaptureComposited: request composited screenshot (Play Mode)
CaptureComposited->>CaptureCompositedAfterFrame: call with superSize
CaptureCompositedAfterFrame->>ScreenshotCapturer: Begin(superSize, onComplete)
ScreenshotCapturer->>WaitForEndOfFrame: coroutine yields end of frame
WaitForEndOfFrame->>ScreenCapture: CaptureScreenshotAsTexture(superSize)
ScreenCapture->>ScreenshotCapturer: return Texture2D or fail
loop editor spin
EditorApplication->>EditorApplication: Step() until onComplete invoked
end
CaptureCompositedAfterFrame->>CaptureComposited: return Texture2D
CaptureComposited->>ManageUI: return/compress image and set pending fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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 |
![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 (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
261-266: 💤 Low valueSet
s_pendingCompositedDone = trueon exception to avoid unnecessary spinning.When an exception occurs, the coroutine is effectively complete (the
GameObjectis destroyed at line 268), buts_pendingCompositedDone = falsecausesCaptureCompositedAfterFrameto continue spinningEditorApplication.Step()until timeout. Thenulltexture already signals failure.Suggested fix
catch (Exception ex) { Debug.LogError($"[MCP for Unity] CaptureScreenshotAsTexture failed: {ex.Message}"); s_pendingCompositedTex = null; - s_pendingCompositedDone = false; + s_pendingCompositedDone = true; }🤖 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 `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs` around lines 261 - 266, The catch block in CaptureScreenshotAsTexture leaves s_pendingCompositedDone = false which causes CaptureCompositedAfterFrame to keep spinning; update the catch to set s_pendingCompositedDone = true (while keeping s_pendingCompositedCompositedTex = null and logging) so the coroutine loop in CaptureCompositedAfterFrame will exit promptly when an exception occurs.
🤖 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 `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 274-281: Reset the pending-composite state unconditionally at the
start of the capture routine to avoid returning a stale texture: set
s_pendingCompositedTex = null, s_pendingCompositedDone = false, and
s_pendingCompositedStarted = false immediately on entry to the ScreenshotUtility
capture method (the method that contains the existing s_pendingCompositedStarted
check), rather than only when s_pendingCompositedStarted is true, so any
background coroutine completion after a timeout cannot cause a stale
s_pendingCompositedTex to be returned.
---
Nitpick comments:
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 261-266: The catch block in CaptureScreenshotAsTexture leaves
s_pendingCompositedDone = false which causes CaptureCompositedAfterFrame to keep
spinning; update the catch to set s_pendingCompositedDone = true (while keeping
s_pendingCompositedCompositedTex = null and logging) so the coroutine loop in
CaptureCompositedAfterFrame will exit promptly when an exception occurs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfeedb41-7689-49a0-93fb-c14e8e48959a
📒 Files selected for processing (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
…ale state Two robustness tweaks to CompositedFrameCapturer / CaptureCompositedAfterFrame flagged by CodeRabbit on the parent commit: 1. On exception inside the WaitForEndOfFrame coroutine, mark s_pendingCompositedDone = true so CaptureCompositedAfterFrame's spin loop exits immediately. The null texture already signals failure to the caller, which falls through to the camera-based fallback. Previously the loop would spin for the full timeout despite the coroutine being effectively done. 2. Reset s_pendingCompositedTex / s_pendingCompositedDone unconditionally on entry. A coroutine from a previous call that timed out can complete asynchronously and leave a stale texture / done flag behind; clearing on entry prevents the next call from picking up that stale capture.
Removing the if (s_pendingCompositedStarted) guard in the previous commit
left the field with only assignments and no reads, which Unity surfaces as
CS0414 ("assigned but its value is never used"). Drop the field and its
three write sites.
No behavior change.
|
Hi thanks for the PR! I think its a little over-engineered and this is actually duplication of ManageUI.cs at line 819, so it would be great if you reuse the wheel or call into that wheel. Trim the comments also, keep it under 1-2 lines. |
…nd CaptureComposited Per review feedback: ManageUI.MCP_ScreenCapturer and the new CompositedFrameCapturer added in this PR were near-duplicates. Extract a single public ScreenshotCapturer MonoBehaviour to Runtime/Helpers that takes a callback, and have both call sites use it: - ManageUI.render_ui (two-call pending/ready protocol): callback writes the texture and flips its own static done/started flags. - ScreenshotUtility.CaptureCompositedAfterFrame (single-call sync spin): callback writes a local; spin loop pumps EditorApplication.Step until set. Trimmed verbose comments in the same pass. No behaviour change. Both paths re-verified end-to-end with a magenta UITK panel-clear repro.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 242-251: The CaptureCompositedAfterFrame method can return before
the ScreenshotCapturer.Begin callback runs, leaking the Texture2D the callback
creates; update the callback to detect whether the capture was still wanted and
destroy late textures. Concretely, introduce a local bool flag (e.g., accepted
or captured) captured by the callback and set it to true only when you assign
result and before returning; inside the Begin callback, if the flag shows the
caller already timed out, immediately dispose the incoming Texture2D (using
UnityEngine.Object.DestroyImmediate or Destroy appropriately) instead of
assigning it; keep references to ScreenshotCapturer.Begin,
CaptureCompositedAfterFrame, timeoutSteps and the EditorApplication.Step loop
when making this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: acfb4308-4880-4000-bae0-a0db70b78f52
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
Drop the "Shared by ManageUI.render_ui ... and ScreenshotUtility's editor synchronous-spin path" line. The first sentence already describes the class on its own; naming specific callers in the doc is brittle (goes stale if a call site is added or renamed) and pushed the summary to 4 content lines, over the 1-2 line guidance.
…callback If the editor spin loop times out before ScreenshotCapturer's coroutine fires the callback, the callback would still later assign the captured Texture2D to the (now-dead) local result variable. The texture itself would never reach the consumer's finally-block DestroyTexture, leaking a Unity object until the next domain reload. Track caller-returned state; if the callback runs after timeout, destroy the incoming texture immediately instead of assigning it. Flagged by CodeRabbit on the prior commit.
|
Done in Follow-up nit from CodeRabbit on the refactor (late-callback texture leak after timeout) addressed in |
…nch to ternary The 4-line inline comment in CaptureComposited duplicated CaptureCompositedAfterFrame's own header and exceeded the 1-2 line guidance. Trim to 2 lines and collapse the play-mode / edit-mode branch to a ternary (matches the file's existing style).
|
Hi, can you keep it updated to the beta so we could run the CI so it would not fail on other Unity versions |
…enshot-wait-for-end-of-frame
|
Merged latest Update ( |
…enshot-wait-for-end-of-frame # Conflicts: # MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
2 participants
Description
manage_camera action=screenshot capture_source=game_view include_image=true(no explicit camera, play mode) returned fully-transparent PNGs (every pixel RGBA(0, 0, 0, 0)). The path was supposed to capture UI Toolkit overlays — #1040 wired it toScreenCapture.CaptureScreenshotAsTexturefor that reason — but called the API inline, before the next frame was rendered, so the captured backbuffer was unwritten and the result was always blank.This change routes the call through a
WaitForEndOfFramecoroutine and advances the player loop withEditorApplication.Step()so the captured texture actually contains the composited frame. Single-call API is preserved.Type of Change
Changes Made
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs:CompositedFrameCapturerMonoBehaviour(nested,#if UNITY_EDITOR): yieldsWaitForEndOfFrame, then callsScreenCapture.CaptureScreenshotAsTexture(SuperSize)and stores the result in static state.CaptureCompositedAfterFrame(int superSize, int timeoutSteps = 5): spawns the capturer GameObject, advances the player loop withEditorApplication.Step()up totimeoutStepstimes until the coroutine completes, returns the texture (ornull, which falls through to existing camera fallback).CaptureComposited's editor + play-mode path throughCaptureCompositedAfterFrame. Edit-mode editor path and non-editor builds retain the previous direct synchronous call.The public API (
ScreenshotUtility.CaptureCompositedsignature,manage_camera screenshottool surface) is unchanged.Testing/Screenshots/Recordings
Tested locally against
betaHEAD (a2a5edf) with Unity 6000.3.14f1, URP, Windows 11.Repro:
UIDocumentusing aPanelSettingsasset.PanelSettings.clearColor = true,colorClearValue = magenta(unambiguous signal for overlay capture).manage_camera action=screenshot capture_source=game_view include_image=true(nocamera).manage_camera screenshot(no camera,include_image=true, play mode)manage_camera screenshot camera="Main Camera"(explicit camera)manage_ui render_ui(uses its own coroutine path)No automated test added. The fix's correctness depends on play-mode end-of-frame behavior with a live
UIDocument— out of scope for the existing edit-mode test fixtures inTestProjects/UnityMCPTests/Assets/Tests/EditMode/. Happy to add one if reviewers can point me at the right play-mode test harness pattern in this repo.Documentation Updates
This change fixes the behavior of
manage_camera action=screenshot's composited path but does not change its parameters, response schema, or add/remove any tool or resource. No documentation updates needed.Related Issues
Fixes #1039 (the underlying behavior described in the issue persisted after #1040 — the API choice in #1040 was correct, but the synchronous invocation produced empty captures).
Additional Notes
ScreenCapture.CaptureScreenshotAsTextureinstead of camera render), and that line is preserved — just moved inside the coroutine.ManageUI.csalready contains an equivalentMCP_ScreenCapturerMonoBehaviour used byrender_ui's two-call pending/ready protocol. A future refactor could shareCompositedFrameCapturerbetween both call sites and offer a single-call shape forrender_uitoo. Left out to keep this PR focused.CaptureCompositedAfterFrameresets stale in-flight state on entry rather than refusing, so a previous failed capture does not poison subsequent attempts.EditorApplication.Step()is play-mode-only; this is fine because the fixed code path is already gated byApplication.isPlaying.ScreenCapture.CaptureScreenshotAsTexture()returnsnullwhen called from a script in edit mode (confirmed empirically), andEditorApplication.Step()is a no-op outside play mode — so neither the API nor the spin primitive used by this fix is usable there. Edit-modeinclude_image=truecontinues to fall through to the existing camera-based path, which excludes UI Toolkit overlays by design. A real edit-mode overlay capture would need a different mechanism (camera RT +PanelSettings.targetTexturereadback composited manually) — out of scope for this PR.Summary by CodeRabbit
Bug Fixes
Refactor