Merged
Conversation
Use a version-gated helper that calls GetEntityId on UNITY_6000_5_OR_NEWER while preserving GetInstanceID for older Unity versions.
Suppress the transitional CS0619 cast warning when serializing GetEntityId so projects with warnings-as-errors can compile while keeping backward-compatible instanceID payloads.
Replace direct GetInstanceID calls with a compatibility helper and update serialization to handle EntityId-era identifiers, so the package compiles on Unity 6.5 while preserving behavior on older versions.
Place the compatibility extension in MCPForUnity.Runtime so Runtime/Serialization can resolve GetInstanceIDCompat, fixing Unity 2022 compile errors from assembly visibility.
Keep instanceID in outgoing payloads for wire compatibility, add entityID on newer Unity, continue fallback resolution when entityID lookup fails, and make reflective entity resolution exception-safe.
Addresses review feedback on the Unity 6.5 GetInstanceID migration: - UnityObjectIdCompatExtensions: drop the reflective method lookup in favor of a simple #if UNITY_6000_5_OR_NEWER / #else split calling GetEntityId() or GetInstanceID() directly. Also wrap the class in the MCPForUnity.Runtime.Helpers namespace. - UnityTypeConverters: remove the reflective EntityIdToObject probe and call EditorUtility.EntityIdToObject(EntityId) directly under the same version gate. Serialize entityID as EntityId.ToULong() rather than ToString(), since Unity's docs explicitly warn that the textual form is not a stable serialization contract. - Drop #pragma warning disable 0619 from 22 files that no longer make any direct calls to obsolete APIs. The remaining 7 files still need it (FindObjectsOfType, InstanceIDToObject fallback) and are left as-is — those deprecations are out of scope for this PR. - Add the MCPForUnity.Runtime.Helpers using to every file that calls GetInstanceIDCompat() now that the extension method lives in a namespace.
Contributor
Reviewer's GuideAdds Unity 6.5-compatible object ID and object-finding helpers, switches existing systems to use them, and extends Unity object JSON serialization/deserialization to support the new EntityId API while preserving backward compatibility with instanceID-based workflows. Sequence diagram for UnityEngine.Object JSON serialization/deserialization with entityID supportsequenceDiagram
participant Caller
participant Converter as UnityEngineObjectConverter
participant Obj as UnityEngine.Object
participant IdExt as UnityObjectIdCompatExtensions
participant EntityId
participant EditorUtil as UnityEditor.EditorUtility
Caller->>Converter: WriteJson(writer, Obj, serializer)
activate Converter
Converter->>Obj: name
Converter->>IdExt: GetInstanceIDCompat(Obj)
IdExt-->>Converter: instanceID:int
Converter->>Obj: GetEntityId()
Obj-->>Converter: EntityId
Converter->>EntityId: ToULong(EntityId)
EntityId-->>Converter: entityIdUlong:ulong
Converter-->>Caller: JSON with name, instanceID, entityID
deactivate Converter
Caller->>Converter: ReadJson(reader, objectType, existingValue, serializer)
activate Converter
Converter->>Converter: parse JObject jo
Converter->>jo: try get entityID
alt entityID present and valid
Converter->>EntityId: FromULong(parsedEntityId)
EntityId-->>Converter: eid
Converter->>EditorUtil: EntityIdToObject(eid)
EditorUtil-->>Converter: entityObj
alt entityObj matches objectType
Converter-->>Caller: entityObj
else objectType is Transform and entityObj is GameObject
Converter-->>Caller: entityObj.transform
else objectType is Component
Converter->>entityObj: GetComponent(objectType)
entityObj-->>Converter: component
Converter-->>Caller: component or null
end
else no valid entityID
Converter->>jo: try get instanceID
note over Converter,Caller: If entityID resolution fails or is missing Converter falls back to legacy instanceID guid or path logic.
end
deactivate Converter
Class diagram for Unity 6.5-compatible object ID helpers and convertersclassDiagram
class UnityEngineObjectConverter {
+WriteJson(writer, value, serializer)
+ReadJson(reader, objectType, existingValue, serializer) UnityEngine.Object
-WriteSerializedObjectId(writer, value)
}
class UnityFindObjectsCompat {
+FindAll~T~() T[]
+FindAll(type) Object[]
+FindAll(type, includeInactive) Object[]
+FindAny(type) Object
}
class UnityObjectIdCompatExtensions {
+GetInstanceIDCompat(obj) int
}
class UnityEngineObject {
<<UnityEngine>>
+name string
+GetEntityId() EntityId
}
class EntityId {
<<UnityEngine>>
+FromULong(value) EntityId
+ToULong(id) ulong
}
class UnityEditorEditorUtility {
<<UnityEditor>>
+EntityIdToObject(id) UnityEngineObject
}
UnityEngineObjectConverter ..> UnityFindObjectsCompat : uses
UnityEngineObjectConverter ..> UnityObjectIdCompatExtensions : uses
UnityEngineObjectConverter ..> UnityEditorEditorUtility : resolves_entityID
UnityObjectIdCompatExtensions ..> UnityEngineObject : extends
UnityFindObjectsCompat ..> UnityEngineObject : finds
UnityEditorEditorUtility ..> EntityId : converts
UnityEngineObjectConverter ..> EntityId : serialize_entityID
UnityEngineObject <|-- EntityId
Flow diagram for UnityEngine.Object deserialization resolution orderflowchart TD
A["Start ReadJson for UnityEngine.Object"] --> B{"JSON has entityID field?"}
B -- "Yes" --> C["Parse entityID string to ulong"]
C --> D{"Parse succeeded?"}
D -- "Yes" --> E["Create EntityId from ulong"]
E --> F["EditorUtility.EntityIdToObject(eid)"]
F --> G{"entityObj not null?"}
G -- "Yes" --> H{"objectType assignable from entityObj type?"}
H -- "Yes" --> R1["Return entityObj"]
H -- "No" --> I{"objectType is Transform and entityObj is GameObject?"}
I -- "Yes" --> R2["Return entityObj.transform"]
I -- "No" --> J{"objectType is Component and entityObj is GameObject?"}
J -- "Yes" --> K["GetComponent(objectType) on GameObject"]
K --> L{"component not null?"}
L -- "Yes" --> R3["Return component"]
L -- "No" --> Bfallback["Log warning: entityID could not be resolved"]
G -- "No" --> Bfallback
D -- "No" --> Bfallback
Bfallback --> M["Fall back to instanceID/guid/path resolution"]
B -- "No" --> N{"JSON has instanceID field?"}
N -- "Yes" --> O["Resolve by instanceID"]
O --> P{"Resolved object not null?"}
P -- "Yes" --> R4["Return resolved object"]
P -- "No" --> Q{"JSON has guid or path field?"}
N -- "No" --> Q
Q -- "Yes" --> R["Resolve by guid/path (asset lookup)"]
R --> S{"Resolved object not null?"}
S -- "Yes" --> R5["Return resolved object"]
S -- "No" --> T["Log warning: could not resolve object"]
Q -- "No" --> T
T --> U["Return null"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces compatibility helpers for handling Unity version differences—specifically supporting Unity 6.0.5+ which changed instance ID retrieval—and systematically updates ~30 files across editor tools, serialization, and runtime helpers to use these new abstractions instead of version-specific APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In UnityTypeConverters.ReadJson the new entityID resolution block is only wrapped in UNITY_6000_5_OR_NEWER, but it still calls UnityEditor.EditorUtility; this code should also be gated by UNITY_EDITOR (or moved to an Editor-only path) to avoid UnityEditor references in runtime/player builds.
- The new GetInstanceIDCompat implementation truncates a ulong EntityId to int on Unity 6.5+, which can introduce collisions for large id ranges; if these IDs are used as keys or for lookup, consider either documenting this limitation clearly at call sites or preferring the new entityID field in those flows instead of relying on the compat int.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In UnityTypeConverters.ReadJson the new entityID resolution block is only wrapped in UNITY_6000_5_OR_NEWER, but it still calls UnityEditor.EditorUtility; this code should also be gated by UNITY_EDITOR (or moved to an Editor-only path) to avoid UnityEditor references in runtime/player builds.
- The new GetInstanceIDCompat implementation truncates a ulong EntityId to int on Unity 6.5+, which can introduce collisions for large id ranges; if these IDs are used as keys or for lookup, consider either documenting this limitation clearly at call sites or preferring the new entityID field in those flows instead of relying on the compat int.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates MCP for Unity to compile and run cleanly on Unity 6.5 by replacing deprecated Object.GetInstanceID() call sites with compatibility helpers and extending object serialization to support Unity 6.5’s entity-style identifiers.
Changes:
- Introduces
GetInstanceIDCompat()and replacesGetInstanceID()usages across editor/runtime tooling. - Adds
UnityFindObjectsCompatwrappers and updates callers to use version-compatible object-finding APIs. - Extends
UnityEngine.ObjectJSON serialization/deserialization to emit/consumeentityID(Unity 6.5+) while keepinginstanceIDfor backward compatibility.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | Emits instanceID via compat helper and optionally emits/reads entityID for Unity 6.5+ object reference round-trips. |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs | Adds GetInstanceIDCompat() to avoid Unity 6.5 GetInstanceID() compile breaks. |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.meta | Adds Unity meta for new script. |
| MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs | Adds version-gated wrappers around Unity’s object-finding APIs. |
| MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs.meta | Adds Unity meta for new script. |
| MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | Uses UnityFindObjectsCompat to locate cameras across Unity versions. |
| MCPForUnity/Editor/Tools/Vfx/ParticleControl.cs | Uses GetInstanceIDCompat() in tool responses. |
| MCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.cs | Uses GetInstanceIDCompat() for returned instance IDs. |
| MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | Uses GetInstanceIDCompat() for prefab and hierarchy IDs. |
| MCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.cs | Uses GetInstanceIDCompat() and UnityFindObjectsCompat for rigidbody enumeration. |
| MCPForUnity/Editor/Tools/Physics/PhysicsRigidbodyOps.cs | Uses GetInstanceIDCompat() in rigidbody info responses. |
| MCPForUnity/Editor/Tools/Physics/PhysicsQueryOps.cs | Uses GetInstanceIDCompat() for collider/gameobject IDs in query results. |
| MCPForUnity/Editor/Tools/Physics/JointOps.cs | Uses GetInstanceIDCompat() for joint/gameobject IDs. |
| MCPForUnity/Editor/Tools/ManageUI.cs | Uses GetInstanceIDCompat() for PanelSettings identification. |
| MCPForUnity/Editor/Tools/ManageScene.cs | Uses UnityFindObjectsCompat for camera/renderer enumeration and GetInstanceIDCompat() in summaries. |
| MCPForUnity/Editor/Tools/ManageMaterial.cs | Uses GetInstanceIDCompat() for unique material asset naming. |
| MCPForUnity/Editor/Tools/ManageComponents.cs | Uses GetInstanceIDCompat() for component and gameobject IDs in responses. |
| MCPForUnity/Editor/Tools/ManageAsset.cs | Uses GetInstanceIDCompat() in asset/component payloads and minor cleanup. |
| MCPForUnity/Editor/Tools/Graphics/VolumeOps.cs | Uses GetInstanceIDCompat() and UnityFindObjectsCompat for volume operations. |
| MCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs | Uses GetInstanceIDCompat() for sun/gameobject IDs. |
| MCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.cs | Uses GetInstanceIDCompat() for renderer feature IDs. |
| MCPForUnity/Editor/Tools/Graphics/LightBakingOps.cs | Uses GetInstanceIDCompat() for created/baked object IDs. |
| MCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.cs | Uses UnityFindObjectsCompat for volume lookup. |
| MCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.cs | Uses GetInstanceIDCompat() in instance-id based searches and UnityFindObjectsCompat for type searches. |
| MCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.cs | Uses GetInstanceIDCompat() in response payloads. |
| MCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.cs | Uses GetInstanceIDCompat() for original object ID reporting. |
| MCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.cs | Uses GetInstanceIDCompat() when reporting deleted object IDs. |
| MCPForUnity/Editor/Tools/Cameras/CameraHelpers.cs | Uses UnityFindObjectsCompat for camera/brain discovery. |
| MCPForUnity/Editor/Tools/Cameras/CameraCreate.cs | Uses GetInstanceIDCompat() in create/ensure responses. |
| MCPForUnity/Editor/Tools/Cameras/CameraControl.cs | Uses UnityFindObjectsCompat enumeration and GetInstanceIDCompat() in payloads. |
| MCPForUnity/Editor/Tools/Cameras/CameraConfigure.cs | Uses GetInstanceIDCompat() in configuration responses. |
| MCPForUnity/Editor/Resources/Scene/GameObjectResource.cs | Uses GetInstanceIDCompat() for scene serialization IDs. |
| MCPForUnity/Editor/Resources/Editor/Windows.cs | Uses GetInstanceIDCompat() for editor window IDs. |
| MCPForUnity/Editor/Resources/Editor/Selection.cs | Uses GetInstanceIDCompat() for selection IDs. |
| MCPForUnity/Editor/Helpers/GameObjectSerializer.cs | Uses GetInstanceIDCompat() throughout serialized object/component payloads. |
| MCPForUnity/Editor/Helpers/GameObjectLookup.cs | Uses GetInstanceIDCompat() for search result IDs. |
| MCPForUnity/Editor/Helpers/ComponentOps.cs | Uses GetInstanceIDCompat() in logging for sprite file-id lookup failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+20
to
+27
| #if UNITY_6000_5_OR_NEWER | ||
| // GetInstanceID() is obsolete-as-error on Unity 6.5+. Truncate the EntityId's | ||
| // underlying ulong to int; this is lossy but stable within a session and | ||
| // preserves the int-shaped wire format that older consumers expect. | ||
| return (int)EntityId.ToULong(obj.GetEntityId()); | ||
| #else | ||
| return obj.GetInstanceID(); | ||
| #endif |
There was a problem hiding this comment.
GetInstanceIDCompat() derives the legacy int ID by casting the 64-bit EntityId to int. This is inherently lossy (upper 32 bits discarded), so different objects can collide to the same instanceID on Unity 6.5+, which can break lookups/commands that rely on instanceID uniqueness. Consider maintaining a session-scoped bijective mapping from EntityId (ulong) to a generated int handle (or otherwise ensure uniqueness), and/or make the cast explicitly unchecked to avoid overflow exceptions in checked builds.
Comment on lines
+1
to
+2
| fileFormatVersion: 2 | ||
| guid: 5ea8b580f3e80459cab9019f71d0ccb6 No newline at end of file |
There was a problem hiding this comment.
This new .meta file only contains fileFormatVersion and guid. Unity typically writes a full MonoImporter block for C# scripts; committing a minimal meta often results in Unity regenerating it on import and causing noisy diffs/merge conflicts. Please include the standard MonoImporter section (matching other script metas in the repo).
Collaborator
Author
There was a problem hiding this comment.
@copilot apply changes based on this feedback
Comment on lines
+1
to
+2
| fileFormatVersion: 2 | ||
| guid: 7c3a9f12e4b60438d9a1c52ef8d07b3e No newline at end of file |
There was a problem hiding this comment.
This new .meta file only contains fileFormatVersion and guid. Unity typically writes a full MonoImporter block for C# scripts; committing a minimal meta often results in Unity regenerating it on import and causing noisy diffs/merge conflicts. Please include the standard MonoImporter section (matching other script metas in the repo).
Collaborator
Author
There was a problem hiding this comment.
@copilot apply changes based on this feedback
![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: 4
🧹 Nitpick comments (2)
MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs (1)
28-31: Add explicit null checks fortypeparameters.These public methods currently pass null through to Unity APIs, which can surface less actionable exceptions. Guarding early improves diagnostics.
♻️ Suggested guard clauses
public static UObject[] FindAll(Type type) { + if (type == null) throw new ArgumentNullException(nameof(type)); `#if` UNITY_6000_5_OR_NEWER return UObject.FindObjectsByType(type, UnityEngine.FindObjectsInactive.Exclude); `#elif` UNITY_2022_2_OR_NEWER return UObject.FindObjectsByType(type, UnityEngine.FindObjectsSortMode.None); `#else` @@ public static UObject[] FindAll(Type type, bool includeInactive) { + if (type == null) throw new ArgumentNullException(nameof(type)); `#if` UNITY_6000_5_OR_NEWER return UObject.FindObjectsByType(type, includeInactive ? UnityEngine.FindObjectsInactive.Include : UnityEngine.FindObjectsInactive.Exclude); @@ public static UObject FindAny(Type type) { + if (type == null) throw new ArgumentNullException(nameof(type)); `#if` UNITY_2022_2_OR_NEWER return UObject.FindAnyObjectByType(type); `#else`Also applies to: 40-44, 55-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs` around lines 28 - 31, Add an explicit null guard at the start of each public method that accepts a Type (e.g., FindAll(Type) and the other FindAll overloads referenced) so you don't pass null into Unity APIs; check if (type == null) and throw new ArgumentNullException(nameof(type)) before calling UObject.FindObjectsByType or UObject.FindObjectsOfTypeAll to provide a clear, actionable exception and prevent downstream Unity errors.MCPForUnity/Editor/Tools/ManageScene.cs (1)
817-818: Optional cleanup: remove redundant active-state checks afterFindAll<Renderer>().If
UnityFindObjectsCompat.FindAll<Renderer>()already excludes inactive objects, theseactiveInHierarchychecks are unnecessary branches.♻️ Proposed cleanup
- if (r == null || !r.gameObject.activeInHierarchy) continue; + if (r == null) continue;Also applies to: 958-959, 1257-1258
🤖 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 817 - 818, The code redundantly checks renderer active state after calling UnityFindObjectsCompat.FindAll<Renderer>() — remove the unnecessary '!r.gameObject.activeInHierarchy' checks (and the associated short-circuiting branches) where you already filter by the compat finder; keep the null check for safety. Update the occurrences around the blocks that iterate renderers in ManageScene.cs (the if handling 'r == null || !r.gameObject.activeInHierarchy' near the current lines and the similar checks around lines referenced 958-959 and 1257-1258), leaving the null guard and any logic that depends on activeInHierarchy only if truly required elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/ManageMaterial.cs`:
- Line 421: The material path uses GetInstanceIDCompat() (matPath, safeName, go)
which truncates the object ID and can collide; change the filename generation to
use the full non-lossy identifier for the GameObject (e.g., replace
GetInstanceIDCompat() with your project's non-lossy ID getter such as the
EntityId/GUID provider or a GetInstanceIDNonLossy() equivalent) so matPath uses
that full identifier to produce stable, non-colliding material filenames.
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Line 1036: The code uses GetInstanceIDCompat() for psId which can be lossy on
newer Unity versions and cause collisions; replace that key with a non-lossy
identifier (e.g., use panelSettings.GetInstanceID() or another unique stable id
derived from the object) and propagate that change to all places that rely on
psId (s_panelRTs dictionary keys and any RT asset naming that currently uses
psId in ManageUI.cs, e.g., where s_panelRTs is accessed and RT names are
constructed). Ensure the chosen id is used consistently for lookup, creation,
and naming so unrelated panels cannot collide.
In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs`:
- Line 24: The cast from ulong to int in UnityObjectIdCompatExtensions (return
(int)EntityId.ToULong(obj.GetEntityId());) should be made explicit about lossy
behavior by wrapping it in unchecked(); update the return expression to use
unchecked((int)EntityId.ToULong(obj.GetEntityId())) so the intent is clear and
future overflow-checking settings won't change behavior.
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 379-383: In UnityTypeConverters.cs update the entityID token type
check so numeric tokens are accepted: in the block that checks
jo.TryGetValue("entityID", out JToken entityIdToken) and currently requires
entityIdToken.Type == JTokenType.String, change it to allow JTokenType.Integer
as well (e.g. entityIdToken.Type == JTokenType.String || entityIdToken.Type ==
JTokenType.Integer), then continue using entityIdToken.ToString() and
ulong.TryParse into rawEntityId as before to resolve the entity instead of
falling back to instanceID; also add unit tests covering both payload shapes
("entityID":"123" and "entityID":123) to ensure both code paths use the
entity-resolution logic (look for variables/methods named entityIdToken,
serializedEntityId, rawEntityId, and the instanceID fallback to validate
behavior).
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 817-818: The code redundantly checks renderer active state after
calling UnityFindObjectsCompat.FindAll<Renderer>() — remove the unnecessary
'!r.gameObject.activeInHierarchy' checks (and the associated short-circuiting
branches) where you already filter by the compat finder; keep the null check for
safety. Update the occurrences around the blocks that iterate renderers in
ManageScene.cs (the if handling 'r == null || !r.gameObject.activeInHierarchy'
near the current lines and the similar checks around lines referenced 958-959
and 1257-1258), leaving the null guard and any logic that depends on
activeInHierarchy only if truly required elsewhere.
In `@MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs`:
- Around line 28-31: Add an explicit null guard at the start of each public
method that accepts a Type (e.g., FindAll(Type) and the other FindAll overloads
referenced) so you don't pass null into Unity APIs; check if (type == null) and
throw new ArgumentNullException(nameof(type)) before calling
UObject.FindObjectsByType or UObject.FindObjectsOfTypeAll to provide a clear,
actionable exception and prevent downstream Unity errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed363935-6a11-4a84-8482-df661c8587d5
📒 Files selected for processing (37)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/GameObjectSerializer.csMCPForUnity/Editor/Resources/Editor/Selection.csMCPForUnity/Editor/Resources/Editor/Windows.csMCPForUnity/Editor/Resources/Scene/GameObjectResource.csMCPForUnity/Editor/Tools/Cameras/CameraConfigure.csMCPForUnity/Editor/Tools/Cameras/CameraControl.csMCPForUnity/Editor/Tools/Cameras/CameraCreate.csMCPForUnity/Editor/Tools/Cameras/CameraHelpers.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDelete.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.csMCPForUnity/Editor/Tools/Graphics/GraphicsHelpers.csMCPForUnity/Editor/Tools/Graphics/LightBakingOps.csMCPForUnity/Editor/Tools/Graphics/RendererFeatureOps.csMCPForUnity/Editor/Tools/Graphics/SkyboxOps.csMCPForUnity/Editor/Tools/Graphics/VolumeOps.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageComponents.csMCPForUnity/Editor/Tools/ManageMaterial.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Editor/Tools/Physics/JointOps.csMCPForUnity/Editor/Tools/Physics/PhysicsQueryOps.csMCPForUnity/Editor/Tools/Physics/PhysicsRigidbodyOps.csMCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.csMCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csMCPForUnity/Editor/Tools/ProBuilder/ManageProBuilder.csMCPForUnity/Editor/Tools/Vfx/ParticleControl.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.csMCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs.metaMCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.csMCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.metaMCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
| } | ||
|
|
||
| string matPath = $"{materialFolder}/{safeName}_{go.GetInstanceID()}_mat.mat"; | ||
| string matPath = $"{materialFolder}/{safeName}_{go.GetInstanceIDCompat()}_mat.mat"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "GetInstanceIDCompat\\(|CreateUniqueAndAssign\\(|_mat\\.mat|EntityId\\.ToULong" \
MCPForUnity/Editor/Tools/ManageMaterial.cs \
MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.csRepository: CoplayDev/unity-mcp
Length of output: 733
🏁 Script executed:
# First, let's see the complete CreateUniqueAndAssign method and how matPath is used
sed -n '407,450p' MCPForUnity/Editor/Tools/ManageMaterial.csRepository: CoplayDev/unity-mcp
Length of output: 2142
🏁 Script executed:
# Also check if there's any deduplication or collision handling logic
rg -A 10 -B 5 "CreateUniqueAndAssign" MCPForUnity/Editor/Tools/ManageMaterial.cs | head -80Repository: CoplayDev/unity-mcp
Length of output: 1324
🏁 Script executed:
# Check the complete create_unique mode logic
rg -A 5 -B 5 "create_unique" MCPForUnity/Editor/Tools/ManageMaterial.csRepository: CoplayDev/unity-mcp
Length of output: 582
🏁 Script executed:
# Find all uses of GetInstanceIDCompat in the codebase
rg "GetInstanceIDCompat" --type csRepository: CoplayDev/unity-mcp
Length of output: 14215
🏁 Script executed:
# Also check if GetStableObjectKey or similar non-lossy ID getter already exists
rg "ToULong|GetEntityId" MCPForUnity --type cs | head -30Repository: CoplayDev/unity-mcp
Length of output: 478
Use non-lossy object ID for stable material file naming.
Line 421 uses GetInstanceIDCompat() for persistent asset file naming. This truncates EntityId to int on Unity 6.5+, creating collision risk: different GameObjects with IDs differing only in high bits will map to the same material path, causing unintended overwrites or reuse.
For persistent asset paths, use the full non-lossy identifier:
🔧 Proposed fix
+ private static string GetStableObjectKey(UnityEngine.Object obj)
+ {
+#if UNITY_6000_5_OR_NEWER
+ return EntityId.ToULong(obj.GetEntityId()).ToString();
+#else
+ return obj.GetInstanceID().ToString();
+#endif
+ }
...
- string matPath = $"{materialFolder}/{safeName}_{go.GetInstanceIDCompat()}_mat.mat";
+ string matPath = $"{materialFolder}/{safeName}_{GetStableObjectKey(go)}_mat.mat";📝 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
| string matPath = $"{materialFolder}/{safeName}_{go.GetInstanceIDCompat()}_mat.mat"; | |
| private static string GetStableObjectKey(UnityEngine.Object obj) | |
| { | |
| `#if` UNITY_6000_5_OR_NEWER | |
| return EntityId.ToULong(obj.GetEntityId()).ToString(); | |
| `#else` | |
| return obj.GetInstanceID().ToString(); | |
| `#endif` | |
| } | |
| string matPath = $"{materialFolder}/{safeName}_{GetStableObjectKey(go)}_mat.mat"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageMaterial.cs` at line 421, The material path
uses GetInstanceIDCompat() (matPath, safeName, go) which truncates the object ID
and can collide; change the filename generation to use the full non-lossy
identifier for the GameObject (e.g., replace GetInstanceIDCompat() with your
project's non-lossy ID getter such as the EntityId/GUID provider or a
GetInstanceIDNonLossy() equivalent) so matPath uses that full identifier to
produce stable, non-colliding material filenames.
|
|
||
| var panelSettings = uiDoc.panelSettings; | ||
| int psId = panelSettings.GetInstanceID(); | ||
| int psId = panelSettings.GetInstanceIDCompat(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "GetInstanceIDCompat\\(|EntityId\\.ToULong|s_panelRTs|RT_MCP_UI_Render_" \
MCPForUnity/Editor/Tools/ManageUI.cs \
MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.csRepository: CoplayDev/unity-mcp
Length of output: 1398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context around the psId usage in ManageUI.cs
echo "=== Full context of psId usage (lines 1030-1100) ==="
sed -n '1030,1100p' MCPForUnity/Editor/Tools/ManageUI.cs | cat -n
# Check all references to s_panelRTs
echo -e "\n=== All s_panelRTs usages ==="
rg -n "s_panelRTs" MCPForUnity/Editor/Tools/ManageUI.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 4788
Avoid lossy compat ID as internal cache key.
Line 1036 uses GetInstanceIDCompat() for psId, but on Unity 6.5+ that value is a truncated ulong and can collide. Here it drives s_panelRTs identity (lines 1047, 1074, 1093) and RT asset naming (lines 1053, 1059), so collisions can mix unrelated panels' render textures and asset paths.
🔧 Proposed fix (use non-lossy key for internal identity)
- private static readonly Dictionary<int, RenderTexture> s_panelRTs = new();
+ private static readonly Dictionary<string, RenderTexture> s_panelRTs = new();
+
+ private static string GetPanelSettingsCacheKey(PanelSettings panelSettings)
+ {
+#if UNITY_6000_5_OR_NEWER
+ return EntityId.ToULong(panelSettings.GetEntityId()).ToString();
+#else
+ return panelSettings.GetInstanceID().ToString();
+#endif
+ }
...
- int psId = panelSettings.GetInstanceIDCompat();
+ string psId = GetPanelSettingsCacheKey(panelSettings);
...
rt.name = $"MCP_UI_Render_{psId}";
...
string rtAssetPath = $"{rtFolder}/RT_MCP_UI_Render_{psId}.renderTexture";📝 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
| int psId = panelSettings.GetInstanceIDCompat(); | |
| private static readonly Dictionary<string, RenderTexture> s_panelRTs = new(); | |
| private static string GetPanelSettingsCacheKey(PanelSettings panelSettings) | |
| { | |
| `#if` UNITY_6000_5_OR_NEWER | |
| return EntityId.ToULong(panelSettings.GetEntityId()).ToString(); | |
| `#else` | |
| return panelSettings.GetInstanceID().ToString(); | |
| `#endif` | |
| } | |
| // ... other code ... | |
| string psId = GetPanelSettingsCacheKey(panelSettings); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageUI.cs` at line 1036, The code uses
GetInstanceIDCompat() for psId which can be lossy on newer Unity versions and
cause collisions; replace that key with a non-lossy identifier (e.g., use
panelSettings.GetInstanceID() or another unique stable id derived from the
object) and propagate that change to all places that rely on psId (s_panelRTs
dictionary keys and any RT asset naming that currently uses psId in ManageUI.cs,
e.g., where s_panelRTs is accessed and RT names are constructed). Ensure the
chosen id is used consistently for lookup, creation, and naming so unrelated
panels cannot collide.
| // GetInstanceID() is obsolete-as-error on Unity 6.5+. Truncate the EntityId's | ||
| // underlying ulong to int; this is lossy but stable within a session and | ||
| // preserves the int-shaped wire format that older consumers expect. | ||
| return (int)EntityId.ToULong(obj.GetEntityId()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for checked overflow contexts or compiler flags that may affect explicit casts.
rg -n --iglob '*.cs' '\bchecked\s*\('
rg -n --iglob '*.rsp' --iglob '*.csproj' --iglob '*.sln' --iglob '*.json' 'checked\+|/checked|CheckForOverflowUnderflow|checkForOverflowUnderflow'Repository: CoplayDev/unity-mcp
Length of output: 45
Consider wrapping the lossy cast in unchecked() for explicit intent.
The cast from ulong to int at line 24 truncates silently. While no checked-overflow contexts were found in the repository, wrapping the cast explicitly with unchecked() documents the intended lossy behavior and protects against future changes to overflow-checking settings.
💡 Suggested improvement
- return (int)EntityId.ToULong(obj.GetEntityId());
+ return unchecked((int)EntityId.ToULong(obj.GetEntityId()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs` at line 24, The
cast from ulong to int in UnityObjectIdCompatExtensions (return
(int)EntityId.ToULong(obj.GetEntityId());) should be made explicit about lossy
behavior by wrapping it in unchecked(); update the return expression to use
unchecked((int)EntityId.ToULong(obj.GetEntityId())) so the intent is clear and
future overflow-checking settings won't change behavior.
Comment on lines
+379
to
+383
| if (jo.TryGetValue("entityID", out JToken entityIdToken) && entityIdToken.Type == JTokenType.String) | ||
| { | ||
| string serializedEntityId = entityIdToken.ToString(); | ||
| if (ulong.TryParse(serializedEntityId, out ulong rawEntityId)) | ||
| { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "UnityTypeConverters.cs" | head -5Repository: CoplayDev/unity-mcp
Length of output: 121
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '375,390p'Repository: CoplayDev/unity-mcp
Length of output: 1054
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '405,415p'Repository: CoplayDev/unity-mcp
Length of output: 619
🏁 Script executed:
# Let me check the context around the fallback behavior and see what happens after the entityID block
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '410,430p'Repository: CoplayDev/unity-mcp
Length of output: 1083
🏁 Script executed:
# Check if there are any tests for this serialization
find . -type f -name "*.cs" | xargs rg -l "UnityEngineObjectConverter|entityID" | grep -i testRepository: CoplayDev/unity-mcp
Length of output: 9448
🏁 Script executed:
# Let me search for test files more carefully
find . -type f -name "*Test*.cs" -o -name "*test*.cs" | head -20Repository: CoplayDev/unity-mcp
Length of output: 2074
🏁 Script executed:
# Search for serialization tests
find . -type f -name "*.cs" | xargs rg -l "Serialization|UnityEngineObjectConverter" | grep -i testRepository: CoplayDev/unity-mcp
Length of output: 13467
🏁 Script executed:
cat -n ./TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UIDocumentSerializationTests.cs | head -100Repository: CoplayDev/unity-mcp
Length of output: 4554
🏁 Script executed:
# Search for tests related to UnityEngineObjectConverter
rg "UnityEngineObjectConverter|entityID" --type cs ./TestProjects/UnityMCPTests/Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Let me check the broader context of the UnityEngineObjectConverter class
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '350,420p'Repository: CoplayDev/unity-mcp
Length of output: 4276
🏁 Script executed:
rg "entityID" --type cs -B 2 -A 2 | head -80Repository: CoplayDev/unity-mcp
Length of output: 3363
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '430,440p'Repository: CoplayDev/unity-mcp
Length of output: 800
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '260,290p'Repository: CoplayDev/unity-mcp
Length of output: 1568
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '275,330p'Repository: CoplayDev/unity-mcp
Length of output: 2753
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '450,480p'Repository: CoplayDev/unity-mcp
Length of output: 2176
🏁 Script executed:
cat -n ./MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs | sed -n '490,520p'Repository: CoplayDev/unity-mcp
Length of output: 1496
Accept numeric entityID tokens in addition to strings.
Current deserialization only enters the entity-resolution path when entityID is a JSON string. Numeric emitters will bypass this and fall back to the lossy instanceID path. Update the type check at line 379 to accept both JTokenType.String and JTokenType.Integer, since JToken.ToString() handles both. Add test cases for both payload shapes: "entityID":"123" and "entityID":123.
🛠️ Suggested fix
- if (jo.TryGetValue("entityID", out JToken entityIdToken) && entityIdToken.Type == JTokenType.String)
+ if (jo.TryGetValue("entityID", out JToken entityIdToken) &&
+ (entityIdToken.Type == JTokenType.String || entityIdToken.Type == JTokenType.Integer))
{
string serializedEntityId = entityIdToken.ToString();
if (ulong.TryParse(serializedEntityId, out ulong rawEntityId))
{
EntityId eid = EntityId.FromULong(rawEntityId);
UnityEngine.Object entityObj = UnityEditor.EditorUtility.EntityIdToObject(eid);
...
}
}🤖 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 379 -
383, In UnityTypeConverters.cs update the entityID token type check so numeric
tokens are accepted: in the block that checks jo.TryGetValue("entityID", out
JToken entityIdToken) and currently requires entityIdToken.Type ==
JTokenType.String, change it to allow JTokenType.Integer as well (e.g.
entityIdToken.Type == JTokenType.String || entityIdToken.Type ==
JTokenType.Integer), then continue using entityIdToken.ToString() and
ulong.TryParse into rawEntityId as before to resolve the entity instead of
falling back to instanceID; also add unit tests covering both payload shapes
("entityID":"123" and "entityID":123) to ensure both code paths use the
entity-resolution logic (look for variables/methods named entityIdToken,
serializedEntityId, rawEntityId, and the instanceID fallback to validate
behavior).
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.
Pickup on #1051 for cleanup
Summary by Sourcery
Add Unity 6.5+ compatible object identification and find-object helpers while preserving backward-compatible instanceID-based serialization.
Enhancements:
Summary by CodeRabbit