Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @Scriptwonder, your pull request is larger than the review limit of 300000 diff characters
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full graphics management subsystem (volumes, post-processing, light baking, pipeline settings, URP renderer features, rendering stats), new server CLI/tool surface and MCP resources, package deploy/restore editor actions, instance-ID resolution refactor, camera-based screenshot migration, Unity 2022.2+ API guards, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User CLI
participant Server as Server (FastMCP)
participant Unity as Unity Editor (MCP Agent)
participant Ops as Graphics Ops (Editor code)
CLI->>Server: run "graphics" / "editor deploy" command
Server->>Unity: send manage_graphics / manage_editor action + params
Unity->>Ops: ManageGraphics.HandleCommand / ManageEditor.Dispatch
Ops->>Unity: query/modify Assets, Scene, ProjectSettings (AssetDatabase, RenderSettings, LightingSettings)
Ops-->>Unity: return result (data or ErrorResponse)
Unity-->>Server: MCP response forwarded
Server-->>CLI: formatted output / status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new unified manage_graphics tool (and related CLI/resources/docs/tests) for controlling rendering/pipeline features (Volumes, baking, stats, URP renderer features, skybox/environment), and routes screenshot usage away from manage_scene to manage_camera. It also adds editor package deploy/restore actions for faster iterative development.
Changes:
- Added
manage_graphicstool end-to-end (Unity C# tool + Python server tool + CLI command), plus 3 new graphics resources (volumes,rendering_stats,renderer_features). - Removed screenshot support from
manage_sceneand updated docs/CLI examples to usemanage_camerascreenshot commands. - Added
deploy_package/restore_packageactions tomanage_editorand documented the workflow.
Reviewed changes
Copilot reviewed 61 out of 62 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| unity-mcp-skill/references/workflows.md | Adds graphics & package deployment workflows; updates screenshot calls to manage_camera. |
| unity-mcp-skill/references/tools-reference.md | Adds manage_graphics documentation and updates screenshot examples to manage_camera. |
| unity-mcp-skill/references/resources-reference.md | Documents new graphics resources and adds pipeline/rendering categories. |
| unity-mcp-skill/references/probuilder-guide.md | Updates screenshot guidance to manage_camera. |
| unity-mcp-skill/SKILL.md | Updates screenshot examples + adds manage_graphics tool overview. |
| .claude/skills/unity-mcp-skill/SKILL.md | Mirrors SKILL.md updates for Claude skill docs. |
| manifest.json | Registers new manage_graphics tool in the published manifest. |
| README.md / docs/i18n/README-zh.md | Adds release notes + updates tools/resources lists for graphics additions. |
| docs/guides/CLI_USAGE.md / docs/guides/CLI_EXAMPLE.md / Server/src/cli/CLI_USAGE_GUIDE.md | Moves screenshot CLI examples from scene screenshot to camera screenshot and adds references to new CLI patterns. |
| Server/src/services/tools/manage_scene.py | Removes screenshot action/params and related ToolResult/image extraction handling. |
| Server/src/services/tools/manage_graphics.py | Adds server-side manage_graphics tool wrapper and parameter mapping. |
| Server/src/services/tools/manage_editor.py | Extends editor tool actions with deploy/restore package support. |
| Server/src/services/resources/volumes.py | Adds mcpforunity://scene/volumes resource. |
| Server/src/services/resources/rendering_stats.py | Adds mcpforunity://rendering/stats resource. |
| Server/src/services/resources/renderer_features.py | Adds mcpforunity://pipeline/renderer-features resource. |
| Server/src/cli/main.py | Registers new graphics CLI command group. |
| Server/src/cli/commands/scene.py | Removes scene screenshot CLI command. |
| Server/src/cli/commands/graphics.py | Adds graphics CLI command group mapping to manage_graphics. |
| Server/src/cli/commands/editor.py | Adds editor deploy / editor restore CLI commands. |
| Server/tests/test_cli.py | Removes scene screenshot CLI test. |
| Server/tests/integration/test_manage_scene_screenshot_params.py | Removes manage_scene screenshot param forwarding + image extraction tests. |
| Server/uv.lock | Updates server package version entry. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs | Adds extensive EditMode coverage for manage_graphics actions. |
| MCPForUnity/Editor/Tools/Graphics/* | Adds Unity-side ManageGraphics implementation + Volume/Bake/Stats/Pipeline/Features/Skybox ops + helpers. |
| MCPForUnity/Editor/Resources/Scene/*Resource.cs | Adds Unity-side resources for volumes/rendering stats/renderer features. |
| MCPForUnity/Editor/Tools/ManageEditor.cs | Adds Unity-side deploy_package / restore_package actions. |
| MCPForUnity/Editor/Tools/ManageScene.cs | Updates screenshot internals and instance-id resolution APIs. |
| MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | Updates camera discovery implementation. |
| MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | Changes instance-id resolution path during JSON object deserialization. |
| MCPForUnity/Editor/Helpers/ComponentOps.cs | Updates object reference resolution for instance IDs. |
| MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs | Updates instance-id resolution for resources. |
| MCPForUnity/Editor/Resources/Editor/Selection.cs | Changes how active instance ID is reported. |
| MCPForUnity/Editor/Helpers/AssetPathUtility.cs | Adjusts sanitization edge-case for Assets root. |
| MCPForUnity/Editor/Tools/Cameras/CameraHelpers.cs / CameraControl.cs | Updates camera lookup implementations. |
| MCPForUnity/Editor/Tools/ManageAsset.cs | Qualifies Graphics.Blit to UnityEngine.Graphics.Blit. |
💡 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
+28
to
+33
| return new PendingResponse( | ||
| "Light bake started (async). Use bake_get_status to check progress.", | ||
| pollIntervalSeconds: 2.0, | ||
| data: new { mode = "async" } | ||
| ); | ||
| } |
There was a problem hiding this comment.
The pending response message says to poll bake_get_status, but the actual action implemented/registered is bake_status. This makes the guidance incorrect for tool users; update the message to reference the correct action name.
Comment on lines
+50
to
+52
| @mcp_for_unity_tool( | ||
| group="core", | ||
| description=( |
There was a problem hiding this comment.
manage_graphics is registered under group="core", but the PR description/documentation refers to a new "graphics" tool group. If the intent is to scope visibility under the graphics group, this should be group="graphics" (and kept consistent with the Unity-side tool metadata).
Comment on lines
+301
to
+305
| var cubemap = AssetDatabase.LoadAssetAtPath<Texture>(cubemapPath); | ||
| if (cubemap != null) | ||
| RenderSettings.customReflectionTexture = cubemap; | ||
| else | ||
| return new ErrorResponse($"Cubemap not found at '{cubemapPath}'."); |
There was a problem hiding this comment.
skybox_set_reflection accepts a user-provided cubemap asset path but loads it without AssetPathUtility.SanitizeAssetPath(). Please sanitize before LoadAssetAtPath to match repo conventions and prevent path traversal edge cases.
Suggested change
| var cubemap = AssetDatabase.LoadAssetAtPath<Texture>(cubemapPath); | |
| if (cubemap != null) | |
| RenderSettings.customReflectionTexture = cubemap; | |
| else | |
| return new ErrorResponse($"Cubemap not found at '{cubemapPath}'."); | |
| var sanitizedCubemapPath = AssetPathUtility.SanitizeAssetPath(cubemapPath); | |
| var cubemap = AssetDatabase.LoadAssetAtPath<Texture>(sanitizedCubemapPath); | |
| if (cubemap != null) | |
| RenderSettings.customReflectionTexture = cubemap; | |
| else | |
| return new ErrorResponse($"Cubemap not found at '{sanitizedCubemapPath}'."); |
Comment on lines
+448
to
+449
|
|
||
| var allVolumes = UnityEngine.Object.FindObjectsByType(GraphicsHelpers.VolumeType, FindObjectsSortMode.None); |
There was a problem hiding this comment.
FindObjectsByType is not available in Unity 2021.3. Since the package targets 2021.3, this volume enumeration should use FindObjectsOfType (or conditional compilation) to avoid breaking compilation.
Suggested change
| var allVolumes = UnityEngine.Object.FindObjectsByType(GraphicsHelpers.VolumeType, FindObjectsSortMode.None); | |
| #if UNITY_2022_1_OR_NEWER | |
| var allVolumes = UnityEngine.Object.FindObjectsByType(GraphicsHelpers.VolumeType, FindObjectsSortMode.None); | |
| #else | |
| var allVolumes = UnityEngine.Object.FindObjectsOfType(GraphicsHelpers.VolumeType, true); | |
| #endif |
Comment on lines
+94
to
96
| var cams = UnityEngine.Object.FindObjectsByType<Camera>(FindObjectsSortMode.None); | ||
| return cams.FirstOrDefault(); | ||
| } |
There was a problem hiding this comment.
FindObjectsByType<T>(FindObjectsSortMode) is not available in Unity 2021.3 (the package.json declares Unity 2021.3 support), so this change will break compilation on the supported Unity version. Keep FindObjectsOfType<T>() (or wrap these calls in #if UNITY_2022_2_OR_NEWER compatibility).
Comment on lines
+144
to
145
| return UnityEngine.Object.FindFirstObjectByType(_cmBrainType) as Component; | ||
| } |
There was a problem hiding this comment.
FindFirstObjectByType was introduced after Unity 2021.3 and will break compilation on the package's declared supported Unity version. Use FindObjectOfType (or conditional compilation) here.
Comment on lines
+37
to
+41
| foreach (var go in UnityEngine.Object.FindObjectsByType<GameObject>(FindObjectsSortMode.None)) | ||
| { | ||
| if (go.name.StartsWith("GfxTest_")) | ||
| UnityEngine.Object.DestroyImmediate(go); | ||
| } |
There was a problem hiding this comment.
This test uses Object.FindObjectsByType<T>(FindObjectsSortMode.None), which is not available in Unity 2021.3 (the package's declared target). Tests will fail to compile on supported Unity versions unless this is reverted to FindObjectsOfType<T>() or guarded with conditional compilation.
Comment on lines
+40
to
+52
| if (!string.IsNullOrEmpty(profilePath)) | ||
| { | ||
| // Load existing or create new profile asset | ||
| profile = AssetDatabase.LoadAssetAtPath(profilePath, GraphicsHelpers.VolumeProfileType); | ||
| if (profile == null) | ||
| { | ||
| profile = ScriptableObject.CreateInstance(GraphicsHelpers.VolumeProfileType); | ||
| // Ensure directory exists | ||
| var dir = System.IO.Path.GetDirectoryName(profilePath); | ||
| if (!string.IsNullOrEmpty(dir) && !System.IO.Directory.Exists(dir)) | ||
| System.IO.Directory.CreateDirectory(dir); | ||
| AssetDatabase.CreateAsset((UnityEngine.Object)profile, profilePath); | ||
| } |
There was a problem hiding this comment.
VolumeProfile asset paths come from user input (profile_path) but are used directly with LoadAssetAtPath/CreateAsset. To match the existing codebase pattern and avoid path traversal/invalid paths, sanitize these with AssetPathUtility.SanitizeAssetPath() before use.
|
|
||
| GameObject go = null; | ||
| if (int.TryParse(target, out int instanceId)) | ||
| go = EditorUtility.EntityIdToObject(instanceId) as GameObject; |
There was a problem hiding this comment.
EditorUtility.EntityIdToObject is not available in Unity 2021.3 (the package’s declared target), so this will fail to compile. Use EditorUtility.InstanceIDToObject (or a compatibility helper) for instance ID lookups.
Suggested change
| go = EditorUtility.EntityIdToObject(instanceId) as GameObject; | |
| go = EditorUtility.InstanceIDToObject(instanceId) as GameObject; |
|
|
||
| if (int.TryParse(target, out int instanceId)) | ||
| { | ||
| var byId = EditorUtility.EntityIdToObject(instanceId) as GameObject; |
There was a problem hiding this comment.
EditorUtility.EntityIdToObject is not a Unity 2021.3 API and will break compilation on supported Unity versions. Replace with EditorUtility.InstanceIDToObject (or guard with conditional compilation).
Suggested change
| var byId = EditorUtility.EntityIdToObject(instanceId) as GameObject; | |
| var byId = EditorUtility.InstanceIDToObject(instanceId) as GameObject; |
![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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs-50-53 (1)
50-53:⚠️ Potential issue | 🟡 MinorKeep
IsValidAssetPath()aligned with the new root-path behavior.Line 50 now treats
Assetsas a valid sanitized result, butIsValidAssetPath("Assets")still returnsfalsebecause it only acceptsAssets/. Any flow that sanitizes and then validates will still reject the root folder.Suggested follow-up
- // Must start with Assets/ - if (!normalized.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) + // Must be the Assets root or start with Assets/ + if (!string.Equals(normalized, "Assets", StringComparison.OrdinalIgnoreCase) && + !normalized.StartsWith("Assets/", StringComparison.OrdinalIgnoreCase)) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs` around lines 50 - 53, The IsValidAssetPath method must be updated to accept the root "Assets" result returned by the sanitizer: modify IsValidAssetPath (or its validation logic) to treat "Assets" as valid in addition to "Assets/" (e.g., allow exact match "Assets" or trim trailing slash before validation) so that flows that sanitize to "Assets" will pass validation; update any checks referencing IsValidAssetPath to use the new acceptance behavior.unity-mcp-skill/references/tools-reference.md-116-116 (1)
116-116:⚠️ Potential issue | 🟡 MinorFinish the screenshot-tool migration in this block.
Only the first example switched to
manage_camera; the inline, batch, and positioned screenshot examples immediately below still usemanage_scene(action="screenshot"), so this section now gives mixed guidance about the canonical screenshot path.🤖 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 116, The documentation still mixes the old manage_scene(action="screenshot") usage with the new manage_camera(action="screenshot"): update the inline, batch, and positioned screenshot examples to use manage_camera(action="screenshot") (and any camera-specific parameters) so all examples consistently use manage_camera; search for occurrences of manage_scene(action="screenshot") and replace them with the corresponding manage_camera calls and parameter names used in the first example.unity-mcp-skill/SKILL.md-166-166 (1)
166-166:⚠️ Potential issue | 🟡 MinorUpdate the graphics summary to include skybox support.
This row is already stale:
manage_graphicsalso exposes theskybox_*environment actions, so “33 actions across 5 groups” under-documents the actual tool surface and will misroute users away from supported capabilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/SKILL.md` at line 166, Update the Graphics row for manage_graphics to reflect skybox support: mention the exposed skybox_* environment actions and adjust the action/group count and/or group list to include a Skybox group (or add skybox to an existing group) so the description accurately lists groups like Volume, Bake, Stats, Pipeline, Features, and Skybox; ensure the resources and ping note remain and update the “33 actions across 5 groups” text to the correct number/groups to avoid under-documenting capabilities of manage_graphics and to reference skybox_* explicitly.unity-mcp-skill/references/tools-reference.md-929-1070 (1)
929-1070:⚠️ Potential issue | 🟡 MinorAdd the skybox/environment actions to the public graphics reference.
This section documents volume, bake, stats, pipeline, and renderer-feature operations, but none of the
skybox_*actions that are wired intomanage_graphics. That leaves the public API reference incomplete right where users look for exact action names and parameters.🤖 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 929 - 1070, The public graphics reference omitted the skybox/environment actions wired into manage_graphics; update the document to add a "Skybox / Environment" subsection describing each skybox_* action (e.g., skybox_get, skybox_set, skybox_list, skybox_create_material, skybox_set_exposure), their required parameters (action, target/name, material/path, properties like exposure/rotation/scale), expected types, and usage examples showing manage_graphics(action="skybox_set", target="Skybox", material="Assets/Materials/Sky.mat", properties={...}) and manage_graphics(action="skybox_get"); reference the existing manage_graphics header and example style and include resources/mcpforunity://scene/skybox or similar endpoint used by the implementation so users can locate scene-level skybox listings.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs-423-447 (1)
423-447:⚠️ Potential issue | 🟡 MinorRestore global lighting settings in
finally, and keep the original type.This test mutates project-wide lighting settings. If an assertion between the write and restore fails, or if the original
lightmapResolutionis non-integer, later tests inherit the modified value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs` around lines 423 - 447, The test currently captures original lightmapResolution as an int and restores it outside the assertion path; change it to capture the original JToken (or its raw value) from original["data"]["lightmapResolution"] so you preserve the original type, then wrap the mutation and verification (the calls to ManageGraphics.HandleCommand that set and assert the new value) in a try block and move the restore call into a finally block that calls ManageGraphics.HandleCommand to set "lightmapResolution" back to the saved original value; reference ManageGraphics.HandleCommand and the variables original/origResolution/verify/changed when making the change.
🤖 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/Graphics/GraphicsHelpers.cs`:
- Around line 31-47: The properties HasURP and HasHDRP currently memoize results
using _hasURP/_hasHDRP and only call DetectPackages() on first access; change
them to always refresh the active pipeline flags instead of caching so they
reflect runtime changes in the editor. Remove or bypass the memoization in
HasURP and HasHDRP (and the analogous properties around lines 85-96) so each
getter either calls DetectPackages() unconditionally or directly recomputes the
pipeline detection logic, and stop relying on the nullable _hasURP/_hasHDRP
fields (or ensure those fields are updated whenever pipeline/quality settings
change) so ping/URP/HDRP gating always returns the current pipeline state.
Ensure you reference the existing DetectPackages(), HasURP, HasHDRP, _hasURP and
_hasHDRP symbols when making the change.
- Around line 158-177: ReadSerializedValue currently returns objectReference
names, formatted color strings and anonymous vector objects which don't match
what SetSerializedValue expects; update ReadSerializedValue and/or
SetSerializedValue so they round-trip: have ReadSerializedValue return asset
path or reference (matching SetSerializedValue's input), color as a float[4] (or
same array/struct SetSerializedValue accepts), and Vector2/Vector3 as a
Vector2/Vector3 instance (or a float[] matching the setter), and ensure enum
output matches the setter's accepted form (index vs name); update
SetSerializedValue to also accept the alternate shapes produced (e.g., accept
both name and index for enums, null/name/path for ObjectReference, and arrays or
Vector structs for colors/vectors) so read-modify-write flows succeed between
ReadSerializedValue and SetSerializedValue.
- Around line 128-137: The code incorrectly calls
EditorUtility.EntityIdToObject(instanceId) when resolving numeric targets in the
int.TryParse block; replace that call with
EditorUtility.InstanceIDToObject(instanceId) so the instanceId (from
GameObject.GetInstanceID()) resolves to the correct GameObject (the byId
variable) before calling byId.GetComponent(VolumeType); update the call in the
method handling target parsing to use the correct API.
In `@MCPForUnity/Editor/Tools/Graphics/LightBakingOps.cs`:
- Around line 508-520: The current switch branch for
"compresslightmaps"/"lightmap_compression" always returns true and mishandles
boolean parsing (it ignores the parsed boolVal and treats case variants like
"TRUE" incorrectly); update the logic in the block that sets
settings.lightmapCompression so that: 1) use the actual parsed boolVal from
bool.TryParse instead of re-checking the string (and treat "0"/"1" by mapping
them to false/true via int parsing or explicit checks), 2) only set
settings.lightmapCompression when a parse path actually succeeds (Enum.TryParse,
bool.TryParse, or int.TryParse), and 3) return false if none of the parsing
attempts succeeded; reference the settings.lightmapCompression assignment, the
boolVal variable, and the LightmapCompression enum to locate and fix the code.
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs`:
- Around line 358-374: The reorder logic only permutes m_RendererFeatures (via
rendererFeaturesPropSo) causing m_RendererFeatureMap to get out of sync with
AddFeature/RemoveFeature expectations; update RendererFeatureOps to also locate
the SerializedProperty for "m_RendererFeatureMap", read its current elements
into an array (similar to how current is built for m_RendererFeatures), and
apply the same newOrder permutation to that map property before calling
so.ApplyModifiedProperties() so the map remains parallel to the features array
(use the same rendererData/so context and mirror the loops that reference
rendererFeaturesPropSo and current).
- Around line 130-137: The code wrongly stores ephemeral GetInstanceID() into
the serialized m_RendererFeatureMap (which is unstable across sessions) and
fails to keep m_RendererFeatureMap in sync when ReorderFeatures() rearranges
m_RendererFeatures; replace storing feature.GetInstanceID() with a stable
identifier (e.g., serialize and store a GlobalObjectId for the feature) when
populating m_RendererFeatureMap, and update ReorderFeatures() to reorder the
corresponding entries in m_RendererFeatureMap in the same way it reorders
m_RendererFeatures so both arrays remain aligned.
In `@MCPForUnity/Editor/Tools/Graphics/RenderingStatsOps.cs`:
- Around line 43-46: GetStats is creating short-lived ProfilerRecorder instances
and reading recorder.LastValue immediately, which returns zero/stale data;
change to read recorder.CurrentValue (or make recorders long-lived) so you
capture the instantaneous value instead of an end-of-frame sample — update the
loop that uses ProfilerRecorder.StartNew and replace uses of recorder.LastValue
with recorder.CurrentValue in GetStats (or refactor to store persistent
ProfilerRecorder instances keyed by COUNTER_MAP for end-of-frame snapshots).
In `@MCPForUnity/Editor/Tools/Graphics/RenderPipelineOps.cs`:
- Around line 236-247: The SetSettings flow currently only marks the SRP asset
dirty (EditorUtility.SetDirty(pipelineAsset)) but does not persist changes; call
AssetDatabase.SaveAssets() after setting the pipelineAsset dirty and before
returning the success object so the mutated SRP asset is written to disk
immediately (keep the existing msg construction and returned anonymous object,
just add a SaveAssets() call right after EditorUtility.SetDirty(pipelineAsset)
and before the return).
In `@MCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs`:
- Around line 153-161: The method currently calls EditorUtility.SetDirty(skyMat)
and MarkSceneDirty() but never persists the changed material to disk; after
marking the material dirty, call AssetDatabase.SaveAssets() (optionally guarded
by AssetDatabase.Contains(skyMat) if you want to ensure it’s an asset) to write
the modified skyMat asset to disk so the changes are not lost; update the code
around EditorUtility.SetDirty(skyMat) / MarkSceneDirty() to invoke
AssetDatabase.SaveAssets() before returning the success result.
- Around line 337-340: The code attempts to convert the parsed int instanceId
using EditorUtility.EntityIdToObject which expects an EntityId; change this to
EditorUtility.InstanceIDToObject so the parsed int instanceId resolves correctly
to a GameObject. Locate the block that parses target with int.TryParse and
assigns go (GameObject go = null; if (int.TryParse(target, out int instanceId))
go = EditorUtility.EntityIdToObject(instanceId) as GameObject;) and replace the
call to EntityIdToObject with InstanceIDToObject while keeping the int.TryParse
logic and null check for go intact.
In `@MCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs.meta`:
- Around line 1-2: Add the missing MonoImporter metadata to SkyboxOps.cs.meta so
it matches the other C# script .meta files: add a MonoImporter section with keys
externalObjects (empty), serializedVersion (match others, e.g., 2),
defaultReferences (empty list), executionOrder (0), icon (null), userData
(empty), assetBundleName (empty), and assetBundleVariant (empty), retaining the
existing fileFormatVersion and guid; locate the SkyboxOps.cs.meta entry (guid
93b73ba8237dee84088417864958084a) and update its contents to include the
standard MonoImporter block used in VolumeOps.cs.meta, GraphicsHelpers.cs.meta,
RenderPipelineOps.cs.meta, RenderingStatsOps.cs.meta, ManageGraphics.cs.meta,
LightBakingOps.cs.meta, and RendererFeatureOps.cs.meta.
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs`:
- Around line 18-25: CreateVolume currently dereferences
GraphicsHelpers.VolumeType and GraphicsHelpers.VolumeProfileType and calls
AddComponent/CreateInstance without checking for null, which throws when the
Volume system is unavailable; update CreateVolume to first check if
GraphicsHelpers.VolumeType and GraphicsHelpers.VolumeProfileType are non-null
and if either is null return a clean error result (mirroring existing error
response pattern) instead of proceeding, and only call AddComponent and
CreateInstance when their corresponding types are non-null to avoid calling
AddComponent(null)/CreateInstance(null).
- Around line 551-564: The code currently sets the parameter's overrideState to
true before attempting conversion and writing the value, which leaves the
override flag flipped even when conversion/SetValue fails; change the flow in
the block handling param, overrideProp, valueProp so that you perform
ConvertToParameterType(value, valueProp.PropertyType) and call
valueProp.SetValue(param, converted) inside a try and only after those succeed
set overrideProp.SetValue(param, true); if conversion or SetValue throws, do not
change overrideState and return false; apply the same change to the other
similar block that sets overrideState at lines handling the second value write
(the block that mirrors this logic).
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 999-1001: Replace the incorrect API call
EditorUtility.EntityIdToObject with the correct Unity API
EditorUtility.InstanceIDToObject in this file (and across the codebase); locate
occurrences like the snippet using EditorUtility.EntityIdToObject(id) (e.g., in
ManageScene.cs where obj is obtained) and change them to
EditorUtility.InstanceIDToObject(id) ensuring the passed identifier is an int
instanceID and that UnityEditor is available in the compilation unit.
In `@MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs`:
- Around line 322-325: The screenshot action buttons were moved under
IsManageCameraTool but still use CreateManageSceneActions which calls
ManageScene.ExecuteScreenshot / ExecuteMultiviewScreenshot; replace the injected
UI and/or wiring so the row created for IsManageCameraTool uses the
manage_camera implementations instead of ManageScene: update
CreateManageSceneActions (or create a new CreateManageCameraActions) to call the
manage_camera screenshot and multiview handlers, and update any renamed
helpers/handlers/tooltips that reference ManageScene.ExecuteScreenshot /
ExecuteMultiviewScreenshot to call the manage_camera screenshot/multiview
methods so the buttons route to the new manage_camera path.
---
Minor comments:
In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs`:
- Around line 50-53: The IsValidAssetPath method must be updated to accept the
root "Assets" result returned by the sanitizer: modify IsValidAssetPath (or its
validation logic) to treat "Assets" as valid in addition to "Assets/" (e.g.,
allow exact match "Assets" or trim trailing slash before validation) so that
flows that sanitize to "Assets" will pass validation; update any checks
referencing IsValidAssetPath to use the new acceptance behavior.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs`:
- Around line 423-447: The test currently captures original lightmapResolution
as an int and restores it outside the assertion path; change it to capture the
original JToken (or its raw value) from original["data"]["lightmapResolution"]
so you preserve the original type, then wrap the mutation and verification (the
calls to ManageGraphics.HandleCommand that set and assert the new value) in a
try block and move the restore call into a finally block that calls
ManageGraphics.HandleCommand to set "lightmapResolution" back to the saved
original value; reference ManageGraphics.HandleCommand and the variables
original/origResolution/verify/changed when making the change.
In `@unity-mcp-skill/references/tools-reference.md`:
- Line 116: The documentation still mixes the old
manage_scene(action="screenshot") usage with the new
manage_camera(action="screenshot"): update the inline, batch, and positioned
screenshot examples to use manage_camera(action="screenshot") (and any
camera-specific parameters) so all examples consistently use manage_camera;
search for occurrences of manage_scene(action="screenshot") and replace them
with the corresponding manage_camera calls and parameter names used in the first
example.
- Around line 929-1070: The public graphics reference omitted the
skybox/environment actions wired into manage_graphics; update the document to
add a "Skybox / Environment" subsection describing each skybox_* action (e.g.,
skybox_get, skybox_set, skybox_list, skybox_create_material,
skybox_set_exposure), their required parameters (action, target/name,
material/path, properties like exposure/rotation/scale), expected types, and
usage examples showing manage_graphics(action="skybox_set", target="Skybox",
material="Assets/Materials/Sky.mat", properties={...}) and
manage_graphics(action="skybox_get"); reference the existing manage_graphics
header and example style and include resources/mcpforunity://scene/skybox or
similar endpoint used by the implementation so users can locate scene-level
skybox listings.
In `@unity-mcp-skill/SKILL.md`:
- Line 166: Update the Graphics row for manage_graphics to reflect skybox
support: mention the exposed skybox_* environment actions and adjust the
action/group count and/or group list to include a Skybox group (or add skybox to
an existing group) so the description accurately lists groups like Volume, Bake,
Stats, Pipeline, Features, and Skybox; ensure the resources and ping note remain
and update the “33 actions across 5 groups” text to the correct number/groups to
avoid under-documenting capabilities of manage_graphics and to reference
skybox_* explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab9b26d4-c80d-495e-9c2b-be1728775d71
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
.claude/skills/unity-mcp-skill/SKILL.mdMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Resources/Editor/Selection.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Resources/Scene/RendererFeaturesResource.csMCPForUnity/Editor/Resources/Scene/RendererFeaturesResource.cs.metaMCPForUnity/Editor/Resources/Scene/RenderingStatsResource.csMCPForUnity/Editor/Resources/Scene/RenderingStatsResource.cs.metaMCPForUnity/Editor/Resources/Scene/VolumesResource.csMCPForUnity/Editor/Resources/Scene/VolumesResource.cs.metaMCPForUnity/Editor/Tools/Cameras/CameraControl.csMCPForUnity/Editor/Tools/Cameras/CameraHelpers.csMCPForUnity/Editor/Tools/Graphics.metaMCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.csMCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.cs.metaMCPForUnity/Editor/Tools/Graphics/LightBakingOps.csMCPForUnity/Editor/Tools/Graphics/LightBakingOps.cs.metaMCPForUnity/Editor/Tools/Graphics/ManageGraphics.csMCPForUnity/Editor/Tools/Graphics/ManageGraphics.cs.metaMCPForUnity/Editor/Tools/Graphics/RenderPipelineOps.csMCPForUnity/Editor/Tools/Graphics/RenderPipelineOps.cs.metaMCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.csMCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs.metaMCPForUnity/Editor/Tools/Graphics/RenderingStatsOps.csMCPForUnity/Editor/Tools/Graphics/RenderingStatsOps.cs.metaMCPForUnity/Editor/Tools/Graphics/SkyboxOps.csMCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs.metaMCPForUnity/Editor/Tools/Graphics/VolumeOps.csMCPForUnity/Editor/Tools/Graphics/VolumeOps.cs.metaMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageEditor.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csREADME.mdServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/commands/editor.pyServer/src/cli/commands/graphics.pyServer/src/cli/commands/scene.pyServer/src/cli/main.pyServer/src/services/resources/renderer_features.pyServer/src/services/resources/rendering_stats.pyServer/src/services/resources/volumes.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_graphics.pyServer/src/services/tools/manage_scene.pyServer/tests/integration/test_manage_scene_screenshot_params.pyServer/tests/test_cli.pyServer/tests/test_manage_graphics.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.csdocs/guides/CLI_EXAMPLE.mddocs/guides/CLI_USAGE.mddocs/i18n/README-zh.mdmanifest.jsonunity-mcp-skill/SKILL.mdunity-mcp-skill/references/probuilder-guide.mdunity-mcp-skill/references/resources-reference.mdunity-mcp-skill/references/tools-reference.mdunity-mcp-skill/references/workflows.md
💤 Files with no reviewable changes (2)
- Server/tests/test_cli.py
- Server/tests/integration/test_manage_scene_screenshot_params.py
Comment on lines
+128
to
+137
| if (string.IsNullOrEmpty(target)) | ||
| { | ||
| var allVolumes = UnityEngine.Object.FindObjectsByType(VolumeType, FindObjectsSortMode.None); | ||
| return allVolumes.Length > 0 ? allVolumes[0] as Component : null; | ||
| } | ||
|
|
||
| if (int.TryParse(target, out int instanceId)) | ||
| { | ||
| var byId = EditorUtility.EntityIdToObject(instanceId) as GameObject; | ||
| if (byId != null) return byId.GetComponent(VolumeType); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Unity Editor API, what method should be used to resolve a GameObject.GetInstanceID()back to the object, and isEditorUtility.EntityIdToObject(int) the correct API for that?
💡 Result:
Use UnityEditor.EditorUtility.InstanceIDToObject(int instanceID) to turn a GameObject.GetInstanceID() (or any UnityEngine.Object.GetInstanceID()) back into a UnityEngine.Object in the Editor. [1] (docs.unity3d.com)
EditorUtility.EntityIdToObject(...) is not the correct API for resolving a GetInstanceID() value—Instance IDs and the newer Editor “EntityId” identifiers are different ID systems (Unity is moving some Editor APIs from int instanceId to EntityId). [2] (unity.com)
Also note that instance IDs are not stable across Editor/player sessions, so they’re not suitable as persistent identifiers. [3] (docs.unity3d.com)
References
- [1] (docs.unity3d.com)
- [2] (unity.com)
- [3] (docs.unity3d.com)
Citations:
- 1: https://docs.unity3d.com/cn/2019.2/ScriptReference/EditorUtility.InstanceIDToObject.html
- 2: https://unity.com/de/releases/editor/beta/6000.4.0b10
- 3: https://docs.unity3d.com/cn/2022.1/ScriptReference/Object.GetInstanceID.html
- 4: https://docs.unity3d.com/cn/2019.2/ScriptReference/EditorUtility.InstanceIDToObject.html
- 5: https://unity.com/de/releases/editor/beta/6000.4.0b10
- 6: https://docs.unity3d.com/cn/2022.1/ScriptReference/Object.GetInstanceID.html
Replace EditorUtility.EntityIdToObject() with EditorUtility.InstanceIDToObject().
The code uses EditorUtility.EntityIdToObject(instanceId) to resolve numeric targets, but this is the wrong API. Instance IDs (from GameObject.GetInstanceID()) and Entity IDs are different ID systems in Unity. The correct method is EditorUtility.InstanceIDToObject(int). Without this fix, the code will fail to find the intended GameObjects, leaving the first-volume fallback to arbitrarily affect scene objects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.cs` around lines 128 - 137,
The code incorrectly calls EditorUtility.EntityIdToObject(instanceId) when
resolving numeric targets in the int.TryParse block; replace that call with
EditorUtility.InstanceIDToObject(instanceId) so the instanceId (from
GameObject.GetInstanceID()) resolves to the correct GameObject (the byId
variable) before calling byId.GetComponent(VolumeType); update the call in the
method handling target parsing to use the correct API.
Comment on lines
+130
to
+137
| // Also update the map (m_RendererFeatureMap) if it exists | ||
| var mapProp = so.FindProperty("m_RendererFeatureMap"); | ||
| if (mapProp != null) | ||
| { | ||
| mapProp.arraySize++; | ||
| var mapElement = mapProp.GetArrayElementAtIndex(mapProp.arraySize - 1); | ||
| mapElement.longValue = feature.GetInstanceID(); | ||
| so.ApplyModifiedProperties(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the entire file to understand context
wc -l MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.csRepository: CoplayDev/unity-mcp
Length of output: 122
🏁 Script executed:
# Read the relevant section around lines 130-137
sed -n '120,150p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 1689
🏁 Script executed:
# Search for m_RendererFeatureMap usage throughout the file
rg "m_RendererFeatureMap" MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs -n -A 2 -B 2Repository: CoplayDev/unity-mcp
Length of output: 527
🏁 Script executed:
# Check how m_RendererFeatureMap is declared (find the class/field definition)
rg "m_RendererFeatureMap" MCPForUnity --type csharp -B 3 -A 1Repository: CoplayDev/unity-mcp
Length of output: 1131
🏁 Script executed:
# Search for GetInstanceID usage to see if it's used consistently
rg "GetInstanceID" MCPForUnity --type csharp -n -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 43864
🏁 Script executed:
# Check the removal operation context (lines 200-230)
sed -n '200,230p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 1609
🏁 Script executed:
# Search for how m_RendererFeatureMap is actually consumed or read
rg "m_RendererFeatureMap" -B 5 -A 5 --type csharpRepository: CoplayDev/unity-mcp
Length of output: 2423
🏁 Script executed:
# Look for the class that owns this property and understand its type
sed -n '1,50p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 2608
🏁 Script executed:
# Search for any documentation or comments about m_RendererFeatureMap
rg "RendererFeatureMap" MCPForUnity --type csharp -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 1099
🏁 Script executed:
# Search for any comments or documentation about why the map is maintained
sed -n '100,180p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 4134
🏁 Script executed:
# Check if there are any other similar patterns of maintaining parallel serialized arrays
rg "arraySize\+\+" MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs -B 3 -A 5Repository: CoplayDev/unity-mcp
Length of output: 922
🏁 Script executed:
# Look for any reorder operations mentioned in the review snippet (around lines 358-374)
sed -n '350,380p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 1626
🏁 Script executed:
# Get the complete reorder function to see if there's any map reordering
sed -n '340,400p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 3115
🏁 Script executed:
# Confirm: does the reorder function ever touch m_RendererFeatureMap?
rg "feature_reorder" MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs -A 100 | grep -i "map"Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Get full context of the reorder function
sed -n '315,400p' MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 4377
🌐 Web query:
Unity GetInstanceID persistence across Editor sessions documentation
💡 Result:
UnityEngine.Object.GetInstanceID() is not stable/persistent across Unity Editor sessions (or other “between sessions” workflows). Unity’s scripting docs explicitly state that the InstanceID “is not reliable for performing actions that could span between sessions (for example, loading an object state from a file).” [1]
If you need an identifier that does survive Editor restarts, Unity provides GlobalObjectId (Editor-side), documented as “stable, project-global object identifiers” where “the ID is persistent and unique for a given Unity Object.” [2]
Sources:
[1] Unity Scripting API — Object.GetInstanceID [1]
[2] Unity Scripting API — GlobalObjectId [2]
Don't persist GetInstanceID() into the serialized feature map, and sync reorder operations with it.
m_RendererFeatureMap is serialized, but GetInstanceID() is only an in-memory handle that changes between Editor sessions and project restarts. If this field needs a stable identifier, use GlobalObjectId instead.
Additionally, the ReorderFeatures() function (lines 358–374) reorders m_RendererFeatures but does not reorder m_RendererFeatureMap, leaving the two parallel arrays misaligned after any reorder operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs` around lines 130 -
137, The code wrongly stores ephemeral GetInstanceID() into the serialized
m_RendererFeatureMap (which is unstable across sessions) and fails to keep
m_RendererFeatureMap in sync when ReorderFeatures() rearranges
m_RendererFeatures; replace storing feature.GetInstanceID() with a stable
identifier (e.g., serialize and store a GlobalObjectId for the feature) when
populating m_RendererFeatureMap, and update ReorderFeatures() to reorder the
corresponding entries in m_RendererFeatureMap in the same way it reorders
m_RendererFeatures so both arrays remain aligned.
Comment on lines
+18
to
+25
| internal static object CreateVolume(JObject @params) | ||
| { | ||
| var p = new ToolParams(@params); | ||
| string name = p.Get("name") ?? "Volume"; | ||
| bool isGlobal = p.GetBool("is_global", true); | ||
| float weight = p.GetFloat("weight") ?? 1.0f; | ||
| float priority = p.GetFloat("priority") ?? 0f; | ||
| string profilePath = p.Get("profile_path"); |
Contributor
There was a problem hiding this comment.
Fail cleanly when the Volume system is unavailable.
CreateVolume dereferences GraphicsHelpers.VolumeType and GraphicsHelpers.VolumeProfileType directly. On built-in projects, or before the RP core package is present, AddComponent(null) / CreateInstance(null) will throw instead of returning a normal error response.
Also applies to: 27-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs` around lines 18 - 25,
CreateVolume currently dereferences GraphicsHelpers.VolumeType and
GraphicsHelpers.VolumeProfileType and calls AddComponent/CreateInstance without
checking for null, which throws when the Volume system is unavailable; update
CreateVolume to first check if GraphicsHelpers.VolumeType and
GraphicsHelpers.VolumeProfileType are non-null and if either is null return a
clean error result (mirroring existing error response pattern) instead of
proceeding, and only call AddComponent and CreateInstance when their
corresponding types are non-null to avoid calling
AddComponent(null)/CreateInstance(null).
Comment on lines
+551
to
+564
| // Set overrideState = true | ||
| var overrideProp = param.GetType().GetProperty("overrideState"); | ||
| if (overrideProp != null) | ||
| overrideProp.SetValue(param, true); | ||
|
|
||
| // Set value with type conversion | ||
| var valueProp = param.GetType().GetProperty("value"); | ||
| if (valueProp == null) return false; | ||
|
|
||
| try | ||
| { | ||
| object converted = ConvertToParameterType(value, valueProp.PropertyType); | ||
| valueProp.SetValue(param, converted); | ||
| return true; |
Contributor
There was a problem hiding this comment.
Don't enable overrideState before the write succeeds.
If conversion or SetValue() throws, the method returns false but the override flag is already true, so the effect has still been mutated despite the failure response.
Suggested fix
- // Set overrideState = true
- var overrideProp = param.GetType().GetProperty("overrideState");
- if (overrideProp != null)
- overrideProp.SetValue(param, true);
-
// Set value with type conversion
var valueProp = param.GetType().GetProperty("value");
if (valueProp == null) return false;
try
{
object converted = ConvertToParameterType(value, valueProp.PropertyType);
valueProp.SetValue(param, converted);
+ var overrideProp = param.GetType().GetProperty("overrideState");
+ if (overrideProp != null)
+ overrideProp.SetValue(param, true);
return true;
}Also applies to: 566-569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs` around lines 551 - 564, The
code currently sets the parameter's overrideState to true before attempting
conversion and writing the value, which leaves the override flag flipped even
when conversion/SetValue fails; change the flow in the block handling param,
overrideProp, valueProp so that you perform ConvertToParameterType(value,
valueProp.PropertyType) and call valueProp.SetValue(param, converted) inside a
try and only after those succeed set overrideProp.SetValue(param, true); if
conversion or SetValue throws, do not change overrideState and return false;
apply the same change to the other similar block that sets overrideState at
lines handling the second value write (the block that mirrors this logic).
Comment on lines
999
to
1001
| var obj = EditorUtility.EntityIdToObject(id); | ||
| if (obj is Camera cam) return cam; | ||
| if (obj is GameObject go) return go.GetComponent<Camera>(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Unity EditorUtility EntityIdToObject API
💡 Result:
Unity’s public Scripting API (as of Unity 6.0 / 6000.0) does not expose an EditorUtility.EntityIdToObject(...) method. The documented “ID → Object” conversion API on EditorUtility is still:
UnityEditor.EditorUtility.InstanceIDToObject(int instanceID)→UnityEngine.Object(Editor-only). [1][2]
If what you actually need is a stable/persistent identifier that you can later resolve back to an object, Unity provides UnityEditor.GlobalObjectId, including:
GlobalObjectId.GetGlobalObjectIdSlow(...)GlobalObjectId.GlobalObjectIdentifierToObjectSlow(...)[3]
Separately, Unity’s 6000.4 beta release notes indicate a transition from int instance IDs toward an editor EntityId type for some editor APIs (for example, EditorUtility.PingObject(int) → EditorUtility.PingObject(EntityId), and ObjectSelectorSearchContext.allowedInstanceIds → allowedEntityIds). [4] However, those notes don’t indicate a corresponding public EntityIdToObject conversion API. [4]
Sources: [1][2][3][4]
🏁 Script executed:
# First, check if ManageScene.cs exists and examine the code around line 999
fd "ManageScene.cs" --type fRepository: CoplayDev/unity-mcp
Length of output: 147
🏁 Script executed:
# Check the code around line 999 in ManageScene.cs
sed -n '990,1010p' "MCPForUnity/Editor/Tools/ManageScene.cs"Repository: CoplayDev/unity-mcp
Length of output: 921
🏁 Script executed:
# Search for EntityIdToObject definition anywhere in the codebase
rg "EntityIdToObject" --type csRepository: CoplayDev/unity-mcp
Length of output: 1483
🏁 Script executed:
# Search for where EntityIdToObject is defined (method declaration)
rg "static.*EntityIdToObject|EntityIdToObject\s*\(" --type cs -B 2 -A 2 | head -100Repository: CoplayDev/unity-mcp
Length of output: 5216
🏁 Script executed:
# Check if it's an extension method in any utility class
rg "this.*EditorUtility.*EntityIdToObject|extension.*EntityIdToObject" --type csRepository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Search for EntityIdToObject in all cs files and look for method/property definitions
ast-grep --pattern 'static Object EntityIdToObject($_, $_) { $$$ }'Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Search for all occurrences of EntityIdToObject including potential definitions
rg "EntityIdToObject" --type cs -C 5 | grep -E "(public|private|static|EntityIdToObject)" | head -50Repository: CoplayDev/unity-mcp
Length of output: 1483
🏁 Script executed:
# Look for this in nested types, utility classes, or extension definitions
find . -name "*.cs" -type f | xargs grep -l "EntityIdToObject" | head -10Repository: CoplayDev/unity-mcp
Length of output: 9219
🏁 Script executed:
# Check if there's a definition in EditorUtility wrapper or extension
rg "class.*Utility|static.*class" --type cs -A 20 | grep -B 5 -A 5 "EntityIdToObject"Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Search for method definitions of EntityIdToObject more broadly
rg "def EntityIdToObject|EntityIdToObject\s*=" --type csRepository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Check if maybe it's imported or aliased with a using statement
rg "using.*EntityId|EntityId\s*=" --type cs | head -20Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Look at the file that first uses it to see context
head -50 "MCPForUnity/Editor/Tools/ManageScene.cs"Repository: CoplayDev/unity-mcp
Length of output: 2421
Replace EditorUtility.EntityIdToObject with EditorUtility.InstanceIDToObject.
EditorUtility.EntityIdToObject is not a valid Unity API and will cause compilation errors. The correct method is EditorUtility.InstanceIDToObject(int instanceID). This pattern appears in multiple files across the codebase and should be corrected uniformly.
🤖 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 999 - 1001, Replace the
incorrect API call EditorUtility.EntityIdToObject with the correct Unity API
EditorUtility.InstanceIDToObject in this file (and across the codebase); locate
occurrences like the snippet using EditorUtility.EntityIdToObject(id) (e.g., in
ManageScene.cs where obj is obtained) and change them to
EditorUtility.InstanceIDToObject(id) ensuring the passed identifier is an int
instanceID and that UnityEditor is available in the compilation unit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
322-325:⚠️ Potential issue | 🟠 MajorRoute the
manage_camerabuttons throughManageCamera, notManageScene.Line 324 still injects
CreateManageSceneActions(), and those handlers later callManageScene.ExecuteScreenshot()/ExecuteMultiviewScreenshot()directly. So the buttons shown undermanage_camerabypass the new tool path entirely; the updated tooltip at Line 599 now saysmanage_camera, but this UI never exercises it.Suggested direction
- if (IsManageCameraTool(tool)) - { - row.Add(CreateManageSceneActions()); - } + if (IsManageCameraTool(tool)) + { + row.Add(CreateManageCameraActions()); + }Then have the camera-specific handlers dispatch through
ManageCamera(action: "screenshot"/"screenshot_multiview"), or add camera-facing wrappers and call those instead.🤖 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 322 - 325, The manage_camera UI is still using CreateManageSceneActions(), causing buttons to call ManageScene.ExecuteScreenshot/ExecuteMultiviewScreenshot directly; replace that injection so manage_camera routes through the ManageCamera handlers (or create camera-specific wrappers). Update the code where IsManageCameraTool(tool) is true to add CreateManageCameraActions() (or wire the existing handlers to dispatch ManageCamera with action: "screenshot" / "screenshot_multiview") and ensure the handlers no longer call ManageScene.ExecuteScreenshot/ExecuteMultiviewScreenshot but instead invoke ManageCamera with the appropriate action names.MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs (1)
31-31:⚠️ Potential issue | 🟠 MajorFail cleanly when the Volume system is unavailable.
GraphicsHelpers.VolumeType/GraphicsHelpers.VolumeProfileTypeare used unconditionally in both creation paths. When those helpers are unavailable, these actions exit via exception instead of returning anErrorResponse. Guard both types up front and reuse the same early return inCreateVolumeandCreateProfile.Also applies to: 43-57, 431-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs` at line 31, Check for availability of GraphicsHelpers.VolumeType and GraphicsHelpers.VolumeProfileType up front and return the same ErrorResponse when either is null instead of letting code throw; add a single guard block reused by both CreateVolume and CreateProfile that validates GraphicsHelpers.VolumeType and GraphicsHelpers.VolumeProfileType (or the specific one needed per method) and returns the consistent ErrorResponse early, then proceed with go.AddComponent(GraphicsHelpers.VolumeType) and profile creation only after the guard passes so both creation paths fail cleanly.MCPForUnity/Editor/Helpers/GameObjectLookup.cs (1)
71-77:⚠️ Potential issue | 🔴 Critical
EntityIdToObjectis wired to the wrong ID type here.
ResolveInstanceIDstill acceptsintinstance IDs, but Unity 6.3 declaresEditorUtility.EntityIdToObjectasEntityId -> Object, notint -> Object. That makes theUNITY_6000_3_OR_NEWERbranch invalid as written and breaks every caller that still passes instance IDs through this helper. (docs.unity3d.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/GameObjectLookup.cs` around lines 71 - 77, ResolveInstanceID currently takes an int but under UNITY_6000_3_OR_NEWER you call EditorUtility.EntityIdToObject which expects an EntityId type, breaking callers; fix by keeping ResolveInstanceID(int instanceId) to always call EditorUtility.InstanceIDToObject and add a new overload ResolveEntityID(EntityId entityId) (or rename appropriately) that calls EditorUtility.EntityIdToObject under UNITY_6000_3_OR_NEWER, or alternatively change the UNITY_6000_3_OR_NEWER branch to call InstanceIDToObject and provide a separate method that accepts the proper EntityId type and invokes EditorUtility.EntityIdToObject; update callers to use the correct method for entity vs instance IDs (reference ResolveInstanceID, ResolveEntityID, EditorUtility.EntityIdToObject, EditorUtility.InstanceIDToObject).MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs (1)
130-137:⚠️ Potential issue | 🔴 Critical
m_RendererFeatureMapis being written in the wrong format.Checked-in URP renderer assets serialize
m_RendererFeatureMapas packed hex data for feature file IDs, not as a parallel YAML list of runtime instance IDs. In one asset,m_RendererFeaturesreferences fileID7833122117494664109whilem_RendererFeatureMapisad6b866f10d7b46c; that hex decodes little-endian back to the same file ID. Another asset with multiple features stores a longer contiguous hex blob, so thearraySize == featuresList.Countassumption andfeature.GetInstanceID()writes here are both off-spec. Add/remove/reorder can therefore leave renderer features broken after reload. (git.gabrielkaszewski.dev)Also applies to: 214-218, 373-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs` around lines 130 - 137, The code is writing runtime instance IDs into m_RendererFeatureMap as an array (using mapProp.arraySize and feature.GetInstanceID()), but URP expects a packed hex blob of serialized fileIDs (little-endian) that corresponds to the order of m_RendererFeatures; this breaks add/remove/reorder and corrupts assets. Change the map update logic (where you touch m_RendererFeatureMap, mapProp, mapElement and where you read m_RendererFeatures) to: compute each renderer feature's persistent fileID (not GetInstanceID()), pack those 64-bit fileIDs into a contiguous little-endian byte buffer and write it back as the hex string that Unity uses (preserve existing blob format when present), and ensure the produced blob length/order always matches m_RendererFeatures so add/remove/reorder works; apply the same fix to the other occurrences noted (around the other mapProp updates).MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
382-387:⚠️ Potential issue | 🔴 CriticalThis 6000.3 branch is still passing an
intto anEntityIdAPI.The serialized payload here still carries
instanceID, but Unity 6.3'sEditorUtility.EntityIdToObjecttakes anEntityId. This branch therefore no longer matches the data contract it is deserializing. Either keep resolving instance IDs with an instance-ID-based path, or change the JSON payload to carry actualEntityIdvalues end-to-end before switching APIs. (docs.unity3d.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs` around lines 382 - 387, The UNITY_6000_3_OR_NEWER branch is calling EditorUtility.EntityIdToObject with an int parsed from idToken (instanceId), which mismatches the API; update UnityTypeConverters.cs so that when using UnityEditor.EditorUtility.EntityIdToObject you pass an actual UnityEditor.EntityId (construct/deserialize an EntityId from the JSON payload or provide a conversion helper from the int), or keep the existing instance-ID flow by calling EditorUtility.InstanceIDToObject for payloads that carry instance IDs; change the code handling idToken/instanceId to either deserialize into UnityEditor.EntityId (and pass that to EntityIdToObject) or keep using InstanceIDToObject consistently.
🧹 Nitpick comments (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs (2)
679-686: Helper method doesn't verify volume creation success.
CreateTestVolumeignores the result ofHandleCommand. If volume creation fails (e.g., volume system unavailable), subsequent tests will fail with confusing errors instead of clear setup failures.🛠️ Suggested fix
private void CreateTestVolume(string name) { - ManageGraphics.HandleCommand(new JObject + var result = ToJObject(ManageGraphics.HandleCommand(new JObject { ["action"] = "volume_create", ["name"] = name - }); + })); + Assert.IsTrue(result.Value<bool>("success"), $"Failed to create test volume '{name}': {result}"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs` around lines 679 - 686, CreateTestVolume currently calls ManageGraphics.HandleCommand and ignores its result; update CreateTestVolume to capture the return value from ManageGraphics.HandleCommand (for the "volume_create" action), verify it indicates success, and fail the test/setup immediately if it reports an error or null (include the volume name and any error details from the response in the failure message). Locate the CreateTestVolume helper and add a simple check/assert on the handled response (or throw an informative exception) so tests stop with a clear setup failure when volume creation fails.
531-543: Test creates GameObject but relies on implicit cleanup.Line 534 creates
"GfxTest_NoProbe"directly withnew GameObject()rather than throughManageGraphics.HandleCommand. While the naming convention ensures TearDown cleanup works, consider making this explicit or adding a comment.💡 Minor clarity improvement
[Test] public void BakeSetProbePositions_WrongComponent_ReturnsError() { - var go = new GameObject("GfxTest_NoProbe"); + // Creates a bare GameObject (cleaned up by TearDown via "GfxTest_" prefix) + var go = new GameObject("GfxTest_NoProbe"); var result = ToJObject(ManageGraphics.HandleCommand(new JObject🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs` around lines 531 - 543, The test BakeSetProbePositions_WrongComponent_ReturnsError creates a GameObject via new GameObject("GfxTest_NoProbe") but relies on implicit cleanup; make teardown explicit by either creating the GameObject through ManageGraphics.HandleCommand (so existing teardown logic handles it) or add explicit cleanup in the test (call Object.DestroyImmediate(go) after assertions or register it for teardown), and/or add a brief comment explaining why cleanup is handled to avoid relying on naming conventions.MCPForUnity/Editor/Tools/Graphics/LightBakingOps.cs (1)
392-409: Consider logging when LightingSettings creation fails.
EnsureLightingSettingssilently swallows exceptions and returnsnull. While the caller handlesnull, adding a log message in the catch block would help diagnose why lighting settings couldn't be created (e.g., permission issues, asset database locked).💡 Suggested improvement
catch { /* getter throws when no asset exists */ } try { var settings = new LightingSettings { name = "LightingSettings" }; Lightmapping.lightingSettings = settings; return Lightmapping.lightingSettings; } - catch { return null; } + catch (Exception ex) + { + McpLog.Warn($"[LightBakingOps] Failed to create LightingSettings: {ex.Message}"); + return null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/LightBakingOps.cs` around lines 392 - 409, EnsureLightingSettings currently swallows exceptions silently; update its catch blocks to log failures with exception details and context so failures to read or create Lightmapping.lightingSettings are visible in the Editor log. Specifically, in the first try/catch around reading Lightmapping.lightingSettings and the second try/catch around creating a new LightingSettings (method: EnsureLightingSettings), catch the exception as a variable and call UnityEditor.EditorUtility.DisplayDialog or UnityEngine.Debug.LogError / Debug.LogException with a clear message mentioning "EnsureLightingSettings" and the exception to aid debugging (include that it failed to read or create Lightmapping.lightingSettings and any relevant state).
🤖 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/Graphics/RendererFeatureOps.cs`:
- Around line 414-439: The current helper silently falls back to the default
renderer when a caller passes an out-of-range rendererIndex, causing mutations
to target the wrong asset; update the logic in the method that inspects
pipelineAsset/renderersProp so that if rendererIndex >= 0 but rendererIndex >=
renderersProp.arraySize you do not fall through to the m_DefaultRendererIndex
branch—instead return null (or an explicit error) immediately; specifically,
modify the block that checks rendererIndex, rendererIndex vs
renderersProp.arraySize and element.objectReferenceValue so an invalid
rendererIndex does not proceed to the subsequent default-renderer lookup
(references: rendererIndex, pipelineAsset, renderersProp / "m_RendererDataList",
defaultIndex / "m_DefaultRendererIndex").
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs`:
- Around line 20-25: The CreateVolume code reads profilePath from ToolParams and
calls AssetDatabase APIs directly; normalize profilePath the same way as
CreateProfile before using AssetDatabase.LoadAssetAtPath or
AssetDatabase.CreateAsset by ensuring the path has the "Assets/" prefix and a
".asset" suffix (apply the same normalization logic used in CreateProfile),
i.e., update the handling of the local variable profilePath inside CreateVolume
so it is normalized prior to calls to LoadAssetAtPath and CreateAsset.
---
Duplicate comments:
In `@MCPForUnity/Editor/Helpers/GameObjectLookup.cs`:
- Around line 71-77: ResolveInstanceID currently takes an int but under
UNITY_6000_3_OR_NEWER you call EditorUtility.EntityIdToObject which expects an
EntityId type, breaking callers; fix by keeping ResolveInstanceID(int
instanceId) to always call EditorUtility.InstanceIDToObject and add a new
overload ResolveEntityID(EntityId entityId) (or rename appropriately) that calls
EditorUtility.EntityIdToObject under UNITY_6000_3_OR_NEWER, or alternatively
change the UNITY_6000_3_OR_NEWER branch to call InstanceIDToObject and provide a
separate method that accepts the proper EntityId type and invokes
EditorUtility.EntityIdToObject; update callers to use the correct method for
entity vs instance IDs (reference ResolveInstanceID, ResolveEntityID,
EditorUtility.EntityIdToObject, EditorUtility.InstanceIDToObject).
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs`:
- Around line 130-137: The code is writing runtime instance IDs into
m_RendererFeatureMap as an array (using mapProp.arraySize and
feature.GetInstanceID()), but URP expects a packed hex blob of serialized
fileIDs (little-endian) that corresponds to the order of m_RendererFeatures;
this breaks add/remove/reorder and corrupts assets. Change the map update logic
(where you touch m_RendererFeatureMap, mapProp, mapElement and where you read
m_RendererFeatures) to: compute each renderer feature's persistent fileID (not
GetInstanceID()), pack those 64-bit fileIDs into a contiguous little-endian byte
buffer and write it back as the hex string that Unity uses (preserve existing
blob format when present), and ensure the produced blob length/order always
matches m_RendererFeatures so add/remove/reorder works; apply the same fix to
the other occurrences noted (around the other mapProp updates).
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs`:
- Line 31: Check for availability of GraphicsHelpers.VolumeType and
GraphicsHelpers.VolumeProfileType up front and return the same ErrorResponse
when either is null instead of letting code throw; add a single guard block
reused by both CreateVolume and CreateProfile that validates
GraphicsHelpers.VolumeType and GraphicsHelpers.VolumeProfileType (or the
specific one needed per method) and returns the consistent ErrorResponse early,
then proceed with go.AddComponent(GraphicsHelpers.VolumeType) and profile
creation only after the guard passes so both creation paths fail cleanly.
In `@MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs`:
- Around line 322-325: The manage_camera UI is still using
CreateManageSceneActions(), causing buttons to call
ManageScene.ExecuteScreenshot/ExecuteMultiviewScreenshot directly; replace that
injection so manage_camera routes through the ManageCamera handlers (or create
camera-specific wrappers). Update the code where IsManageCameraTool(tool) is
true to add CreateManageCameraActions() (or wire the existing handlers to
dispatch ManageCamera with action: "screenshot" / "screenshot_multiview") and
ensure the handlers no longer call
ManageScene.ExecuteScreenshot/ExecuteMultiviewScreenshot but instead invoke
ManageCamera with the appropriate action names.
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 382-387: The UNITY_6000_3_OR_NEWER branch is calling
EditorUtility.EntityIdToObject with an int parsed from idToken (instanceId),
which mismatches the API; update UnityTypeConverters.cs so that when using
UnityEditor.EditorUtility.EntityIdToObject you pass an actual
UnityEditor.EntityId (construct/deserialize an EntityId from the JSON payload or
provide a conversion helper from the int), or keep the existing instance-ID flow
by calling EditorUtility.InstanceIDToObject for payloads that carry instance
IDs; change the code handling idToken/instanceId to either deserialize into
UnityEditor.EntityId (and pass that to EntityIdToObject) or keep using
InstanceIDToObject consistently.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/Graphics/LightBakingOps.cs`:
- Around line 392-409: EnsureLightingSettings currently swallows exceptions
silently; update its catch blocks to log failures with exception details and
context so failures to read or create Lightmapping.lightingSettings are visible
in the Editor log. Specifically, in the first try/catch around reading
Lightmapping.lightingSettings and the second try/catch around creating a new
LightingSettings (method: EnsureLightingSettings), catch the exception as a
variable and call UnityEditor.EditorUtility.DisplayDialog or
UnityEngine.Debug.LogError / Debug.LogException with a clear message mentioning
"EnsureLightingSettings" and the exception to aid debugging (include that it
failed to read or create Lightmapping.lightingSettings and any relevant state).
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs`:
- Around line 679-686: CreateTestVolume currently calls
ManageGraphics.HandleCommand and ignores its result; update CreateTestVolume to
capture the return value from ManageGraphics.HandleCommand (for the
"volume_create" action), verify it indicates success, and fail the test/setup
immediately if it reports an error or null (include the volume name and any
error details from the response in the failure message). Locate the
CreateTestVolume helper and add a simple check/assert on the handled response
(or throw an informative exception) so tests stop with a clear setup failure
when volume creation fails.
- Around line 531-543: The test
BakeSetProbePositions_WrongComponent_ReturnsError creates a GameObject via new
GameObject("GfxTest_NoProbe") but relies on implicit cleanup; make teardown
explicit by either creating the GameObject through ManageGraphics.HandleCommand
(so existing teardown logic handles it) or add explicit cleanup in the test
(call Object.DestroyImmediate(go) after assertions or register it for teardown),
and/or add a brief comment explaining why cleanup is handled to avoid relying on
naming conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b13d55d-2cbe-4d08-8a87-35e660d6da44
📒 Files selected for processing (18)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Tools/Cameras/CameraControl.csMCPForUnity/Editor/Tools/Cameras/CameraHelpers.csMCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.csMCPForUnity/Editor/Tools/Graphics/LightBakingOps.csMCPForUnity/Editor/Tools/Graphics/RenderPipelineOps.csMCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.csMCPForUnity/Editor/Tools/Graphics/RenderingStatsOps.csMCPForUnity/Editor/Tools/Graphics/SkyboxOps.csMCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs.metaMCPForUnity/Editor/Tools/Graphics/VolumeOps.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- MCPForUnity/Editor/Helpers/ComponentOps.cs
- MCPForUnity/Editor/Tools/Cameras/CameraControl.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs (1)
130-137:⚠️ Potential issue | 🔴 CriticalDon't persist
GetInstanceID()into the serialized feature map.
m_RendererFeatureMapis serialized to disk, butGetInstanceID()is an in-memory handle that changes between Editor sessions. This will cause the feature map to become invalid after project reloads.🔧 Proposed fix using GlobalObjectId
// Also update the map (m_RendererFeatureMap) if it exists var mapProp = so.FindProperty("m_RendererFeatureMap"); if (mapProp != null) { mapProp.arraySize++; var mapElement = mapProp.GetArrayElementAtIndex(mapProp.arraySize - 1); - mapElement.longValue = feature.GetInstanceID(); + // Use GlobalObjectId for a stable identifier, or use the feature's file ID + var globalId = GlobalObjectId.GetGlobalObjectIdSlow(feature); + mapElement.longValue = (long)globalId.targetObjectId; so.ApplyModifiedProperties(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs` around lines 130 - 137, The code incorrectly persists an in-memory handle via GetInstanceID() into m_RendererFeatureMap; replace this with a stable GlobalObjectId for the feature by calling GlobalObjectId.GetGlobalObjectIdSlow(feature) and storing its identifier (e.g. globalId.ToString()) into the serialized map instead of mapElement.longValue; update the serialized map element storage to a string property (or a long-term stable field) and adjust any lookup code that consumes m_RendererFeatureMap to resolve back to an object using GlobalObjectId parsing/resolution rather than relying on Instance IDs (references: m_RendererFeatureMap, GetInstanceID(), GlobalObjectId.GetGlobalObjectIdSlow, feature, mapElement).MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs (2)
35-45:⚠️ Potential issue | 🟠 MajorGuard against null
VolumeTypebefore callingAddComponent.
GraphicsHelpers.VolumeTypecan benullif the Volume system (RP Core) isn't installed. Line 39 will throwArgumentNullExceptionfromAddComponent(null)instead of returning a clean error.🛡️ Proposed fix
var go = new GameObject(name); Undo.RegisterCreatedObjectUndo(go, $"Create Volume '{name}'"); + if (GraphicsHelpers.VolumeType == null || GraphicsHelpers.VolumeProfileType == null) + { + UnityEngine.Object.DestroyImmediate(go); + return new ErrorResponse("Volume system types not found. Ensure URP or HDRP (com.unity.render-pipelines.core) is installed."); + } + // Add Volume component via reflection var volumeComp = go.AddComponent(GraphicsHelpers.VolumeType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs` around lines 35 - 45, Guard against a null GraphicsHelpers.VolumeType before calling AddComponent to avoid ArgumentNullException; in the Create Volume flow in VolumeOps.cs check if GraphicsHelpers.VolumeType is null, if so log or show a user-facing error (e.g., EditorUtility.DisplayDialog or Debug.LogError), clean up the created GameObject (Undo.DestroyObjectImmediate or DestroyImmediate) and return early, otherwise proceed to call go.AddComponent(GraphicsHelpers.VolumeType) and the subsequent SetProperty calls.
196-263:⚠️ Potential issue | 🟠 MajorMark the owning volume dirty after parameter edits.
SetEffectmarks only the profile object dirty (line 251), but for embedded scene profiles this doesn't mark the scene dirty. The change can be lost on save/reload.🔧 Proposed fix
if (profile is UnityEngine.Object profileObj) EditorUtility.SetDirty(profileObj); + GraphicsHelpers.MarkDirty(volume); var msg = $"Set {setParams.Count} parameter(s) on '{effectName}'";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs` around lines 196 - 263, SetEffect currently only calls EditorUtility.SetDirty(profileObj) which doesn't mark the owning scene/Volume dirty for embedded profiles; after successful parameter edits (where 'volume' is the object returned by GraphicsHelpers.FindVolume and 'profile' is the profile object), also mark the owning Volume and its scene dirty: call EditorUtility.SetDirty(volume) (and optionally EditorSceneManager.MarkSceneDirty(volume.gameObject.scene) for scene-level persistence) right after setting profile dirty so changes aren't lost on save/reload.
🤖 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/Graphics/SkyboxOps.cs`:
- Around line 382-392: ParseColorToken currently only handles JArray and returns
null for string hex values, causing SetAmbient/SetFog to ignore hex color
inputs; add a branch to detect string tokens and parse HTML/hex color strings
(e.g., "#RRGGBB" or "#RRGGBBAA") using
UnityEngine.ColorUtility.TryParseHtmlString (or equivalent) and return the
parsed Color (including alpha if present), falling back to null on parse
failure; keep existing JArray handling intact and reference the ParseColorToken
method and its callers SetAmbient and SetFog when making the change.
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs`:
- Around line 451-475: The ListVolumes method in VolumeOps is not reachable from
the ManageGraphics dispatcher (ManageGraphics), so either document it as
resource-only or register it in the dispatcher; to fix, add a case branch in the
ManageGraphics switch that maps the "volume_list" action to
VolumeOps.ListVolumes(`@params`) (i.e., add `case "volume_list": return
VolumeOps.ListVolumes(`@params`);`), or alternatively add a clear comment in
VolumeOps/ListVolumes indicating it is intentionally exposed only via
VolumesResource so reviewers know it’s not missed.
---
Duplicate comments:
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs`:
- Around line 130-137: The code incorrectly persists an in-memory handle via
GetInstanceID() into m_RendererFeatureMap; replace this with a stable
GlobalObjectId for the feature by calling
GlobalObjectId.GetGlobalObjectIdSlow(feature) and storing its identifier (e.g.
globalId.ToString()) into the serialized map instead of mapElement.longValue;
update the serialized map element storage to a string property (or a long-term
stable field) and adjust any lookup code that consumes m_RendererFeatureMap to
resolve back to an object using GlobalObjectId parsing/resolution rather than
relying on Instance IDs (references: m_RendererFeatureMap, GetInstanceID(),
GlobalObjectId.GetGlobalObjectIdSlow, feature, mapElement).
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs`:
- Around line 35-45: Guard against a null GraphicsHelpers.VolumeType before
calling AddComponent to avoid ArgumentNullException; in the Create Volume flow
in VolumeOps.cs check if GraphicsHelpers.VolumeType is null, if so log or show a
user-facing error (e.g., EditorUtility.DisplayDialog or Debug.LogError), clean
up the created GameObject (Undo.DestroyObjectImmediate or DestroyImmediate) and
return early, otherwise proceed to call
go.AddComponent(GraphicsHelpers.VolumeType) and the subsequent SetProperty
calls.
- Around line 196-263: SetEffect currently only calls
EditorUtility.SetDirty(profileObj) which doesn't mark the owning scene/Volume
dirty for embedded profiles; after successful parameter edits (where 'volume' is
the object returned by GraphicsHelpers.FindVolume and 'profile' is the profile
object), also mark the owning Volume and its scene dirty: call
EditorUtility.SetDirty(volume) (and optionally
EditorSceneManager.MarkSceneDirty(volume.gameObject.scene) for scene-level
persistence) right after setting profile dirty so changes aren't lost on
save/reload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 694c9ad3-8870-4da9-a17f-feae72aaccb1
📒 Files selected for processing (4)
MCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.csMCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.csMCPForUnity/Editor/Tools/Graphics/SkyboxOps.csMCPForUnity/Editor/Tools/Graphics/VolumeOps.cs
Comment on lines
+382
to
+392
| private static Color? ParseColorToken(JToken token) | ||
| { | ||
| if (token == null || token.Type == JTokenType.Null) return null; | ||
| if (token is JArray arr && arr.Count >= 3) | ||
| { | ||
| return new Color( | ||
| (float)arr[0], (float)arr[1], (float)arr[2], | ||
| arr.Count >= 4 ? (float)arr[3] : 1f); | ||
| } | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
ParseColorToken silently returns null for hex strings.
The method handles JArray colors but returns null for string tokens (e.g., "#FF0000"). Since SetAmbient and SetFog use this for color parsing, hex color input will be silently ignored.
💡 Suggested enhancement
private static Color? ParseColorToken(JToken token)
{
if (token == null || token.Type == JTokenType.Null) return null;
if (token is JArray arr && arr.Count >= 3)
{
return new Color(
(float)arr[0], (float)arr[1], (float)arr[2],
arr.Count >= 4 ? (float)arr[3] : 1f);
}
+ if (token.Type == JTokenType.String)
+ {
+ if (ColorUtility.TryParseHtmlString(token.ToString(), out Color c))
+ return c;
+ }
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs` around lines 382 - 392,
ParseColorToken currently only handles JArray and returns null for string hex
values, causing SetAmbient/SetFog to ignore hex color inputs; add a branch to
detect string tokens and parse HTML/hex color strings (e.g., "#RRGGBB" or
"#RRGGBBAA") using UnityEngine.ColorUtility.TryParseHtmlString (or equivalent)
and return the parsed Color (including alpha if present), falling back to null
on parse failure; keep existing JArray handling intact and reference the
ParseColorToken method and its callers SetAmbient and SetFog when making the
change.
Comment on lines
+451
to
+475
| // === ListVolumes (used by VolumesResource) === | ||
| internal static object ListVolumes(JObject @params) | ||
| { | ||
| if (!GraphicsHelpers.HasVolumeSystem) | ||
| return new { success = true, message = "Volume system not available.", data = new { volumes = new List<object>() } }; | ||
|
|
||
| #if UNITY_2022_2_OR_NEWER | ||
| var allVolumes = UnityEngine.Object.FindObjectsByType(GraphicsHelpers.VolumeType, FindObjectsSortMode.None); | ||
| #else | ||
| var allVolumes = UnityEngine.Object.FindObjectsOfType(GraphicsHelpers.VolumeType); | ||
| #endif | ||
| var volumeList = new List<object>(); | ||
|
|
||
| foreach (Component vol in allVolumes) | ||
| { | ||
| volumeList.Add(BuildVolumeInfo(vol)); | ||
| } | ||
|
|
||
| return new | ||
| { | ||
| success = true, | ||
| message = $"Found {volumeList.Count} volume(s).", | ||
| data = new { pipeline = GraphicsHelpers.GetPipelineName(), volumes = volumeList } | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
ListVolumes is unreachable from ManageGraphics.
Per the relevant code snippet (ManageGraphics.cs:44-70), the dispatcher registers 8 volume actions but does not include "volume_list" → VolumeOps.ListVolumes. This method can only be called via VolumesResource, not the primary tool entry point.
If intentional (resource-only), consider adding a comment. Otherwise, register it in ManageGraphics:
case "volume_list":
return VolumeOps.ListVolumes(`@params`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs` around lines 451 - 475, The
ListVolumes method in VolumeOps is not reachable from the ManageGraphics
dispatcher (ManageGraphics), so either document it as resource-only or register
it in the dispatcher; to fix, add a case branch in the ManageGraphics switch
that maps the "volume_list" action to VolumeOps.ListVolumes(`@params`) (i.e., add
`case "volume_list": return VolumeOps.ListVolumes(`@params`);`), or alternatively
add a clear comment in VolumeOps/ListVolumes indicating it is intentionally
exposed only via VolumesResource so reviewers know it’s not missed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs (1)
308-316: Method namedTogglebut defaults totrueinstead of toggling.When
activeis omitted, the method defaults to enabling the feature rather than flipping its current state. Consider either:
- Querying
isActiveand inverting whenactiveis null for true toggle behavior, or- Rename to
SetFeatureActiveto clarify it sets rather than toggles♻️ Proposed toggle behavior
var setActiveMethod = feature.GetType().GetMethod("SetActive", BindingFlags.Public | BindingFlags.Instance); if (setActiveMethod == null) return new ErrorResponse("SetActive method not found on feature."); - bool newState = active ?? true; + bool newState; + if (active.HasValue) + { + newState = active.Value; + } + else + { + // True toggle: invert current state + var isActiveProp = feature.GetType().GetProperty("isActive", + BindingFlags.Public | BindingFlags.Instance); + bool currentState = isActiveProp != null ? (bool)isActiveProp.GetValue(feature) : true; + newState = !currentState; + } Undo.RecordObject(feature, "Toggle Renderer Feature");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs` around lines 308 - 316, The Toggle method currently defaults to enabling the feature when active is null instead of flipping the current state; change behavior so that when active is null you query the feature's current active state (try property "isActive" first, then "active" if absent using feature.GetType().GetProperty with BindingFlags.Public | BindingFlags.Instance), invert that value to compute newState, and only fall back to true if neither property is found; keep using Undo.RecordObject(feature, "Toggle Renderer Feature") and invoke SetActive(newState) via the existing setActiveMethod.Invoke call.
🤖 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/Graphics/RendererFeatureOps.cs`:
- Around line 130-141: The code currently always appends localId to
m_RendererFeatureMap even when
AssetDatabase.TryGetGUIDAndLocalFileIdentifier(feature, out _, out localId)
returns false (leaving localId == 0); fix by checking the boolean result and
only incrementing mapProp.arraySize, getting the new element, setting
mapElement.longValue and calling so.ApplyModifiedProperties() when
TryGetGUIDAndLocalFileIdentifier returns true; if it returns false, skip adding
the entry (and optionally log or Debug.LogWarning referencing feature and the
mapProp) to avoid persisting an invalid 0 ID.
- Around line 346-357: The code currently casts orderToken entries via (int)t
which will throw InvalidCastException for non-integer values; update the logic
around orderToken => newOrder generation (the newOrder variable and related
validation in RendererFeatureOps) to defensively validate/parse each element:
check each token type (e.g., ensure it's numeric or can be parsed to int), skip
or abort on invalid entries and return a meaningful ErrorResponse (instead of
letting an exception bubble) when any element is not an integer; ensure the
error message clearly states "Order array contains non-integer or malformed
entries" and preserve the existing length/index validation for featuresList
after parsing.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs`:
- Around line 308-316: The Toggle method currently defaults to enabling the
feature when active is null instead of flipping the current state; change
behavior so that when active is null you query the feature's current active
state (try property "isActive" first, then "active" if absent using
feature.GetType().GetProperty with BindingFlags.Public | BindingFlags.Instance),
invert that value to compute newState, and only fall back to true if neither
property is found; keep using Undo.RecordObject(feature, "Toggle Renderer
Feature") and invoke SetActive(newState) via the existing setActiveMethod.Invoke
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9ab5a00-286b-4e0f-9c75-5ba61d7cfa42
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Helpers/GameObjectLookup.cs
Comment on lines
+130
to
+141
| // Also update the map (m_RendererFeatureMap) if it exists | ||
| // Map stores persistent local file IDs, not transient instance IDs | ||
| var mapProp = so.FindProperty("m_RendererFeatureMap"); | ||
| if (mapProp != null) | ||
| { | ||
| long localId = 0; | ||
| AssetDatabase.TryGetGUIDAndLocalFileIdentifier(feature, out _, out localId); | ||
| mapProp.arraySize++; | ||
| var mapElement = mapProp.GetArrayElementAtIndex(mapProp.arraySize - 1); | ||
| mapElement.longValue = localId; | ||
| so.ApplyModifiedProperties(); | ||
| } |
Contributor
There was a problem hiding this comment.
Check TryGetGUIDAndLocalFileIdentifier return value before storing.
If the call fails (returns false), localId remains 0 and gets persisted into m_RendererFeatureMap, potentially corrupting the map with invalid entries.
🛡️ Proposed fix
var mapProp = so.FindProperty("m_RendererFeatureMap");
if (mapProp != null)
{
long localId = 0;
- AssetDatabase.TryGetGUIDAndLocalFileIdentifier(feature, out _, out localId);
- mapProp.arraySize++;
- var mapElement = mapProp.GetArrayElementAtIndex(mapProp.arraySize - 1);
- mapElement.longValue = localId;
- so.ApplyModifiedProperties();
+ if (AssetDatabase.TryGetGUIDAndLocalFileIdentifier(feature, out _, out localId) && localId != 0)
+ {
+ mapProp.arraySize++;
+ var mapElement = mapProp.GetArrayElementAtIndex(mapProp.arraySize - 1);
+ mapElement.longValue = localId;
+ so.ApplyModifiedProperties();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs` around lines 130 -
141, The code currently always appends localId to m_RendererFeatureMap even when
AssetDatabase.TryGetGUIDAndLocalFileIdentifier(feature, out _, out localId)
returns false (leaving localId == 0); fix by checking the boolean result and
only incrementing mapProp.arraySize, getting the new element, setting
mapElement.longValue and calling so.ApplyModifiedProperties() when
TryGetGUIDAndLocalFileIdentifier returns true; if it returns false, skip adding
the entry (and optionally log or Debug.LogWarning referencing feature and the
mapProp) to avoid persisting an invalid 0 ID.
Comment on lines
+346
to
+357
| var newOrder = orderToken.Select(t => (int)t).ToList(); | ||
| if (newOrder.Count != featuresList.Count) | ||
| return new ErrorResponse( | ||
| $"Order array length ({newOrder.Count}) must match feature count ({featuresList.Count})."); | ||
|
|
||
| // Validate all indices are present | ||
| var sorted = newOrder.OrderBy(x => x).ToList(); | ||
| for (int i = 0; i < sorted.Count; i++) | ||
| { | ||
| if (sorted[i] != i) | ||
| return new ErrorResponse("Order array must contain each index exactly once (0 to N-1)."); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle potential cast exception for malformed order array.
If order contains non-integer values, the (int)t cast throws an unhandled InvalidCastException.
🛡️ Proposed defensive handling
- var newOrder = orderToken.Select(t => (int)t).ToList();
+ List<int> newOrder;
+ try
+ {
+ newOrder = orderToken.Select(t => t.Value<int>()).ToList();
+ }
+ catch (Exception)
+ {
+ return new ErrorResponse("'order' array must contain integer indices.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var newOrder = orderToken.Select(t => (int)t).ToList(); | |
| if (newOrder.Count != featuresList.Count) | |
| return new ErrorResponse( | |
| $"Order array length ({newOrder.Count}) must match feature count ({featuresList.Count})."); | |
| // Validate all indices are present | |
| var sorted = newOrder.OrderBy(x => x).ToList(); | |
| for (int i = 0; i < sorted.Count; i++) | |
| { | |
| if (sorted[i] != i) | |
| return new ErrorResponse("Order array must contain each index exactly once (0 to N-1)."); | |
| } | |
| List<int> newOrder; | |
| try | |
| { | |
| newOrder = orderToken.Select(t => t.Value<int>()).ToList(); | |
| } | |
| catch (Exception) | |
| { | |
| return new ErrorResponse("'order' array must contain integer indices."); | |
| } | |
| if (newOrder.Count != featuresList.Count) | |
| return new ErrorResponse( | |
| $"Order array length ({newOrder.Count}) must match feature count ({featuresList.Count})."); | |
| // Validate all indices are present | |
| var sorted = newOrder.OrderBy(x => x).ToList(); | |
| for (int i = 0; i < sorted.Count; i++) | |
| { | |
| if (sorted[i] != i) | |
| return new ErrorResponse("Order array must contain each index exactly once (0 to N-1)."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs` around lines 346 -
357, The code currently casts orderToken entries via (int)t which will throw
InvalidCastException for non-integer values; update the logic around orderToken
=> newOrder generation (the newOrder variable and related validation in
RendererFeatureOps) to defensively validate/parse each element: check each token
type (e.g., ensure it's numeric or can be parsed to int), skip or abort on
invalid entries and return a meaningful ErrorResponse (instead of letting an
exception bubble) when any element is not an integer; ensure the error message
clearly states "Order array contains non-integer or malformed entries" and
preserve the existing length/index validation for featuresList after parsing.
Seems EntityId is implicitly casted to Int so no need to prevent it.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
383-387: Centralize the Unity-version-specific instance lookup within this file.This branch duplicates the same
UNITY_6000_3_OR_NEWERlogic already present inMCPForUnity/Editor/Helpers/GameObjectLookup.cs:70-77. BecauseUnityTypeConverters.csis in the Runtime assembly and cannot reference Editor code (assembly boundary), you cannot reuseGameObjectLookup.ResolveInstanceID()directly. Extract the duplicated logic into a local static helper method in this file instead to avoid future drift if the threshold or API changes.Suggested refactor
+#if UNITY_EDITOR +private static UnityEngine.Object ResolveInstanceId(int instanceId) +{ +#if UNITY_6000_3_OR_NEWER + return UnityEditor.EditorUtility.EntityIdToObject(instanceId); +#else + return UnityEditor.EditorUtility.InstanceIDToObject(instanceId); +#endif +} +#endif `#if` UNITY_6000_3_OR_NEWER - UnityEngine.Object obj = UnityEditor.EditorUtility.EntityIdToObject(instanceId); + UnityEngine.Object obj = ResolveInstanceId(instanceId); `#else` - UnityEngine.Object obj = UnityEditor.EditorUtility.InstanceIDToObject(instanceId); + UnityEngine.Object obj = ResolveInstanceId(instanceId); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs` around lines 383 - 387, The UNITY_6000_3_OR_NEWER conditional that chooses between UnityEditor.EditorUtility.EntityIdToObject and InstanceIDToObject is duplicated here; extract that compile-time conditional into a private static helper inside UnityTypeConverters.cs (e.g., a method named ResolveEditorObjectFromInstanceId(int instanceId)) and replace the inline conditional with a call to that method so all version-specific lookup logic is centralized in this file (keep the method internal/private static and only use it from methods like the code that currently declares obj). Ensure the helper uses the same UNITY_6000_3_OR_NEWER preprocessor branches as existing code.
🤖 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/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 383-387: The UNITY_6000_3_OR_NEWER conditional that chooses
between UnityEditor.EditorUtility.EntityIdToObject and InstanceIDToObject is
duplicated here; extract that compile-time conditional into a private static
helper inside UnityTypeConverters.cs (e.g., a method named
ResolveEditorObjectFromInstanceId(int instanceId)) and replace the inline
conditional with a call to that method so all version-specific lookup logic is
centralized in this file (keep the method internal/private static and only use
it from methods like the code that currently declares obj). Ensure the helper
uses the same UNITY_6000_3_OR_NEWER preprocessor branches as existing code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 039d9a3a-85b4-4bbc-95ac-1480a73081e8
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Helpers/GameObjectLookup.cs
4 tasks
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
Route all the graphics layer to manage_graphics, and add explicit RP control, including Volume Profile, Post Processing, and Skybox
Type of Change
Save your change type
Changes Made
New feature: manage_graphics tool (graphics group)
New MCP tool with 33 actions across 5 groups: Volume, Bake, Stats, Pipeline, Features
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by CodeRabbit
New Features
Improvements