fix(screenshot): wait for end-of-frame before composited capture#1132
Open
KamilDev wants to merge 3 commits into
Open
fix(screenshot): wait for end-of-frame before composited capture#1132KamilDev wants to merge 3 commits into
KamilDev wants to merge 3 commits into
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an editor-only deferred capture helper that waits until end-of-frame to call ScreenCapture.CaptureScreenshotAsTexture and updates CaptureComposited to use this helper during Play Mode in the Unity Editor so UI Toolkit overlays are included. ChangesEditor composited-frame screenshot capture
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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}
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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