Feature/multi-view screenshot#840
Merged
Scriptwonder merged 4 commits intoCoplayDev:betafrom Feb 28, 2026
Merged
Conversation
…499094106 chore: update Unity package to beta version 9.4.8-beta.8
- Batch surround/orbit modes now return a single composite contact sheet image - Added orbit mode with configurable azimuth, elevations, distance, FOV - Contact sheet tiles are labeled with compass directions (front, back_left, etc.) - CLI saves contact sheet PNGs to Assets/Screenshots/ by default - ScreenshotUtility: RenderCameraToTexture, ComposeContactSheet with 5x7 pixel font - Server manage_scene.py: orbit params forwarding - CLI raw command: nargs fix for Windows argument splitting - Updated docs: CLI_USAGE, CLI_EXAMPLE, SKILL.md, tools-reference
Contributor
Reviewer's GuideAdds a configurable multi-view screenshot workflow that captures orbit/surround batches as a labeled contact-sheet image, wires it through the Unity bridge, CLI, and tool metadata, and updates docs and examples accordingly. Sequence diagram for CLI orbit batch screenshot contact sheet workflowsequenceDiagram
actor Dev
participant CLI as unity_mcp_CLI
participant Tool as manage_scene_tool
participant Server as Unity_MCP_server
participant Editor as ManageScene
participant Util as ScreenshotUtility
Dev->>CLI: scene screenshot --batch orbit --look-at Player --orbit-angles 8
CLI->>Tool: manage_scene(action=screenshot, batch=orbit, orbit_angles, orbit_elevations, ...)
Tool->>Server: run_command manage_scene params
Server->>Editor: CaptureScreenshot(SceneCommand cmd)
Editor->>Editor: CaptureOrbitBatch(cmd)
loop elevations x azimuth_angles
Editor->>Util: RenderCameraToTexture(tempCam, maxResolution)
Util-->>Editor: Texture2D tile
end
Editor->>Util: ComposeContactSheet(tiles, tileLabels)
Util-->>Editor: base64, width, height
Editor-->>Server: SuccessResponse{imageBase64, imageWidth, imageHeight, shots, screenshotsFolder}
Server-->>CLI: JSON result
CLI->>CLI: unwrap result, extract imageBase64, screenshotsFolder
CLI->>CLI: decode base64 and write PNG to output_dir
CLI-->>Dev: print_success Contact_sheet_saved_to_path
Updated class diagram for SceneCommand, ManageScene, and ScreenshotUtilityclassDiagram
class SceneCommand {
+string camera
+bool includeImage
+int maxResolution
+string batch
+JToken lookAt
+Vector3 viewPosition
+Vector3 viewRotation
+int orbitAngles
+float[] orbitElevations
+float orbitDistance
+float orbitFov
+JToken sceneViewTarget
+bool includeComponents
+bool includeTransform
}
class ManageScene {
+SceneCommand ToSceneCommand(JObject p)
+object CaptureScreenshot(SceneCommand cmd)
+object CaptureSurroundBatch(SceneCommand cmd)
+object CaptureOrbitBatch(SceneCommand cmd)
+float[] ParseFloatArray(JToken token)
+string GetDirectionLabel(float azimuthDeg)
}
class ScreenshotUtility {
+string RenderCameraToBase64(Camera camera,int maxResolution)
+Texture2D RenderCameraToTexture(Camera camera,int maxResolution)
+(string base64,int width,int height) ComposeContactSheet(List_Texture2D_tiles,List_string_labels,int padding)
+Texture2D DownscaleTexture(Texture2D source,int maxEdge)
+void DrawText(Texture2D tex,string text,int startX,int startY,int charHeight,Color color)
+ulong GetGlyph(char c)
+void DestroyTexture(Texture2D tex)
}
ManageScene --> SceneCommand : uses
ManageScene --> ScreenshotUtility : calls
ScreenshotUtility --> Texture2D : produces
ScreenshotUtility --> Camera : renders_from
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds orbit-based batch screenshot capture and contact-sheet composition across editor, runtime, CLI, and docs; introduces in-memory camera rendering and composition helpers, CLI/Service parameter plumbing for orbit options, and updates documentation and package versioning. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Editor as UnityEditor
participant Util as ScreenshotUtility
Client->>Server: screenshot(batch="orbit", orbit_angles, orbit_elevations, ...)
Server->>Editor: manage_scene(action="screenshot", batch="orbit", orbitAngles, orbitElevations, ...)
Editor->>Editor: resolve target (look_at or bounds) and compute orbit grid
loop for each elevation × azimuth
Editor->>Util: RenderCameraToTexture(tempCamera, maxResolution)
Util-->>Editor: Texture2D tile
Editor->>Editor: generate tile label (GetDirectionLabel)
end
Editor->>Util: ComposeContactSheet(tiles[], labels[], padding)
Util-->>Editor: base64 composite image + width/height
Editor-->>Server: return composite image + metadata (center, radius, tiles)
Server->>Server: save composite image to output_dir (timestamped)
Server-->>Client: return saved path + metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ComposeContactSheet, you calltiles[idx].GetPixels32()but then ignore the result and repeatedly callGetPixelin the inner loops; switching to use theGetPixels32()buffer (orSetPixels32by row) would significantly reduce per-pixel method call overhead for larger grids. - The screenshot CLI response handling now unwraps
result/innerin several ways (status,success, nesteddata); it might be worth centralizing this into a small helper to avoid subtle discrepancies if the backend response shape evolves again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ComposeContactSheet`, you call `tiles[idx].GetPixels32()` but then ignore the result and repeatedly call `GetPixel` in the inner loops; switching to use the `GetPixels32()` buffer (or `SetPixels32` by row) would significantly reduce per-pixel method call overhead for larger grids.
- The screenshot CLI response handling now unwraps `result`/`inner` in several ways (`status`, `success`, nested `data`); it might be worth centralizing this into a small helper to avoid subtle discrepancies if the backend response shape evolves again.
## Individual Comments
### Comment 1
<location path="Server/src/cli/commands/scene.py" line_range="348" />
<code_context>
- count = len(data.get("screenshots", []))
- print_success(f"Captured {count} batch screenshots")
+ # Unwrap the response: {"status":"success","result":{"success":true,"data":{...}}}
+ inner = result.get("result", result) # fallback to result if no nesting
+ is_success = (result.get("status") == "success"
+ or inner.get("success", False)
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard `inner.get(...)` with a type check to avoid attribute errors when `inner` is not a dict.
Because `inner` comes from `result.get("result", result)`, it may be non-dict values (e.g. string, list) depending on `run_command`’s error shape. In those cases `inner.get("success", False)` will raise `AttributeError` and hide the real error. A safer pattern is:
```python
inner = result.get("result", result)
if isinstance(inner, dict):
is_success = (
result.get("status") == "success"
or inner.get("success", False)
or result.get("success", False)
)
data = inner.get("data", inner)
else:
is_success = result.get("status") == "success" or result.get("success", False)
data = {}
```
This preserves current behavior for valid responses while avoiding secondary failures for unexpected shapes.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs" line_range="352" />
<code_context>
+ for (int i = 0; i < bgPixels.Length; i++) bgPixels[i] = bgColor;
+ sheet.SetPixels32(bgPixels);
+
+ for (int idx = 0; idx < count; idx++)
+ {
+ int col = idx % cols;
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid per-pixel `GetPixel` calls and use the already-fetched pixel buffer for better performance.
In `ComposeContactSheet`, you populate `tilePixels = tiles[idx].GetPixels32()` but then ignore it and call `GetPixel` inside the inner loops, which is much slower at scale.
Instead, index directly into `tilePixels` when composing the sheet, e.g.:
```csharp
Color32[] tilePixels = tiles[idx].GetPixels32();
for (int ty = 0; ty < tileH; ty++)
{
int rowOffset = ty * tileW;
for (int tx = 0; tx < tileW; tx++)
{
sheet.SetPixel(x + tx,
y + labelHeight + ty,
tilePixels[rowOffset + tx]);
}
}
```
For more gain, you could build a row buffer and use `SetPixels32` per row to reduce call overhead on large grids.
Suggested implementation:
```csharp
// Draw tile pixels using pre-fetched pixel buffer for better performance
Color32[] tilePixels = tiles[idx].GetPixels32();
for (int ty = 0; ty < tileH; ty++)
{
int rowOffset = ty * tileW;
for (int tx = 0; tx < tileW; tx++)
{
sheet.SetPixel(
x + tx,
y + labelHeight + ty,
tilePixels[rowOffset + tx]
);
}
}
```
For even better performance on large sheets, you can further optimize by:
1. Allocating a `Color32[] rowBuffer = new Color32[tileW];`
2. Inside the `ty` loop, copy the corresponding slice from `tilePixels` into `rowBuffer` and call `sheet.SetPixels32(x, y + labelHeight + ty, tileW, 1, rowBuffer);`
This would reduce the overhead of many `SetPixel` calls but is not required to satisfy the current suggestion.
</issue_to_address>
### Comment 3
<location path="unity-mcp-skill/SKILL.md" line_range="85" />
<code_context>
+| `batch` | string | `"surround"` (6 fixed angles) or `"orbit"` (configurable grid) |
+| `look_at` | string | Target: GameObject name/path/ID, or `"x,y,z"` world position |
+| `view_position` | list | Camera position `[x,y,z]` for positioned screenshot |
+| `view_rotation` | list | Camera euler rotation `[x,y,z]` for positioned screenshot |
+| `orbit_angles` | int | Number of azimuth samples around the target (default 8) |
+| `orbit_elevations` | list | Vertical angles in degrees, e.g. `[0, 30, -15]` (default `[0]`) |
</code_context>
<issue_to_address>
**suggestion (typo):** Capitalize “Euler” when referring to Euler angles in the description.
Since “Euler” is a proper noun here, please capitalize it in the description, e.g., “Camera Euler rotation `[x,y,z]` for positioned screenshot.”
```suggestion
| `view_rotation` | list | Camera Euler rotation `[x,y,z] for positioned screenshot |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| count = len(data.get("screenshots", [])) | ||
| print_success(f"Captured {count} batch screenshots") | ||
| # Unwrap the response: {"status":"success","result":{"success":true,"data":{...}}} | ||
| inner = result.get("result", result) # fallback to result if no nesting |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard inner.get(...) with a type check to avoid attribute errors when inner is not a dict.
Because inner comes from result.get("result", result), it may be non-dict values (e.g. string, list) depending on run_command’s error shape. In those cases inner.get("success", False) will raise AttributeError and hide the real error. A safer pattern is:
inner = result.get("result", result)
if isinstance(inner, dict):
is_success = (
result.get("status") == "success"
or inner.get("success", False)
or result.get("success", False)
)
data = inner.get("data", inner)
else:
is_success = result.get("status") == "success" or result.get("success", False)
data = {}This preserves current behavior for valid responses while avoiding secondary failures for unexpected shapes.
| | `batch` | string | `"surround"` (6 fixed angles) or `"orbit"` (configurable grid) | | ||
| | `look_at` | string | Target: GameObject name/path/ID, or `"x,y,z"` world position | | ||
| | `view_position` | list | Camera position `[x,y,z]` for positioned screenshot | | ||
| | `view_rotation` | list | Camera euler rotation `[x,y,z]` for positioned screenshot | |
Contributor
There was a problem hiding this comment.
suggestion (typo): Capitalize “Euler” when referring to Euler angles in the description.
Since “Euler” is a proper noun here, please capitalize it in the description, e.g., “Camera Euler rotation [x,y,z] for positioned screenshot.”
Suggested change
| | `view_rotation` | list | Camera euler rotation `[x,y,z]` for positioned screenshot | | |
| | `view_rotation` | list | Camera Euler rotation `[x,y,z] for positioned screenshot | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/guides/CLI_EXAMPLE.md (2)
89-89: Consider clarifying "contact sheet" terminology.The term "contact sheet" is used to describe the batch output format. While this is a well-known photography term, users unfamiliar with it might benefit from a brief explanation (e.g., "a grid layout of multiple screenshots") or a reference to the PR images that show the feature visually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_EXAMPLE.md` at line 89, Update the CLI docs line for the `--batch, -b` flag to clarify "contact sheet" by adding a brief parenthetical definition (e.g., "contact sheet (a grid layout of multiple screenshots)") and/or a short reference to the PR images that demonstrate the output; adjust the text around the existing tokens `--batch, -b`, `surround`, `orbit`, and `contact sheet` so the meaning is immediately clear to readers unfamiliar with photography jargon.
93-96: Consider noting parameter scope.The orbit-specific parameters (
--orbit-angles,--orbit-elevations,--orbit-distance,--orbit-fov) only apply when using--batch orbitmode. Adding a note like "Used with--batch orbit" in the descriptions would help users understand parameter interactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_EXAMPLE.md` around lines 93 - 96, The descriptions for the orbit-specific CLI flags (`--orbit-angles`, `--orbit-elevations`, `--orbit-distance`, `--orbit-fov`) should explicitly state they only apply when using `--batch orbit`; update each flag's description in CLI_EXAMPLE.md to append a short note like "Used with `--batch orbit`" so users understand the parameter scope and avoid confusion when using other batch modes.MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
360-368: Consider usingSetPixels32for better performance.The nested
SetPixelloops for copying tile pixels are O(width × height) per tile with per-pixel method call overhead. For large contact sheets with many tiles, this could be slow.♻️ Potential optimization using block copy
- // Draw tile pixels - Color32[] tilePixels = tiles[idx].GetPixels32(); - for (int ty = 0; ty < tileH; ty++) - { - for (int tx = 0; tx < tileW; tx++) - { - sheet.SetPixel(x + tx, y + labelHeight + ty, tiles[idx].GetPixel(tx, ty)); - } - } + // Draw tile pixels using block copy for each row + Color32[] tilePixels = tiles[idx].GetPixels32(); + for (int ty = 0; ty < tileH; ty++) + { + int srcStart = ty * tileW; + int dstStart = (y + labelHeight + ty) * sheetW + x; + for (int tx = 0; tx < tileW; tx++) + { + bgPixels[dstStart + tx] = tilePixels[srcStart + tx]; + } + }This would require restructuring to use
bgPixelsthroughout and callSetPixels32once at the end.🤖 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 360 - 368, The current nested per-pixel SetPixel loop in ScreenshotUtility (using sheet.SetPixel and tiles[idx].GetPixel) is slow; replace it by using the existing tiles[idx].GetPixels32() (tilePixels) and copy those Color32 blocks into the sheet's bgPixels buffer at the computed (x, y + labelHeight) offset (using Array.Copy or Buffer.BlockCopy with the per-row stride) and then call sheet.SetPixels32(bgPixels) once (or use Texture2D.SetPixels32(x,y,width,height,tilePixels) if you prefer per-tile block calls) to update the texture; update references to tilePixels, tileW, tileH, sheet, bgPixels, and labelHeight and remove the per-pixel SetPixel loops.Server/src/services/tools/manage_scene.py (1)
210-219: Consider adding range validation fororbit_distanceandorbit_fov.The current implementation validates that these are numeric but doesn't check for invalid ranges. For example:
- Negative
orbit_distanceis likely invalidorbit_fovoutside typical bounds (e.g., 1-179 degrees) could cause rendering issuesIf Unity already handles these edge cases gracefully, this is fine as-is. Otherwise, consider adding bounds checks similar to the
max_resolutionvalidation at lines 169-170.🤖 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 210 - 219, Add explicit range validation for orbit_distance and orbit_fov after converting them to floats in the block handling orbit parameters: ensure orbit_distance is >= 0 (or a sensible positive minimum) and ensure orbit_fov is within a safe degree range (e.g., 1 <= orbit_fov <= 179). Update the same branches that currently set params["orbitDistance"] and params["orbitFov"] (and their error returns) to return {"success": False, "message": "..."} with clear messages when values are out of range, mirroring the style used for max_resolution validation.
🤖 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/ManageScene.cs`:
- Around line 58-72: ParseFloatArray currently calls arr[i].ToObject<float>()
which can throw for malformed elements; wrap the per-element conversion in a
try-catch inside the for loop in ParseFloatArray and on failure throw a
descriptive exception (e.g., JsonException) that includes the array index and
the offending token value and preserves the original exception as inner; keep
the single-value path using ParamCoercion.CoerceFloatNullable unchanged.
In `@Server/src/services/tools/manage_scene.py`:
- Around line 202-209: Remove the redundant json re-import and instead use the
existing json module to parse orbit_elevations when it's a str, and after
parsing validate that orbit_elevations is a list containing only numeric types
(int or float); if parsing fails or the value is not a list of numbers return
the same error object, otherwise set params["orbitElevations"] =
orbit_elevations. Ensure you reference the orbit_elevations variable and
params["orbitElevations"] in your changes and perform the type check after
json.loads succeeds.
---
Nitpick comments:
In `@docs/guides/CLI_EXAMPLE.md`:
- Line 89: Update the CLI docs line for the `--batch, -b` flag to clarify
"contact sheet" by adding a brief parenthetical definition (e.g., "contact sheet
(a grid layout of multiple screenshots)") and/or a short reference to the PR
images that demonstrate the output; adjust the text around the existing tokens
`--batch, -b`, `surround`, `orbit`, and `contact sheet` so the meaning is
immediately clear to readers unfamiliar with photography jargon.
- Around line 93-96: The descriptions for the orbit-specific CLI flags
(`--orbit-angles`, `--orbit-elevations`, `--orbit-distance`, `--orbit-fov`)
should explicitly state they only apply when using `--batch orbit`; update each
flag's description in CLI_EXAMPLE.md to append a short note like "Used with
`--batch orbit`" so users understand the parameter scope and avoid confusion
when using other batch modes.
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 360-368: The current nested per-pixel SetPixel loop in
ScreenshotUtility (using sheet.SetPixel and tiles[idx].GetPixel) is slow;
replace it by using the existing tiles[idx].GetPixels32() (tilePixels) and copy
those Color32 blocks into the sheet's bgPixels buffer at the computed (x, y +
labelHeight) offset (using Array.Copy or Buffer.BlockCopy with the per-row
stride) and then call sheet.SetPixels32(bgPixels) once (or use
Texture2D.SetPixels32(x,y,width,height,tilePixels) if you prefer per-tile block
calls) to update the texture; update references to tilePixels, tileW, tileH,
sheet, bgPixels, and labelHeight and remove the per-pixel SetPixel loops.
In `@Server/src/services/tools/manage_scene.py`:
- Around line 210-219: Add explicit range validation for orbit_distance and
orbit_fov after converting them to floats in the block handling orbit
parameters: ensure orbit_distance is >= 0 (or a sensible positive minimum) and
ensure orbit_fov is within a safe degree range (e.g., 1 <= orbit_fov <= 179).
Update the same branches that currently set params["orbitDistance"] and
params["orbitFov"] (and their error returns) to return {"success": False,
"message": "..."} with clear messages when values are out of range, mirroring
the style used for max_resolution validation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.claude/skills/unity-mcp-skill/references/tools-reference.mdMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/package.jsonServer/src/cli/commands/scene.pyServer/src/cli/main.pyServer/src/services/tools/manage_scene.pydocs/guides/CLI_EXAMPLE.mddocs/guides/CLI_USAGE.mdunity-mcp-skill/SKILL.mdunity-mcp-skill/references/tools-reference.md
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a multi-view (contact sheet) screenshot capability to manage_scene, exposing both fixed-angle surround and configurable orbit batch modes across Unity, server tool wrapper, CLI, and docs.
Changes:
- Implement contact-sheet composition for
batch="surround"and newbatch="orbit"screenshot mode (Unity). - Extend server tool + CLI to pass orbit parameters and save returned composite images to disk.
- Update skill/docs/tool references and CLI guides to document new screenshot behavior and options.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
MCPForUnity/Editor/Tools/ManageScene.cs |
Adds orbit batch mode and changes batch outputs to a single composite contact sheet + metadata. |
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs |
Adds camera-to-texture rendering and contact-sheet compositor utilities. |
Server/src/services/tools/manage_scene.py |
Exposes orbit params (angles/elevations/distance/fov) to Unity via tool wrapper. |
Server/src/cli/commands/scene.py |
Adds CLI flags for orbit + output dir; saves composite base64 images to disk for batch/positioned captures. |
Server/src/cli/main.py |
Improves raw command JSON arg handling by joining split args (Windows-friendly). |
docs/guides/CLI_USAGE.md |
Documents new screenshot usage, including batch contact sheets and orbit parameters. |
docs/guides/CLI_EXAMPLE.md |
Updates example commands for new screenshot flags and batch modes. |
unity-mcp-skill/SKILL.md |
Documents screenshot parameters and introduces batch contact-sheet guidance. |
unity-mcp-skill/references/tools-reference.md |
Updates tool reference for contact-sheet batch behavior and adds orbit batch example. |
.claude/skills/unity-mcp-skill/references/tools-reference.md |
Mirrors tool reference updates for Claude skill bundle. |
MCPForUnity/package.json |
Bumps package version to 9.4.8-beta.8. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+292
to
+296
|
|
||
| RenderTexture.active = rt; | ||
| var tex = new Texture2D(width, height, TextureFormat.RGBA32, false); | ||
| tex.ReadPixels(new Rect(0, 0, width, height), 0, 0); | ||
| tex.Apply(); |
There was a problem hiding this comment.
RenderCameraToTexture allocates a Texture2D inside the try block but never destroys it if ReadPixels/Apply (or DownscaleTexture) throws. This can leak textures in editor sessions. Consider mirroring RenderCameraToBase64’s pattern: keep references outside the try and DestroyTexture(...) in a finally when the method exits via exception.
Comment on lines
+360
to
+364
| // Draw tile pixels | ||
| Color32[] tilePixels = tiles[idx].GetPixels32(); | ||
| for (int ty = 0; ty < tileH; ty++) | ||
| { | ||
| for (int tx = 0; tx < tileW; tx++) |
There was a problem hiding this comment.
ComposeContactSheet is doing per-pixel GetPixel/SetPixel work inside nested loops; this will be extremely slow for orbit grids (potentially 100+ tiles) and can freeze the editor. Also tilePixels is currently computed but unused. Prefer bulk operations (e.g., SetPixels32 into a preallocated sheet buffer, Graphics.CopyTexture, or blitting via a RenderTexture) and remove/replace the unused tilePixels allocation.
unity-mcp-skill/SKILL.md
Outdated
| | `view_position` | list | Camera position `[x,y,z]` for positioned screenshot | | ||
| | `view_rotation` | list | Camera euler rotation `[x,y,z]` for positioned screenshot | | ||
| | `orbit_angles` | int | Number of azimuth samples around the target (default 8) | | ||
| | `orbit_elevations` | list | Vertical angles in degrees, e.g. `[0, 30, -15]` (default `[0]`) | |
There was a problem hiding this comment.
The docs claim orbit_elevations defaults to [0], but the Unity implementation defaults to [0, 30, -15] when not provided. Please update the documented default to match actual behavior (or adjust the code default if [0] is intended).
Suggested change
| | `orbit_elevations` | list | Vertical angles in degrees, e.g. `[0, 30, -15]` (default `[0]`) | | |
| | `orbit_elevations` | list | Vertical angles in degrees, e.g. `[0, 30, -15]` (default `[0, 30, -15]`) | |
| batch="orbit", # str - "orbit" for configurable angle grid | ||
| look_at="Player", # str|int|list[float] - target to orbit around | ||
| orbit_angles=8, # int, default 8 - number of azimuth steps | ||
| orbit_elevations=[0, 30], # list[float], default [0] - vertical angles in degrees |
There was a problem hiding this comment.
The tools reference says orbit_elevations has default [0], but ManageScene.CaptureOrbitBatch uses [0, 30, -15] when orbitElevations is omitted. Update this default to avoid confusing callers.
Suggested change
| orbit_elevations=[0, 30], # list[float], default [0] - vertical angles in degrees | |
| orbit_elevations=[0, 30], # list[float], default [0, 30, -15] - vertical angles in degrees |
| /// <summary> | ||
| /// Captures screenshots from a configurable orbit around a target for visual QA. | ||
| /// Supports custom azimuth count, elevation angles, distance, and FOV. | ||
| /// Returns all images as inline base64 PNGs (no files saved to disk). |
There was a problem hiding this comment.
The XML doc for CaptureOrbitBatch says it “Returns all images as inline base64 PNGs”, but the implementation returns a single composite contact-sheet image (imageBase64) plus shot metadata. Update the comment to match the actual response shape so tool consumers aren’t misled.
Suggested change
| /// Returns all images as inline base64 PNGs (no files saved to disk). | |
| /// Returns a single composite contact-sheet image as an inline base64 PNG (imageBase64) | |
| /// plus per-shot metadata; no files are saved to disk. |
Comment on lines
+331
to
+332
| if orbit_angles: | ||
| params["orbitAngles"] = orbit_angles |
There was a problem hiding this comment.
These truthiness checks will ignore explicit values like 0 (e.g., --orbit-angles 0) and silently fall back to defaults, which is confusing. Use is not None and validate ranges (e.g., orbit_angles >= 1) so invalid values produce a clear error instead of being dropped.
Comment on lines
+340
to
+342
| if orbit_distance: | ||
| params["orbitDistance"] = orbit_distance | ||
| if orbit_fov: |
There was a problem hiding this comment.
orbit_distance/orbit_fov are parsed as floats by click, but these if orbit_distance: / if orbit_fov: checks will drop legitimate falsy values (0.0) instead of validating and erroring. Prefer is not None and reject non-positive distances / out-of-range FOV early so CLI behavior matches the help text.
Suggested change
| if orbit_distance: | |
| params["orbitDistance"] = orbit_distance | |
| if orbit_fov: | |
| if orbit_distance is not None: | |
| if orbit_distance <= 0: | |
| print_error(f"Invalid orbit-distance {orbit_distance}: must be positive.") | |
| sys.exit(1) | |
| params["orbitDistance"] = orbit_distance | |
| if orbit_fov is not None: | |
| # Require a sensible vertical field-of-view in degrees. | |
| if orbit_fov <= 0 or orbit_fov >= 180: | |
| print_error( | |
| f"Invalid orbit-fov {orbit_fov}: must be between 0 and 180 degrees (exclusive)." | |
| ) | |
| sys.exit(1) |
| batch="orbit", # str - "orbit" for configurable angle grid | ||
| look_at="Player", # str|int|list[float] - target to orbit around | ||
| orbit_angles=8, # int, default 8 - number of azimuth steps | ||
| orbit_elevations=[0, 30], # list[float], default [0] - vertical angles in degrees |
There was a problem hiding this comment.
The tools reference says orbit_elevations has default [0], but ManageScene.CaptureOrbitBatch uses [0, 30, -15] when orbitElevations is omitted. Update this default to avoid confusing callers.
Suggested change
| orbit_elevations=[0, 30], # list[float], default [0] - vertical angles in degrees | |
| orbit_elevations=[0, 30], # list[float], default [0, 30, -15] - vertical angles in degrees |
| "Elevation angles in degrees for batch='orbit' (default [0, 30, -15]). " | ||
| "E.g., [0, 30, 60] for ground-level, mid, and high views."] | None = None, | ||
| orbit_distance: Annotated[float | str, | ||
| "Camera distance from target for batch='orbit' (default auto from bounds)."] | None = None, |
There was a problem hiding this comment.
manage_scene’s orbit_distance docstring says the default is “auto from bounds”, but on the Unity side if look_at is provided as a raw position (not a GameObject with renderers), the radius defaults to 5 when orbit_distance is omitted. Consider clarifying the docstring to describe this behavior (auto-fit when bounds are available, otherwise a fixed fallback).
Suggested change
| "Camera distance from target for batch='orbit' (default auto from bounds)."] | None = None, | |
| "Camera distance from target for batch='orbit'. Default auto-fits from renderer bounds when available; " | |
| "if only a raw position is provided (no bounds), a fixed fallback distance (5 units) is used."] | None = None, |
Comment on lines
+198
to
+202
| # orbit batch params | ||
| coerced_orbit_angles = coerce_int(orbit_angles, default=None) | ||
| if coerced_orbit_angles is not None: | ||
| params["orbitAngles"] = coerced_orbit_angles | ||
| if orbit_elevations is not None: |
There was a problem hiding this comment.
Orbit parameters were added here, but there are existing integration/unit tests for manage_scene screenshot param forwarding in Server/tests/integration/test_manage_scene_screenshot_params.py. Adding similar tests for orbit_angles/orbit_elevations/orbit_distance/orbit_fov (including the JSON-string parsing path) would help prevent regressions.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
unity-mcp-skill/references/tools-reference.md (1)
139-150: Minor clarification: example uses custom elevations, not the default.Line 145 shows
orbit_elevations=[0, 30]which is a valid custom value, while the inline comment notes the default is[0, 30, -15]. This is correct as written (demonstrating customization), but consider either using the default value in the example or adding a brief note that this overrides the default.🤖 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 139 - 150, The example for manage_scene shows orbit_elevations=[0, 30] but the inline comment mentions the default [0, 30, -15]; update the snippet to make intent clear by either reverting orbit_elevations to the default (orbit_elevations=[0, 30, -15]) or keep the custom [0, 30] and add a short clarifying comment after orbit_elevations such as "overrides default [0, 30, -15]" so readers know this example demonstrates customization; refer to manage_scene and the orbit_elevations parameter when making the change.MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
331-332: Assumes all tiles have uniform dimensions.The method reads
tileWandtileHfromtiles[0]only. If callers pass tiles with varying dimensions, the contact sheet will be corrupted. This is likely fine sinceRenderCameraToTextureproduces consistent sizes, but consider adding a validation or documenting this precondition.📝 Optional: Add precondition check
int tileW = tiles[0].width; int tileH = tiles[0].height; int count = tiles.Count; + +#if UNITY_ASSERTIONS +for (int i = 1; i < count; i++) +{ + UnityEngine.Assertions.Assert.AreEqual(tileW, tiles[i].width, $"Tile {i} width mismatch"); + UnityEngine.Assertions.Assert.AreEqual(tileH, tiles[i].height, $"Tile {i} height mismatch"); +} +#endif🤖 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 331 - 332, The code assumes uniform tile dimensions by reading tileW/tileH from tiles[0]; add a validation in the same method (the contact-sheet builder that uses tiles[] and tileW/tileH) to verify every Texture2D in tiles has the same width and height as tiles[0] (or explicitly document the precondition that callers of RenderCameraToTexture must produce uniform sizes); if a mismatch is found, either throw an ArgumentException (or log an error and abort/return) with a clear message referencing tiles and expected vs actual dimensions so corrupted contact sheets are avoided.MCPForUnity/Editor/Tools/ManageScene.cs (1)
105-109: Consider adoptingToolParamsfor parameter validation in future refactors.As per coding guidelines,
ToolParamswith methods likeGetInt(),RequireString()is preferred for C# Editor tools. The current code usesParamCoercionwhich is consistent with the existing pattern in this file. This is noted as a future improvement opportunity rather than a blocking issue for this PR.🤖 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 105 - 109, The current parameter coercion in ManageScene.cs uses ParamCoercion and ParseFloatArray for orbitAngles, orbitElevations, orbitDistance and orbitFov; refactor these usages to use the Editor tooling's ToolParams API in a future change by replacing ParamCoercion.CoerceIntNullable/CoerceFloatNullable and ParseFloatArray calls with the appropriate ToolParams methods (e.g., GetInt(), GetFloat(), RequireString(), or nullable variants) so validation and error messages are consistent; target the code that constructs orbitAngles, orbitElevations, orbitDistance and orbitFov and switch to reading from a ToolParams instance instead of directly using ParamCoercion/ParseFloatArray.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 105-109: The current parameter coercion in ManageScene.cs uses
ParamCoercion and ParseFloatArray for orbitAngles, orbitElevations,
orbitDistance and orbitFov; refactor these usages to use the Editor tooling's
ToolParams API in a future change by replacing
ParamCoercion.CoerceIntNullable/CoerceFloatNullable and ParseFloatArray calls
with the appropriate ToolParams methods (e.g., GetInt(), GetFloat(),
RequireString(), or nullable variants) so validation and error messages are
consistent; target the code that constructs orbitAngles, orbitElevations,
orbitDistance and orbitFov and switch to reading from a ToolParams instance
instead of directly using ParamCoercion/ParseFloatArray.
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 331-332: The code assumes uniform tile dimensions by reading
tileW/tileH from tiles[0]; add a validation in the same method (the
contact-sheet builder that uses tiles[] and tileW/tileH) to verify every
Texture2D in tiles has the same width and height as tiles[0] (or explicitly
document the precondition that callers of RenderCameraToTexture must produce
uniform sizes); if a mismatch is found, either throw an ArgumentException (or
log an error and abort/return) with a clear message referencing tiles and
expected vs actual dimensions so corrupted contact sheets are avoided.
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 139-150: The example for manage_scene shows orbit_elevations=[0,
30] but the inline comment mentions the default [0, 30, -15]; update the snippet
to make intent clear by either reverting orbit_elevations to the default
(orbit_elevations=[0, 30, -15]) or keep the custom [0, 30] and add a short
clarifying comment after orbit_elevations such as "overrides default [0, 30,
-15]" so readers know this example demonstrates customization; refer to
manage_scene and the orbit_elevations parameter when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.claude/skills/unity-mcp-skill/references/tools-reference.mdMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csServer/src/services/tools/manage_scene.pydocs/guides/CLI_USAGE.mdunity-mcp-skill/SKILL.mdunity-mcp-skill/references/tools-reference.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/skills/unity-mcp-skill/references/tools-reference.md
- unity-mcp-skill/SKILL.md
- docs/guides/CLI_USAGE.md
This was referenced Mar 6, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Provide a multi-view screenshot feature that lets you save multiple screenshots in a contact sheet-like layout.
Type of Change
Save your change type
Changes Made
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Add configurable multi-angle screenshot contact sheets and extend screenshot tooling and docs to support the new orbit batch mode.
New Features:
Enhancements:
Build:
Documentation:
Summary by CodeRabbit
New Features
Documentation