Skip to content

Comments

Update on skill/UI/camera#818

Merged
Scriptwonder merged 26 commits intoCoplayDev:betafrom
Scriptwonder:update-on-skill/ui/camera
Feb 24, 2026
Merged

Update on skill/UI/camera#818
Scriptwonder merged 26 commits intoCoplayDev:betafrom
Scriptwonder:update-on-skill/ui/camera

Conversation

@Scriptwonder
Copy link
Collaborator

@Scriptwonder Scriptwonder commented Feb 24, 2026

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

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Test update

Changes Made

Screenshot & Camera Enhancements (C# + Python)

  • New screenshot parameters: camera, include_image, max_resolution, batch, look_at, view_position, view_rotation
  • batch="surround" captures 6 angles (front/back/left/right/top/bird_eye) around the scene or a target, returning inline base64 images
  • Positioned screenshot support: place a temporary camera at arbitrary world coordinates and aim it at a target
  • include_image=true returns screenshots as inline base64 PNG for AI vision
  • New scene_view_frame action to frame the Scene View on a specific GameObject or entire scene
  • RenderCameraToBase64 and DownscaleTexture added to ScreenshotUtility.cs for in-memory camera rendering
  • Python side extracts ImageContent blocks from Unity responses so AI assistants see screenshots inline
  • look_at action on manage_gameobject — rotates a GameObject to face a world position or another GameObject

Helper Improvements

  • ComponentOps.cs — object reference resolution now supports lookup by name (falls back to scene hierarchy search)
  • RenderPipelineUtility.cs / RendererHelpers.cs — extended material and renderer pipeline support
  • ManageMaterial.cs — additional material handling improvements
  • ParticleControl.cs / ParticleWrite.cs — VFX particle system updates
  • ProjectInfo.cs — project info resource enhancements (include the render pipeline, input handling)
  • UnityTypeConverters.cs — serialization improvements

Updated tools-reference and workflows

Testing/Screenshots/Recordings

  • test_manage_gameobject_look_at.py — integration tests for the look_at action
  • test_manage_scene_screenshot_params.py — 258 lines of integration tests covering camera selection, inline image, batch surround, positioned capture, and scene_view_frame

Documentation Updates

  • I have added/removed/modified tools or resources
  • If yes, I have updated all documentation files using:
    • The LLM prompt at tools/UPDATE_DOCS_PROMPT.md (recommended)
    • Manual updates following the guide at tools/UPDATE_DOCS.md

Related Issues

Additional Notes

  • The include_image and batch screenshot modes return inline base64 images (no files saved to disk), optimized for AI assistant vision workflows
  • Skill reference docs updated in both .claude/skills/ and unity-mcp-skill/

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:

  • Add advanced screenshot options to manage_scene, including camera selection, inline base64 images, max-resolution control, batch surround capture, positioned camera shots, and Scene View framing.
  • Introduce a manage_gameobject look_at action to rotate GameObjects toward world positions or other objects.
  • Expose project-level info about render pipeline, input system, and key packages via the project info resource.

Enhancements:

  • Improve material and renderer utilities to auto-select pipeline-appropriate shaders, create default scene/VFX materials, and support persistent unique material creation in set_renderer_color.
  • Extend component object reference resolution to support scene GameObject lookup by name and more flexible vector serialization formats.
  • Enhance particle system tools to ensure valid materials and report when automatic replacements occur.
  • Document new screenshot, camera, UI, and input workflows, and raise batch_execute limits for CLI usage.
  • Update SKILL guidance to emphasize use of project info and richer screenshot workflows for AI agents.

Build:

  • Extend CLI scene screenshot command with camera, inline image, batch surround, and positioned capture options, plus increase batch command limits.
  • Adjust logging guardrails to allow stdout printing from specific scene generator test scripts while keeping the rest of the server strict.

Documentation:

  • Expand tools-reference and workflows documentation with project info usage, UI creation best practices, input system selection, and new screenshot modes and parameters in both skill and runtime copies.

Tests:

  • Add integration tests for manage_scene screenshot parameters and image extraction behavior.
  • Add integration tests for manage_gameobject look_at action and tool parameter forwarding.

Summary by CodeRabbit

  • New Features

    • Enhanced screenshots: inline/base64 images, camera positioning, positioned/look-at captures, batch surround, max-resolution, and scene framing
    • New look_at action to orient GameObjects
    • Project info reporting to detect render pipeline, input handler, and UI package availability
  • Improvements

    • Safer material handling with clear replacement reasons
    • Expanded UI/workflow guidance and examples; increased CLI batch limits
  • Tests

    • Added integration tests for look_at, screenshot handling, and image extraction logic

Scriptwonder and others added 21 commits February 7, 2026 19:53
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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>
Copilot AI review requested due to automatic review settings February 24, 2026 05:03
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Feb 24, 2026

Reviewer's Guide

Extends 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 image

sequenceDiagram
    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
Loading

Sequence diagram for manage_gameobject look_at action

sequenceDiagram
    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
Loading

Class diagram for screenshot and camera utilities

classDiagram
    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
Loading

Class diagram for render pipeline aware material and VFX utilities

classDiagram
    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
Loading

Class diagram for project info and object reference resolution

classDiagram
    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
Loading

File-Level Changes

Change Details Files
Enhance manage_scene screenshot and Scene View controls with camera selection, inline/base64 images, batch multi-angle capture, and positioned shots.
  • Extend SceneCommand to accept camera, includeImage, maxResolution, batch, lookAt, viewPosition, viewRotation, and sceneViewTarget parameters.
  • Refactor screenshot handling to route through a new CaptureScreenshot(SceneCommand) that supports camera-based capture, ScreenCapture fallback, and inline image data.
  • Add CaptureSurroundBatch for 6-angle surround captures and CapturePositionedScreenshot for temp-camera, position/target-based captures that only return base64 PNGs.
  • Introduce SceneView FrameSceneView action to frame the Scene View on a target GameObject or entire scene.
  • Expose RenderCameraToBase64 and DownscaleTexture helpers in ScreenshotUtility, and extend ScreenshotCaptureResult to optionally carry base64 image data and dimensions.
MCPForUnity/Editor/Tools/ManageScene.cs
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
Wire screenshot/image capabilities through the Python tool layer and CLI, returning Text+Image ToolResults for AI vision and adding new CLI flags.
  • Implement _extract_images in manage_scene.py to convert Unity responses with inline base64 images into ToolResult objects containing TextContent and ImageContent blocks.
  • Extend manage_scene tool signature and parameter coercion to include camera, include_image, max_resolution, batch, look_at, view_position, view_rotation, and scene_view_target, and to return ToolResult when appropriate.
  • Extend unity-mcp CLI scene screenshot command with camera/include-image/max-resolution/batch/look-at/view-position/view-rotation flags and adapt output/messages for batch vs single shots.
  • Stub fastmcp.server.server.ToolResult and mcp.types content classes in test conftest to support ToolResult-based tests.
  • Add integration tests for manage_scene screenshot params and image extraction behavior.
Server/src/services/tools/manage_scene.py
Server/src/cli/commands/scene.py
Server/tests/integration/conftest.py
Server/tests/integration/test_manage_scene_screenshot_params.py
Add manage_gameobject look_at action and improve object reference resolution and type conversion robustness.
  • Introduce GameObjectLookAt handler on the C# side to rotate a GameObject toward a world position or another GameObject with optional up vector, and wire it into ManageGameObject HandleCommand.
  • Expose look_at action in the Python manage_gameobject tool, including look_at_target and look_at_up parameters, and update error messaging for valid actions.
  • Add integration tests verifying look_at parameter forwarding semantics for vector and string targets.
  • Extend ComponentOps object reference handling to support {"name": ...} and bare string names by resolving scene objects via GameObjectLookup, including component-type matching.
  • Broaden UnityTypeConverters to accept array-form vectors/quaternions in addition to object-form JSON, improving resiliency of vector parsing.
MCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs
MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs
Server/src/services/tools/manage_gameobject.py
Server/tests/integration/test_manage_gameobject_look_at.py
MCPForUnity/Editor/Helpers/ComponentOps.cs
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
Improve render-pipeline-aware material and VFX handling, including default scene materials and safer color setting and particle material assignment.
  • Extend RenderPipelineUtility with particle shader lists, material validity checks (including error shaders and pipeline mismatches), default scene material creation, and helper predicates for built-in vs SRP shaders.
  • Change RendererHelpers.EnsureMaterial to return an EnsureMaterialResult, choose replacements via RenderPipelineUtility (covering VFX and general renderers), and use pipeline-aware material validity checks.
  • Update ParticleWrite and ParticleControl tools to call EnsureMaterial and propagate material replacement info in responses, ensuring particles always have compatible materials before configuration or play.
  • Enhance ManageMaterial.SetRendererColor to ensure renderer materials exist, support a new create_unique mode that creates persistent .mat assets per object, and centralize color-property setting across _BaseColor/_Color fallbacks.
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs
MCPForUnity/Editor/Helpers/RendererHelpers.cs
MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs
MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs
MCPForUnity/Editor/Tools/ManageMaterial.cs
Expose richer project info (render pipeline, input system, packages) and update batch/scene generator/UI workflows and tools-reference docs accordingly.
  • Extend ProjectInfo resource to include renderPipeline, activeInputHandler, and booleans for key packages (ugui, textmeshpro, inputsystem), using reflection and PackageInfo to detect configuration.
  • Update UI workflows in workflows.md to emphasize reading project info first, choose EventSystem modules based on activeInputHandler, and add detailed RectTransform sizing and component wiring templates for UI controls (Button, Slider, TMP_InputField, Toggle, layouts, etc.), plus Input System guidance.
  • Add a Scene Generator Build Workflow section describing fresh-scene creation, object reference wiring via set_property and {"name": ...}, trigger physics requirements, and script update patterns.
  • Enhance tools-reference docs to document project info usage, expanded manage_scene screenshot and scene_view_frame options, manage_gameobject look_at, manage_components object reference formats, and manage_material set_renderer_color modes.
  • Increase batch CLI command size limit from 25 to 40 and add a small exemption list for allowed stdout prints in logging tests.
  • Clarify SKILL.md template guidance, especially around screenshot usage patterns and project info for UI.
MCPForUnity/Editor/Resources/Project/ProjectInfo.cs
.claude/skills/unity-mcp-skill/references/workflows.md
unity-mcp-skill/references/workflows.md
.claude/skills/unity-mcp-skill/references/tools-reference.md
unity-mcp-skill/references/tools-reference.md
unity-mcp-skill/SKILL.md
.claude/skills/unity-mcp-skill/SKILL.md
Server/src/cli/commands/batch.py
Server/tests/integration/test_logging_stdout.py

Possibly linked issues

  • #Add Claude skill for unity mcp: PR significantly expands the Unity MCP Claude skill docs and workflows, including refresh_unity, batch_execute, and schema guidance requested.

Tips and commands

Interacting with Sourcery

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

Customizing Your Experience

Access your dashboard to:

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

Getting Help

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 24, 2026

📝 Walkthrough

Walkthrough

This 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

Cohort / File(s) Summary
Screenshot Capture Enhancement
MCPForUnity/Editor/Tools/ManageScene.cs, MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs, Server/src/cli/commands/scene.py, Server/src/services/tools/manage_scene.py
Added camera selection, include_image/max_resolution, batch surround and positioned-view captures, inline base64 embedding, CaptureSurroundBatch/CapturePositionedScreenshot flows, and extended ScreenshotCaptureResult to carry image base64/width/height. CLI and server forwarding/response shaping updated.
GameObject Look At Action
MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs, MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs.meta, MCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs, Server/src/services/tools/manage_gameobject.py, Server/tests/integration/test_manage_gameobject_look_at.py
Introduced look_at action that rotates a GameObject toward a world position or another GameObject (optional up vector). Wired editor handler, server API params, and integration tests.
Material & Renderer Validation
MCPForUnity/Editor/Helpers/RendererHelpers.cs, MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs, MCPForUnity/Editor/Tools/ManageMaterial.cs, MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs, MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs
Refactored EnsureMaterial to return EnsureMaterialResult with replacement reason, added pipeline-aware shader validation/defaulting, per-scene default material caching, SetColorProperties helper, and create_unique material creation path. VFX/particle tools now surface material replacement metadata.
Object Reference Resolution & Project Info
MCPForUnity/Editor/Helpers/ComponentOps.cs, MCPForUnity/Editor/Resources/Project/ProjectInfo.cs, MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
Added ResolveSceneObjectByName to allow name-based scene lookups in SetObjectReference fallbacks. ProjectInfo now reports renderPipeline, activeInputHandler, and package flags (ugui, textmeshpro, inputsystem). JSON converters accept numeric arrays for vectors/quaternions.
UI & Scene Workflow Documentation
.claude/skills/unity-mcp-skill/SKILL.md, .claude/skills/unity-mcp-skill/references/tools-reference.md, .claude/skills/unity-mcp-skill/references/workflows.md, unity-mcp-skill/SKILL.md, unity-mcp-skill/references/tools-reference.md, unity-mcp-skill/references/workflows.md
Expanded screenshot examples (inline, camera-specific, batch, look_at/view_position), added best-practice guidance and an agentic camera loop, and enriched UI build/workflow docs with wiring patterns, RectTransform guidance, and input-system conditional flows using project_info.
Batch Command Limits & Misc
Server/src/cli/commands/batch.py, .gitignore
Raised batch command limits from 25 to 40 for batch_run and batch_inline. Added .env to .gitignore.
Tests & Test Stubs
Server/tests/integration/conftest.py, Server/tests/integration/test_logging_stdout.py, Server/tests/integration/test_manage_scene_screenshot_params.py, Server/tests/integration/test_manage_gameobject_look_at.py
Added ToolResult/content stubs, allowlist for stdout printing, and extensive tests covering screenshot parameter forwarding, _extract_images image extraction and shaping, batch/positioned outputs, and manage_gameobject look_at behavior.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Suggested labels

codex

Suggested reviewers

  • msanatan

"🐰
I hop through scenes to catch the light,
Six little views in a carousel flight,
I nudge a head to look, then snap the sight,
Materials shine and wiring snaps tight,
Hooray — pixels, docs, and tests delight!"

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.72% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title "Update on skill/UI/camera" is vague and generic, lacking specific details about the main changes despite the PR encompassing significant feature additions and improvements. Use a more descriptive title that captures the primary change, such as "Add advanced screenshot modes, look_at action, and camera controls to manage_scene/manage_gameobject tools" or "Enhance screenshot tooling with inline images, batch capture, and camera controls".
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The PR description is comprehensive and well-structured, covering all required sections including description, type of change (multiple categories), detailed changes made, testing updates, documentation updates with checklist, and additional notes.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 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>

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

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +288 to +295
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]
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
private static object CreateUniqueAndAssign(Renderer renderer, GameObject go, Color color, int slot)
{
string safeName = go.name.Replace(" ", "_");
string matPath = $"Assets/Materials/{safeName}_mat.mat";
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
string matPath = $"Assets/Materials/{safeName}_mat.mat";
int instanceId = go.GetInstanceID();
string matPath = $"Assets/Materials/{safeName}_{instanceId}_mat.mat";

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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 | 🟠 Major

Consider migrating new param parsing to ToolParams.

The expanded screenshot/scene_view_frame surface is still parsed via ad‑hoc JObject access. Using ToolParams would give consistent validation (especially as parameters grow).
As per coding guidelines: MCPForUnity/Editor/Tools/*.cs: Use ToolParams for consistent parameter validation in C# Editor tools with methods like GetInt(), 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 with Assets/ now fall through to scene name lookup. This aligns well with the JObject name path 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: Clarify maxResolution default behavior in docs.
The implementation treats maxResolution <= 0 as 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

📥 Commits

Reviewing files that changed from the base of the PR and between 95b6ddb and 67883c8.

⛔ Files ignored due to path filters (1)
  • Server/uv.lock is 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
  • .gitignore
  • MCPForUnity/Editor/Helpers/ComponentOps.cs
  • MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs
  • MCPForUnity/Editor/Helpers/RendererHelpers.cs
  • MCPForUnity/Editor/Resources/Project/ProjectInfo.cs
  • MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs
  • MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs.meta
  • MCPForUnity/Editor/Tools/GameObjects/ManageGameObject.cs
  • MCPForUnity/Editor/Tools/ManageMaterial.cs
  • MCPForUnity/Editor/Tools/ManageScene.cs
  • MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs
  • MCPForUnity/Editor/Tools/Vfx/ParticleWrite.cs
  • MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
  • MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
  • Server/src/cli/commands/batch.py
  • Server/src/cli/commands/scene.py
  • Server/src/services/tools/manage_gameobject.py
  • Server/src/services/tools/manage_scene.py
  • Server/tests/integration/conftest.py
  • Server/tests/integration/test_logging_stdout.py
  • Server/tests/integration/test_manage_gameobject_look_at.py
  • Server/tests/integration/test_manage_scene_screenshot_params.py
  • unity-mcp-skill/SKILL.md
  • unity-mcp-skill/references/tools-reference.md
  • unity-mcp-skill/references/workflows.md

Comment on lines +92 to +102
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}
}}
])
```
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines 1128 to 1131
### 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.**

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +12 to +14
async def fake_send(cmd, params, **kwargs):
captured["params"] = params
return {"success": True, "data": {"rotation": [0, 90, 0]}}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +69 to +77
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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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"].

Comment on lines +113 to +115
async def fake_send(cmd, params, **kwargs):
captured["params"] = params
return {"success": True, "data": {"filePath": "Assets/shot.png"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +119 to +126
resp = await manage_scene_mod.manage_scene(
ctx=DummyContext(),
action="screenshot",
camera="MainCamera",
include_image=True,
max_resolution=256,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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).

Comment on lines 112 to 118
# 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)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
# 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
Server/src/services/tools/manage_scene.py (3)

217-220: Guard _extract_images call behind an action check.

_extract_images is invoked on every successful response regardless of action, but it short-circuits immediately for all non-screenshot actions (line 64: return None). An explicit guard communicates intent and avoids the unnecessary call overhead for get_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 to max_resolution validation.

The guard only rejects non-positive values. An arbitrarily large value (e.g., 99999) is accepted and forwarded to Unity's DownscaleTexture, 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 over screenshots into a single loop.

screenshots is walked twice: once (lines 32–33) to build summary_screenshots, and again (lines 44–48) to emit ImageContent blocks. 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.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67883c8 and 4cfc198.

📒 Files selected for processing (1)
  • Server/src/services/tools/manage_scene.py


from fastmcp import Context
from mcp.types import ToolAnnotations
from fastmcp.server.server import ToolResult
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 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 ToolResult

This 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (3)
Server/src/services/tools/manage_scene.py (1)

4-10: Use the public ToolResult import path (already raised previously).
Still importing ToolResult from 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 ToolResult
fastmcp 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: Past max_resolution default 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 on DownscaleTexture — addressed in current code.

Both guards (ArgumentNullException for null source, ArgumentOutOfRangeException for maxEdge ≤ 0) are now present. The zero-dimension edge case is handled implicitly by Mathf.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 to ColorConverter for consistency.

The four updated converters now gracefully handle array inputs and produce descriptive errors for unexpected token types. ColorConverter still uses JObject.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

📥 Commits

Reviewing files that changed from the base of the PR and between 4cfc198 and 1980f53.

📒 Files selected for processing (6)
  • .claude/skills/unity-mcp-skill/references/tools-reference.md
  • MCPForUnity/Editor/Tools/ManageMaterial.cs
  • MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
  • MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
  • Server/src/services/tools/manage_scene.py
  • unity-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);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find RendererHelpers.cs and inspect EnsureMaterial definition
find . -name "RendererHelpers.cs" -type f

Repository: CoplayDev/unity-mcp

Length of output: 110


🏁 Script executed:

# Search for EnsureMaterialResult definition and EnsureMaterial method
rg -n "EnsureMaterialResult|public.*EnsureMaterial" --type cs

Repository: CoplayDev/unity-mcp

Length of output: 1320


🏁 Script executed:

# Get more context around EnsureMaterial definition
rg -n -B 5 -A 20 "public.*EnsureMaterial\(" --type cs

Repository: CoplayDev/unity-mcp

Length of output: 2707


🏁 Script executed:

# Examine EnsureMaterialResult structure in detail
rg -n -A 15 "public readonly struct EnsureMaterialResult" --type cs

Repository: 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.cs

Repository: 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 cs

Repository: 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.cs

Repository: 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.cs

Repository: 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.cs

Repository: 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.

Comment on lines +427 to +434
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})");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

@Scriptwonder Scriptwonder merged commit 6e7bd42 into CoplayDev:beta Feb 24, 2026
2 checks passed
@Scriptwonder Scriptwonder deleted the update-on-skill/ui/camera branch February 24, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant