Skip to content

Conversation

@toxifly
Copy link
Contributor

@toxifly toxifly commented Jan 21, 2026

Description

Fixes manage_scene screenshot captures that could appear blank in UI-heavy scenes by preferring Game View capture when available and handling async screenshot import timing.
Also cleans up related implementation details.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update
  • Refactoring (no functional changes)

Changes Made

  • Refactored screenshot logic so ManageScene uses a single capture path and relies on ScreenshotUtility for version-specific behavior.
  • Added isAsync signaling to screenshot results and import-on-file-exists logic to avoid race conditions with ScreenCapture.CaptureScreenshot.
  • Improved Unity 2021.3 fallback to pick a better camera (not only Camera.main) and fixed temporary Texture2D cleanup.
  • Removed dead recursive hierarchy method and added debug-only logging for a previously silent exception.
  • Updated dev docs to describe screenshot behavior for Unity 2022.1+ vs 2021.3.

Testing/Screenshots/Recordings

  • Manual: captured screenshots in Assets/Screenshots/ from Edit Mode and verified output is non-empty and includes UI on Unity 2022.1+.
  • Manual: verified Unity 2021.3 fallback captures camera output (overlay UI not included).

Related Issues

  • Relates to: (none)

Additional Notes

  • Unity 2022.1+ uses ScreenCapture.CaptureScreenshot which writes asynchronously; the tool now imports the asset once the PNG exists on disk.
  • Unity 2021.3 fallback captures camera output only; Screen Space - Overlay UI may not be included.

Summary by Sourcery

Unify manage_scene screenshot capture through ScreenshotUtility to prefer Game View screenshots when available and handle async file import, while cleaning up related scene inspection utilities and docs.

Bug Fixes:

  • Prevent blank or missing manage_scene screenshots in UI-heavy scenes by preferring Game View capture and correctly importing asynchronously written files.
  • Avoid screenshot failures on older Unity versions by falling back to the best available camera when ScreenCapture is not supported.
  • Ensure temporary textures created during camera-based screenshot capture are properly cleaned up to prevent leaks.
  • Log previously silent exceptions when enumerating GameObject components in scene summaries to aid debugging.

Enhancements:

  • Refactor screenshot capture so ManageScene delegates to ScreenshotUtility for version-specific behavior and a single capture path.
  • Add best-effort Game View preparation before capture to improve reliability of screenshots in the editor.
  • Introduce async-awareness in ScreenshotCaptureResult and schedule asset imports once the screenshot file exists on disk instead of using a blanket AssetDatabase.Refresh.
  • Remove unused recursive GameObject hierarchy helper to simplify ManageScene implementation.

Documentation:

  • Document manage_scene screenshot behavior and differences between Unity 2022.1+ Game View capture and the Unity 2021.3 camera fallback in both English and Chinese dev READMEs.

Summary by CodeRabbit

  • New Features

    • Unified screenshot flow with async capture support and deferred asset import for more reliable saves.
    • Improved Game View/UI preparation to ensure captures work across Unity versions.
    • Improved object search compatibility for newer Unity releases.
  • Bug Fixes

    • More robust error handling, verbose logging, and import scheduling for captured screenshots.
  • Documentation

    • Added manage_scene(action="screenshot") docs detailing Unity-version behaviors.
  • Chores

    • Package/manifest version bumped to 9.2.0.

✏️ Tip: You can customize this high-level summary in your review settings.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jan 21, 2026

Reviewer's Guide

Centralizes manage_scene screenshot capture through ScreenshotUtility, adds async-aware import handling for Game View captures, improves camera-based fallback behavior in Unity 2021.3, and tightens robustness/logging while updating developer docs to describe the new behavior.

Sequence diagram for manage_scene async-aware screenshot capture

sequenceDiagram
    actor User
    participant ManageScene
    participant ScreenshotUtility
    participant ScreenCapture
    participant Camera
    participant FileSystem
    participant EditorApplication
    participant AssetDatabase

    User->>ManageScene: CaptureScreenshot(fileName, superSize)
    ManageScene->>ManageScene: BestEffortPrepareGameViewForScreenshot()
    ManageScene->>ScreenshotUtility: CaptureToAssetsFolder(fileName, resolvedSuperSize, ensureUniqueFileName)

    alt Unity_2022_1_or_newer_async_GameView
        ScreenshotUtility->>ScreenCapture: CaptureScreenshot(assetsRelativePath, size)
        ScreenshotUtility-->>ManageScene: ScreenshotCaptureResult(IsAsync = true)
        ManageScene->>ManageScene: ScheduleAssetImportWhenFileExists(assetsRelativePath, fullPath, timeoutSeconds)
        ManageScene-->>User: SuccessResponse(isAsync = true)

        loop Until_file_exists_or_timeout
            EditorApplication-->>EditorApplication: update event invokes tick
            EditorApplication->>FileSystem: File.Exists(fullPath)
            alt File_exists
                FileSystem-->>EditorApplication: true
                EditorApplication->>AssetDatabase: ImportAsset(assetsRelativePath, ForceSynchronousImport)
                EditorApplication-->>EditorApplication: unsubscribe tick
            else Timeout
                FileSystem-->>EditorApplication: false
                EditorApplication-->>EditorApplication: check timeout and unsubscribe if exceeded
            end
        end
    else Unity_2021_3_camera_fallback_sync
        ScreenshotUtility->>Camera: FindBestCamera()
        alt Camera_found
            ScreenshotUtility->>Camera: CaptureFromCameraToAssetsFolder(camera, captureName, size, ensureUniqueFileName)
            ScreenshotUtility-->>ManageScene: ScreenshotCaptureResult(IsAsync = false)
            ManageScene->>AssetDatabase: ImportAsset(assetsRelativePath, ForceSynchronousImport)
            ManageScene-->>User: SuccessResponse(isAsync = false)
        else No_camera_found
            ScreenshotUtility-->>ManageScene: throws InvalidOperationException
            ManageScene-->>User: ErrorResponse("No camera found to capture screenshot.")
        end
    end

    Note over ManageScene,User: Verb in success message reflects IsAsync ("Screenshot requested" vs "Screenshot captured")
Loading

Updated class diagram for screenshot capture utilities and ManageScene integration

classDiagram
    class ManageScene {
        <<static>>
        +object CaptureScreenshot(string fileName, int? superSize)
        +void BestEffortPrepareGameViewForScreenshot()
        +void ScheduleAssetImportWhenFileExists(string assetsRelativePath, string fullPath, double timeoutSeconds)
        +object GetActiveSceneInfo()
        +object BuildGameObjectSummary(GameObject go, bool includeTransform, int maxChildrenPerNode)
        +string GetGameObjectPath(GameObject go)
    }

    class ScreenshotCaptureResult {
        +ScreenshotCaptureResult(string fullPath, string assetsRelativePath, int superSize)
        +ScreenshotCaptureResult(string fullPath, string assetsRelativePath, int superSize, bool isAsync)
        +string FullPath
        +string AssetsRelativePath
        +int SuperSize
        +bool IsAsync
    }

    class ScreenshotUtility {
        <<static>>
        -const string ScreenshotsFolderName
        +ScreenshotCaptureResult CaptureToAssetsFolder(string fileName, int superSize, bool ensureUniqueFileName)
        +ScreenshotCaptureResult CaptureFromCameraToAssetsFolder(Camera camera, string fileName, int superSize, bool ensureUniqueFileName)
        -Camera FindBestCamera()
        -string GetProjectRootPath()
    }

    class UnityEngine_Application {
        +static bool isPlaying
        +static bool isBatchMode
    }

    class UnityEditor_EditorApplication {
        +static double timeSinceStartup
        +static event Action update
        +static bool ExecuteMenuItem(string menuPath)
        +static void QueuePlayerLoopUpdate()
    }

    class UnityEditor_AssetDatabase {
        +static void ImportAsset(string path, ImportAssetOptions options)
    }

    class UnityEditor_EditorWindow {
        +static EditorWindow GetWindow(System.Type type)
        +void Repaint()
    }

    class UnityEditor_SceneView {
        +static void RepaintAll()
    }

    class UnityEngine_ScreenCapture {
        +static void CaptureScreenshot(string filename, int superSize)
    }

    class UnityEngine_Camera {
        +static Camera main
        +RenderTexture targetTexture
        +void Render()
    }

    class UnityEngine_RenderTexture {
        +static RenderTexture GetTemporary(int width, int height, int depthBuffer, RenderTextureFormat format)
        +static void ReleaseTemporary(RenderTexture temp)
        +static RenderTexture active
    }

    class UnityEngine_Texture2D {
        +Texture2D(int width, int height, TextureFormat textureFormat, bool mipChain)
        +void ReadPixels(Rect source, int destX, int destY)
        +void Apply()
        +byte[] EncodeToPNG()
    }

    class System_IO_File {
        +static void WriteAllBytes(string path, byte[] bytes)
        +static bool Exists(string path)
    }

    class McpLog {
        +static void Debug(string message)
    }

    ManageScene ..> ScreenshotUtility : uses
    ManageScene ..> ScreenshotCaptureResult : creates
    ManageScene ..> UnityEditor_AssetDatabase : imports_assets
    ManageScene ..> UnityEditor_EditorApplication : schedules_update
    ScreenshotUtility ..> ScreenshotCaptureResult : returns
    ScreenshotUtility ..> UnityEngine_ScreenCapture : calls
    ScreenshotUtility ..> UnityEngine_Camera : uses
    ScreenshotUtility ..> UnityEngine_RenderTexture : uses
    ScreenshotUtility ..> UnityEngine_Texture2D : uses
    ScreenshotUtility ..> System_IO_File : writes_png
    ManageScene ..> McpLog : debug_logging
    ManageScene ..> System_IO_File : checks_exists
    ManageScene ..> UnityEditor_EditorWindow : opens_GameView
    ManageScene ..> UnityEditor_SceneView : repaints
    ManageScene ..> UnityEngine_Application : checks_modes
    ScreenshotUtility ..> UnityEngine_Application : checks_isPlaying
    ManageScene ..> UnityEngine_Application : checks_isBatchMode
Loading

File-Level Changes

Change Details Files
Unify ManageScene screenshot capture to use ScreenshotUtility and handle async vs sync imports correctly.
  • Replace custom Edit/Play Mode branching in CaptureScreenshot with a single ScreenshotUtility.CaptureToAssetsFolder call.
  • Add BestEffortPrepareGameViewForScreenshot to open/repaint the Game View and queue editor updates before capture when not in batch mode.
  • Switch from AssetDatabase.Refresh to either immediate ImportAsset for sync results or deferred import via ScheduleAssetImportWhenFileExists for async results.
  • Include isAsync in the screenshot response payload and adjust user-facing status message based on async vs sync capture.
MCPForUnity/Editor/Tools/ManageScene.cs
Extend ScreenshotUtility to expose async capture status and provide a robust camera fallback for pre-2022 Unity.
  • Extend ScreenshotCaptureResult with an IsAsync flag and new constructor overload to track async capture semantics.
  • Introduce FindBestCamera helper that prefers Camera.main but falls back to the first available Camera using FindObjectsOfType with error handling.
  • Update CaptureToAssetsFolder to compute assetsRelativePath, call ScreenCapture.CaptureScreenshot on 2022.1+ (async) and camera-based fallback otherwise (sync), and return an appropriately flagged ScreenshotCaptureResult.
  • Fix CaptureFromCameraToAssetsFolder resource handling by reusing a single Texture2D variable and explicitly destroying it (Destroy/DestroyImmediate) after use while restoring previous render targets.
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
Improve robustness and observability in scene inspection helpers.
  • Wrap component enumeration in BuildGameObjectSummary in a typed catch that logs a debug message with GameObject name and exception message instead of silently swallowing errors.
  • Remove the unused GetGameObjectDataRecursive method that built a full recursive GameObject tree representation.
MCPForUnity/Editor/Tools/ManageScene.cs
Document the updated manage_scene screenshot behavior for different Unity versions.
  • Add README-DEV section describing manage_scene(action="screenshot") output location and Game View vs camera fallback behavior, including async import notes for Unity 2022.1+ and UI limitations on 2021.3.
  • Add equivalent documentation in README-DEV-zh with localized description of screenshot storage, version differences, and async behavior.
docs/README-DEV.md
docs/README-DEV-zh.md

Possibly linked issues

  • #Screenshot action putting image in wrong folder when in play mode: PR changes CaptureToAssetsFolder to pass assetsRelativePath into ScreenCapture.CaptureScreenshot, fixing Play Mode folder location.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 21, 2026

Caution

Review failed

The pull request is closed.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Unified screenshot capture flow with UI preparation and async-aware import scheduling; added camera selection and async flag to capture results; conditional object-finding for Unity 2023.1+; package/version bumps and documentation for manage_scene(action="screenshot").

Changes

Cohort / File(s) Summary
Screenshot Capture Core
MCPForUnity/Editor/Tools/ManageScene.cs, MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
Consolidated to a single capture path with EnsureGameView() prep, added ScreenshotCaptureResult.IsAsync, switched to ScreenCapture.CaptureScreenshot on Unity 2022.1+, added camera-fallback, introduced ScheduleAssetImportWhenFileExists(...) for deferred import, and improved error logging and cleanup.
Object Finding
MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs
Added conditional compilation to use FindObjectsByType(..., FindObjectsInactive) on Unity 2023.1+ while preserving older FindObjectsOfType behavior for prior versions.
Version & Metadata
MCPForUnity/package.json, Server/pyproject.toml, manifest.json, Server/README.md
Bumped package/manifest versions 9.1.0 → 9.2.0; manifest.json reformatted (multiline args/tools) with no semantic changes.
Documentation
docs/development/README-DEV.md, docs/development/README-DEV-zh.md
Added manage_scene(action="screenshot") docs detailing Unity-version-specific capture semantics (async ScreenCapture for 2022.1+, camera RenderTexture fallback for 2021.3).

Sequence Diagram(s)

sequenceDiagram
    participant Editor as Editor/ManageScene
    participant UI as GameView/UI
    participant Capture as ScreenshotUtility
    participant FS as FileSystem
    participant Importer as AssetImporter

    Editor->>UI: EnsureGameView() / trigger repaints
    Editor->>Capture: CaptureToAssetsFolder()
    alt Unity 2022.1+
        Capture->>Capture: ScreenCapture.CaptureScreenshot()\n(async write)
        Capture-->>Editor: ScreenshotCaptureResult(IsAsync=true)
    else older Unity
        Capture->>Capture: FindAvailableCamera() -> RenderTexture -> PNG\n(sync write)
        Capture-->>Editor: ScreenshotCaptureResult(IsAsync=false)
    end

    alt IsAsync = true
        Editor->>Editor: ScheduleAssetImportWhenFileExists(path, timeout)
        Editor->>FS: Poll for file existence
        FS-->>Editor: File appears
        Editor->>Importer: Import asset
        Importer-->>Editor: Import complete
    else IsAsync = false
        Editor->>Importer: Import synchronously
        Importer-->>Editor: Import complete
    end

    Editor-->>Editor: Log result (includes isAsync)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • #600: Mirrors ManageScene + ScreenshotUtility unification (EnsureGameView, import scheduling, IsAsync) — very closely related.
  • #477: Changes to ScreenshotUtility capture/path handling and assets-relative normalization.
  • #449: Earlier updates to ManageScene screenshot flow and camera-selection behavior.

Poem

🐰📸 I hopped to ready the Game View bright,
I nudged repaints gently into the night.
Async or sync, I wait and I peep—
Then import arrives from the file I keep.
Hooray for captures—snaps safe and neat!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: improve manage_scene screenshot capture' clearly and concisely summarizes the main objective of the PR, which is to fix and improve screenshot capture functionality in manage_scene.
Description check ✅ Passed The PR description is comprehensive and follows the template structure with all major sections completed: Description, Type of Change, Changes Made, Testing/Screenshots/Recordings, Related Issues, and Additional Notes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • There are a lot of broad catch { } blocks in the new screenshot-related helpers (e.g., BestEffortPrepareGameViewForScreenshot, ScheduleAssetImportWhenFileExists); consider narrowing these catches or adding at least optional/conditional logging so that unexpected failures don’t get completely masked during debugging.
  • In ScreenshotUtility.CaptureToAssetsFolder on pre-2022 Unity, the Debug.LogWarning about the ScreenCapture fallback will fire on every screenshot; consider downgrading this to a Log or gating it (e.g., only once) to avoid log noise when the tool is used frequently.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- There are a lot of broad `catch { }` blocks in the new screenshot-related helpers (e.g., `BestEffortPrepareGameViewForScreenshot`, `ScheduleAssetImportWhenFileExists`); consider narrowing these catches or adding at least optional/conditional logging so that unexpected failures don’t get completely masked during debugging.
- In `ScreenshotUtility.CaptureToAssetsFolder` on pre-2022 Unity, the `Debug.LogWarning` about the ScreenCapture fallback will fire on every screenshot; consider downgrading this to a `Log` or gating it (e.g., only once) to avoid log noise when the tool is used frequently.

## Individual Comments

### Comment 1
<location> `MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs:83-89` </location>
<code_context>
 #else
             Debug.LogWarning("ScreenCapture is supported after Unity 2022.1. Using main camera capture as fallback.");
-            CaptureFromCameraToAssetsFolder(Camera.main, captureName, size, false);
+            var cam = FindBestCamera();
+            if (cam == null)
+            {
+                throw new InvalidOperationException("No camera found to capture screenshot.");
+            }
+            string captureName = Path.GetFileName(normalizedFullPath);
+            CaptureFromCameraToAssetsFolder(cam, captureName, size, false);
+            isAsync = false;
 #endif      
</code_context>

<issue_to_address>
**question (bug_risk):** Clarify/align the returned paths with the actual file path used in the pre-2022 camera fallback.

In this path we compute `normalizedFullPath` / `assetsRelativePath` for `ScreenshotCaptureResult`, but the actual file path is recomputed inside `CaptureFromCameraToAssetsFolder` from the screenshots folder and `captureName`. If that method ever changes its folder/uniqueness rules, the returned `FullPath` / `AssetsRelativePath` may not match where the file is actually written. Consider having `CaptureFromCameraToAssetsFolder` return a `ScreenshotCaptureResult` and propagate its paths, or factor out the path-building into a shared helper so both code paths use identical logic.
</issue_to_address>

### Comment 2
<location> `MCPForUnity/Editor/Tools/ManageScene.cs:458-467` </location>
<code_context>
+        private static void ScheduleAssetImportWhenFileExists(string assetsRelativePath, string fullPath, double timeoutSeconds)
</code_context>

<issue_to_address>
**suggestion:** Consider surfacing timeout or failure information in the async import scheduler for easier diagnosis.

Right now the scheduler unsubscribes on timeout or exceptions without any signal to the caller. If the file never appears (e.g., permission issues or incorrect path), the only symptom is the missing asset. Consider logging a warning when the timeout is reached or when repeated failures occur (potentially wrapped in `#if UNITY_EDITOR`) so users can understand why the screenshot wasn’t imported, without adding heavy overhead to the polling loop.

Suggested implementation:

```csharp
        private static void ScheduleAssetImportWhenFileExists(string assetsRelativePath, string fullPath, double timeoutSeconds)
        {
            if (string.IsNullOrWhiteSpace(assetsRelativePath) || string.IsNullOrWhiteSpace(fullPath))
            {
                return;
            }

            double start = EditorApplication.timeSinceStartup;
            int failureCount = 0;
            const int maxLoggedFailures = 3;

            Action tick = null;
            tick = () =>
            {
                // Stop polling if we've timed out
                if (EditorApplication.timeSinceStartup - start > timeoutSeconds)
                {
#if UNITY_EDITOR
                    Debug.LogWarning(
                        $"[ManageScene] Timed out waiting for asset at path '{fullPath}' (Assets relative: '{assetsRelativePath}') " +
                        $"after {timeoutSeconds:F1} seconds. The screenshot asset was not imported.");
#endif
                    EditorApplication.update -= tick;
                    return;
                }

                try
                {
                    if (!System.IO.File.Exists(fullPath))
                    {
                        // Still waiting for the file to appear; keep polling.
                        return;
                    }

                    // File exists; import the asset.
                    AssetDatabase.ImportAsset(assetsRelativePath, ImportAssetOptions.ForceUpdate);
#if UNITY_EDITOR
                    Debug.Log($"[ManageScene] Imported asset at '{assetsRelativePath}' after detecting file on disk.");
#endif
                    EditorApplication.update -= tick;
                }
                catch (Exception ex)
                {
                    failureCount++;

                    if (failureCount <= maxLoggedFailures)
                    {
#if UNITY_EDITOR
                        Debug.LogWarning(
                            $"[ManageScene] Exception while importing asset '{assetsRelativePath}' from '{fullPath}' " +
                            $"(attempt {failureCount}): {ex}");
#endif
                    }

                    // If failures are persistent beyond the first few attempts, stop polling to avoid spamming.
                    if (failureCount >= maxLoggedFailures)
                    {
#if UNITY_EDITOR
                        Debug.LogWarning(
                            $"[ManageScene] Stopping import polling for '{assetsRelativePath}' after {failureCount} failures.");
#endif
                        EditorApplication.update -= tick;
                    }
                }
            };

            // Start polling on the editor update loop
            EditorApplication.update += tick;

```

1. Ensure `using UnityEditor;` and `using UnityEngine;` are present at the top of `ManageScene.cs` so that `AssetDatabase`, `EditorApplication`, and `Debug` resolve correctly.
2. If this method is used for assets other than screenshots, you may want to adjust or generalize the log messages (e.g., remove "screenshot" wording or pass a context string into the method).
3. If there is already a centralized logging facility in this project, you may want to replace the direct `Debug.Log/LogWarning` calls with that logger to keep logs consistent.
</issue_to_address>

### Comment 3
<location> `docs/README-DEV.md:205` </location>
<code_context>
+### `manage_scene(action="screenshot")`
+
+- Saves PNGs under `Assets/Screenshots/`.
+- Unity **2022.1+**: captures the **Game View** via `ScreenCapture.CaptureScreenshot`, so `Screen Space - Overlay` UI is included. Note this write is **async**, so the file may appear/import a moment later.
+- Unity **2021.3**: falls back to rendering the best available `Camera` into a `RenderTexture` (camera output only; `Screen Space - Overlay` UI is not included).
+
</code_context>

<issue_to_address>
**nitpick (typo):** Consider adding "that" in "Note this write is async" for smoother grammar.

You could also rephrase to something likeThis write is **async**, so the file may appear/import a moment laterif you want to make the note read more smoothly while emphasizing the async behavior.

```suggestion
- Unity **2022.1+**: captures the **Game View** via `ScreenCapture.CaptureScreenshot`, so `Screen Space - Overlay` UI is included. This write is **async**, so the file may appear/import a moment later.
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

- Gate pre-2022 ScreenCapture fallback warning to log only once
- Downgrade warning to Debug.Log to reduce log noise
- Refactor path-building into shared PrepareCaptureResult() helper
- Add conditional logging to catch blocks in BestEffortPrepareGameViewForScreenshot
- Add timeout/failure logging to ScheduleAssetImportWhenFileExists
- Fix grammar in README-DEV.md
@msanatan
Copy link
Member

@toxifly can you do a before and after screenshot, visuals will definitely help with a change like this 🙂

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs`:
- Around line 157-166: The current use of FindObjectsSortMode.None in the
FindObjectsByType call can change which object is returned when only the first
match is used (see findAll), so change the sort mode based on whether findAll is
requested: when findAll is false use FindObjectsSortMode.InstanceID to retain
legacy deterministic ordering, otherwise use FindObjectsSortMode.None (or the
existing behavior); update the FindObjectsByType invocation that sets
searchPoolComp (using componentType and searchInactive) to pick the sort mode
conditionally so the first selected gameObject remains stable.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageScene.cs (1)

476-546: Consider waiting for file size to stabilize before importing.

If ScreenCapture.CaptureScreenshot creates the file before the write completes, importing immediately on File.Exists may still race. A lightweight “stable size for N ticks” guard can avoid partial imports on some platforms.

🛠️ Example stabilization guard
-            bool hasSeenFile = false;
+            bool hasSeenFile = false;
+            long lastSize = -1;
+            int stableTicks = 0;
+            const int requiredStableTicks = 2;

             EditorApplication.CallbackFunction tick = null;
             tick = () =>
             {
                 try
                 {
                     if (File.Exists(fullPath))
                     {
                         hasSeenFile = true;
-
-                        AssetDatabase.ImportAsset(assetsRelativePath, ImportAssetOptions.ForceSynchronousImport);
+                        long size = new FileInfo(fullPath).Length;
+                        if (size <= 0)
+                        {
+                            return;
+                        }
+                        if (size == lastSize) stableTicks++;
+                        else { stableTicks = 0; lastSize = size; }
+                        if (stableTicks < requiredStableTicks) return;
+
+                        AssetDatabase.ImportAsset(assetsRelativePath, ImportAssetOptions.ForceSynchronousImport);
                         try { McpLog.Debug($"[ManageScene] Imported asset at '{assetsRelativePath}'."); } catch { }
                         EditorApplication.update -= tick;
                         return;
                     }
                 }

Comment on lines +157 to +166
#if UNITY_2023_1_OR_NEWER
var inactive = searchInactive ? FindObjectsInactive.Include : FindObjectsInactive.Exclude;
searchPoolComp = UnityEngine.Object.FindObjectsByType(componentType, inactive, FindObjectsSortMode.None)
.Cast<Component>()
.Select(c => c.gameObject);
#else
searchPoolComp = UnityEngine.Object.FindObjectsOfType(componentType, searchInactive)
.Cast<Component>()
.Select(c => c.gameObject);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find the file
find . -name "ManageGameObjectCommon.cs" -type f

Repository: CoplayDev/unity-mcp

Length of output: 127


🏁 Script executed:

# Read the file around the specified lines to understand context
cat -n MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs | sed -n '140,180p'

Repository: CoplayDev/unity-mcp

Length of output: 2371


🏁 Script executed:

# Search for findAll variable usage in the file
rg -n "findAll" MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs

Repository: CoplayDev/unity-mcp

Length of output: 370


🏁 Script executed:

# Read the broader context to see how findAll is used and what happens when it's false
cat -n MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs | sed -n '1,50p'

Repository: CoplayDev/unity-mcp

Length of output: 2302


🏁 Script executed:

# Check what happens around line 209 where findAll is used
cat -n MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs | sed -n '200,220p'

Repository: CoplayDev/unity-mcp

Length of output: 1026


🏁 Script executed:

# Look at the FindObjectsInternal method signature and overall structure
cat -n MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs | sed -n '30,45p'

Repository: CoplayDev/unity-mcp

Length of output: 812


🏁 Script executed:

# Check if there are other search methods to understand the pattern
cat -n MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs | sed -n '50,180p'

Repository: CoplayDev/unity-mcp

Length of output: 7042


🌐 Web query:

FindObjectsOfType InstanceID sorting Unity documentation

💡 Result:

Short answer: Unity's Object.FindObjectsOfType always sorts its results by InstanceID. Use Object.FindObjectsByType with FindObjectsSortMode.None if you don't need sorting (faster). [1][2][3]

Sources:

  • Unity docs: Object.FindObjectsOfType (notes it always sorts by InstanceID; recommends FindObjectsByType). [1]
  • Unity docs: FindObjectsSortMode enum (InstanceID vs None). [2]
  • Community confirmation / example showing ordering by GetInstanceID. [3]

🌐 Web query:

FindObjectsByType FindObjectsSortMode.None documentation Unity 2023.1

💡 Result:

FindObjectsByType is documented in the Unity 2023.1 Scripting API. Use the FindObjectsSortMode.None option to return results unsorted (fastest, order not guaranteed).

Key points

  • Signature/examples: Object.FindObjectsByType(Type type, FindObjectsSortMode sortMode) and generic overloads (also overloads with FindObjectsInactive). Example: FindObjectsByType(FindObjectsSortMode.None). [1]
  • FindObjectsSortMode.None: "Don't sort the objects." It's the least resource‑intensive option; result order may vary between calls. [2]
  • Notes: This API finds loaded (non-asset) objects; it doesn't return assets or objects with HideFlags.DontSave. Use Resources.FindObjectsOfTypeAll to include assets. FindObjectsOfType (legacy) is deprecated in favor of FindObjectsByType which lets you choose sorting. [3]

Example

  • C#: var found = FindObjectsByType(FindObjectsSortMode.None); [1]

Sources
[1] Unity Scripting API — Object.FindObjectsByType (2023.1).
[2] Unity Scripting API — FindObjectsSortMode.None.
[3] Unity Scripting API — Object.FindObjectsOfType (notes on deprecation and behavior).


Preserve deterministic selection when only returning first match.

FindObjectsSortMode.None produces unsorted results, whereas the previous FindObjectsOfType always returns results sorted by InstanceID. Since the code returns only the first match when findAll is false (line 211), this sorting change can alter which object is selected. To preserve legacy behavior, use FindObjectsSortMode.InstanceID when findAll is false.

🔧 Suggested adjustment
`#if` UNITY_2023_1_OR_NEWER
                            var inactive = searchInactive ? FindObjectsInactive.Include : FindObjectsInactive.Exclude;
+                           var sortMode = findAll ? FindObjectsSortMode.None : FindObjectsSortMode.InstanceID;
-                           searchPoolComp = UnityEngine.Object.FindObjectsByType(componentType, inactive, FindObjectsSortMode.None)
+                           searchPoolComp = UnityEngine.Object.FindObjectsByType(componentType, inactive, sortMode)
                                .Cast<Component>()
                                .Select(c => c.gameObject);
`#else`
                            searchPoolComp = UnityEngine.Object.FindObjectsOfType(componentType, searchInactive)
                                .Cast<Component>()
                                .Select(c => c.gameObject);
`#endif`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#if UNITY_2023_1_OR_NEWER
var inactive = searchInactive ? FindObjectsInactive.Include : FindObjectsInactive.Exclude;
searchPoolComp = UnityEngine.Object.FindObjectsByType(componentType, inactive, FindObjectsSortMode.None)
.Cast<Component>()
.Select(c => c.gameObject);
#else
searchPoolComp = UnityEngine.Object.FindObjectsOfType(componentType, searchInactive)
.Cast<Component>()
.Select(c => c.gameObject);
#endif
`#if` UNITY_2023_1_OR_NEWER
var inactive = searchInactive ? FindObjectsInactive.Include : FindObjectsInactive.Exclude;
var sortMode = findAll ? FindObjectsSortMode.None : FindObjectsSortMode.InstanceID;
searchPoolComp = UnityEngine.Object.FindObjectsByType(componentType, inactive, sortMode)
.Cast<Component>()
.Select(c => c.gameObject);
`#else`
searchPoolComp = UnityEngine.Object.FindObjectsOfType(componentType, searchInactive)
.Cast<Component>()
.Select(c => c.gameObject);
`#endif`
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs` around lines
157 - 166, The current use of FindObjectsSortMode.None in the FindObjectsByType
call can change which object is returned when only the first match is used (see
findAll), so change the sort mode based on whether findAll is requested: when
findAll is false use FindObjectsSortMode.InstanceID to retain legacy
deterministic ordering, otherwise use FindObjectsSortMode.None (or the existing
behavior); update the FindObjectsByType invocation that sets searchPoolComp
(using componentType and searchInactive) to pick the sort mode conditionally so
the first selected gameObject remains stable.

@toxifly toxifly force-pushed the fix/manage-scene-screenshot branch from 177c786 to e45052b Compare January 21, 2026 18:26
@toxifly toxifly force-pushed the fix/manage-scene-screenshot branch from e45052b to 20b9188 Compare January 21, 2026 18:30
@toxifly
Copy link
Contributor Author

toxifly commented Jan 22, 2026

@toxifly can you do a before and after screenshot, visuals will definitely help with a change like this 🙂

To clear things up: screenshots were working before, but only when in play mode:

Before (origin/main)

  • manage_scene screenshot was reliable only in Play Mode (running or paused)
  • even in Play Mode, Unity writes screenshots asynchronously, but we tried to refresh/import immediately—so the file/asset could appear late (sometimes only after leaving Play Mode).
  • in Edit Mode, it used a camera/RenderTexture fallback that could produce a blank/transparent PNG (and doesn’t capture Game View UI reliably - shown below)
manage_scene_before

After (this branch)

  • Uses ScreenCapture for Unity 2022.1+ in both Edit Mode and Play Mode and waits to import until the file actually exists (isAsync reflects this)
  • Keeps the camera fallback for older Unity; when that fallback produces an “all-alpha-zero” image, we force alpha to opaque (now conditional + opt-out).
manage_scene_after_branch

@msanatan msanatan changed the base branch from main to beta January 22, 2026 19:18
@Scriptwonder
Copy link
Collaborator

Hi! Looks like this is a very important fix! Can you resolve the Readme conflicts first? I will try it in the mean time and merge it to the beta branch. Thanks!

@toxifly
Copy link
Contributor Author

toxifly commented Jan 24, 2026

Hi! Looks like this is a very important fix! Can you resolve the Readme conflicts first? I will try it in the mean time and merge it to the beta branch. Thanks!

Done.

@Scriptwonder Scriptwonder merged commit 67dda7f into CoplayDev:beta Jan 24, 2026
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants