Merged
Conversation
Contributor
Reviewer's GuideAdds experimental Sequence diagram for a manage_probuilder mesh edit callsequenceDiagram
actor User
participant LLM
participant Server as manage_probuilder_py
participant Transport as UnityTransport
participant Editor as ManageProBuilder_cs
participant ProBuilder as ProBuilder_Package
User->>LLM: Request to extrude top face of cube
LLM->>Server: manage_probuilder(action="extrude_faces",\n target="MyCube", properties={faceIndices:[2],distance:1.5})
Server->>Transport: send_with_unity_instance("manage_probuilder", params)
Transport->>Editor: HandleCommand(params)
Editor->>Editor: EnsureProBuilder()\n(reflect types, patch material)
Editor->>Editor: RequireProBuilderMesh(target)
Editor->>ProBuilder: ExtrudeElements.Extrude(mesh, faces, method, distance)
ProBuilder-->>Editor: Mesh modified in memory
Editor->>ProBuilder: ToMesh() and Refresh()
Editor-->>Transport: SuccessResponse(data)
Transport-->>Server: result
Server-->>LLM: JSON tool result
LLM-->>User: Describes change and next steps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ProBuilder mesh-editing support across the stack: Unity Editor C# utilities and tests, server-side manage_probuilder tool and CLI commands, docs and skill updates, tool registry/UI integration, and scene screenshot capture enhancements. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI/User
participant Server as MCP Server
participant Unity as Unity Editor
participant PB as ProBuilder
User->>Server: probuilder <action, target, params>
Server->>Unity: send_with_unity_instance("manage_probuilder", params)
Unity->>PB: ManageProBuilder invokes action (reflection / ProBuilder API)
PB-->>Unity: return result (JSON payload)
Unity-->>Server: forward result
Server-->>User: print formatted result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 5 issues, and left some high level feedback:
- The
ManageProBuilderclass has grown into a very large, reflection-heavy monolith (~2.5k lines); consider splitting per concern (e.g. shape creation, mesh editing, vertex ops, query/smoothing) or using partial classes to keep the code easier to navigate and reason about. - Given that
set_pivotandconvert_to_probuilderare documented as not yet working, you might want to explicitly gate or short‑circuit those actions (e.g. return a clear error before attempting the operation) so callers get consistent failure modes instead of potentially fragile partial behavior. - The ProBuilder material patch in
PatchProBuilderDefaultMaterialrelies on a hard‑coded package asset path; it may be worth guarding it with a version/exists check or centralizing the path as a constant so that Unity/ProBuilder package layout changes are easier to handle.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ManageProBuilder` class has grown into a very large, reflection-heavy monolith (~2.5k lines); consider splitting per concern (e.g. shape creation, mesh editing, vertex ops, query/smoothing) or using partial classes to keep the code easier to navigate and reason about.
- Given that `set_pivot` and `convert_to_probuilder` are documented as not yet working, you might want to explicitly gate or short‑circuit those actions (e.g. return a clear error before attempting the operation) so callers get consistent failure modes instead of potentially fragile partial behavior.
- The ProBuilder material patch in `PatchProBuilderDefaultMaterial` relies on a hard‑coded package asset path; it may be worth guarding it with a version/exists check or centralizing the path as a constant so that Unity/ProBuilder package layout changes are easier to handle.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.cs" line_range="2106-2115" />
<code_context>
+ var meshRenderer = pbMesh.gameObject.GetComponent<MeshRenderer>();
</code_context>
<issue_to_address>
**issue (bug_risk):** MeshRenderer changes in SetFaceMaterial are not recorded for Undo, and the material compaction logic mutates both faces and renderer without separate undo tracking.
Because only `pbMesh` is registered with Undo, the changes to `Face.submeshIndex` and `meshRenderer.sharedMaterials` won’t roll back together. Please also register the `MeshRenderer` (and, if applicable, the underlying `Mesh`) in the same undo group before modifying `sharedMaterials` and submesh indices so the operation is fully reversible.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs" line_range="209-212" />
<code_context>
+ });
+ }
+
+ private static void SetVertexPositions(Component pbMesh, Vector3[] positions)
+ {
+ var positionsProp = ManageProBuilder._proBuilderMeshType.GetProperty("positions");
+ if (positionsProp != null && positionsProp.CanWrite)
+ positionsProp.SetValue(pbMesh, new List<Vector3>(positions));
+ }
</code_context>
<issue_to_address>
**issue:** SetVertexPositions only handles the writable positions property and lacks the fallbacks used elsewhere, which may break on some ProBuilder versions.
This assumes `ProBuilderMesh.positions` is writable and accepts a `List<Vector3>`. In `ManageProBuilder.MoveVertices`, you already handle the non-writable case via `SetPositions` / `RebuildWithPositionsAndFaces`. To keep behavior consistent across ProBuilder versions, please reuse that fallback logic here (e.g., via a shared helper) instead of this reduced implementation.
</issue_to_address>
### Comment 3
<location path="Server/src/services/tools/manage_probuilder.py" line_range="117-118" />
<code_context>
+ ctx: Context,
+ action: Annotated[str, "Action to perform."],
+ target: Annotated[str | None, "Target GameObject (name/path/id)."] = None,
+ search_method: Annotated[
+ Literal["by_id", "by_name", "by_path", "by_tag", "by_layer"] | None,
+ "How to find the target GameObject.",
+ ] = None,
</code_context>
<issue_to_address>
**issue (bug_risk):** The search_method Literal excludes 'by_component', which other tools and the CLI helper allow.
The type for `manage_probuilder`’s `search_method` doesn’t include `by_component`, even though other tools (and the CLI via `SEARCH_METHOD_CHOICE_TAGGED`) support it. If `ObjectResolver` accepts `by_component` here, please add it to the `Literal` so the annotation matches the actual valid values and doesn’t mislead type‑aware tooling or clients.
</issue_to_address>
### Comment 4
<location path="Server/src/cli/commands/probuilder.py" line_range="16-24" />
<code_context>
+_PB_TOP_LEVEL_KEYS = {"action", "target", "searchMethod", "properties"}
+
+
+def _parse_edges_param(edges: str) -> dict[str, Any]:
+ """Parse edge JSON into either 'edges' (vertex pairs) or 'edgeIndices' (flat indices)."""
+ import json
+ try:
+ parsed = json.loads(edges)
+ except json.JSONDecodeError:
+ print_error("Invalid JSON for edges parameter")
+ raise SystemExit(1)
+ if parsed and isinstance(parsed[0], dict):
+ return {"edges": parsed}
+ return {"edgeIndices": parsed}
</code_context>
<issue_to_address>
**issue:** _parse_edges_param assumes the parsed JSON is indexable, which will break for non-list JSON inputs.
In `_parse_edges_param`, `parsed[0]` is accessed without confirming `parsed` is a list. If the JSON decodes to a scalar or dict, this will raise a `TypeError` instead of producing a clear, user-facing error. Consider checking `isinstance(parsed, list)` (and perhaps that it’s non-empty) before indexing and return a friendly message when the structure is invalid.
</issue_to_address>
### Comment 5
<location path="unity-mcp-skill/references/probuilder-guide.md" line_range="202" />
<code_context>
+### Auto-Smooth (Recommended Default)
+
+```python
+# Apply auto-smoothing with default 30 degree threshold
+manage_probuilder(action="auto_smooth", target="MyMesh",
+ properties={"angleThreshold": 30})
</code_context>
<issue_to_address>
**nitpick (typo):** Hyphenate "30-degree" when used as a compound modifier.
As a unit modifying “threshold,” it should be written as “30-degree threshold.”
```suggestion
# Apply auto-smoothing with default 30-degree threshold
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+2106
to
+2115
| var meshRenderer = pbMesh.gameObject.GetComponent<MeshRenderer>(); | ||
| if (meshRenderer != null) | ||
| { | ||
| var allFacesList = (System.Collections.IList)GetFacesArray(pbMesh); | ||
| var submeshIndexProp = _faceType.GetProperty("submeshIndex"); | ||
| var currentMats = meshRenderer.sharedMaterials; | ||
|
|
||
| var usedIndices = new SortedSet<int>(); | ||
| foreach (var f in allFacesList) | ||
| usedIndices.Add((int)submeshIndexProp.GetValue(f)); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): MeshRenderer changes in SetFaceMaterial are not recorded for Undo, and the material compaction logic mutates both faces and renderer without separate undo tracking.
Because only pbMesh is registered with Undo, the changes to Face.submeshIndex and meshRenderer.sharedMaterials won’t roll back together. Please also register the MeshRenderer (and, if applicable, the underlying Mesh) in the same undo group before modifying sharedMaterials and submesh indices so the operation is fully reversible.
Comment on lines
+209
to
+212
| private static void SetVertexPositions(Component pbMesh, Vector3[] positions) | ||
| { | ||
| var positionsProp = ManageProBuilder._proBuilderMeshType.GetProperty("positions"); | ||
| if (positionsProp != null && positionsProp.CanWrite) |
Contributor
There was a problem hiding this comment.
issue: SetVertexPositions only handles the writable positions property and lacks the fallbacks used elsewhere, which may break on some ProBuilder versions.
This assumes ProBuilderMesh.positions is writable and accepts a List<Vector3>. In ManageProBuilder.MoveVertices, you already handle the non-writable case via SetPositions / RebuildWithPositionsAndFaces. To keep behavior consistent across ProBuilder versions, please reuse that fallback logic here (e.g., via a shared helper) instead of this reduced implementation.
Comment on lines
+117
to
+118
| search_method: Annotated[ | ||
| Literal["by_id", "by_name", "by_path", "by_tag", "by_layer"] | None, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The search_method Literal excludes 'by_component', which other tools and the CLI helper allow.
The type for manage_probuilder’s search_method doesn’t include by_component, even though other tools (and the CLI via SEARCH_METHOD_CHOICE_TAGGED) support it. If ObjectResolver accepts by_component here, please add it to the Literal so the annotation matches the actual valid values and doesn’t mislead type‑aware tooling or clients.
Comment on lines
+16
to
+24
| def _parse_edges_param(edges: str) -> dict[str, Any]: | ||
| """Parse edge JSON into either 'edges' (vertex pairs) or 'edgeIndices' (flat indices).""" | ||
| import json | ||
| try: | ||
| parsed = json.loads(edges) | ||
| except json.JSONDecodeError: | ||
| print_error("Invalid JSON for edges parameter") | ||
| raise SystemExit(1) | ||
| if parsed and isinstance(parsed[0], dict): |
Contributor
There was a problem hiding this comment.
issue: _parse_edges_param assumes the parsed JSON is indexable, which will break for non-list JSON inputs.
In _parse_edges_param, parsed[0] is accessed without confirming parsed is a list. If the JSON decodes to a scalar or dict, this will raise a TypeError instead of producing a clear, user-facing error. Consider checking isinstance(parsed, list) (and perhaps that it’s non-empty) before indexing and return a friendly message when the structure is invalid.
| ### Auto-Smooth (Recommended Default) | ||
|
|
||
| ```python | ||
| # Apply auto-smoothing with default 30 degree threshold |
Contributor
There was a problem hiding this comment.
nitpick (typo): Hyphenate "30-degree" when used as a compound modifier.
As a unit modifying “threshold,” it should be written as “30-degree threshold.”
Suggested change
| # Apply auto-smoothing with default 30 degree threshold | |
| # Apply auto-smoothing with default 30-degree threshold |
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/i18n/README-zh.md (1)
94-95:⚠️ Potential issue | 🟡 Minor
manage_probuilderis missing from the available tools list.The new release notes above mention ProBuilder support, but this tool inventory still omits
manage_probuilder. Readers scanning the tool list won't discover the new feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/i18n/README-zh.md` around lines 94 - 95, The tools list is missing the new manage_probuilder entry; update the tools inventory line that enumerates available tools (the backticked list containing manage_animation, manage_asset, manage_components, etc.) to include `manage_probuilder` among the other manage_* items so the README reflects the ProBuilder support mentioned in the release notes.
🧹 Nitpick comments (7)
MCPForUnity/Editor/Tools/ManageScene.cs (3)
940-941: Minor inefficiency: redundant base64 encode/decode round-trip.The image is rendered, encoded to base64 by
RenderCameraToBase64, then decoded back to bytes here for disk write. For large images, this adds unnecessary CPU overhead.Consider having
ScreenshotUtilityexpose a method that returns raw bytes (or both bytes and base64), allowing direct disk write without the decode step:♻️ Suggested approach
// In ScreenshotUtility, add or modify to return raw bytes: // public static (byte[] pngBytes, string base64, int w, int h) RenderCameraToBytes(...) // Then in CapturePositionedScreenshot: var (pngBytes, b64, w, h) = ScreenshotUtility.RenderCameraToBytes(tempCam, maxRes); File.WriteAllBytes(fullPath, pngBytes);This is a minor optimization and can be deferred if the utility refactor isn't trivial.
🤖 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 940 - 941, The code does an unnecessary base64 round-trip: RenderCameraToBase64 in ScreenshotUtility returns a base64 string which CapturePositionedScreenshot immediately decodes into bytes before File.WriteAllBytes; modify ScreenshotUtility to expose a bytes-returning API (e.g., add RenderCameraToBytes or change RenderCameraToBase64 to also return byte[]/tuple) and update CapturePositionedScreenshot to call that new method and pass the returned pngBytes directly to File.WriteAllBytes(fullPath, pngBytes) instead of decoding a base64 string; keep the original base64 overload if needed for backward compatibility.
929-939: Optional: Add upper bound to filename collision loop.The while loop has no upper bound. While practically impossible to exhaust (would require thousands of screenshots in the same second), a defensive upper limit improves robustness:
♻️ Defensive improvement
int counter = 1; -while (File.Exists(fullPath)) +const int maxAttempts = 1000; +while (File.Exists(fullPath) && counter < maxAttempts) { fullPath = Path.Combine(screenshotsFolder, $"{baseName}_{counter}{ext}"); counter++; } +if (counter >= maxAttempts) + return new ErrorResponse($"Could not generate unique filename after {maxAttempts} attempts.");This is a minor defensive coding suggestion and can be safely deferred.
🤖 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 929 - 939, The filename-collision while loop that increments counter on fullPath (using baseName, ext and screenshotsFolder) has no upper bound; add a defensive maximum attempts constant (e.g., maxAttempts) and stop the loop after that many iterations, then handle the failure (log/error or throw/return a fallback filename) so you don't loop indefinitely — change the loop that uses counter and File.Exists(fullPath) to also check counter < maxAttempts and handle the exceeded-attempts case.
668-671: Minor performance consideration with per-frame refresh calls.These refresh calls are executed inside the capture loop (6 iterations here, but up to 108+ in
CaptureOrbitBatch). While necessary to ensure materials render correctly, this adds overhead per shot.If performance becomes a concern for large orbit batches, consider batching the refresh: call once before the loop, then delay briefly (e.g., using
EditorApplication.Step()or a small frame wait) rather than refreshing before every individual capture. That said, if this resolves material rendering issues reliably, the current approach is acceptable.🤖 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 668 - 671, The per-iteration material refresh calls (EditorApplication.QueuePlayerLoopUpdate and SceneView.RepaintAll) inside the capture loop in ManageScene (see methods CaptureOrbit and/or CaptureOrbitBatch) cause extra overhead; move these calls out of the inner loop and invoke them once before starting the loop to batch refresh materials, and if needed add a short frame advance/wait (e.g., using EditorApplication.Step or a small frame delay) after that single refresh to ensure materials have updated before the first capture; update CaptureOrbitBatch/CaptureOrbit to remove the per-iteration QueuePlayerLoopUpdate/RepaintAll and perform a single pre-loop refresh instead.MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
207-208: Key this off the actual group name, not the prefs key.
prefsSuffixis a persistence/detail string, so tying experimental behavior to"group-probuilder"makes this warning depend on the current EditorPrefs naming scheme. Passing the semantic group key intoBuildCategorywould keep the behavior aligned with the registeredprobuildergroup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs` around lines 207 - 208, The code gates experimental behavior by comparing prefsSuffix to the persistence string "group-probuilder"; instead compare the actual semantic group key instead — update BuildCategory's call sites to accept and pass the registered group key (e.g., "probuilder") into the method, and change the isExperimental check to compare that group key (not prefsSuffix) using StringComparison.OrdinalIgnoreCase so experimental flags are driven by the registered group name rather than the EditorPrefs suffix; adjust the BuildCategory signature and all callers accordingly to propagate the semantic group identifier.Server/src/cli/commands/probuilder.py (1)
19-26: Use explicit exception chaining for cleaner tracebacks.Per static analysis (B904), within an
exceptclause, useraise ... from Noneto suppress the exception chain, providing cleaner error output.Suggested fix
try: parsed = json.loads(edges) except json.JSONDecodeError: print_error("Invalid JSON for edges parameter") - raise SystemExit(1) + raise SystemExit(1) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/cli/commands/probuilder.py` around lines 19 - 26, The except block catching json.JSONDecodeError should suppress exception chaining by re-raising the SystemExit with explicit exception chaining; update the except that currently calls print_error("Invalid JSON for edges parameter") and raise SystemExit(1) so it uses "raise SystemExit(1) from None" after the print_error call to avoid exposing the original JSONDecodeError traceback; locate the json.loads(edges) try/except and the print_error / raise SystemExit(1) lines to make this change.MCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs (1)
233-261: Consider logging swallowed exceptions for debugging.The bare
catchblocks silently swallow all exceptions when falling back between overloads. While this is intentional for API compatibility, unexpected exceptions could make debugging difficult.Optional: Add debug logging
catch { + // First overload failed, try alternate signature // Some overloads differ; try without faces param try { // ... existing fallback code ... } - catch + catch (Exception fallbackEx) { - // Ignore fallback failure + // Both overloads failed - log for debugging + UnityEngine.Debug.LogWarning($"RepairMesh fallback failed: {fallbackEx.Message}"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs` around lines 233 - 261, The catch blocks around removeMethod.Invoke and the fallback altMethod.Invoke silently swallow all exceptions; update them to log the caught exceptions (at debug/verbose level) so unexpected errors are visible while preserving the fallback behavior: in the first catch capture the exception from removeMethod.Invoke and log it via the existing logger (or UnityEngine.Debug.LogException) including context (method name RemoveDegenerateTriangles, pbMesh, allFaces) before attempting the overload fallback, and in the inner catch similarly log the exception from altMethod.Invoke (referencing ManageProBuilder._meshValidationType, ManageProBuilder._proBuilderMeshType and the repaired variable) instead of leaving it empty.Server/src/services/registry/tool_registry.py (1)
25-25: Consider using a standard hyphen for consistency.The description contains an EN DASH (
–) which may cause inconsistency with other entries that use the standard hyphen-minus (-). This could also affect searchability.Suggested fix
- "probuilder": "ProBuilder 3D modeling – requires com.unity.probuilder package", + "probuilder": "ProBuilder 3D modeling - requires com.unity.probuilder package",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/registry/tool_registry.py` at line 25, The "probuilder" registry entry value contains an EN DASH (–); locate the "probuilder" mapping in tool_registry (the string value "ProBuilder 3D modeling – requires com.unity.probuilder package") and replace the EN DASH with a standard hyphen-minus (-) so the description reads "ProBuilder 3D modeling - requires com.unity.probuilder package" to ensure consistency and searchability.
🤖 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/tools-reference.md:
- Around line 849-869: The catalog lists experimental actions
(convert_to_probuilder, set_pivot) as exposed but omits them from the Query and
Mesh Utilities sections, creating a contradiction; update the reference so the
action lists and the "choose an action" guidance match by either (A) adding
convert_to_probuilder to the Query or Mesh Utilities lists and set_pivot to Mesh
Utilities with a clear "(experimental / known-bug: see Not Yet Working)" tag, or
(B) remove these actions from the exposed action surface and only keep them in
the "Not Yet Working" section—also update the line that instructs readers to
choose an action from the categories to mention that experimental/buggy actions
are listed with that tag if you choose option A.
- Around line 813-815: The "create_shape" tool description incorrectly states
"13 types" while listing 12 shapes; update the documentation by either adding
the missing shape name to the list of ProBuilder primitives or changing the
count to "12 types" so they match; locate the create_shape entry (the heading
"Shape Creation:" and the list that follows) and either append the missing
primitive name to the existing list of Cube, Cylinder, Sphere, Plane, Cone,
Torus, Pipe, Arch, Stair, CurvedStair, Door, Prism or replace "13 types:" with
"12 types" to keep the description consistent.
In `@docs/guides/CLI_USAGE.md`:
- Around line 285-288: Update the hard-coded header "Raw ProBuilder actions
(access all 21 actions)" so it no longer understates available actions: either
remove the parenthetical count entirely or replace "21" with a dynamic/sourced
value from the authoritative ProBuilder action list; adjust the related examples
(the three sample commands including "unity-mcp probuilder raw extrude_faces",
"bevel_edges", and "set_face_material") as needed to avoid implying a fixed
action count and ensure the header accurately reflects that all raw ProBuilder
actions are accessible.
In `@MCPForUnity/Editor/Tools/ProBuilder/ProBuilderSmoothing.cs`:
- Around line 90-94: The response is reporting facesList.Count (all faces)
instead of the actual number smoothed; change the SuccessResponse payload to
report the count of faces actually processed (e.g., facesToSmooth.Count or the
variable used after filtering) so when faceIndices is provided the returned
faceCount reflects the smoothed subset; update the call that constructs the
SuccessResponse (the return statement creating new SuccessResponse(...) near the
end of ProBuilderSmoothing) to use the filtered collection's Count and keep
angleThreshold as-is.
In `@Server/src/cli/commands/probuilder.py`:
- Line 155: SEARCH_METHODS_TAGGED currently omits "by_layer" which causes the
CLI option SEARCH_METHOD_CHOICE_TAGGED to be out of sync with the backend
(manage_probuilder.py accepts "by_layer"); update the constants by adding
"by_layer" to SEARCH_METHODS_TAGGED (and any derived SEARCH_METHOD_CHOICE_TAGGED
or related choice lists) so the click.option("--search-method",
type=SEARCH_METHOD_CHOICE_TAGGED) accepts the same Literal values as
manage_probuilder.py; ensure any tests or usages referencing
SEARCH_METHODS_TAGGED/SEARCH_METHOD_CHOICE_TAGGED are updated accordingly.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.cs`:
- Around line 248-254: The test currently asserts that data["faces"] is not null
even though GetMeshInfo default include is "summary" and does not return faces;
update the test to either remove the faces assertion or call GetMeshInfo with
include="faces" so the response contains faces; specifically modify the code
path that produces result["data"] (the GetMeshInfo invocation used by this test)
or the assertion block that checks data["faces"] to align with
GetMeshInfo_DefaultInclude_ReturnsSummaryOnly behavior.
In `@unity-mcp-skill/references/probuilder-guide.md`:
- Around line 139-146: The Python examples use JSON boolean literals
`false`/`true` which will raise NameError in Python; update the
`manage_probuilder` example calls (the properties dicts used with action
"detach_faces" and target "MyCube") to use Python booleans `False` and `True`
(e.g., replace `deleteSourceFaces: false` with `deleteSourceFaces: False` and
`deleteSourceFaces: true` with `deleteSourceFaces: True`) so the snippets are
valid Python.
- Around line 41-82: Update the section heading text from "All 13 Shape Types"
to "All 12 Shape Types" to accurately reflect the primitives created by
manage_probuilder(action="create_shape"); leave create_poly_shape
(manage_probuilder(action="create_poly_shape")) as a separate action and do not
include it in the count, and ensure any nearby explanatory text references only
the 12 create_shape primitives (Cube, Cylinder, Sphere, Plane, Cone, Prism,
Torus, Pipe, Arch, Stair, CurvedStair, Door).
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 867-869: The documentation lists actions `set_pivot` and
`convert_to_probuilder` only in the "Not Yet Working" caveats but omits them
from the main action catalog, causing inconsistency; either add entries for
`set_pivot` and `convert_to_probuilder` to the action catalog above (include
their parameter shape and mark them as "experimental" or "broken" with a short
note and link to the caveat), or remove these two action names from the public
reference entirely; update the catalog section that says "see categories below"
to reflect whichever choice so the reference is consistent.
- Around line 813-815: The documentation for create_shape lists 13 types but
enumerates 12 names; update the tools-reference entry for create_shape to
reconcile them by either adding the missing primitive name to the enumerated
list (so it contains 13 actual types) or changing the count from 13 to 12;
ensure the enum text near the create_shape signature and the listed types (Cube,
Cylinder, Sphere, Plane, Cone, Torus, Pipe, Arch, Stair, CurvedStair, Door,
Prism) are consistent.
In `@unity-mcp-skill/SKILL.md`:
- Around line 61-62: Update the example in SKILL.md that shows
manage_scene(action="screenshot") so it states the screenshot is saved to the
Screenshots subfolder under Assets (Assets/Screenshots/) instead of just Assets;
edit the example text to reference this correct output directory and ensure any
returned file path example matches the Assets/Screenshots/ location.
---
Outside diff comments:
In `@docs/i18n/README-zh.md`:
- Around line 94-95: The tools list is missing the new manage_probuilder entry;
update the tools inventory line that enumerates available tools (the backticked
list containing manage_animation, manage_asset, manage_components, etc.) to
include `manage_probuilder` among the other manage_* items so the README
reflects the ProBuilder support mentioned in the release notes.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 940-941: The code does an unnecessary base64 round-trip:
RenderCameraToBase64 in ScreenshotUtility returns a base64 string which
CapturePositionedScreenshot immediately decodes into bytes before
File.WriteAllBytes; modify ScreenshotUtility to expose a bytes-returning API
(e.g., add RenderCameraToBytes or change RenderCameraToBase64 to also return
byte[]/tuple) and update CapturePositionedScreenshot to call that new method and
pass the returned pngBytes directly to File.WriteAllBytes(fullPath, pngBytes)
instead of decoding a base64 string; keep the original base64 overload if needed
for backward compatibility.
- Around line 929-939: The filename-collision while loop that increments counter
on fullPath (using baseName, ext and screenshotsFolder) has no upper bound; add
a defensive maximum attempts constant (e.g., maxAttempts) and stop the loop
after that many iterations, then handle the failure (log/error or throw/return a
fallback filename) so you don't loop indefinitely — change the loop that uses
counter and File.Exists(fullPath) to also check counter < maxAttempts and handle
the exceeded-attempts case.
- Around line 668-671: The per-iteration material refresh calls
(EditorApplication.QueuePlayerLoopUpdate and SceneView.RepaintAll) inside the
capture loop in ManageScene (see methods CaptureOrbit and/or CaptureOrbitBatch)
cause extra overhead; move these calls out of the inner loop and invoke them
once before starting the loop to batch refresh materials, and if needed add a
short frame advance/wait (e.g., using EditorApplication.Step or a small frame
delay) after that single refresh to ensure materials have updated before the
first capture; update CaptureOrbitBatch/CaptureOrbit to remove the per-iteration
QueuePlayerLoopUpdate/RepaintAll and perform a single pre-loop refresh instead.
In `@MCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs`:
- Around line 233-261: The catch blocks around removeMethod.Invoke and the
fallback altMethod.Invoke silently swallow all exceptions; update them to log
the caught exceptions (at debug/verbose level) so unexpected errors are visible
while preserving the fallback behavior: in the first catch capture the exception
from removeMethod.Invoke and log it via the existing logger (or
UnityEngine.Debug.LogException) including context (method name
RemoveDegenerateTriangles, pbMesh, allFaces) before attempting the overload
fallback, and in the inner catch similarly log the exception from
altMethod.Invoke (referencing ManageProBuilder._meshValidationType,
ManageProBuilder._proBuilderMeshType and the repaired variable) instead of
leaving it empty.
In `@MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs`:
- Around line 207-208: The code gates experimental behavior by comparing
prefsSuffix to the persistence string "group-probuilder"; instead compare the
actual semantic group key instead — update BuildCategory's call sites to accept
and pass the registered group key (e.g., "probuilder") into the method, and
change the isExperimental check to compare that group key (not prefsSuffix)
using StringComparison.OrdinalIgnoreCase so experimental flags are driven by the
registered group name rather than the EditorPrefs suffix; adjust the
BuildCategory signature and all callers accordingly to propagate the semantic
group identifier.
In `@Server/src/cli/commands/probuilder.py`:
- Around line 19-26: The except block catching json.JSONDecodeError should
suppress exception chaining by re-raising the SystemExit with explicit exception
chaining; update the except that currently calls print_error("Invalid JSON for
edges parameter") and raise SystemExit(1) so it uses "raise SystemExit(1) from
None" after the print_error call to avoid exposing the original JSONDecodeError
traceback; locate the json.loads(edges) try/except and the print_error / raise
SystemExit(1) lines to make this change.
In `@Server/src/services/registry/tool_registry.py`:
- Line 25: The "probuilder" registry entry value contains an EN DASH (–); locate
the "probuilder" mapping in tool_registry (the string value "ProBuilder 3D
modeling – requires com.unity.probuilder package") and replace the EN DASH with
a standard hyphen-minus (-) so the description reads "ProBuilder 3D modeling -
requires com.unity.probuilder package" to ensure consistency and searchability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32fe01d4-fdb8-488f-b66d-0576b7b01142
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.claude/skills/unity-mcp-skill/SKILL.md.claude/skills/unity-mcp-skill/references/tools-reference.md.claude/skills/unity-mcp-skill/references/workflows.mdMCPForUnity/Editor/Tools/GameObjects/GameObjectModify.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ProBuilder.metaMCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.csMCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.cs.metaMCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.csMCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs.metaMCPForUnity/Editor/Tools/ProBuilder/ProBuilderSmoothing.csMCPForUnity/Editor/Tools/ProBuilder/ProBuilderSmoothing.cs.metaMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.csREADME.mdServer/src/cli/commands/probuilder.pyServer/src/cli/main.pyServer/src/services/registry/tool_registry.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_probuilder.pyServer/tests/test_manage_probuilder.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.cs.metadocs/guides/CLI_USAGE.mddocs/i18n/README-zh.mdunity-mcp-skill/SKILL.mdunity-mcp-skill/references/probuilder-guide.mdunity-mcp-skill/references/tools-reference.mdunity-mcp-skill/references/workflows.md
Comment on lines
+849
to
+869
| **Query:** | ||
| - `get_mesh_info` — Get mesh details with `include` parameter: | ||
| - `"summary"` (default): counts, bounds, materials | ||
| - `"faces"`: + face normals, centers, and direction labels (capped at 100) | ||
| - `"edges"`: + edge vertex pairs with world positions (capped at 200, deduplicated) | ||
| - `"all"`: everything | ||
| - `ping` — Check if ProBuilder is available | ||
|
|
||
| **Smoothing:** | ||
| - `set_smoothing` — Set smoothing group on faces (faceIndices, smoothingGroup: 0=hard, 1+=smooth) | ||
| - `auto_smooth` — Auto-assign smoothing groups by angle (angleThreshold: default 30) | ||
|
|
||
| **Mesh Utilities:** | ||
| - `center_pivot` — Move pivot to mesh bounds center | ||
| - `freeze_transform` — Bake transform into vertices, reset transform | ||
| - `validate_mesh` — Check mesh health (read-only diagnostics) | ||
| - `repair_mesh` — Auto-fix degenerate triangles | ||
|
|
||
| **Not Yet Working (known bugs):** | ||
| - `set_pivot` — Vertex positions don't persist through mesh rebuild. Use `center_pivot` or Transform positioning instead. | ||
| - `convert_to_probuilder` — MeshImporter throws internally. Create shapes natively instead. |
Contributor
There was a problem hiding this comment.
The action catalog is incomplete for experimental actions.
convert_to_probuilder and set_pivot are still exposed actions, but they are omitted from the Query/Mesh Utilities lists and only mentioned later as known-bug cases. Since Line 806 tells readers to choose an action from the categories below, this makes the reference self-contradictory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/unity-mcp-skill/references/tools-reference.md around lines
849 - 869, The catalog lists experimental actions (convert_to_probuilder,
set_pivot) as exposed but omits them from the Query and Mesh Utilities sections,
creating a contradiction; update the reference so the action lists and the
"choose an action" guidance match by either (A) adding convert_to_probuilder to
the Query or Mesh Utilities lists and set_pivot to Mesh Utilities with a clear
"(experimental / known-bug: see Not Yet Working)" tag, or (B) remove these
actions from the exposed action surface and only keep them in the "Not Yet
Working" section—also update the line that instructs readers to choose an action
from the categories to mention that experimental/buggy actions are listed with
that tag if you choose option A.
Comment on lines
+90
to
+94
| return new SuccessResponse($"Auto-smoothed with angle threshold {angleThreshold}°", new | ||
| { | ||
| angleThreshold, | ||
| faceCount = facesList.Count, | ||
| }); |
Contributor
There was a problem hiding this comment.
faceCount may not reflect actual smoothed faces when filtering.
When faceIndices is provided, facesToSmooth contains only the specified subset, but the response reports facesList.Count (all faces). Consider reporting the actual count of smoothed faces for accuracy.
Suggested fix
+ int smoothedCount = facesToSmooth is Array arr ? arr.Length : facesList.Count;
+
return new SuccessResponse($"Auto-smoothed with angle threshold {angleThreshold}°", new
{
angleThreshold,
- faceCount = facesList.Count,
+ faceCount = smoothedCount,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ProBuilder/ProBuilderSmoothing.cs` around lines 90 -
94, The response is reporting facesList.Count (all faces) instead of the actual
number smoothed; change the SuccessResponse payload to report the count of faces
actually processed (e.g., facesToSmooth.Count or the variable used after
filtering) so when faceIndices is provided the returned faceCount reflects the
smoothed subset; update the call that constructs the SuccessResponse (the return
statement creating new SuccessResponse(...) near the end of ProBuilderSmoothing)
to use the filtered collection's Count and keep angleThreshold as-is.
Comment on lines
+41
to
+82
| ### All 13 Shape Types | ||
|
|
||
| ```python | ||
| # Basic shapes | ||
| manage_probuilder(action="create_shape", properties={"shape_type": "Cube", "name": "MyCube"}) | ||
| manage_probuilder(action="create_shape", properties={"shape_type": "Sphere", "name": "MySphere"}) | ||
| manage_probuilder(action="create_shape", properties={"shape_type": "Cylinder", "name": "MyCyl"}) | ||
| manage_probuilder(action="create_shape", properties={"shape_type": "Plane", "name": "MyPlane"}) | ||
| manage_probuilder(action="create_shape", properties={"shape_type": "Cone", "name": "MyCone"}) | ||
| manage_probuilder(action="create_shape", properties={"shape_type": "Prism", "name": "MyPrism"}) | ||
|
|
||
| # Parametric shapes | ||
| manage_probuilder(action="create_shape", properties={ | ||
| "shape_type": "Torus", "name": "MyTorus", | ||
| "rows": 16, "columns": 24, "innerRadius": 0.5, "outerRadius": 1.0 | ||
| }) | ||
| manage_probuilder(action="create_shape", properties={ | ||
| "shape_type": "Pipe", "name": "MyPipe", | ||
| "radius": 1.0, "height": 2.0, "thickness": 0.2 | ||
| }) | ||
| manage_probuilder(action="create_shape", properties={ | ||
| "shape_type": "Arch", "name": "MyArch", | ||
| "radius": 2.0, "angle": 180, "segments": 12 | ||
| }) | ||
|
|
||
| # Architectural shapes | ||
| manage_probuilder(action="create_shape", properties={ | ||
| "shape_type": "Stair", "name": "MyStairs", "steps": 10 | ||
| }) | ||
| manage_probuilder(action="create_shape", properties={ | ||
| "shape_type": "CurvedStair", "name": "Spiral", "steps": 12 | ||
| }) | ||
| manage_probuilder(action="create_shape", properties={ | ||
| "shape_type": "Door", "name": "MyDoor" | ||
| }) | ||
|
|
||
| # Custom polygon | ||
| manage_probuilder(action="create_poly_shape", properties={ | ||
| "points": [[0,0,0], [5,0,0], [5,0,5], [2.5,0,7], [0,0,5]], | ||
| "extrudeHeight": 3.0, "name": "Pentagon" | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find files related to ProBuilder implementation
find . -type f -name "*probuilder*" -o -name "*shape*" | head -20Repository: CoplayDev/unity-mcp
Length of output: 241
🏁 Script executed:
# Search for shape type definitions or lists
rg "shape_type.*=|shape.*type.*:|Create.*Shape|SHAPE" --type md --type py --type json | head -40Repository: CoplayDev/unity-mcp
Length of output: 4801
🏁 Script executed:
# Look at the probuilder-guide.md file directly to see full content
wc -l unity-mcp-skill/references/probuilder-guide.md 2>/dev/null || find . -name "probuilder-guide.md" -exec wc -l {} \;Repository: CoplayDev/unity-mcp
Length of output: 113
🏁 Script executed:
# Read manage_probuilder.py to find shape type definitions
cat ./Server/src/services/tools/manage_probuilder.py | head -100Repository: CoplayDev/unity-mcp
Length of output: 5522
🏁 Script executed:
# Search for shape type lists or constants in the implementation
rg "Cube|Cylinder|Sphere|Plane|Cone|Torus|Pipe|Arch|Stair|Door|Prism" ./Server/src/services/tools/manage_probuilder.py -A 2 -B 2Repository: CoplayDev/unity-mcp
Length of output: 592
🏁 Script executed:
# Look for shape type handling/validation in the implementation
rg "create_shape|shape_type.*:" ./Server/src/services/tools/manage_probuilder.py -A 5 | head -80Repository: CoplayDev/unity-mcp
Length of output: 730
🏁 Script executed:
# Check the CLI command definition
cat ./Server/src/cli/commands/probuilder.py | grep -A 20 "def create_shape"Repository: CoplayDev/unity-mcp
Length of output: 1096
Change heading from "All 13 Shape Types" to "All 12 Shape Types".
The section heading claims 13 shape types, but create_shape supports only 12 primitives: Cube, Cylinder, Sphere, Plane, Cone, Torus, Pipe, Arch, Stair, CurvedStair, Door, and Prism. The create_poly_shape is a separate action and should not be counted toward this count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@unity-mcp-skill/references/probuilder-guide.md` around lines 41 - 82, Update
the section heading text from "All 13 Shape Types" to "All 12 Shape Types" to
accurately reflect the primitives created by
manage_probuilder(action="create_shape"); leave create_poly_shape
(manage_probuilder(action="create_poly_shape")) as a separate action and do not
include it in the count, and ensure any nearby explanatory text references only
the 12 create_shape primitives (Cube, Cylinder, Sphere, Plane, Cone, Prism,
Torus, Pipe, Arch, Stair, CurvedStair, Door).
Comment on lines
+867
to
+869
| **Not Yet Working (known bugs):** | ||
| - `set_pivot` — Vertex positions don't persist through mesh rebuild. Use `center_pivot` or Transform positioning instead. | ||
| - `convert_to_probuilder` — MeshImporter throws internally. Create shapes natively instead. |
Contributor
There was a problem hiding this comment.
Document these actions consistently, or hide them consistently.
set_pivot and convert_to_probuilder are called out as known-bug actions here, but they are omitted from the action catalog above. Right now the reference says “see categories below” for valid actions, yet these two real action names only appear in the caveats section with no parameter shape. Either list them in the main catalog with a broken/experimental note, or remove them from the public reference until they are supported.
🤖 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 867 - 869, The
documentation lists actions `set_pivot` and `convert_to_probuilder` only in the
"Not Yet Working" caveats but omits them from the main action catalog, causing
inconsistency; either add entries for `set_pivot` and `convert_to_probuilder` to
the action catalog above (include their parameter shape and mark them as
"experimental" or "broken" with a short note and link to the caveat), or remove
these two action names from the public reference entirely; update the catalog
section that says "see categories below" to reflect whichever choice so the
reference is consistent.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds experimental end-to-end ProBuilder support to MCP for Unity (Unity editor tool + server tool + CLI + docs/tests), enabling in-editor parametric shape creation and mesh editing operations via a single manage_probuilder entrypoint.
Changes:
- Introduces
manage_probuilderacross Unity (C#), Server (Python), and CLI (Click) with action routing and tool-grouping. - Adds documentation/workflows + a dedicated ProBuilder guide, plus updates tool references and recent updates.
- Extends/adjusts screenshot capture behavior and adds test coverage (Unity edit-mode + Python unit tests) for the new tool.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
unity-mcp-skill/references/workflows.md |
Adds ProBuilder workflows section and patterns. |
unity-mcp-skill/references/tools-reference.md |
Documents manage_probuilder parameters/actions. |
unity-mcp-skill/references/probuilder-guide.md |
New detailed ProBuilder guide and best practices. |
unity-mcp-skill/SKILL.md |
Updates skill quickstart/docs; adds ProBuilder row; edits screenshot guidance. |
docs/i18n/README-zh.md |
Adds “recent updates” mentioning ProBuilder support. |
docs/guides/CLI_USAGE.md |
Adds CLI examples and command reference entry for ProBuilder. |
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.cs |
New Unity edit-mode tests for ProBuilder tool behavior. |
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.cs.meta |
Unity meta for new test file. |
Server/uv.lock |
Lockfile updates (incl. fastmcp constraint bump). |
Server/tests/test_manage_probuilder.py |
New Python unit tests for server-side manage_probuilder wrapper. |
Server/src/services/tools/manage_probuilder.py |
New server tool wrapper + action validation and routing to Unity. |
Server/src/services/tools/manage_material.py |
Expands search_method to include by_id. |
Server/src/services/registry/tool_registry.py |
Registers new probuilder tool group label. |
Server/src/cli/main.py |
Registers optional probuilder CLI command group. |
Server/src/cli/commands/probuilder.py |
Adds ProBuilder CLI subcommands + raw escape hatch. |
README.md |
Mentions ProBuilder in recent updates and tools list. |
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs |
Adds ProBuilder tool group UI + experimental warning. |
MCPForUnity/Editor/Tools/ProBuilder/ProBuilderSmoothing.cs |
New smoothing operations for ProBuilder meshes. |
MCPForUnity/Editor/Tools/ProBuilder/ProBuilderSmoothing.cs.meta |
Unity meta for smoothing helper. |
MCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs |
New pivot/transform/validate/repair utilities for ProBuilder meshes. |
MCPForUnity/Editor/Tools/ProBuilder/ProBuilderMeshUtils.cs.meta |
Unity meta for mesh utils helper. |
MCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.cs |
New Unity tool implementing ProBuilder operations via reflection. |
MCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.cs.meta |
Unity meta for new tool. |
MCPForUnity/Editor/Tools/ProBuilder.meta |
Folder meta for new ProBuilder tools directory. |
MCPForUnity/Editor/Tools/ManageScene.cs |
Adjusts batch screenshot radius + refresh + positioned screenshot saving behavior. |
MCPForUnity/Editor/Tools/GameObjects/GameObjectModify.cs |
Adds new_name / newName rename alias support. |
.claude/skills/unity-mcp-skill/references/workflows.md |
Mirrors ProBuilder workflow docs for Claude skill bundle. |
.claude/skills/unity-mcp-skill/references/tools-reference.md |
Mirrors ProBuilder tools reference for Claude skill bundle. |
.claude/skills/unity-mcp-skill/SKILL.md |
Mirrors ProBuilder row addition for Claude skill bundle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+16
to
+26
| def _parse_edges_param(edges: str) -> dict[str, Any]: | ||
| """Parse edge JSON into either 'edges' (vertex pairs) or 'edgeIndices' (flat indices).""" | ||
| import json | ||
| try: | ||
| parsed = json.loads(edges) | ||
| except json.JSONDecodeError: | ||
| print_error("Invalid JSON for edges parameter") | ||
| raise SystemExit(1) | ||
| if parsed and isinstance(parsed[0], dict): | ||
| return {"edges": parsed} | ||
| return {"edgeIndices": parsed} |
There was a problem hiding this comment.
_parse_edges_param assumes json.loads(edges) returns a list and then indexes parsed[0]. If the user passes a JSON object (e.g. '{"a":0,"b":1}') this will raise at runtime (KeyError/TypeError). Validate that the parsed value is a non-empty list before indexing, and emit a clear CLI error if it's not a list of ints or list of {a,b} objects.
docs/guides/CLI_USAGE.md
Outdated
| # Get mesh info | ||
| unity-mcp probuilder info "MyCube" | ||
|
|
||
| # Raw ProBuilder actions (access all 21 actions) |
There was a problem hiding this comment.
This documentation hard-codes "access all 21 actions", but manage_probuilder currently exposes more actions than that. To avoid docs drifting out of date, either update the count to match the actual action list or remove the specific number (e.g., "access all actions").
Suggested change
| # Raw ProBuilder actions (access all 21 actions) | |
| # Raw ProBuilder actions (access all actions) |
Comment on lines
412
to
+413
| | `vfx` | `raw` (access all 60+ actions) | | ||
| | `probuilder` | `create-shape`, `create-poly`, `info`, `raw` (access all 21 actions) | |
There was a problem hiding this comment.
The command reference table also says ProBuilder raw can "access all 21 actions", which will become inaccurate as actions are added/removed. Prefer removing the numeric count or ensuring it is generated/kept in sync with the actual action list.
Suggested change
| | `vfx` | `raw` (access all 60+ actions) | | |
| | `probuilder` | `create-shape`, `create-poly`, `info`, `raw` (access all 21 actions) | | |
| | `vfx` | `raw` (access all VFX actions) | | |
| | `probuilder` | `create-shape`, `create-poly`, `info`, `raw` (access all ProBuilder actions) | |
|
|
||
| ```python | ||
| # Basic screenshot (saves to Assets/Screenshots/, returns file path) | ||
| # Basic screenshot (saves to Assets/, returns file path only) |
There was a problem hiding this comment.
This says screenshots save to "Assets/", but the implementation (ScreenshotUtility / manage_scene) saves under Assets/Screenshots/. Please update the text so users look in the correct folder and the docs match the tool’s response paths.
Suggested change
| # Basic screenshot (saves to Assets/, returns file path only) | |
| # Basic screenshot (saves to Assets/Screenshots/, returns file path only) |
Comment on lines
+668
to
+671
| // Force material refresh before capture | ||
| EditorApplication.QueuePlayerLoopUpdate(); | ||
| SceneView.RepaintAll(); | ||
|
|
There was a problem hiding this comment.
Calling EditorApplication.QueuePlayerLoopUpdate() and SceneView.RepaintAll() inside the per-angle loop will scale linearly with the number of tiles and can significantly slow down batch screenshots. Consider moving these refresh calls outside the loop (once before the capture series) or gating them behind a flag when a refresh is actually needed.
Comment on lines
+811
to
+814
| // Force material refresh before capture | ||
| EditorApplication.QueuePlayerLoopUpdate(); | ||
| SceneView.RepaintAll(); | ||
|
|
There was a problem hiding this comment.
Calling EditorApplication.QueuePlayerLoopUpdate() and SceneView.RepaintAll() for every orbit tile can become a major performance bottleneck for larger azimuth/elevation grids. Consider doing a single refresh before the orbit capture (or only when scene content/materials changed) rather than per-frame per-tile.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
unity-mcp-skill/SKILL.md (1)
61-62:⚠️ Potential issue | 🟡 MinorFix the screenshot save location in this example.
manage_scene(action="screenshot")saves intoAssets/Screenshots/, not directly intoAssets/. Leaving this path wrong will send follow-up asset/file operations to the wrong location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/SKILL.md` around lines 61 - 62, The example shows manage_scene(action="screenshot") saving to Assets/, but the function actually writes screenshots to Assets/Screenshots/; update the example text to reflect the correct save location (Assets/Screenshots/) so follow-up asset/file operations use the right path and continue to mention that the function returns the file path only; refer to manage_scene(action="screenshot") in the doc update.unity-mcp-skill/references/tools-reference.md (1)
806-809:⚠️ Potential issue | 🟡 MinorThe action catalog is still incomplete for exposed-but-buggy actions.
convert_to_probuilderandset_pivotare valid actions in bothServer/src/services/tools/manage_probuilder.py:12-47andMCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.cs:179-240, but this reference only mentions them in the “Not Yet Working” caveat block. Since Line 806 tells readers to choose from the categories below, the catalog should either list both actions with an experimental/known-bug note or remove them from the exposed surface entirely.Also applies to: 849-869
🤖 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 806 - 809, The actions catalog omits two exposed but buggy actions—convert_to_probuilder and set_pivot—which are implemented and exposed in manage_probuilder.py (functions handling those actions) and ManageProBuilder.cs (methods around the ProBuilder management class), so update the reference table to include both actions with an “experimental / known issues” note (or alternatively remove them from the public catalog if you want them hidden); specifically add rows for `convert_to_probuilder` and `set_pivot` in the action table and add a short parenthetical or footnote linking them to the ManageProBuilder/manage_probuilder implementations and their known-bug status so readers aren’t misled by the “choose from the categories below” instruction..claude/skills/unity-mcp-skill/references/tools-reference.md (1)
806-809:⚠️ Potential issue | 🟡 MinorThis catalog still hides two valid actions behind the caveat section.
convert_to_probuilderandset_pivotare accepted actions in both the Python validator and the Unity handler, but this reference omits them from the categorized action list and only mentions them later as broken. That leaves the “choose an action from the categories below” guidance incomplete.Also applies to: 849-869
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/unity-mcp-skill/references/tools-reference.md around lines 806 - 809, The action catalog table omits two valid actions—convert_to_probuilder and set_pivot—that are accepted by the Python validator and Unity handler; update the categorized action list (the sections referenced by “choose an action from the categories below”) to include these two action names under the appropriate category, and remove the caveat-only mention later so the main list is complete; ensure the table row/description for `action` and any category subsections reference `convert_to_probuilder` and `set_pivot` with brief descriptions consistent with how other actions are listed.docs/guides/CLI_USAGE.md (1)
413-413:⚠️ Potential issue | 🟡 MinorUpdate or remove the hard-coded ProBuilder action count.
unity-mcp probuilder rawexposes far more than 21 actions inServer/src/cli/commands/probuilder.py:670-703, so this reference is already stale. Dropping the parenthetical count entirely would avoid future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_USAGE.md` at line 413, Remove the hard-coded "(access all 21 actions)" text from the `probuilder` CLI entry in CLI_USAGE.md (or replace it with a non-count phrase like "access all actions" or a dynamic reference), since the actual actions are defined in the probuilder command implementation (the action list used by the `raw` subcommand in probuilder.py) and the number can change; update the line that contains the `probuilder` entry to drop the parenthetical count or make it descriptive/dynamic instead.
🤖 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/tools-reference.md:
- Around line 804-809: Update the tool reference entry for the `properties`
parameter to reflect that it can be either a dict or a JSON string (e.g., "dict
or JSON string") and expand the Description column to note that the server will
accept and parse JSON strings into dicts (see manage_probuilder.py handling
around the `properties` parameter); this ensures the docs match the actual API
behavior and the server-side parsing logic.
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 923-943: The code unconditionally sets fileName to a timestamped
string, ignoring a caller-supplied cmd.fileName; update the save logic to use
cmd.fileName when present (fall back to the timestamped name only if
cmd.fileName is null/empty), ensure the filename has a .png extension, compute
fullPath from that chosen name, keep the existing unique-filename loop that
references baseName/ext, write the bytes to fullPath, and set assetsRelativePath
using Path.GetFileName(fullPath) so callers receive the honored filename (refer
to the variables fileName, cmd.fileName, fullPath, baseName, ext,
assetsRelativePath).
- Line 862: CapturePositionedScreenshot writes the PNG file directly to disk but
doesn't call AssetDatabase.ImportAsset, so the newly created file isn't
registered by Unity until an automatic refresh; update
CapturePositionedScreenshot to call UnityEditor.AssetDatabase.ImportAsset(path)
after successfully writing the PNG (same as the other branches), and ensure the
method returns the imported asset path or the same path after import so the file
is immediately available to editor operations.
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 804-809: The docs currently list the `properties` parameter as
only `dict`, but the `manage_probuilder` service accepts either a structured
object or a JSON string; update the tools-reference table row for `properties`
to read `dict | JSON string` (or similar concise type union) and add a short
note/example showing both forms; verify against the `manage_probuilder` function
signature/handling to ensure wording matches expected payload parsing and adjust
any examples or schema comments to reflect both accepted shapes.
---
Duplicate comments:
In @.claude/skills/unity-mcp-skill/references/tools-reference.md:
- Around line 806-809: The action catalog table omits two valid
actions—convert_to_probuilder and set_pivot—that are accepted by the Python
validator and Unity handler; update the categorized action list (the sections
referenced by “choose an action from the categories below”) to include these two
action names under the appropriate category, and remove the caveat-only mention
later so the main list is complete; ensure the table row/description for
`action` and any category subsections reference `convert_to_probuilder` and
`set_pivot` with brief descriptions consistent with how other actions are
listed.
In `@docs/guides/CLI_USAGE.md`:
- Line 413: Remove the hard-coded "(access all 21 actions)" text from the
`probuilder` CLI entry in CLI_USAGE.md (or replace it with a non-count phrase
like "access all actions" or a dynamic reference), since the actual actions are
defined in the probuilder command implementation (the action list used by the
`raw` subcommand in probuilder.py) and the number can change; update the line
that contains the `probuilder` entry to drop the parenthetical count or make it
descriptive/dynamic instead.
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 806-809: The actions catalog omits two exposed but buggy
actions—convert_to_probuilder and set_pivot—which are implemented and exposed in
manage_probuilder.py (functions handling those actions) and ManageProBuilder.cs
(methods around the ProBuilder management class), so update the reference table
to include both actions with an “experimental / known issues” note (or
alternatively remove them from the public catalog if you want them hidden);
specifically add rows for `convert_to_probuilder` and `set_pivot` in the action
table and add a short parenthetical or footnote linking them to the
ManageProBuilder/manage_probuilder implementations and their known-bug status so
readers aren’t misled by the “choose from the categories below” instruction.
In `@unity-mcp-skill/SKILL.md`:
- Around line 61-62: The example shows manage_scene(action="screenshot") saving
to Assets/, but the function actually writes screenshots to Assets/Screenshots/;
update the example text to reflect the correct save location
(Assets/Screenshots/) so follow-up asset/file operations use the right path and
continue to mention that the function returns the file path only; refer to
manage_scene(action="screenshot") in the doc update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaac8b65-4b6e-48ef-8135-a65297b240c8
📒 Files selected for processing (7)
.claude/skills/unity-mcp-skill/references/tools-reference.mdMCPForUnity/Editor/Tools/ManageScene.csServer/src/cli/utils/constants.pydocs/guides/CLI_USAGE.mdunity-mcp-skill/SKILL.mdunity-mcp-skill/references/probuilder-guide.mdunity-mcp-skill/references/tools-reference.md
🚧 Files skipped from review as they are similar to previous changes (1)
- unity-mcp-skill/references/probuilder-guide.md
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
unity-mcp-skill/references/tools-reference.md (1)
867-869:⚠️ Potential issue | 🟡 MinorDocument these actions consistently, or hide them consistently.
set_pivotandconvert_to_probuilderare valid action names (present inUTILITY_ACTIONSandQUERY_ACTIONSrespectively perServer/src/services/tools/manage_probuilder.py:12-47) that pass server validation, but they are omitted from the main action catalog above. The current documentation only mentions them in this caveats section with no parameter shape.Either:
- Add entries for
set_pivot(under Mesh Utilities) andconvert_to_probuilder(under Query) to the main catalog with their parameter signatures and mark them as "⚠️ experimental/broken" with a link to this caveat, OR- Remove these action names from the public reference entirely until they are fully supported
The current approach—mentioning them only in caveats—misleads readers about which actions are available, even if buggy.
🤖 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 867 - 869, The doc currently omits two valid action names—set_pivot and convert_to_probuilder—that exist in UTILITY_ACTIONS and QUERY_ACTIONS (see manage_probuilder module) and pass server validation; either add canonical entries for set_pivot (under Mesh Utilities) and convert_to_probuilder (under Query) to the main action catalog including their parameter signatures and a clear "⚠️ experimental/broken" label linking to the caveats section, or remove those names from the public reference entirely so the catalog matches what is supported; update the catalog entries or the caveat text accordingly so the main action list and UTILITY_ACTIONS/QUERY_ACTIONS remain consistent.MCPForUnity/Editor/Tools/ManageScene.cs (1)
923-943:⚠️ Potential issue | 🟡 MinorHonor
cmd.fileNamehere too.This branch still hardcodes a timestamped name, so positioned captures ignore caller-selected output names while the other screenshot flows respect them.
Suggested fix
- string fileName = $"screenshot-{DateTime.Now:yyyyMMdd-HHmmss}.png"; + string baseFileName = string.IsNullOrWhiteSpace(cmd.fileName) + ? $"screenshot-{DateTime.Now:yyyyMMdd-HHmmss}" + : Path.GetFileNameWithoutExtension(cmd.fileName); + string fileName = $"{baseFileName}.png"; string fullPath = Path.Combine(screenshotsFolder, fileName);🤖 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 923 - 943, The save-to-disk branch currently hardcodes a timestamped fileName (using DateTime) instead of honoring the caller-provided cmd.fileName; update the creation of fileName/fullPath so that if cmd.fileName is non-empty you use that (ensure it has a .png extension), otherwise fall back to the timestamped name, then continue the uniqueness loop that updates fullPath and finally write pngBytes as before; look for symbols screenshotsFolder, fileName, fullPath, cmd.fileName, b64 and preserve the existing File.Exists uniqueness logic and assetsRelativePath = "Assets/Screenshots/" + Path.GetFileName(fullPath).
🧹 Nitpick comments (3)
unity-mcp-skill/references/tools-reference.md (2)
800-800: Clarify when ProBuilder preference applies.The guidance to "prefer ProBuilder over primitive GameObjects" is broad. This advice is most valuable for editable geometry, multi-material surfaces, or complex shapes, but adds unnecessary overhead for simple static primitives. Consider qualifying the preference with criteria such as "when geometry editing, face-level materials, or complex shapes are needed" to help agents make context-appropriate choices.
🤖 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` at line 800, The guidance line "prefer ProBuilder over primitive GameObjects" is too broad; update the sentence in the tools-reference text so it qualifies when ProBuilder should be preferred by adding criteria like "when geometry editing, face-level/multi-material surfaces, or complex shapes are needed" and note that simple static primitives can remain as GameObjects; reference the existing phrase "prefer ProBuilder over primitive GameObjects" and the package mention "com.unity.probuilder" when making this clarification.
809-809: Specify "JSON string" for clarity.The
propertiesparameter is documented asdict | string, but the server accepts structured objects or JSON strings. Using "JSON string" instead of "string" would clarify the expected serialized format and align with the implementation inServer/src/services/tools/manage_probuilder.py:113-127.🤖 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` at line 809, Update the tools reference entry for the properties parameter to use "JSON string" instead of "string" so it clearly documents the serialized format accepted by the server; specifically change the table cell for `properties` to "dict | JSON string" and ensure the description still mentions action-specific parameters (dict or JSON string) to align with the implementation in manage_probuilder.py where the server accepts structured objects or JSON-serialized strings.MCPForUnity/Editor/Tools/ManageScene.cs (1)
946-959: Keep the asset-path key consistent with the other screenshot responses.The camera/default screenshot branches return
path, but this branch only emitsfilePath.Server/src/services/tools/manage_scene.py:16-64forwards that payload to clients as-is, so positioned captures become the only single-image screenshot response with a different asset-path key. Consider keepingpathas the canonical field and addingfilePathonly as an alias if you want the new name.Suggested fix
var data = new Dictionary<string, object> { + { "path", assetsRelativePath }, { "imageBase64", b64 }, { "imageWidth", w }, { "imageHeight", h }, { "viewPosition", new[] { camPos.x, camPos.y, camPos.z } }, { "screenshotsFolder", screenshotsFolder }, { "filePath", assetsRelativePath }, };🤖 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 946 - 959, The response payload for positioned screenshots uses the key "filePath" which is inconsistent with other branches that use "path"; update the dictionary construction in ManageScene (the block building 'data' around imageBase64/imageWidth/imageHeight/viewPosition/screenshotsFolder) to include "path" with the same assetsRelativePath value (and optionally also set "filePath" = assetsRelativePath if you want an alias), so clients receive the canonical "path" field like the camera/default screenshot responses before returning the SuccessResponse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 923-943: The save-to-disk branch currently hardcodes a timestamped
fileName (using DateTime) instead of honoring the caller-provided cmd.fileName;
update the creation of fileName/fullPath so that if cmd.fileName is non-empty
you use that (ensure it has a .png extension), otherwise fall back to the
timestamped name, then continue the uniqueness loop that updates fullPath and
finally write pngBytes as before; look for symbols screenshotsFolder, fileName,
fullPath, cmd.fileName, b64 and preserve the existing File.Exists uniqueness
logic and assetsRelativePath = "Assets/Screenshots/" +
Path.GetFileName(fullPath).
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 867-869: The doc currently omits two valid action names—set_pivot
and convert_to_probuilder—that exist in UTILITY_ACTIONS and QUERY_ACTIONS (see
manage_probuilder module) and pass server validation; either add canonical
entries for set_pivot (under Mesh Utilities) and convert_to_probuilder (under
Query) to the main action catalog including their parameter signatures and a
clear "⚠️ experimental/broken" label linking to the caveats section, or remove
those names from the public reference entirely so the catalog matches what is
supported; update the catalog entries or the caveat text accordingly so the main
action list and UTILITY_ACTIONS/QUERY_ACTIONS remain consistent.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 946-959: The response payload for positioned screenshots uses the
key "filePath" which is inconsistent with other branches that use "path"; update
the dictionary construction in ManageScene (the block building 'data' around
imageBase64/imageWidth/imageHeight/viewPosition/screenshotsFolder) to include
"path" with the same assetsRelativePath value (and optionally also set
"filePath" = assetsRelativePath if you want an alias), so clients receive the
canonical "path" field like the camera/default screenshot responses before
returning the SuccessResponse.
In `@unity-mcp-skill/references/tools-reference.md`:
- Line 800: The guidance line "prefer ProBuilder over primitive GameObjects" is
too broad; update the sentence in the tools-reference text so it qualifies when
ProBuilder should be preferred by adding criteria like "when geometry editing,
face-level/multi-material surfaces, or complex shapes are needed" and note that
simple static primitives can remain as GameObjects; reference the existing
phrase "prefer ProBuilder over primitive GameObjects" and the package mention
"com.unity.probuilder" when making this clarification.
- Line 809: Update the tools reference entry for the properties parameter to use
"JSON string" instead of "string" so it clearly documents the serialized format
accepted by the server; specifically change the table cell for `properties` to
"dict | JSON string" and ensure the description still mentions action-specific
parameters (dict or JSON string) to align with the implementation in
manage_probuilder.py where the server accepts structured objects or
JSON-serialized strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6548e05-56d8-4d57-b579-2ec1ebb097e7
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/ManageScene.csunity-mcp-skill/references/tools-reference.md
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
Experimental ProBuilder support that offers full support to the current Native API latest to ProBuilder V6.
Type of Change
Save your change type
Changes Made
Add a new manage_probuilder feature that could directly manipulate probuilder modeling.
Shape Creation
Face Editing
Vertex Operations
Selection
UV & Materials
Smoothing
Utilities
faces/edges/vertices/bounds/materials
unused verts)
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
#862
Additional Notes
Summary by Sourcery
Add experimental ProBuilder mesh editing support across Unity MCP tools, CLI, and documentation, along with related screenshot improvements and minor API tweaks.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Tests