fix(screenshot): restore pause state after composited capture#1230
Open
KamilDev wants to merge 2 commits into
Open
fix(screenshot): restore pause state after composited capture#1230KamilDev wants to merge 2 commits into
KamilDev wants to merge 2 commits into
Conversation
The play-mode composited path drives the player loop with EditorApplication.Step() to reach WaitForEndOfFrame. Step() pauses play mode as a side effect, and the loop never restored it — so any play-mode screenshot left the game paused indefinitely (regression from CoplayDev#1132, which introduced the Step() loop). Record isPaused before the loop and clear it afterward, unless the game was already paused (preserve a caller's intentional pause).
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)
📝 WalkthroughWalkthroughThe change updates ChangesScreenshot Capture Pause Handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
189-197: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider wrapping the restore in try/finally.
Correctly captures and conditionally restores the pause state, matching the PR intent. However, if
EditorApplication.Step()throws (or something else between capture and restore does),isPausedrestoration is skipped — reintroducing the same "stuck paused" symptom this PR fixes.♻️ Suggested defensive wrap
bool wasPaused = UnityEditor.EditorApplication.isPaused; - for (int i = 0; i < timeoutSteps && !done; i++) - { - UnityEditor.EditorApplication.Step(); - } - if (!wasPaused) - UnityEditor.EditorApplication.isPaused = false; + try + { + for (int i = 0; i < timeoutSteps && !done; i++) + { + UnityEditor.EditorApplication.Step(); + } + } + finally + { + if (!wasPaused) + UnityEditor.EditorApplication.isPaused = false; + }🤖 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 189 - 197, The pause-state restore in ScreenshotUtility’s screenshot stepping logic should be made exception-safe. In the code around the EditorApplication.Step loop, wrap the Step() calls and the conditional isPaused restoration in a try/finally so the original pause state captured in wasPaused is always restored even if Step() or intervening logic throws.
🤖 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.
Nitpick comments:
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 189-197: The pause-state restore in ScreenshotUtility’s screenshot
stepping logic should be made exception-safe. In the code around the
EditorApplication.Step loop, wrap the Step() calls and the conditional isPaused
restoration in a try/finally so the original pause state captured in wasPaused
is always restored even if Step() or intervening logic throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c963cfd2-6977-4dc2-add9-55e0b252aa37
📒 Files selected for processing (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
Wrap the Step() loop in try/finally so the pause-state restore runs on every exit path, including if Step() throws — matching the camera-state restore pattern already used in CaptureFromCameraToProjectFolder.
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=screenshotin play mode leaves the game paused. The composited capture path added in #1132 pumps the player loop withEditorApplication.Step()to reach theWaitForEndOfFramecapture.Step()pauses play mode as a side effect, andCaptureCompositedAfterFramenever restored the prior state — so a single screenshot silently pauses a running game and never resumes it.Type of Change
Changes Made
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs:CaptureCompositedAfterFrame, recordEditorApplication.isPausedbefore theStep()loop and clear it afterward — unless the game was already paused, in which case the intentional pause is preserved.Compatibility / Package Source
file:local referenceTesting/Screenshots/Recordings
Verified live: with the fix,
editor_state.play_mode.is_pausedstaysfalseafter a play-modemanage_camera screenshot include_image=true(the path that pumpsStep()); without it, the same call flipsis_pausedtotrueand leaves it there. The capture output is unchanged.Before → After (
is_pausedafter screenshot, running game):true→false.Not applicable: play-mode-only editor side-effect restore, verified live (before/after above). The #1132 path that introduced this shipped without an automated test.
Related Issues
Relates to #1132 (introduced the
Step()loop this restores state around).Summary by CodeRabbit