Update on skill/UI/camera#818
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…into animation
…ity-mcp into planner-executer
Remove files that don't belong in this camera/screenshot PR: - Scene generator pipeline (Server/src/scene_generator/*) - Manage3DGen tool (C# + Python) - Generated test scripts (TestProjects/*/Scripts/*) - Root-level docs and scripts (ProposedTable.md, system-prompt.md, start-scene-builder.*) - Revert MCPForUnity.Editor.asmdef and CLAUDE.md to beta Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewer's GuideExtends the Unity MCP scene and gameobject tools with richer camera/screenshot and look-at controls, strengthens material/VFX handling with render-pipeline-aware defaults, enhances object reference resolution and project info reporting, and updates docs/CLI/tests to support new workflows and inline image responses for AI vision. Sequence diagram for manage_scene screenshot with inline imagesequenceDiagram
actor User
participant Client as Client_or_CLI
participant Server as manage_scene_tool
participant Unity as ManageScene
participant SU as ScreenshotUtility
User->>Client: Request screenshot (include_image=true)
Client->>Server: Call manage_scene(action=screenshot,...)
Server->>Unity: HTTP RPC with params (includeImage,camera,maxResolution,...)
Unity->>Unity: HandleCommand parses SceneCommand
Unity->>Unity: CaptureScreenshot(SceneCommand)
alt batch_mode_surround
Unity->>Unity: CaptureSurroundBatch(cmd)
loop angles 6
Unity->>SU: RenderCameraToBase64(tempCamera,maxResolution)
SU-->>Unity: base64,width,height
end
Unity-->>Server: SuccessResponse(message,data.screenshots[*].imageBase64,...)
else single_camera_or_positioned
alt lookAt_or_viewPosition
Unity->>Unity: CapturePositionedScreenshot(cmd)
Unity->>SU: RenderCameraToBase64(tempCamera,maxResolution)
SU-->>Unity: base64,width,height
else camera_or_includeImage
Unity->>SU: CaptureFromCameraToAssetsFolder(camera,fileName,superSize,includeImage,maxResolution)
SU-->>Unity: ScreenshotCaptureResult(path,fullPath,imageBase64,...)
else default_screen_capture
Unity->>SU: CaptureToAssetsFolder(fileName,superSize)
SU-->>Unity: ScreenshotCaptureResult(path,fullPath,...)
end
Unity-->>Server: SuccessResponse(message,data.imageBase64?,data.path,...)
end
Server->>Server: _extract_images(response,action=screenshot)
alt response_contains_inline_images
Server-->>Client: ToolResult(TextContent+ImageContent blocks)
else no_inline_images
Server-->>Client: JSON {success,message,data}
end
Client-->>User: Display JSON and any inline images
Sequence diagram for manage_gameobject look_at actionsequenceDiagram
actor User
participant Client as Client_or_Agent
participant Server as manage_gameobject_tool
participant Unity as ManageGameObject
participant LookAt as GameObjectLookAt
User->>Client: Request manage_gameobject(action=look_at,...)
Client->>Server: Call manage_gameobject(action=look_at,target,look_at_target,look_at_up)
Server->>Server: Validate and coerce vector params
Server->>Unity: HTTP RPC with params
Unity->>Unity: HandleCommand parses action
Unity->>LookAt: Handle(params,targetToken,searchMethod)
LookAt->>Unity: ManageGameObjectCommon.FindObjectInternal(targetToken,searchMethod)
Unity-->>LookAt: targetGo or null
alt target_found
LookAt->>LookAt: VectorParsing.ParseVector3(look_at_target)
alt parsed_as_position
LookAt->>LookAt: use lookAtPos
else not_a_vector
LookAt->>Unity: ManageGameObjectCommon.FindObjectInternal(look_at_target,by_id_or_name_or_path)
Unity-->>LookAt: lookAtGo or null
end
alt look_at_resolved
LookAt->>LookAt: VectorParsing.ParseVector3OrDefault(look_at_up,Vector3.up)
LookAt->>Unity: Undo.RecordObject(targetGo.transform,...)
LookAt->>Unity: targetGo.transform.LookAt(lookAtPos,upVector)
LookAt-->>Unity: SuccessResponse(message,data{name,instanceID,rotation,lookAtPosition})
else look_at_invalid
LookAt-->>Unity: ErrorResponse("look_at_target ... could not be resolved")
end
else target_missing
LookAt-->>Unity: ErrorResponse("Target GameObject not found")
end
Unity-->>Server: SuccessResponse or ErrorResponse
Server-->>Client: JSON response
Client-->>User: Report rotation result or error
Class diagram for screenshot and camera utilitiesclassDiagram
class ManageScene {
+HandleCommand(params: JObject) object
+ExecuteScreenshot(fileName: string, superSize: int) object
-CaptureScreenshot(cmd: SceneCommand) object
-CaptureSurroundBatch(cmd: SceneCommand) object
-CapturePositionedScreenshot(cmd: SceneCommand) object
-ResolveCamera(cameraRef: string) Camera
-FrameSceneView(cmd: SceneCommand) object
}
class SceneCommand {
+action: string
+name: string
+path: string
+buildIndex: int?
+fileName: string
+superSize: int?
+camera: string
+includeImage: bool?
+maxResolution: int?
+batch: string
+lookAt: JToken
+viewPosition: Vector3?
+viewRotation: Vector3?
+sceneViewTarget: JToken
+parent: JToken
+pageSize: int?
+maxChildrenPerNode: int?
+maxChildrenPerBranch: int?
+includeTransform: bool?
}
class ScreenshotCaptureResult {
+FullPath: string
+AssetsRelativePath: string
+SuperSize: int
+IsAsync: bool
+ImageBase64: string
+ImageWidth: int
+ImageHeight: int
+ScreenshotCaptureResult(fullPath: string, assetsRelativePath: string, superSize: int)
+ScreenshotCaptureResult(fullPath: string, assetsRelativePath: string, superSize: int, isAsync: bool)
+ScreenshotCaptureResult(fullPath: string, assetsRelativePath: string, superSize: int, isAsync: bool, imageBase64: string, imageWidth: int, imageHeight: int)
}
class ScreenshotUtility {
+IsScreenCaptureModuleAvailable: bool
+CaptureFromCameraToAssetsFolder(camera: Camera, fileName: string, superSize: int, ensureUniqueFileName: bool, includeImage: bool, maxResolution: int) ScreenshotCaptureResult
+RenderCameraToBase64(camera: Camera, maxResolution: int) (string,int,int)
+DownscaleTexture(source: Texture2D, maxEdge: int) Texture2D
-DestroyTexture(tex: Texture2D) void
}
ManageScene o-- SceneCommand
ManageScene --> ScreenshotUtility
ScreenshotUtility --> ScreenshotCaptureResult
Class diagram for render pipeline aware material and VFX utilitiesclassDiagram
class RenderPipelineUtility {
<<static>>
+GetActivePipeline() PipelineKind
+GetOrCreateDefaultVFXMaterial(componentType: VFXComponentType) Material
+GetOrCreateDefaultSceneMaterial() Material
+IsMaterialInvalidForActivePipeline(material: Material, reason: string) bool
-ResolveDefaultVFXShader(pipeline: PipelineKind, componentType: VFXComponentType) Shader
-ResolveDefaultLitShader(pipeline: PipelineKind) Shader
-ResolveDefaultUnlitShader(pipeline: PipelineKind) Shader
-LooksLikeBuiltInShader(lowerName: string, shaderLooksSrp: bool) bool
-IsErrorShader(shader: Shader) bool
-IsPipelineMismatch(shaderName: string, activePipeline: PipelineKind) bool
}
class VFXComponentType {
ParticleSystem
LineRenderer
TrailRenderer
}
class PipelineKind {
BuiltIn
Universal
HighDefinition
Custom
}
class RendererHelpers {
<<static>>
+EnsureMaterial(renderer: Renderer) EnsureMaterialResult
+ApplyCommonRendererProperties(renderer: Renderer, params: JObject, changes: List~string~) void
+ApplyLineTrailProperties(params: JObject, changes: List~string~) void
-IsUsableMaterial(material: Material) bool
-ResolveReplacementMaterial(renderer: Renderer) Material
}
class EnsureMaterialResult {
+MaterialReplaced: bool
+ReplacementReason: string
+EnsureMaterialResult(materialReplaced: bool, replacementReason: string)
}
class ManageMaterial {
-SetRendererColor(params: JObject) object
-SetColorProperties(mat: Material, color: Color) void
-CreateUniqueAndAssign(renderer: Renderer, go: GameObject, color: Color, slot: int) object
}
class ParticleWrite {
+SetMain(params: JObject) object
+SetEmission(params: JObject) object
+SetShape(params: JObject) object
+SetColorOverLifetime(params: JObject) object
+SetSizeOverLifetime(params: JObject) object
+SetVelocityOverLifetime(params: JObject) object
+SetNoise(params: JObject) object
+SetRenderer(params: JObject) object
-EnsureParticleRendererMaterial(ps: ParticleSystem) ParticleSystemRenderer
}
class ParticleControl {
+Control(params: JObject, action: string) object
+AddBurst(params: JObject) object
}
RenderPipelineUtility --> VFXComponentType
RenderPipelineUtility --> PipelineKind
RendererHelpers --> EnsureMaterialResult
RendererHelpers --> RenderPipelineUtility
ManageMaterial --> RendererHelpers
ManageMaterial --> RenderPipelineUtility
ParticleWrite --> RendererHelpers
ParticleWrite --> RenderPipelineUtility
ParticleControl --> RendererHelpers
ParticleControl --> RenderPipelineUtility
Class diagram for project info and object reference resolutionclassDiagram
class ProjectInfo {
+HandleCommand(params: JObject) object
-GetActiveInputHandler() string
-IsPackageInstalled(packageName: string) bool
}
class ComponentOps {
-SetObjectReference(prop: SerializedProperty, value: JToken, error: string) bool
-ResolveSceneObjectByName(prop: SerializedProperty, name: string, error: string) bool
-FindChildProperty(root: SerializedProperty, name: string) SerializedProperty
}
class GameObjectLookup {
+SearchGameObjects(method: SearchMethod, query: string, includeInactive: bool, maxResults: int) List~int~
+FindById(id: int) GameObject
}
class SearchMethod {
<<enumeration>>
ByName
ByPath
ById
}
ProjectInfo --> RenderPipelineUtility
ComponentOps --> GameObjectLookup
GameObjectLookup --> SearchMethod
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds advanced screenshot modes (inline base64, camera-specific, positioned views, surround batch), a GameObject look_at action, pipeline-aware material validation and replacement reporting, scene-name object-reference resolution, enhanced project_info fields, array-based vector/quaternion JSON parsing, expanded UI/workflow docs, and tests and CLI/server parameter wiring for the new screenshot and look_at features. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant Editor
participant UnityRenderer as Renderer
Client->>Server: manage_scene(action="screenshot", batch="surround", include_image=true, max_resolution=512)
Server->>Editor: SendCommand(screenshot params)
Editor->>Editor: CaptureSurroundBatch() — compute center & radius
loop For each angle (6)
Editor->>Editor: Create/configure temporary Camera
Editor->>UnityRenderer: Render camera to RenderTexture
UnityRenderer-->>Editor: Texture data
Editor->>Editor: Downscale (if max_resolution)
Editor->>Editor: Encode PNG -> base64
end
Editor-->>Server: Response with imageBase64[] + metadata
Server->>Server: _extract_images -> ToolResult(TextContent, ImageContent[])
Server-->>Client: Return ToolResult with images and summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs" line_range="32-39" />
<code_context>
+ return new ErrorResponse("'look_at_target' parameter is required for 'look_at' action. Provide a world position [x,y,z] or a GameObject name/path/ID.");
+ }
+
+ // Try parsing as a position vector first
+ Vector3? lookAtPos = VectorParsing.ParseVector3(lookAtToken);
+ if (!lookAtPos.HasValue)
+ {
+ // Not a vector — treat as a GO reference
+ GameObject lookAtGo = ManageGameObjectCommon.FindObjectInternal(lookAtToken, "by_id_or_name_or_path");
+ if (lookAtGo == null)
+ {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The look_at_target search method is hardcoded and ignores the caller-provided searchMethod.
For `target` you honor the `searchMethod` passed into `Handle`, but for `look_at_target` you always call `FindObjectInternal` with the literal method string "by_id_or_name_or_path". This bypasses any custom or more restrictive search semantics the caller configured and can lead to ambiguous matches in larger scenes. Consider reusing the same `searchMethod` for `look_at_target` (when it’s a string/ID), or otherwise aligning this lookup with the rest of the tool’s GameObject search configuration.
```suggestion
// Try parsing as a position vector first
Vector3? lookAtPos = VectorParsing.ParseVector3(lookAtToken);
if (!lookAtPos.HasValue)
{
// Not a vector — treat as a GO reference, using the same search method as for the main target
GameObject lookAtGo = ManageGameObjectCommon.FindObjectInternal(lookAtToken, searchMethod);
if (lookAtGo == null)
{
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request significantly enhances the Unity MCP tools with advanced screenshot capabilities, camera controls, and improved object reference resolution. The changes enable AI assistants to better understand Unity scenes through inline base64 image support, batch multi-angle capture, and positioned camera views. Additional improvements include material/VFX pipeline compatibility checks, project capability detection, and enhanced component property wiring.
Changes:
- Extended screenshot functionality with inline image support, batch surround mode (6 angles), positioned camera capture, and scene view framing
- Added look_at action for GameObjects to orient objects toward targets or positions
- Improved object reference resolution to support name-based lookup ({"name": "ObjectName"}) as a fallback
- Enhanced project info resource to detect render pipeline, input system handler, and installed packages (uGUI, TextMeshPro, Input System)
- Refactored material/VFX handling with pipeline compatibility detection and transparent auto-assignment reporting
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/src/services/tools/manage_scene.py | Added _extract_images helper and new screenshot parameters (camera, include_image, max_resolution, batch, look_at, view_position, view_rotation, scene_view_target) |
| Server/src/services/tools/manage_gameobject.py | Added look_at action with look_at_target and look_at_up parameters |
| Server/src/cli/commands/scene.py | Extended screenshot CLI with new camera control options and vector parsing |
| Server/src/cli/commands/batch.py | Increased batch command limit from 25 to 40 |
| Server/tests/integration/test_manage_scene_screenshot_params.py | Comprehensive tests for new screenshot parameters and image extraction |
| Server/tests/integration/test_manage_gameobject_look_at.py | Integration tests for look_at action |
| Server/tests/integration/test_logging_stdout.py | Updated to allow print statements in test_pipeline.py |
| Server/tests/integration/conftest.py | Added stubs for ToolResult, TextContent, ImageContent, and ToolAnnotations |
| MCPForUnity/Editor/Tools/ManageScene.cs | Implemented batch surround capture, positioned screenshots, camera resolution, and scene view framing |
| MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | Added RenderCameraToBase64, DownscaleTexture methods and inline image support |
| MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs | New action to rotate GameObjects toward targets or positions |
| MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | Added array format support for Vector2/3/4 and Quaternion types |
| MCPForUnity/Editor/Helpers/ComponentOps.cs | Added ResolveSceneObjectByName for name-based object reference fallback |
| MCPForUnity/Editor/Resources/Project/ProjectInfo.cs | Added renderPipeline, activeInputHandler, and package detection fields |
| MCPForUnity/Editor/Helpers/RendererHelpers.cs | Refactored EnsureMaterial to return result struct with MaterialReplaced and ReplacementReason |
| MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs | Added IsMaterialInvalidForActivePipeline, IsErrorShader, IsPipelineMismatch, and LooksLikeBuiltInShader |
| MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs | Refactored to use EnsureParticleRendererMaterial helper and return material replacement info |
| MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs | Updated to return material replacement status in responses |
| MCPForUnity/Editor/Tools/ManageMaterial.cs | Added create_unique mode and SetColorProperties helper for persistent material assets |
| .gitignore | Added .env to prevent API key exposure |
| unity-mcp-skill/references/workflows.md | Extensive documentation updates for screenshot modes, UI creation with RectTransform sizing, Input System detection, and scene generator workflow |
| unity-mcp-skill/references/tools-reference.md | Updated with project info resource documentation and new screenshot/look_at parameters |
| unity-mcp-skill/SKILL.md | Updated screenshot examples with inline image and batch modes |
| .claude/skills/unity-mcp-skill/ | Mirrored documentation updates from unity-mcp-skill/ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if view_position: | ||
| parts = view_position.split(",") | ||
| if len(parts) == 3: | ||
| params["viewPosition"] = [float(p.strip()) for p in parts] | ||
| if view_rotation: | ||
| parts = view_rotation.split(",") | ||
| if len(parts) == 3: | ||
| params["viewRotation"] = [float(p.strip()) for p in parts] |
There was a problem hiding this comment.
The CLI screenshot command lacks error handling for invalid vector parsing. If view_position or view_rotation are provided with an invalid format (e.g., "abc,def,ghi"), the float() conversion will raise a ValueError that is not caught, causing the CLI to crash. Consider wrapping the parsing in try-except blocks similar to the look_at parameter handling.
| private static object CreateUniqueAndAssign(Renderer renderer, GameObject go, Color color, int slot) | ||
| { | ||
| string safeName = go.name.Replace(" ", "_"); | ||
| string matPath = $"Assets/Materials/{safeName}_mat.mat"; |
There was a problem hiding this comment.
The create_unique mode uses a simple naming pattern (GameObjectName_mat.mat) which could lead to collisions when multiple GameObjects have the same name. For example, if two different GameObjects named "Cube" exist in different parts of the scene hierarchy, they would both try to use "Assets/Materials/Cube_mat.mat". Consider including a unique identifier (like instance ID) in the material path, or checking if a material with the same name already exists but is assigned to a different GameObject, to prevent unintended material sharing between different objects.
| string matPath = $"Assets/Materials/{safeName}_mat.mat"; | |
| int instanceId = go.GetInstanceID(); | |
| string matPath = $"Assets/Materials/{safeName}_{instanceId}_mat.mat"; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
30-75: 🛠️ Refactor suggestion | 🟠 MajorConsider migrating new param parsing to ToolParams.
The expanded screenshot/scene_view_frame surface is still parsed via ad‑hoc
JObjectaccess. UsingToolParamswould give consistent validation (especially as parameters grow).
As per coding guidelines:MCPForUnity/Editor/Tools/*.cs: UseToolParamsfor consistent parameter validation in C# Editor tools with methods likeGetInt(),RequireString().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageScene.cs` around lines 30 - 75, The ToSceneCommand method currently parses many parameters directly from JObject into SceneCommand (camera, includeImage, maxResolution, batch, lookAt, viewPosition, viewRotation, sceneViewTarget, etc.); migrate this ad-hoc parsing to use the existing ToolParams helper so parameters get consistent validation and coercion. Update ToSceneCommand to construct a ToolParams from the incoming JObject and replace direct JObject access/ParamCoercion calls with ToolParams methods (e.g., GetInt/GetBool/GetString/RequireString/GetToken and the VectorParsing.ParseVector3 calls should consume ToolParams values), ensuring you use the same SceneCommand property names (camera, includeImage, maxResolution, batch, lookAt, viewPosition, viewRotation, sceneViewTarget) so behavior is unchanged but validation is centralized.
🧹 Nitpick comments (3)
MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
645-648: String fallback to name-based search — reasonable heuristic.Strings without
/and not starting withAssets/now fall through to scene name lookup. This aligns well with the JObjectnamepath and enables simpler reference syntax like"MyObject"as a string value.Worth noting: strings containing
/that aren't valid asset paths (e.g., scene hierarchy paths like"Canvas/Panel/Button") will still fail at lines 633-644 without reaching the name-based fallback. This is pre-existing behavior, but if hierarchy-path resolution is desired in the future, this would be the place to add it.🤖 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 645 - 648, Currently string values that don't start with "Assets/" fall back to ResolveSceneObjectByName, but strings containing '/' (e.g., "Canvas/Panel/Button") are rejected earlier and never reach that fallback; add a new resolution branch in the string handling path to detect non-asset strings containing '/' and attempt a hierarchy-path lookup (implement a helper like ResolveSceneObjectByHierarchyPath or extend ResolveSceneObjectByName to accept path syntax) before failing, ensuring you call that helper from the same location where the name fallback is invoked and return its result/out error consistently with existing behavior.unity-mcp-skill/references/workflows.md (1)
1-14: This file is a duplicate of.claude/skills/unity-mcp-skill/references/workflows.md.Both files contain identical content (1438 lines, 51372 bytes). Maintaining two copies increases the risk of documentation drift—edits to one won't automatically propagate to the other. Consider designating one as the canonical source and either symlinking or removing the other.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/references/workflows.md` around lines 1 - 14, This file is a duplicate of the document in .claude/skills/unity-mcp-skill/references/workflows.md; to fix, pick a canonical copy (either the current file or the one under .claude/skills), remove the redundant duplicate, and replace it with a symlink or update repo tooling to reference the canonical file; ensure references in README/TOC or any docs build config point to the chosen canonical file so edits are made in one place only.MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
141-152: ClarifymaxResolutiondefault behavior in docs.
The implementation treatsmaxResolution <= 0as 640, but the XML doc doesn’t mention this, which could surprise callers.💡 Doc tweak
- /// downscaled so the longest edge is at most <paramref name="maxResolution"/>). + /// downscaled so the longest edge is at most <paramref name="maxResolution"/>; uses 640 when <= 0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs` around lines 141 - 152, Update the XML doc for CaptureFromCameraToAssetsFolder to explicitly state that when maxResolution is provided as 0 or a negative value it defaults to 640 and that the image will be downscaled so the longest edge is at most that value when includeImage is true; reference the parameter name maxResolution and the method name CaptureFromCameraToAssetsFolder so callers know the implemented behavior matches the documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/unity-mcp-skill/references/workflows.md:
- Around line 1128-1131: Update the stale documentation references that say
"default 25 command limit per batch" to the current limit of 40: change the text
in the "Complete Example: Main Menu Screen" blurb (mentions project_info and
activeInputHandler) and update the other examples that chunk by 25 (the examples
around lines referenced in the review) so they chunk by 40 to match the
enforcement in batch.py; ensure any explanatory mentions of "25" are replaced
with "40" and keep the note that the limit is configurable in the Unity MCP
Tools window.
- Around line 92-102: The Python snippet passed to batch_execute uses JavaScript
booleans; update the properties dict in the manage_components call (the
{"properties": {"useGravity": false, "isKinematic": true}} entry) to use Python
booleans by replacing false with False and true with True so the batch_execute
invocation (and manage_components params) are valid Python.
In `@MCPForUnity/Editor/Tools/ManageMaterial.cs`:
- Around line 357-360: The new "create_unique" mode is handled ad-hoc by
checking mode string and using JObject parsing; refactor to use the existing
ToolParams-based parameter handling for consistency and validation by moving
mode parsing into the ToolParams flow (e.g., in the same place other modes are
handled) and calling the new branch to invoke CreateUniqueAndAssign(renderer,
go, color, slot) using ToolParams methods (RequireString(), GetInt(), etc.) to
extract and validate parameters instead of direct JObject access; update the
code path that currently returns CreateUniqueAndAssign in the method handling
mode to read parameters from a ToolParams instance and pass validated values to
CreateUniqueAndAssign.
- Around line 385-433: In CreateUniqueAndAssign, SanitizeAssetPath(matPath) can
return null and the current name-only path can collide for GameObjects with
identical names; first check the result of
AssetPathUtility.SanitizeAssetPath(matPath) and if it's null return an
ErrorResponse (or build a safe fallback path), and to avoid collisions append a
unique suffix (e.g., go.GetInstanceID() or a GUID) to the filename before
sanitization so matPath is unique; ensure you still create the Materials folder
if missing, then use the sanitized, non-null matPath for
LoadAssetAtPath/CreateAsset and the success response (refer to
CreateUniqueAndAssign, matPath, AssetPathUtility.SanitizeAssetPath,
go.GetInstanceID()).
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 530-531: The code uses "int maxRes = cmd.maxResolution ?? 480;"
which doesn't guard against callers passing zero or negative values; update the
assignment in the RenderCameraToBase64 path (and the similar occurrence around
the other block flagged at the same file) to clamp invalid values by treating
any maxResolution <= 0 as the default (480) — e.g., compute an effectiveMaxRes =
(cmd.maxResolution.HasValue && cmd.maxResolution.Value > 0) ?
cmd.maxResolution.Value : 480 and use effectiveMaxRes everywhere the current
maxRes is used (refer to the variable name maxRes and the RenderCameraToBase64
capture path to locate the code).
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 273-304: DownscaleTexture should validate its arguments: throw
ArgumentNullException if source is null, throw ArgumentOutOfRangeException if
maxEdge <= 0, and also guard against invalid source dimensions (source.width and
source.height must be > 0) before computing scale/dstW/dstH and calling
RenderTexture.GetTemporary/Graphics.Blit/Texture2D creation; add these checks at
the top of DownscaleTexture so you never create a zero-sized RenderTexture or
return a surprising 1×1 result.
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 28-30: The unsafe direct cast "JObject jo = (JObject)token" in the
Unity converters (e.g., the Vector3 conversion branch shown, and likewise in the
other three converters at the referenced sites) should be replaced with a safe
"as" cast and a null-check that throws a descriptive JsonSerializationException;
specifically, change the fallback to "JObject jo = token as JObject" and if jo
is null throw a JsonSerializationException that includes the target Unity type
(e.g., Vector3), the offending token's Type and string value
(token?.ToString()), and the field/property context if available so under-length
JArray or non-object tokens produce a clear, debuggable error instead of
InvalidCastException.
In `@Server/src/cli/commands/batch.py`:
- Around line 60-62: Docs still mention a 25-command batch limit while the CLI
enforces 40; update unity-mcp-skill/references/workflows.md to replace the
hardcoded "25" references with "40" in the locations called out (the "Execute in
batches of 25" text, the example call "batch_execute(commands=commands[:25],
parallel=True)", the "default 25 command limit per batch" line, and the
loop/slice steps that use 25) so the documentation matches the CLI check in
batch.py (see the len(commands) > 40 check and its error message using
print_error).
In `@Server/src/cli/commands/scene.py`:
- Around line 297-305: The batch-success branch currently skips emitting the
formatted CLI output, which hides payloads and breaks scripting; after calling
run_command("manage_scene", params, config) always emit the standard formatted
output by calling format_output(result, config.format) (e.g., via click.echo or
the existing output path), then if batch and result.get("success") print the
batch summary via print_success with the screenshot count; update the block
around run_command/format_output/print_success so the formatted output is
unconditionally emitted and the batch summary is printed afterwards when
appropriate.
- Around line 288-295: The current parsing of view_position and view_rotation in
scene.py silently ignores invalid formats; update the code that handles
view_position and view_rotation so that if parts length != 3 or any part fails
float conversion you raise a clear CLI error (e.g., raise click.BadParameter or
a ValueError handled by the caller) with the flag name and the offending value;
ensure you still set params["viewPosition"] and params["viewRotation"] to
[float(...)] only on successful parsing and include the exact input in the error
message to aid debugging.
In `@Server/tests/integration/test_manage_gameobject_look_at.py`:
- Around line 12-14: The lint error is caused by unused parameters in the async
fake_send functions; rename the unused positional and kw args to indicate unused
variables (e.g., change cmd to _cmd and **kwargs to **_kwargs) while leaving
used parameters like params intact; update every fake_send definition (including
the other occurrences) to use _cmd and **_kwargs so Ruff ARg001 is satisfied.
- Around line 69-77: The test assigns the result of
manage_go_mod.manage_gameobject to resp but never uses it (Ruff F841); either
remove the unused assignment or add an assertion about the response to make the
test meaningful. Modify the call to manage_gameobject (in the test function
using DummyContext and variables captured and p) to either call await
manage_go_mod.manage_gameobject(...) without assigning to resp, or keep resp and
add an assertion such as asserting resp is not None or matches the expected
structure/value returned by manage_gameobject before the existing checks on
captured["params"].
In `@Server/tests/integration/test_manage_scene_screenshot_params.py`:
- Around line 119-126: The test assigns the result of
manage_scene_mod.manage_scene to an unused variable resp which triggers Ruff;
change the two occurrences (the await calls using DummyContext with
action="screenshot", camera="MainCamera", include_image=True,
max_resolution=256) to either await the coroutine without assignment (await
manage_scene_mod.manage_scene(...)) or assign to a throwaway variable (e.g., _ =
await manage_scene_mod.manage_scene(...)) or make a minimal assertion on the
returned value so resp is used; update both occurrences mentioned (the current
one and the one around lines 145-151).
- Around line 113-115: Several mocked async functions named fake_send capture
only params but leave other args unused, causing lint warnings; update each
fake_send signature to prefix unused arguments with an underscore (e.g., change
(cmd, params, **kwargs) to (_cmd, params, **_kwargs) or remove kwargs if not
used) and do the same for the other fake_send occurrences referenced (the ones
around the captured dict at the other locations) so only used names remain
(params and captured) to silence linters.
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 112-118: The example for manage_scene shows max_resolution
defaulting to 512 but the implementation and parameter docs use 640; update the
example to match the actual default by changing the inline comment for
max_resolution to 640 (or vice versa if you intend to change the
implementation), and ensure the manage_scene parameter documentation and any
other examples consistently state max_resolution default=640 so references to
manage_scene and its max_resolution parameter are aligned.
---
Outside diff comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 30-75: The ToSceneCommand method currently parses many parameters
directly from JObject into SceneCommand (camera, includeImage, maxResolution,
batch, lookAt, viewPosition, viewRotation, sceneViewTarget, etc.); migrate this
ad-hoc parsing to use the existing ToolParams helper so parameters get
consistent validation and coercion. Update ToSceneCommand to construct a
ToolParams from the incoming JObject and replace direct JObject
access/ParamCoercion calls with ToolParams methods (e.g.,
GetInt/GetBool/GetString/RequireString/GetToken and the
VectorParsing.ParseVector3 calls should consume ToolParams values), ensuring you
use the same SceneCommand property names (camera, includeImage, maxResolution,
batch, lookAt, viewPosition, viewRotation, sceneViewTarget) so behavior is
unchanged but validation is centralized.
---
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 645-648: Currently string values that don't start with "Assets/"
fall back to ResolveSceneObjectByName, but strings containing '/' (e.g.,
"Canvas/Panel/Button") are rejected earlier and never reach that fallback; add a
new resolution branch in the string handling path to detect non-asset strings
containing '/' and attempt a hierarchy-path lookup (implement a helper like
ResolveSceneObjectByHierarchyPath or extend ResolveSceneObjectByName to accept
path syntax) before failing, ensuring you call that helper from the same
location where the name fallback is invoked and return its result/out error
consistently with existing behavior.
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 141-152: Update the XML doc for CaptureFromCameraToAssetsFolder to
explicitly state that when maxResolution is provided as 0 or a negative value it
defaults to 640 and that the image will be downscaled so the longest edge is at
most that value when includeImage is true; reference the parameter name
maxResolution and the method name CaptureFromCameraToAssetsFolder so callers
know the implemented behavior matches the documentation.
In `@unity-mcp-skill/references/workflows.md`:
- Around line 1-14: This file is a duplicate of the document in
.claude/skills/unity-mcp-skill/references/workflows.md; to fix, pick a canonical
copy (either the current file or the one under .claude/skills), remove the
redundant duplicate, and replace it with a symlink or update repo tooling to
reference the canonical file; ensure references in README/TOC or any docs build
config point to the chosen canonical file so edits are made in one place only.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.claude/skills/unity-mcp-skill/SKILL.md.claude/skills/unity-mcp-skill/references/tools-reference.md.claude/skills/unity-mcp-skill/references/workflows.md.gitignoreMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/RenderPipelineUtility.csMCPForUnity/Editor/Helpers/RendererHelpers.csMCPForUnity/Editor/Resources/Project/ProjectInfo.csMCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.csMCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs.metaMCPForUnity/Editor/Tools/GameObjects/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.csMCPForUnity/Editor/Tools/Vfx/ParticleWrite.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csServer/src/cli/commands/batch.pyServer/src/cli/commands/scene.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_scene.pyServer/tests/integration/conftest.pyServer/tests/integration/test_logging_stdout.pyServer/tests/integration/test_manage_gameobject_look_at.pyServer/tests/integration/test_manage_scene_screenshot_params.pyunity-mcp-skill/SKILL.mdunity-mcp-skill/references/tools-reference.mdunity-mcp-skill/references/workflows.md
| batch_execute(commands=[ | ||
| {"tool": "manage_components", "params": { | ||
| "action": "add", "target": "Bee_1", "component_type": "Rigidbody" | ||
| }}, | ||
| {"tool": "manage_components", "params": { | ||
| "action": "set_property", "target": "Bee_1", | ||
| "component_type": "Rigidbody", | ||
| "properties": {"useGravity": false, "isKinematic": true} | ||
| }} | ||
| ]) | ||
| ``` |
There was a problem hiding this comment.
Python syntax error: false/true should be False/True.
Line 99 uses JavaScript-style booleans (false, true) inside a Python code block. Python requires capitalized False and True.
📝 Proposed fix
- "properties": {"useGravity": false, "isKinematic": true}
+ "properties": {"useGravity": False, "isKinematic": True}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/unity-mcp-skill/references/workflows.md around lines 92 -
102, The Python snippet passed to batch_execute uses JavaScript booleans; update
the properties dict in the manage_components call (the {"properties":
{"useGravity": false, "isKinematic": true}} entry) to use Python booleans by
replacing false with False and true with True so the batch_execute invocation
(and manage_components params) are valid Python.
| ### Complete Example: Main Menu Screen | ||
|
|
||
| Combines multiple templates into a full menu screen in two batch calls (default 25 command limit per batch, configurable in Unity MCP Tools window up to 100). | ||
| Combines multiple templates into a full menu screen in two batch calls (default 25 command limit per batch, configurable in Unity MCP Tools window up to 100). **Assumes `project_info` has been read and `activeInputHandler` is known.** | ||
|
|
There was a problem hiding this comment.
Stale batch limit reference — says 25, should be 40.
Line 1130 says "default 25 command limit per batch" but batch.py now enforces a limit of 40. Similarly, the examples at lines 180–181 and 1346 chunk by 25.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/unity-mcp-skill/references/workflows.md around lines 1128 -
1131, Update the stale documentation references that say "default 25 command
limit per batch" to the current limit of 40: change the text in the "Complete
Example: Main Menu Screen" blurb (mentions project_info and activeInputHandler)
and update the other examples that chunk by 25 (the examples around lines
referenced in the review) so they chunk by 40 to match the enforcement in
batch.py; ensure any explanatory mentions of "25" are replaced with "40" and
keep the note that the limit is configurable in the Unity MCP Tools window.
| async def fake_send(cmd, params, **kwargs): | ||
| captured["params"] = params | ||
| return {"success": True, "data": {"rotation": [0, 90, 0]}} |
There was a problem hiding this comment.
Ruff ARg001: mark unused fake_send args.
Rename unused parameters to _cmd/**_kwargs to satisfy linting.
Proposed fix
- async def fake_send(cmd, params, **kwargs):
+ async def fake_send(_cmd, params, **_kwargs):
@@
- async def fake_send(cmd, params, **kwargs):
+ async def fake_send(_cmd, params, **_kwargs):
@@
- async def fake_send(cmd, params, **kwargs):
+ async def fake_send(_cmd, params, **_kwargs):Also applies to: 37-39, 63-65
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 12-12: Unused function argument: cmd
(ARG001)
[warning] 12-12: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/integration/test_manage_gameobject_look_at.py` around lines 12 -
14, The lint error is caused by unused parameters in the async fake_send
functions; rename the unused positional and kw args to indicate unused variables
(e.g., change cmd to _cmd and **kwargs to **_kwargs) while leaving used
parameters like params intact; update every fake_send definition (including the
other occurrences) to use _cmd and **_kwargs so Ruff ARg001 is satisfied.
| resp = await manage_go_mod.manage_gameobject( | ||
| ctx=DummyContext(), | ||
| action="look_at", | ||
| target="MainCamera", | ||
| ) | ||
|
|
||
| p = captured["params"] | ||
| assert p["action"] == "look_at" | ||
| assert "look_at_target" not in p |
There was a problem hiding this comment.
Ruff F841: resp is unused.
Either drop the assignment or assert on the response to keep the test meaningful.
Proposed fix
- resp = await manage_go_mod.manage_gameobject(
+ resp = await manage_go_mod.manage_gameobject(
ctx=DummyContext(),
action="look_at",
target="MainCamera",
)
+ assert resp.get("success") is False📝 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.
| resp = await manage_go_mod.manage_gameobject( | |
| ctx=DummyContext(), | |
| action="look_at", | |
| target="MainCamera", | |
| ) | |
| p = captured["params"] | |
| assert p["action"] == "look_at" | |
| assert "look_at_target" not in p | |
| resp = await manage_go_mod.manage_gameobject( | |
| ctx=DummyContext(), | |
| action="look_at", | |
| target="MainCamera", | |
| ) | |
| assert resp.get("success") is False | |
| p = captured["params"] | |
| assert p["action"] == "look_at" | |
| assert "look_at_target" not in p |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 69-69: Local variable resp is assigned to but never used
Remove assignment to unused variable resp
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/integration/test_manage_gameobject_look_at.py` around lines 69 -
77, The test assigns the result of manage_go_mod.manage_gameobject to resp but
never uses it (Ruff F841); either remove the unused assignment or add an
assertion about the response to make the test meaningful. Modify the call to
manage_gameobject (in the test function using DummyContext and variables
captured and p) to either call await manage_go_mod.manage_gameobject(...)
without assigning to resp, or keep resp and add an assertion such as asserting
resp is not None or matches the expected structure/value returned by
manage_gameobject before the existing checks on captured["params"].
| async def fake_send(cmd, params, **kwargs): | ||
| captured["params"] = params | ||
| return {"success": True, "data": {"filePath": "Assets/shot.png"}} |
There was a problem hiding this comment.
Silence unused fake_send arguments to keep lint clean.
Prefix unused args with _ (or remove where possible) across the mocked send functions.
🧼 Example fix
- async def fake_send(cmd, params, **kwargs):
+ async def fake_send(_cmd, params, **_kwargs):
captured["params"] = params
return {"success": True, "data": {"filePath": "Assets/shot.png"}}Also applies to: 139-141, 163-165, 191-193, 215-217, 236-237
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 113-113: Unused function argument: cmd
(ARG001)
[warning] 113-113: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/integration/test_manage_scene_screenshot_params.py` around lines
113 - 115, Several mocked async functions named fake_send capture only params
but leave other args unused, causing lint warnings; update each fake_send
signature to prefix unused arguments with an underscore (e.g., change (cmd,
params, **kwargs) to (_cmd, params, **_kwargs) or remove kwargs if not used) and
do the same for the other fake_send occurrences referenced (the ones around the
captured dict at the other locations) so only used names remain (params and
captured) to silence linters.
| resp = await manage_scene_mod.manage_scene( | ||
| ctx=DummyContext(), | ||
| action="screenshot", | ||
| camera="MainCamera", | ||
| include_image=True, | ||
| max_resolution=256, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Remove unused resp assignments to satisfy Ruff.
🧹 Suggested cleanup
- resp = await manage_scene_mod.manage_scene(
+ await manage_scene_mod.manage_scene(
ctx=DummyContext(),
action="screenshot",
camera="MainCamera",
include_image=True,
max_resolution=256,
)- resp = await manage_scene_mod.manage_scene(
+ await manage_scene_mod.manage_scene(
ctx=DummyContext(),
action="screenshot",
batch="surround",
max_resolution=128,
)Also applies to: 145-151
🧰 Tools
🪛 Ruff (0.15.2)
[error] 119-119: Local variable resp is assigned to but never used
Remove assignment to unused variable resp
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/integration/test_manage_scene_screenshot_params.py` around lines
119 - 126, The test assigns the result of manage_scene_mod.manage_scene to an
unused variable resp which triggers Ruff; change the two occurrences (the await
calls using DummyContext with action="screenshot", camera="MainCamera",
include_image=True, max_resolution=256) to either await the coroutine without
assignment (await manage_scene_mod.manage_scene(...)) or assign to a throwaway
variable (e.g., _ = await manage_scene_mod.manage_scene(...)) or make a minimal
assertion on the returned value so resp is used; update both occurrences
mentioned (the current one and the one around lines 145-151).
| # Screenshot with inline image (base64 PNG returned to AI) | ||
| manage_scene( | ||
| action="screenshot", | ||
| camera="MainCamera", # str, optional - camera name, path, or instance ID | ||
| include_image=True, # bool, default False - return base64 PNG inline | ||
| max_resolution=512 # int, optional - downscale cap (default 512) | ||
| ) |
There was a problem hiding this comment.
Doc default for max_resolution appears mismatched.
The tool defaults to 640px (per parameter docs and implementation), but this example says default 512. Aligning the doc avoids confusion.
✏️ Suggested doc fix
- max_resolution=512 # int, optional - downscale cap (default 512)
+ max_resolution=512 # int, optional - downscale cap (default 640)📝 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.
| # Screenshot with inline image (base64 PNG returned to AI) | |
| manage_scene( | |
| action="screenshot", | |
| camera="MainCamera", # str, optional - camera name, path, or instance ID | |
| include_image=True, # bool, default False - return base64 PNG inline | |
| max_resolution=512 # int, optional - downscale cap (default 512) | |
| ) | |
| # Screenshot with inline image (base64 PNG returned to AI) | |
| manage_scene( | |
| action="screenshot", | |
| camera="MainCamera", # str, optional - camera name, path, or instance ID | |
| include_image=True, # bool, default False - return base64 PNG inline | |
| max_resolution=512 # int, optional - downscale cap (default 640) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@unity-mcp-skill/references/tools-reference.md` around lines 112 - 118, The
example for manage_scene shows max_resolution defaulting to 512 but the
implementation and parameter docs use 640; update the example to match the
actual default by changing the inline comment for max_resolution to 640 (or vice
versa if you intend to change the implementation), and ensure the manage_scene
parameter documentation and any other examples consistently state max_resolution
default=640 so references to manage_scene and its max_resolution parameter are
aligned.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Server/src/services/tools/manage_scene.py (3)
217-220: Guard_extract_imagescall behind an action check.
_extract_imagesis invoked on every successful response regardless of action, but it short-circuits immediately for all non-screenshotactions (line 64:return None). An explicit guard communicates intent and avoids the unnecessary call overhead forget_hierarchy,get_active,save, etc.♻️ Proposed fix
- image_result = _extract_images(response, action) - if image_result is not None: - return image_result + if action == "screenshot": + image_result = _extract_images(response, action) + if image_result is not None: + return image_result🤖 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 217 - 220, The code currently calls _extract_images(response, action) for every successful response; change this to only call it when action == "screenshot" (or whatever screenshot action constant is used) to avoid unnecessary work for actions like get_hierarchy, get_active, save, etc. Locate the response handling return path that currently does image_result = _extract_images(response, action) / if image_result is not None: return image_result and wrap or replace that call with an explicit conditional (e.g., if action == "screenshot": image_result = _extract_images(...)) so _extract_images is only invoked for screenshot actions.
157-158: Add an upper bound tomax_resolutionvalidation.The guard only rejects non-positive values. An arbitrarily large value (e.g.,
99999) is accepted and forwarded to Unity'sDownscaleTexture, which could trigger excessive memory allocation during render.♻️ Proposed fix
- 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."} + _MAX_RESOLUTION = 4096 + if coerced_max_resolution is not None and not (0 < coerced_max_resolution <= _MAX_RESOLUTION): + return {"success": False, "message": f"max_resolution must be between 1 and {_MAX_RESOLUTION}."}🤖 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 157 - 158, The validation for coerced_max_resolution only rejects non-positive values; add an upper bound check (e.g., define a module-level constant MAX_TEXTURE_RESOLUTION) and reject values greater than that to prevent excessive memory allocation when passed to Unity's DownscaleTexture; update the guard in the function that performs this validation (look for the existing check using coerced_max_resolution) to return {"success": False, "message": "max_resolution must be between 1 and <MAX_TEXTURE_RESOLUTION>."} when coerced_max_resolution > MAX_TEXTURE_RESOLUTION, and ensure the constant name (MAX_TEXTURE_RESOLUTION) and the validation location (the function that currently returns the non-positive error) are used so reviewers can find the change.
29-49: Consolidate the two-pass iteration overscreenshotsinto a single loop.
screenshotsis walked twice: once (lines 32–33) to buildsummary_screenshots, and again (lines 44–48) to emitImageContentblocks. A single pass eliminates the redundant traversal and keeps the logic cohesive.♻️ Proposed refactor
blocks: list[TextContent | ImageContent] = [] summary_screenshots = [] - for s in screenshots: - summary_screenshots.append({k: v for k, v in s.items() if k != "imageBase64"}) - text_result = { - "success": True, - "message": response.get("message", ""), - "data": { - "sceneCenter": data.get("sceneCenter"), - "sceneRadius": data.get("sceneRadius"), - "screenshots": summary_screenshots, - }, - } - blocks.append(TextContent(type="text", text=json.dumps(text_result))) - for s in screenshots: - b64 = s.get("imageBase64") - if b64: - blocks.append(TextContent(type="text", text=f"[Angle: {s.get('angle', '?')}]")) - blocks.append(ImageContent(type="image", data=b64, mimeType="image/png")) + image_blocks: list[TextContent | ImageContent] = [] + for s in screenshots: + summary_screenshots.append({k: v for k, v in s.items() if k != "imageBase64"}) + b64 = s.get("imageBase64") + if b64: + image_blocks.append(TextContent(type="text", text=f"[Angle: {s.get('angle', '?')}]")) + image_blocks.append(ImageContent(type="image", data=b64, mimeType="image/png")) + text_result = { + "success": True, + "message": response.get("message", ""), + "data": { + "sceneCenter": data.get("sceneCenter"), + "sceneRadius": data.get("sceneRadius"), + "screenshots": summary_screenshots, + }, + } + blocks.append(TextContent(type="text", text=json.dumps(text_result))) + blocks.extend(image_blocks)🤖 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 29 - 49, The code currently iterates screenshots twice; refactor to a single loop that both builds summary_screenshots and appends per-screenshot blocks (angle text and ImageContent) in one pass: initialize blocks and summary_screenshots, loop through screenshots once collecting summary entries and, for each screenshot with imageBase64, append the angle TextContent and ImageContent to blocks; after the loop construct text_result (using summary_screenshots, sceneCenter, sceneRadius, response.get) and insert the TextContent(json.dumps(text_result)) at the beginning of blocks, then return ToolResult(content=blocks). Ensure you update the existing variables TextContent, ImageContent, summary_screenshots, text_result, and ToolResult accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/services/tools/manage_scene.py`:
- Line 5: Replace the internal import of ToolResult in manage_scene.py: change
the import statement that currently reads "from fastmcp.server.server import
ToolResult" to use the public API by importing ToolResult from
fastmcp.tools.tool; update any import grouping or linter adjustments accordingly
so all references to ToolResult in this file remain unchanged but now come from
the public fastmcp.tools.tool path.
- Around line 117-120: The view_position/view_rotation parameters accept str but
are forwarded raw to Unity where VectorParsing.ParseVector3 only accepts
JArray/JObject, so string tokens like "1.0,2.0,3.0" silently fail; fix by
parsing string-encoded vectors in manage_scene.py before sending to Unity: add a
small helper (e.g., parse_vector_string(s)) used in the code paths that handle
view_position and view_rotation to convert "x,y,z" or "x y z" or JSON array
strings into list[float], validate length==3 and numeric values, and
raise/return a clear error if invalid; also update the type hints/signatures for
view_position and view_rotation (or document them) to reflect that only numeric
3-element lists will be sent to Unity if you prefer stricter typing.
---
Nitpick comments:
In `@Server/src/services/tools/manage_scene.py`:
- Around line 217-220: The code currently calls _extract_images(response,
action) for every successful response; change this to only call it when action
== "screenshot" (or whatever screenshot action constant is used) to avoid
unnecessary work for actions like get_hierarchy, get_active, save, etc. Locate
the response handling return path that currently does image_result =
_extract_images(response, action) / if image_result is not None: return
image_result and wrap or replace that call with an explicit conditional (e.g.,
if action == "screenshot": image_result = _extract_images(...)) so
_extract_images is only invoked for screenshot actions.
- Around line 157-158: The validation for coerced_max_resolution only rejects
non-positive values; add an upper bound check (e.g., define a module-level
constant MAX_TEXTURE_RESOLUTION) and reject values greater than that to prevent
excessive memory allocation when passed to Unity's DownscaleTexture; update the
guard in the function that performs this validation (look for the existing check
using coerced_max_resolution) to return {"success": False, "message":
"max_resolution must be between 1 and <MAX_TEXTURE_RESOLUTION>."} when
coerced_max_resolution > MAX_TEXTURE_RESOLUTION, and ensure the constant name
(MAX_TEXTURE_RESOLUTION) and the validation location (the function that
currently returns the non-positive error) are used so reviewers can find the
change.
- Around line 29-49: The code currently iterates screenshots twice; refactor to
a single loop that both builds summary_screenshots and appends per-screenshot
blocks (angle text and ImageContent) in one pass: initialize blocks and
summary_screenshots, loop through screenshots once collecting summary entries
and, for each screenshot with imageBase64, append the angle TextContent and
ImageContent to blocks; after the loop construct text_result (using
summary_screenshots, sceneCenter, sceneRadius, response.get) and insert the
TextContent(json.dumps(text_result)) at the beginning of blocks, then return
ToolResult(content=blocks). Ensure you update the existing variables
TextContent, ImageContent, summary_screenshots, text_result, and ToolResult
accordingly.
|
|
||
| from fastmcp import Context | ||
| from mcp.types import ToolAnnotations | ||
| from fastmcp.server.server import ToolResult |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
fastmcp 2.14.0 ToolResult import path documentation
💡 Result:
In FastMCP v2.14.0, ToolResult is documented as being imported from:
from fastmcp.tools.tool import ToolResultThis import path appears in the official “Tools” docs section describing “ToolResult and Metadata.” [1] It’s also reflected in the module-level documentation for fastmcp.tools.tool, which lists ToolResult as a class in that module. [2]
Sources: [1], [2]
Use the public fastmcp.tools.tool import path for ToolResult.
The current import from fastmcp.server.server import ToolResult points to an internal module. The fastmcp 2.14.0 documentation specifies the canonical import as from fastmcp.tools.tool import ToolResult. Internal module paths are not part of the public API and can break on minor version updates.
🔧 Proposed fix
-from fastmcp.server.server import ToolResult
+from fastmcp.tools.tool import ToolResult🤖 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` at line 5, Replace the internal
import of ToolResult in manage_scene.py: change the import statement that
currently reads "from fastmcp.server.server import ToolResult" to use the public
API by importing ToolResult from fastmcp.tools.tool; update any import grouping
or linter adjustments accordingly so all references to ToolResult in this file
remain unchanged but now come from the public fastmcp.tools.tool path.
…wonder/unity-mcp into update-on-skill/ui/camera
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Server/src/services/tools/manage_scene.py (1)
4-10: Use the public ToolResult import path (already raised previously).
Still importingToolResultfrom an internal module. Please confirm the current public import path in fastmcp docs and switch if needed to avoid breakage on minor updates.🔧 Proposed change
-from fastmcp.server.server import ToolResult +from fastmcp.tools.tool import ToolResultfastmcp ToolResult public import path documentation🤖 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 4 - 10, The code currently imports ToolResult from an internal module ("from fastmcp.server.server import ToolResult"); update the import to use fastmcp's public export for ToolResult per the library docs (replace the internal path with the documented public path), e.g., change the import statement that brings in ToolResult in manage_scene.py to the official public import; confirm the correct public path in the fastmcp docs and update any usages of ToolResult in this file accordingly.unity-mcp-skill/references/tools-reference.md (1)
112-118: Pastmax_resolutiondefault mismatch — resolved in current code.The inline comment now correctly reads
(default 640), matching the implementation. No further action needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/references/tools-reference.md` around lines 112 - 118, The inline comment for manage_scene's max_resolution parameter was previously wrong but now correctly reads "(default 640)"; no code changes are required—confirm the documentation string or comment in the manage_scene definition (function name manage_scene and parameter max_resolution) matches the implementation default and leave as-is.MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
278-309: Past validation concerns onDownscaleTexture— addressed in current code.Both guards (
ArgumentNullExceptionfor null source,ArgumentOutOfRangeExceptionformaxEdge ≤ 0) are now present. The zero-dimension edge case is handled implicitly byMathf.Max(1, ...)throughout the scale computation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs` around lines 278 - 309, The review note about past validation concerns is redundant because DownscaleTexture already checks for null source and maxEdge > 0 and protects against zero dimensions via Mathf.Max; remove the duplicate comment or collapse it into a single concise note referencing DownscaleTexture's existing ArgumentNullException and ArgumentOutOfRangeException guards to avoid clutter in the review thread.
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
114-123: Consider applying the same JArray/safe-token pattern toColorConverterfor consistency.The four updated converters now gracefully handle array inputs and produce descriptive errors for unexpected token types.
ColorConverterstill usesJObject.Load(reader)directly, which will throw an opaque error on non-object input. Since[r, g, b, a]is a natural array representation for colors, applying the same pattern here would keep the converters uniform.♻️ Proposed change
public override Color ReadJson(JsonReader reader, Type objectType, Color existingValue, bool hasExistingValue, JsonSerializer serializer) { - JObject jo = JObject.Load(reader); + JToken token = JToken.Load(reader); + if (token is JArray arr && arr.Count >= 4) + return new Color((float)arr[0], (float)arr[1], (float)arr[2], (float)arr[3]); + if (token is not JObject jo) + throw new JsonSerializationException($"Cannot deserialize Color from {token.Type}: '{token}'"); return new Color( (float)jo["r"], (float)jo["g"], (float)jo["b"], (float)jo["a"] ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs` around lines 114 - 123, The Color converter's ReadJson currently uses JObject.Load(reader) which throws on non-object tokens; update the ColorConverter.ReadJson to mirror the other converters by first checking reader.TokenType and handling JArray input (accept [r,g,b,a] and map to new Color) and also reading JObject for property form, and throw a descriptive JsonSerializationException for other token types; reference the ReadJson override in the ColorConverter (the method constructing new Color from jo["r"] etc.) and reuse the same safe-token/JArray parsing and error message style used in the other converters.
🤖 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/Tools/ManageMaterial.cs`:
- Around line 427-434: Move the slot bounds check to before any asset creation
or AssetDatabase.SaveAssets() call so you don't write an orphaned material when
the slot is invalid: validate that 'slot' is within
0..renderer.sharedMaterials.Length-1 at the start of the method (before creating
or saving the Material and before calling AssetDatabase.SaveAssets()), and
return the existing ErrorResponse($"Slot {slot} out of bounds (count:
{sharedMats.Length})") immediately if invalid; keep references to
renderer.sharedMaterials, the 'slot' parameter, and the existing ErrorResponse
to locate where to adjust.
- Line 288: Capture the return value from
RendererHelpers.EnsureMaterial(renderer) into an EnsureMaterialResult variable
(as done in ParticleWrite.cs), then check its MaterialReplaced and
ReplacementReason fields before running the subsequent color operations; if
MaterialReplaced is true, either adjust or skip the color modifications and log
or surface ReplacementReason so callers know why the material was substituted.
Ensure you reference the EnsureMaterialResult variable wherever colors are
read/written in ManageMaterial (the same places that currently assume the
original material) so behavior remains consistent with ParticleWrite.cs.
---
Duplicate comments:
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 278-309: The review note about past validation concerns is
redundant because DownscaleTexture already checks for null source and maxEdge >
0 and protects against zero dimensions via Mathf.Max; remove the duplicate
comment or collapse it into a single concise note referencing DownscaleTexture's
existing ArgumentNullException and ArgumentOutOfRangeException guards to avoid
clutter in the review thread.
In `@Server/src/services/tools/manage_scene.py`:
- Around line 4-10: The code currently imports ToolResult from an internal
module ("from fastmcp.server.server import ToolResult"); update the import to
use fastmcp's public export for ToolResult per the library docs (replace the
internal path with the documented public path), e.g., change the import
statement that brings in ToolResult in manage_scene.py to the official public
import; confirm the correct public path in the fastmcp docs and update any
usages of ToolResult in this file accordingly.
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 112-118: The inline comment for manage_scene's max_resolution
parameter was previously wrong but now correctly reads "(default 640)"; no code
changes are required—confirm the documentation string or comment in the
manage_scene definition (function name manage_scene and parameter
max_resolution) matches the implementation default and leave as-is.
---
Nitpick comments:
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 114-123: The Color converter's ReadJson currently uses
JObject.Load(reader) which throws on non-object tokens; update the
ColorConverter.ReadJson to mirror the other converters by first checking
reader.TokenType and handling JArray input (accept [r,g,b,a] and map to new
Color) and also reading JObject for property form, and throw a descriptive
JsonSerializationException for other token types; reference the ReadJson
override in the ColorConverter (the method constructing new Color from jo["r"]
etc.) and reuse the same safe-token/JArray parsing and error message style used
in the other converters.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.claude/skills/unity-mcp-skill/references/tools-reference.mdMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csServer/src/services/tools/manage_scene.pyunity-mcp-skill/references/tools-reference.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/unity-mcp-skill/references/tools-reference.md
| return new ErrorResponse($"GameObject {go.name} has no Renderer component"); | ||
| } | ||
|
|
||
| RendererHelpers.EnsureMaterial(renderer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find RendererHelpers.cs and inspect EnsureMaterial definition
find . -name "RendererHelpers.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 110
🏁 Script executed:
# Search for EnsureMaterialResult definition and EnsureMaterial method
rg -n "EnsureMaterialResult|public.*EnsureMaterial" --type csRepository: CoplayDev/unity-mcp
Length of output: 1320
🏁 Script executed:
# Get more context around EnsureMaterial definition
rg -n -B 5 -A 20 "public.*EnsureMaterial\(" --type csRepository: CoplayDev/unity-mcp
Length of output: 2707
🏁 Script executed:
# Examine EnsureMaterialResult structure in detail
rg -n -A 15 "public readonly struct EnsureMaterialResult" --type csRepository: CoplayDev/unity-mcp
Length of output: 1466
🏁 Script executed:
# Look at ManageMaterial.cs context around line 288
sed -n '280,300p' MCPForUnity/Editor/Tools/ManageMaterial.csRepository: CoplayDev/unity-mcp
Length of output: 826
🏁 Script executed:
# Check how other files handle EnsureMaterialResult (specifically ParticleWrite.cs)
rg -n -B 5 -A 10 "EnsureMaterialResult ensureResult = RendererHelpers.EnsureMaterial" --type csRepository: CoplayDev/unity-mcp
Length of output: 2078
🏁 Script executed:
# Check if ManageMaterial.cs uses ToolParams as per coding guidelines
rg -n "ToolParams|GetInt|RequireString" MCPForUnity/Editor/Tools/ManageMaterial.csRepository: CoplayDev/unity-mcp
Length of output: 161
🏁 Script executed:
# Get full context of EnsureMaterial to see all return paths and line 67
sed -n '32,70p' MCPForUnity/Editor/Helpers/RendererHelpers.csRepository: CoplayDev/unity-mcp
Length of output: 1813
🏁 Script executed:
# Check if ensureResult is actually used anywhere in ParticleWrite.cs
rg -n "ensureResult" MCPForUnity/Editor/Tools/Vfx/ParticleWrite.csRepository: CoplayDev/unity-mcp
Length of output: 387
Capture and check the EnsureMaterialResult to track material substitution.
RendererHelpers.EnsureMaterial(renderer) returns a structured result with MaterialReplaced (bool) and ReplacementReason (string) fields that indicate whether the material was substituted and why. Discarding this result means subsequent color operations (lines 290–365) proceed without knowing if the renderer's material changed. Other tools like ParticleWrite.cs capture and use this result; apply the same pattern here to maintain consistency and visibility into material state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageMaterial.cs` at line 288, Capture the return
value from RendererHelpers.EnsureMaterial(renderer) into an EnsureMaterialResult
variable (as done in ParticleWrite.cs), then check its MaterialReplaced and
ReplacementReason fields before running the subsequent color operations; if
MaterialReplaced is true, either adjust or skip the color modifications and log
or surface ReplacementReason so callers know why the material was substituted.
Ensure you reference the EnsureMaterialResult variable wherever colors are
read/written in ManageMaterial (the same places that currently assume the
original material) so behavior remains consistent with ParticleWrite.cs.
| AssetDatabase.SaveAssets(); | ||
|
|
||
| // Assign to renderer | ||
| Undo.RecordObject(renderer, "Assign unique material"); | ||
| Material[] sharedMats = renderer.sharedMaterials; | ||
| if (slot < 0 || slot >= sharedMats.Length) | ||
| { | ||
| return new ErrorResponse($"Slot {slot} out of bounds (count: {sharedMats.Length})"); |
There was a problem hiding this comment.
Slot validation runs after SaveAssets() — orphaned asset on invalid slot.
AssetDatabase.SaveAssets() (line 427) commits the new material to disk before the bounds check at line 432. If the slot is out of range, the material file is already written and the error response is returned with no indication to the caller that an orphaned asset was created.
Move the slot validation to the top of the method, before any asset creation.
🛠️ Proposed fix — validate slot first
private static object CreateUniqueAndAssign(Renderer renderer, GameObject go, Color color, int slot)
{
+ // Validate slot before touching the asset database
+ Material[] sharedMats = renderer.sharedMaterials;
+ if (slot < 0 || slot >= sharedMats.Length)
+ {
+ return new ErrorResponse($"Slot {slot} out of bounds (count: {sharedMats.Length})");
+ }
+
string safeName = go.name.Replace(" ", "_");
string matPath = $"Assets/Materials/{safeName}_{go.GetInstanceID()}_mat.mat";
// ...existing path / directory / create-or-update logic...
AssetDatabase.SaveAssets();
Undo.RecordObject(renderer, "Assign unique material");
- Material[] sharedMats = renderer.sharedMaterials;
- if (slot < 0 || slot >= sharedMats.Length)
- {
- return new ErrorResponse($"Slot {slot} out of bounds (count: {sharedMats.Length})");
- }
sharedMats[slot] = existing;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageMaterial.cs` around lines 427 - 434, Move the
slot bounds check to before any asset creation or AssetDatabase.SaveAssets()
call so you don't write an orphaned material when the slot is invalid: validate
that 'slot' is within 0..renderer.sharedMaterials.Length-1 at the start of the
method (before creating or saving the Material and before calling
AssetDatabase.SaveAssets()), and return the existing ErrorResponse($"Slot {slot}
out of bounds (count: {sharedMats.Length})") immediately if invalid; keep
references to renderer.sharedMaterials, the 'slot' parameter, and the existing
ErrorResponse to locate where to adjust.
Description
Extends the manage_scene screenshot tool with advanced camera controls, inline image support, and batch multi-angle capture. Also adds a look_at action for GameObjects and improves object reference resolution.
Also, add optimization and bug fixes for various features, such as VFX and Material
Added documentation and CLI commands in correspondence to these changes above.
Type of Change
Save your change type
Changes Made
Screenshot & Camera Enhancements (C# + Python)
Helper Improvements
Updated tools-reference and workflows
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Extend Unity MCP screenshot and camera tooling for inline and multi-angle image capture, improve GameObject and material handling, and update documentation, CLI, and tests accordingly.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Improvements
Tests