feat(scene): add scene_view screenshot capture source and fix viewport handling#872
Closed
jiajunfeng wants to merge 5 commits intoCoplayDev:betafrom
Closed
feat(scene): add scene_view screenshot capture source and fix viewport handling#872jiajunfeng wants to merge 5 commits intoCoplayDev:betafrom
jiajunfeng wants to merge 5 commits intoCoplayDev:betafrom
Conversation
added 5 commits
March 5, 2026 16:58
Contributor
Reviewer's GuideAdds support for capturing screenshots from the Unity Scene View (alongside existing Game View capture), introduces a shared viewport-based Scene View capture utility, tightens parameter validation/forwarding across CLI, Python tool, and Unity editor layers, enhances object reference handling for sprites, and updates Roslyn dependencies and documentation. Sequence diagram for screenshot capture with capture_sourcesequenceDiagram
actor User
participant CLI_scene as CLI_scene_command
participant Py_manage as Python_manage_scene_tool
participant Unity_IPC as Unity_manage_scene_IPC
participant ManageScene as ManageScene
participant EditorSS as EditorWindowScreenshotUtility
User->>CLI_scene: unity-mcp scene screenshot --capture-source scene_view --scene-view-target "Canvas" --include-image
CLI_scene->>Py_manage: build params (captureSource, sceneViewTarget, includeImage, etc.)
Py_manage->>Unity_IPC: run_command action=screenshot params
Unity_IPC->>ManageScene: CaptureScreenshot(SceneCommand)
Note over ManageScene: Normalize and validate captureSource
ManageScene->>ManageScene: captureSource = game_view or scene_view
alt invalid capture_source
ManageScene-->>Unity_IPC: ErrorResponse (invalid capture_source)
Unity_IPC-->>Py_manage: error
Py_manage-->>CLI_scene: tool error
CLI_scene-->>User: show error
else capture_source == scene_view
ManageScene->>ManageScene: validate superSize, batch, lookAt, viewPosition, viewRotation, camera
alt params invalid
ManageScene-->>Unity_IPC: ErrorResponse if invalid combo
Unity_IPC-->>Py_manage: error
Py_manage-->>CLI_scene: tool error
CLI_scene-->>User: show error
else params valid
ManageScene->>ManageScene: CaptureSceneViewScreenshot(cmd, fileName,...)
ManageScene->>ManageScene: FrameSceneView(SceneCommand{sceneViewTarget}) opt sceneViewTarget
ManageScene->>EditorSS: CaptureSceneViewViewportToAssets(sceneView,...)
EditorSS->>EditorSS: FocusAndRepaint(sceneView)
EditorSS->>EditorSS: GetSceneViewViewportPixelRect(sceneView)
EditorSS->>EditorSS: CaptureViewRect(sceneView, viewportRect)
EditorSS-->>ManageScene: ScreenshotCaptureResult (imageBase64?, viewportWidth/Height)
ManageScene-->>Unity_IPC: SuccessResponse data{captureSource=scene_view, captureMode=scene_view_viewport,...}
Unity_IPC-->>Py_manage: success + image metadata
Py_manage-->>CLI_scene: ToolResult (image extracted if include_image)
CLI_scene-->>User: path, inline image, metadata
end
else capture_source == game_view
ManageScene->>ManageScene: existing camera-based capture paths
ManageScene-->>Unity_IPC: SuccessResponse data{captureSource=game_view,...}
Unity_IPC-->>Py_manage: success
Py_manage-->>CLI_scene: ToolResult
CLI_scene-->>User: output
end
Updated class diagram for screenshot capture utilitiesclassDiagram
class SceneCommand {
+string camera
+string captureSource
+bool? includeImage
+int? maxResolution
+string batch
+JToken sceneViewTarget
+JToken lookAt
+int? superSize
+Vector3? viewPosition
+Quaternion? viewRotation
+string fileName
}
class ManageScene {
-static object CaptureScreenshot(SceneCommand cmd)
-static object CaptureSceneViewScreenshot(SceneCommand cmd, string fileName, int resolvedSuperSize, bool includeImage, int maxResolution)
-static object FrameSceneView(SceneCommand cmd)
-static Bounds CalculateFrameBounds(GameObject target)
-static bool TryGetRectTransformBounds(GameObject target, out Bounds bounds)
-static bool TryGetRendererBounds(GameObject target, out Bounds bounds)
-static bool TryGetColliderBounds(GameObject target, out Bounds bounds)
}
class EditorWindowScreenshotUtility {
+const string ScreenshotsFolderName
+const int RepaintSettlingDelayMs
+static ScreenshotCaptureResult CaptureSceneViewViewportToAssets(SceneView sceneView, string fileName, int superSize, bool ensureUniqueFileName, bool includeImage, int maxResolution, out int viewportWidth, out int viewportHeight)
-static void FocusAndRepaint(SceneView sceneView)
-static Rect GetSceneViewViewportPixelRect(SceneView sceneView)
-static Rect GetViewportLocalRectPoints(SceneView sceneView, float pixelsPerPoint)
-static Texture2D CaptureViewRect(SceneView sceneView, Rect viewportRectPixels)
-static object GetHostView(EditorWindow window)
-static Rect? GetRectProperty(object instance, string propertyName)
-static void InvokeMethodIfExists(object instance, string methodName)
-static void FlipTextureVertically(Texture2D texture)
-static ScreenshotCaptureResult PrepareCaptureResult(string fileName, int superSize, bool ensureUniqueFileName)
-static string BuildFileName(string fileName)
-static string EnsureUnique(string fullPath)
-static void DestroyTexture(Texture2D texture)
}
class ScreenshotCaptureResult {
+string FullPath
+string AssetsRelativePath
+int SuperSize
+bool IsAsync
+string ImageBase64
+int ImageWidth
+int ImageHeight
}
class ScreenshotUtility {
+static Texture2D DownscaleTexture(Texture2D source, int maxResolution)
}
class SceneView {
+Camera camera
+GUIContent titleContent
+static SceneView lastActiveSceneView
+void Frame(Bounds bounds, bool instant)
}
class SuccessResponse {
+SuccessResponse(string message, object data)
}
class ErrorResponse {
+ErrorResponse(string message)
}
ManageScene ..> SceneCommand : uses
ManageScene ..> EditorWindowScreenshotUtility : uses
ManageScene ..> ScreenshotCaptureResult : returns
ManageScene ..> SuccessResponse : returns
ManageScene ..> ErrorResponse : returns
EditorWindowScreenshotUtility ..> ScreenshotCaptureResult : creates
EditorWindowScreenshotUtility ..> ScreenshotUtility : calls
EditorWindowScreenshotUtility ..> SceneView : uses
EditorWindowScreenshotUtility ..> Texture2D : uses
EditorWindowScreenshotUtility ..> RenderTexture : uses
EditorWindowScreenshotUtility ..> McpLog : logs
EditorWindowScreenshotUtility ..> EditorWindow : uses
EditorWindowScreenshotUtility ..> Rect : uses
Updated class diagram for ComponentOps and ParamCoercion sprite/object handlingclassDiagram
class ComponentOps {
-static bool TrySetViaReflection(object component, Type type, string propertyName, string normalizedName, JToken value, BindingFlags flags, out string error)
-static bool SetObjectReference(SerializedProperty prop, JToken value, out string error)
-static bool SetEnum(SerializedProperty prop, JToken value, out string error)
-static long GetSpriteFileId(Sprite sprite)
}
class ParamCoercion {
+static int CoerceInt(JToken token, int defaultValue)
+static int? CoerceIntNullable(JToken token)
+static bool? CoerceBoolNullable(JToken token)
+static long CoerceLong(JToken token, long defaultValue)
+static long? CoerceLongNullable(JToken token)
}
class SerializedProperty {
+UnityEngine.Object objectReferenceValue
+string propertyPath
}
class Sprite {
+string name
}
class AssetDatabase {
+static UnityEngine.Object LoadAssetAtPath~UnityEngine.Object~(string path)
+static UnityEngine.Object[] LoadAllAssetsAtPath(string path)
}
class GlobalObjectId {
+static GlobalObjectId GetGlobalObjectIdSlow(UnityEngine.Object obj)
+long targetObjectId
}
ComponentOps ..> ParamCoercion : uses CoerceLong
ComponentOps ..> SerializedProperty : sets
ComponentOps ..> AssetDatabase : loads assets
ComponentOps ..> Sprite : matches by name
ComponentOps ..> GlobalObjectId : uses for fileID
ParamCoercion ..> JToken : consumes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces Scene View screenshot capture capability to complement existing Game View capture. It adds a new EditorWindowScreenshotUtility for viewport rendering, enhances ComponentOps for sprite reference resolution via JSON, extends the CLI with capture-source options, and adds corresponding server-side parameter handling with validation constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CLI Client
participant Server as Python Server
participant Editor as Unity Editor
participant FileSystem as File System
participant SceneView as Scene View
Client->>Server: screenshot<br/>capture_source=scene_view
Server->>Server: Validate capture_source<br/>with action=screenshot
Server->>Editor: Request screenshot<br/>captureSource=scene_view
Editor->>Editor: Validate constraints<br/>(no super_size > 1, etc.)
Editor->>SceneView: Focus & repaint SceneView
SceneView->>SceneView: Render viewport
Editor->>Editor: Capture viewport pixels<br/>via GrabPixels reflection
Editor->>Editor: Flip texture vertically<br/>Encode to PNG
Editor->>FileSystem: Save PNG to<br/>Assets/Screenshots
Editor->>Editor: Compute viewport dimensions<br/>Optional Base64 encode
Editor->>Server: Return metadata<br/>(path, dimensions, image)
Server->>Client: Response with file path<br/>& optional image payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
CoerceLongNullablehelper inParamCoercionis currently unused; consider removing it or wiring it into call sites to avoid dead code. - The checks in
ComponentOps.TrySetViaReflectionforJObjectvalues targetingUnityEngine.Objectfields/properties are duplicated across three branches; consider extracting this into a small helper to reduce repetition and keep the intent in one place. - In
EditorWindowScreenshotUtility.FocusAndRepaint, theThread.Sleepon the editor main thread may briefly stall the editor; if you hit responsiveness issues, consider an async/EditorApplication.delayCall-based approach to wait for repaint instead of blocking.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `CoerceLongNullable` helper in `ParamCoercion` is currently unused; consider removing it or wiring it into call sites to avoid dead code.
- The checks in `ComponentOps.TrySetViaReflection` for `JObject` values targeting `UnityEngine.Object` fields/properties are duplicated across three branches; consider extracting this into a small helper to reduce repetition and keep the intent in one place.
- In `EditorWindowScreenshotUtility.FocusAndRepaint`, the `Thread.Sleep` on the editor main thread may briefly stall the editor; if you hit responsiveness issues, consider an async/`EditorApplication.delayCall`-based approach to wait for repaint instead of blocking.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/src/services/tools/manage_scene.py (1)
186-199:⚠️ Potential issue | 🟠 MajorEnforce the Scene View supersize restriction in the tool layer.
Line 197 only checks the action, so direct
manage_scene(..., action="screenshot", capture_source="scene_view", screenshot_super_size=2)calls still pass through. That bypasses the new unsupported-combination guard and can reintroduce the metadata/behavior mismatch this PR is meant to prevent.Suggested guard
if coerced_max_resolution is not None and coerced_max_resolution <= 0: return {"success": False, "message": "max_resolution must be a positive integer greater than zero."} @@ if coerced_max_resolution is not None: params["maxResolution"] = coerced_max_resolution if capture_source is not None: if action != "screenshot": return {"success": False, "message": "capture_source is only valid for action='screenshot'."} + if capture_source == "scene_view" and coerced_super_size not in (None, 1): + return { + "success": False, + "message": "capture_source='scene_view' only supports screenshot_super_size=1.", + } params["captureSource"] = capture_source🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_scene.py` around lines 186 - 199, manage_scene currently only checks action before accepting capture_source, allowing calls like manage_scene(..., action="screenshot", capture_source="scene_view", screenshot_super_size=2) to bypass the new restriction; add a guard in the screenshot handling block that if capture_source == "scene_view" then reject requests with a supersize > 1 (check coerced_super_size or screenshot_super_size) and return a failure message (similar to the existing capture_source/action check). Target the capture_source handling inside manage_scene and validate coerced_super_size before setting params["captureSource"].Server/src/cli/commands/scene.py (1)
315-327:⚠️ Potential issue | 🟡 MinorReject
--scene-view-targetunless Scene View capture is selected.Line 361 forwards
sceneViewTargeteven when Line 327 keeps the defaultgame_view. That makesunity-mcp scene screenshot --scene-view-target ...silently do the wrong thing instead of failing fast.Suggested guard
def screenshot(filename: Optional[str], supersize: int, camera: Optional[str], include_image: bool, max_resolution: Optional[int], capture_source: str, batch: Optional[str], look_at: Optional[str], view_position: Optional[str], view_rotation: Optional[str], orbit_angles: Optional[int], orbit_elevations: Optional[str], orbit_distance: Optional[float], orbit_fov: Optional[float], output_dir: Optional[str], scene_view_target: Optional[str]): @@ config = get_config() + normalized_capture_source = capture_source.lower() + if scene_view_target and normalized_capture_source != "scene_view": + print_error("--scene-view-target requires --capture-source scene_view") + sys.exit(1) params: dict[str, Any] = {"action": "screenshot"} @@ - if capture_source: - params["captureSource"] = capture_source.lower() + params["captureSource"] = normalized_capture_source @@ if scene_view_target: params["sceneViewTarget"] = scene_view_targetAlso applies to: 361-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/cli/commands/scene.py` around lines 315 - 327, The current params building sends sceneViewTarget even when captureSource is the default game view; update the CLI validation so that when a user supplies scene_view_target (sceneViewTarget) we validate capture_source (captureSource) is set to 'scene_view' before adding it to params — if capture_source is not 'scene_view' and scene_view_target is present, raise a user-facing error/exit early; otherwise, add params["sceneViewTarget"] = scene_view_target only when capture_source.lower() == "scene_view". Ensure this check is performed near where params and capture_source are handled (the existing params dict construction and the code that currently adds sceneViewTarget).
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.cs (1)
104-117: Consider documenting the 75ms delay rationale.The
Thread.Sleep(RepaintSettlingDelayMs)blocks the editor thread. While acceptable for screenshot capture, a brief comment explaining why this delay is necessary (e.g., waiting for GPU rendering to complete) would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.cs` around lines 104 - 117, Add a brief inline comment above the Thread.Sleep(RepaintSettlingDelayMs) call in EditorWindowScreenshotUtility (the repaint block that calls sceneView.Repaint(), InvokeMethodIfExists(sceneView, "RepaintImmediately"), SceneView.RepaintAll(), UnityEditorInternal.InternalEditorUtility.RepaintAllViews(), EditorApplication.QueuePlayerLoopUpdate()) that explains the rationale for the 75ms delay (e.g., to allow the editor repaint and GPU/framebuffer swap to settle before capturing a screenshot) and note that it intentionally blocks the editor thread for a short period; include the constant name RepaintSettlingDelayMs in the comment so future maintainers understand why this specific delay is used and where to adjust it if needed.MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
835-849: Potential overflow when casting targetObjectId to long.
GlobalObjectId.targetObjectIdis aulong, and casting directly tolongcan produce negative values for IDs exceedinglong.MaxValue. While rare, this could cause matching failures if the fileID comparison involves such large values.Defensive approach using unchecked cast
private static long GetSpriteFileId(Sprite sprite) { if (sprite == null) return 0; try { var globalId = GlobalObjectId.GetGlobalObjectIdSlow(sprite); - return (long)globalId.targetObjectId; + return unchecked((long)globalId.targetObjectId); } catch { return 0; } }This makes the overflow explicit, though you should ensure the caller (
CoerceLong) also handles negative values consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 835 - 849, The cast of GlobalObjectId.targetObjectId (ulong) to long in GetSpriteFileId can overflow and produce negative IDs; update the cast to be explicit about overflow by using an unchecked cast (e.g., unchecked((long)globalId.targetObjectId)) so the wraparound is intentional, and keep the existing try/catch around GlobalObjectId.GetGlobalObjectIdSlow; also verify that the caller CoerceLong (and any code that compares file IDs) treats negative values consistently or adds normalization/clamping if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 643-666: The code currently falls through and calls
AssetDatabase.LoadAssetAtPath when fileID was present but no matching sprite was
found; change the behavior so that when jObj["fileID"] exists and
ParamCoercion.CoerceLong yields a non‑zero targetFileId but no sprite in
AssetDatabase.LoadAllAssetsAtPath matches GetSpriteFileId, the method should not
load the main asset — instead set prop.objectReferenceValue = null (or leave
unchanged) and return false and optionally log a clear error including the
targetFileId and path; update the logic around fileIdToken/targetFileId and the
foreach that uses GetSpriteFileId to detect the “not found” case and
early-return false rather than falling through to AssetDatabase.LoadAssetAtPath.
In `@MCPForUnity/Editor/Helpers/ParamCoercion.cs`:
- Around line 105-106: ParamCoercion.CoerceLong currently accepts a float parsed
by double.TryParse and truncates it to a long, which turns non-integral inputs
like "12.9" into 12; change the double.parse branches (the one around the
current TryParse at the top and the similar one around lines ~139-140) to only
succeed when the parsed double is mathematically integral and fits in Int64
(e.g., check that Math.Truncate(d) == d and d is between long.MinValue and
long.MaxValue) and otherwise treat it as a failed coercion; this prevents
CoerceLong from silently converting fractional values used for fileID (used by
ComponentOps.CoerceLong caller for fileID matching) into incorrect identifiers.
---
Outside diff comments:
In `@Server/src/cli/commands/scene.py`:
- Around line 315-327: The current params building sends sceneViewTarget even
when captureSource is the default game view; update the CLI validation so that
when a user supplies scene_view_target (sceneViewTarget) we validate
capture_source (captureSource) is set to 'scene_view' before adding it to params
— if capture_source is not 'scene_view' and scene_view_target is present, raise
a user-facing error/exit early; otherwise, add params["sceneViewTarget"] =
scene_view_target only when capture_source.lower() == "scene_view". Ensure this
check is performed near where params and capture_source are handled (the
existing params dict construction and the code that currently adds
sceneViewTarget).
In `@Server/src/services/tools/manage_scene.py`:
- Around line 186-199: manage_scene currently only checks action before
accepting capture_source, allowing calls like manage_scene(...,
action="screenshot", capture_source="scene_view", screenshot_super_size=2) to
bypass the new restriction; add a guard in the screenshot handling block that if
capture_source == "scene_view" then reject requests with a supersize > 1 (check
coerced_super_size or screenshot_super_size) and return a failure message
(similar to the existing capture_source/action check). Target the capture_source
handling inside manage_scene and validate coerced_super_size before setting
params["captureSource"].
---
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 835-849: The cast of GlobalObjectId.targetObjectId (ulong) to long
in GetSpriteFileId can overflow and produce negative IDs; update the cast to be
explicit about overflow by using an unchecked cast (e.g.,
unchecked((long)globalId.targetObjectId)) so the wraparound is intentional, and
keep the existing try/catch around GlobalObjectId.GetGlobalObjectIdSlow; also
verify that the caller CoerceLong (and any code that compares file IDs) treats
negative values consistently or adds normalization/clamping if needed.
In `@MCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.cs`:
- Around line 104-117: Add a brief inline comment above the
Thread.Sleep(RepaintSettlingDelayMs) call in EditorWindowScreenshotUtility (the
repaint block that calls sceneView.Repaint(), InvokeMethodIfExists(sceneView,
"RepaintImmediately"), SceneView.RepaintAll(),
UnityEditorInternal.InternalEditorUtility.RepaintAllViews(),
EditorApplication.QueuePlayerLoopUpdate()) that explains the rationale for the
75ms delay (e.g., to allow the editor repaint and GPU/framebuffer swap to settle
before capturing a screenshot) and note that it intentionally blocks the editor
thread for a short period; include the constant name RepaintSettlingDelayMs in
the comment so future maintainers understand why this specific delay is used and
where to adjust it if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c850cfbd-970b-4755-bb09-3cd0d8f39a16
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
AGENTS.mdMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.csMCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.cs.metaMCPForUnity/Editor/Helpers/ParamCoercion.csMCPForUnity/Editor/Setup/RoslynInstaller.csMCPForUnity/Editor/Tools/ManageScene.csServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/commands/scene.pyServer/src/services/tools/manage_scene.pyServer/tests/integration/test_manage_scene_screenshot_params.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.cs
Comment on lines
+643
to
666
| var fileIdToken = jObj["fileID"]; | ||
| if (fileIdToken != null) | ||
| { | ||
| long targetFileId = ParamCoercion.CoerceLong(fileIdToken, 0); | ||
| if (targetFileId != 0) | ||
| { | ||
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | ||
| foreach (var asset in allAssets) | ||
| { | ||
| if (asset is Sprite sprite) | ||
| { | ||
| long spriteFileId = GetSpriteFileId(sprite); | ||
| if (spriteFileId == targetFileId) | ||
| { | ||
| prop.objectReferenceValue = sprite; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| prop.objectReferenceValue = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path); | ||
| return true; |
Contributor
There was a problem hiding this comment.
Silent fallthrough when fileID doesn't match any sprite.
When fileID is provided but no sprite in the atlas matches, the code falls through to load the main asset at the path (line 665). This may return an unexpected asset type (e.g., the texture atlas itself) instead of failing with a clear error.
Consider returning an error when fileID is provided but not found:
Proposed fix
var fileIdToken = jObj["fileID"];
if (fileIdToken != null)
{
long targetFileId = ParamCoercion.CoerceLong(fileIdToken, 0);
if (targetFileId != 0)
{
var allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
foreach (var asset in allAssets)
{
if (asset is Sprite sprite)
{
long spriteFileId = GetSpriteFileId(sprite);
if (spriteFileId == targetFileId)
{
prop.objectReferenceValue = sprite;
return true;
}
}
}
+ error = $"Sprite with fileID {targetFileId} not found in atlas '{path}'.";
+ return false;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 643 - 666, The code
currently falls through and calls AssetDatabase.LoadAssetAtPath when fileID was
present but no matching sprite was found; change the behavior so that when
jObj["fileID"] exists and ParamCoercion.CoerceLong yields a non‑zero
targetFileId but no sprite in AssetDatabase.LoadAllAssetsAtPath matches
GetSpriteFileId, the method should not load the main asset — instead set
prop.objectReferenceValue = null (or leave unchanged) and return false and
optionally log a clear error including the targetFileId and path; update the
logic around fileIdToken/targetFileId and the foreach that uses GetSpriteFileId
to detect the “not found” case and early-return false rather than falling
through to AssetDatabase.LoadAssetAtPath.
Comment on lines
+105
to
+106
| if (double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var d)) | ||
| return (long)d; |
Contributor
There was a problem hiding this comment.
Reject fractional long inputs instead of truncating them.
These branches turn non-integral values into a different identifier ("12.9" → 12). The first caller uses CoerceLong for fileID matching in MCPForUnity/Editor/Helpers/ComponentOps.cs:643-663, so malformed input can resolve the wrong asset instead of being treated as invalid. Only convert when the parsed value is exactly integral.
Proposed fix
- if (double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var d))
- return (long)d;
+ if (double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var d) &&
+ !double.IsNaN(d) &&
+ !double.IsInfinity(d) &&
+ d >= long.MinValue &&
+ d <= long.MaxValue &&
+ d == Math.Truncate(d))
+ {
+ return (long)d;
+ }- if (double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var d))
- return (long)d;
+ if (double.TryParse(s, NumberStyles.Float, CultureInfo.InvariantCulture, out var d) &&
+ !double.IsNaN(d) &&
+ !double.IsInfinity(d) &&
+ d >= long.MinValue &&
+ d <= long.MaxValue &&
+ d == Math.Truncate(d))
+ {
+ return (long)d;
+ }Also applies to: 139-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ParamCoercion.cs` around lines 105 - 106,
ParamCoercion.CoerceLong currently accepts a float parsed by double.TryParse and
truncates it to a long, which turns non-integral inputs like "12.9" into 12;
change the double.parse branches (the one around the current TryParse at the top
and the similar one around lines ~139-140) to only succeed when the parsed
double is mathematically integral and fits in Int64 (e.g., check that
Math.Truncate(d) == d and d is between long.MinValue and long.MaxValue) and
otherwise treat it as a failed coercion; this prevents CoerceLong from silently
converting fractional values used for fileID (used by ComponentOps.CoerceLong
caller for fileID matching) into incorrect identifiers.
Author
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.
Adds
capture_sourcetomanage_scene screenshot, including support forscene_view.scene_view_targetto frame a target before Scene View capturecapture_source='scene_view'withsuper_size > 1to avoid unsupported metadata/behavior mismatchValidation:
cd Server && uv run python -m pytest tests/integration/test_manage_scene_screenshot_params.py -q16 passed in 0.20sScreenshot_SceneViewRejectsSupersizeAboveOneSummary by Sourcery
Add support for capturing screenshots from the Unity Scene View, extend object reference handling for sprites, and update tooling, docs, and tests accordingly.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
--capture-sourceand--scene-view-targetCLI options for controlling scene screenshot captureDocumentation
Tests