Fix Unity 6.5 GetInstanceID compile breaks#1051
Open
tomicz wants to merge 6 commits intoCoplayDev:betafrom
Open
Fix Unity 6.5 GetInstanceID compile breaks#1051tomicz wants to merge 6 commits intoCoplayDev:betafrom
tomicz wants to merge 6 commits intoCoplayDev:betafrom
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.
Contributor
📝 WalkthroughWalkthroughReplaced direct Changes
Sequence Diagram(s)sequenceDiagram
participant JSON as JSON input
participant Converter as UnityTypeConverters.ReadJson
participant Resolver as EditorUtility.EntityIdToObject (reflective)
participant Unity as UnityEngine (GameObject/Component)
JSON->>Converter: parse object token
alt contains "entityID"
Converter->>Resolver: TryResolveEntityIdToObject(entityID)
Resolver-->>Converter: UnityEngine.Object or null
alt resolved and assignable
Converter->>Unity: Type conversion (GameObject→Transform / GetComponent)
Unity-->>Converter: resolved object
Converter-->>JSON: return resolved object
else not assignable
Converter-->>JSON: fallback to other resolution or warning
end
else contains "instanceID"/"guid"/"path"
Converter->>Unity: Resolve via instanceID/guid/path
Unity-->>Converter: object or null
Converter-->>JSON: return resolved object or null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
Reviewer's GuideReplaces direct uses of UnityEngine.Object.GetInstanceID with a reflection-based compatibility helper and updates serialization to prefer entity IDs on Unity 6000.5+, ensuring the MCP Unity package compiles on Unity 6.5 while maintaining backward compatibility with existing instanceID-based payloads. Sequence diagram for UnityEngine.Object JSON serialization with ID compatibilitysequenceDiagram
participant Caller
participant Converter as UnityEngineObjectConverter
participant Obj as UnityObject
participant Compat as UnityObjectIdCompatExtensions
Caller->>Converter: WriteJson(writer, Obj, serializer)
Converter->>Converter: WriteSerializedObjectId(writer, Obj)
alt UNITY_6000_5_OR_NEWER
Converter->>Obj: GetEntityId()
Obj-->>Converter: entityId
Converter->>Converter: writer.WritePropertyName("entityID")
Converter->>Converter: writer.WriteValue(entityId.ToString())
else pre_6000_5
Converter->>Compat: GetInstanceIDCompat(Obj)
Compat->>Obj: reflection GetMethod(GetInstanceID)
alt GetInstanceID available
Obj-->>Compat: instanceID
Compat-->>Converter: instanceID
else GetInstanceID missing
Compat->>Obj: reflection GetMethod(GetEntityId)
alt GetEntityId available
Obj-->>Compat: entity
Compat-->>Converter: parsedOrHashedId
else no ID APIs
Compat-->>Converter: Obj.GetHashCode()
end
end
Converter->>Converter: writer.WritePropertyName("instanceID")
Converter->>Converter: writer.WriteValue(instanceID)
end
Class diagram for Unity object ID compatibility and serializationclassDiagram
class UnityEngineObjectConverter {
+WriteJson(writer UnityJsonWriter, value Object, serializer JsonSerializer) void
+ReadJson(reader UnityJsonReader, objectType Type, existingValue Object, serializer JsonSerializer) Object
-WriteSerializedObjectId(writer UnityJsonWriter, value Object) void
-TryResolveEntityIdToObject(serializedEntityId string, obj Object) bool
}
class UnityObjectIdCompatExtensions {
+GetInstanceIDCompat(obj Object) int
}
class Object
UnityEngineObjectConverter ..> UnityObjectIdCompatExtensions : uses
UnityObjectIdCompatExtensions ..> Object : extends
UnityEngineObjectConverter ..> Object : serializes
Flow diagram for UnityEngine.Object deserialization with entityID fallbackflowchart TD
Start([Start ReadJson]) --> Parse[Parse JSON into JObject jo]
Parse --> CheckEntity{Has entityID string?}
CheckEntity -- Yes --> TryEntity["TryResolveEntityIdToObject(serializedEntityId, out entityObj)"]
TryEntity --> EntitySuccess{entityObj resolved and type compatible?}
EntitySuccess -- Yes --> ReturnEntity[Return entityObj or component/transform]
EntitySuccess -- No --> WarnEntity[Log warning: could not resolve entityID]
WarnEntity --> ReturnNull1[Return null]
CheckEntity -- No --> CheckInstance{Has instanceID int?}
CheckInstance -- Yes --> ResolveInstance[Resolve via instanceID APIs]
ResolveInstance --> InstanceResult{Resolved object?}
InstanceResult -- Yes --> ReturnInstance[Return resolved object]
InstanceResult -- No --> ContinueInstance[Fall through]
CheckInstance -- No --> ContinueInstance
ContinueInstance --> CheckGuidPath{Has guid/path?}
CheckGuidPath -- Yes --> ResolveAsset[Resolve via AssetDatabase or path]
ResolveAsset --> AssetResult{Resolved object?}
AssetResult -- Yes --> ReturnAsset[Return resolved asset]
AssetResult -- No --> MissingField
CheckGuidPath -- No --> MissingField[Log warning: missing instanceID, entityID, guid, or path]
MissingField --> ReturnNull2[Return null]
ReturnEntity --> End([End])
ReturnNull1 --> End
ReturnInstance --> End
ReturnAsset --> End
ReturnNull2 --> End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new GetInstanceIDCompat extension uses reflection on every call to resolve GetInstanceID/GetEntityId; consider caching the MethodInfo (and any derived delegates) in static fields so these lookups and invocations don't add per-call overhead in hot paths like physics queries and serialization.
- TryResolveEntityIdToObject reflects over all EditorUtility methods and parses the argument on each call; you may want to narrow this down by caching the resolved EntityIdToObject MethodInfo and expected parameter type once, to avoid repeated method scanning and argument parsing during heavy deserialization.
- You’ve added
#pragma warning disable 0619to many files while also introducing GetInstanceIDCompat; it might be cleaner to limit the pragma to just the compatibility helper (or the minimal call sites that still need deprecated APIs) so that other unrelated obsolete usages continue to surface as warnings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new GetInstanceIDCompat extension uses reflection on every call to resolve GetInstanceID/GetEntityId; consider caching the MethodInfo (and any derived delegates) in static fields so these lookups and invocations don't add per-call overhead in hot paths like physics queries and serialization.
- TryResolveEntityIdToObject reflects over all EditorUtility methods and parses the argument on each call; you may want to narrow this down by caching the resolved EntityIdToObject MethodInfo and expected parameter type once, to avoid repeated method scanning and argument parsing during heavy deserialization.
- You’ve added `#pragma warning disable 0619` to many files while also introducing GetInstanceIDCompat; it might be cleaner to limit the pragma to just the compatibility helper (or the minimal call sites that still need deprecated APIs) so that other unrelated obsolete usages continue to surface as warnings.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs" line_range="378-387" />
<code_context>
+ // Try to resolve by entityID (Unity 6.5+)
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider falling back to instanceID/guid/path if entityID resolution fails instead of returning null early.
This code now returns null as soon as an unresolved entityID is encountered, whereas previously it would still try instanceID/guid/path. For data that might contain both entityID and older identifiers, this can break valid fallback resolution. Instead, resolve via entityID first, and if it fails, log a warning but still fall back to the existing instanceID/guid/path logic rather than returning null here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 5
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs (1)
13-21: Consider caching the MethodInfo lookups for performance.Reflection lookups via
GetMethod()are relatively expensive. Since this method may be called frequently during serialization (e.g., for every physics hit, every camera, every component), caching theMethodInfoinstances would improve performance.⚡ Proposed fix with cached reflection
public static class UnityObjectIdCompatExtensions { + private static readonly MethodInfo s_getInstanceIdMethod; + private static readonly MethodInfo s_getEntityIdMethod; + + static UnityObjectIdCompatExtensions() + { + s_getInstanceIdMethod = typeof(Object).GetMethod("GetInstanceID", BindingFlags.Public | BindingFlags.Instance); + s_getEntityIdMethod = typeof(Object).GetMethod("GetEntityId", BindingFlags.Public | BindingFlags.Instance); + } + public static int GetInstanceIDCompat(this Object obj) { if (obj == null) { return 0; } - MethodInfo getInstanceId = typeof(Object).GetMethod("GetInstanceID", BindingFlags.Public | BindingFlags.Instance); - if (getInstanceId != null) + if (s_getInstanceIdMethod != null) { - object result = getInstanceId.Invoke(obj, null); + object result = s_getInstanceIdMethod.Invoke(obj, null); if (result is int id) { return id; } } - MethodInfo getEntityId = typeof(Object).GetMethod("GetEntityId", BindingFlags.Public | BindingFlags.Instance); - if (getEntityId != null) + if (s_getEntityIdMethod != null) { - object entity = getEntityId.Invoke(obj, null); + object entity = s_getEntityIdMethod.Invoke(obj, null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs` around lines 13 - 21, The reflection call to typeof(Object).GetMethod("GetInstanceID", ...) inside UnityObjectIdCompatExtensions should be cached to avoid repeated expensive lookups; add a private static readonly MethodInfo field (e.g., _getInstanceIdMethod) and initialize it once (static constructor or Lazy<T>) then replace the inline GetMethod call in the method with that cached field before calling Invoke(obj, null); ensure you handle the null case the same way (check _getInstanceIdMethod != null and cast result to int) and apply the same caching pattern for any other repeated reflection lookups in this class.MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
1-1: Scope theCS0619suppression to the exact transition call site.A file-level disable will mask any unrelated obsolete API usage anywhere in this converter. Wrap only the
GetEntityId()serialization line and restore the warning immediately after.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs` at line 1, Remove the file-level "#pragma warning disable 0619" and instead scope the suppression narrowly around the exact call to GetEntityId() in the UnityTypeConverters class: locate the serialization code where GetEntityId() is invoked, add "#pragma warning disable 0619" immediately before that GetEntityId() call and "#pragma warning restore 0619" immediately after so only that transition call is suppressed and other obsolete-API uses remain detectable.
🤖 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/Helpers/GameObjectLookup.cs`:
- Line 159: Search and update the fallback path inside GetInstanceIDCompat() so
that whenever it returns a hash code (i.e., when GetEntityId() is
missing/unparseable) it emits a clear debug/warning log including the object
context (name/type), the original entity id string, and the produced hash code;
this will make round-trip failures visible when ResolveInstanceID()/FindById()
later call EditorUtility.EntityIdToObject() / InstanceIDToObject() with a
non-native id. Ensure the log is concise and only triggers on the fallback
branch to avoid noise, and update any related search call sites (those that
currently return matching.Select(go => go.GetInstanceIDCompat())) to rely on
this improved logging for diagnostics.
In `@MCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs`:
- Around line 4-5: The UnityObjectIdCompatExtensions class is declared in the
global namespace; wrap it in the appropriate namespace (e.g.,
MCPForUnity.Editor.Helpers) to match other helper types and avoid polluting the
global namespace—update the file so the class UnityObjectIdCompatExtensions is
declared inside the MCPForUnity.Editor.Helpers namespace (and adjust any
using/imports or internal references if needed).
- Around line 29-38: The current fallback in UnityObjectIdCompatExtensions that
parses entity.ToString() and then returns GetHashCode() produces unresolvable
IDs; replace that logic to use the proper EntityId persistence APIs instead:
when you detect an EntityId-like object, call EntityId.ToULong() to get a stable
persistent ID and store/return that instead of parsing the string, and on
restore use EntityId.FromULong() (and EditorUtility.EntityIdToObject where
appropriate in UnityTypeConverters.cs) to resolve objects; remove or stop
silently returning GetHashCode()—instead log a clear warning or throw an
exception if reflection extraction fails so downstream deserialization (e.g.,
UnityTypeConverters.cs) does not receive unusable IDs.
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Around line 500-508: WriteSerializedObjectId currently emits only "entityID"
on newer Unity versions, breaking older consumers expecting "instanceID"; modify
the method to emit both fields on UNITY_6000_5_OR_NEWER: keep writing "entityID"
with GetEntityId().ToString() and also write "instanceID" using
GetInstanceIDCompat() (so legacy readers still see the expected field), or
alternatively always write both fields regardless of UNITY version; update the
writer calls in WriteSerializedObjectId to output both property names/values.
- Around line 511-567: In TryResolveEntityIdToObject, guard the reflective calls
to parseMethod.Invoke(...) and method.Invoke(...) with try/catch blocks so
exceptions (at least TargetInvocationException and ArgumentException, plus any
unexpected Exception) are caught, logged as warnings with the serializedEntityId
and method/param info, and then continue to the next candidate instead of
allowing the exception to bubble; ensure the catch paths do not set obj and let
the method ultimately return false if no resolver succeeds.
---
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs`:
- Around line 13-21: The reflection call to
typeof(Object).GetMethod("GetInstanceID", ...) inside
UnityObjectIdCompatExtensions should be cached to avoid repeated expensive
lookups; add a private static readonly MethodInfo field (e.g.,
_getInstanceIdMethod) and initialize it once (static constructor or Lazy<T>)
then replace the inline GetMethod call in the method with that cached field
before calling Invoke(obj, null); ensure you handle the null case the same way
(check _getInstanceIdMethod != null and cast result to int) and apply the same
caching pattern for any other repeated reflection lookups in this class.
In `@MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs`:
- Line 1: Remove the file-level "#pragma warning disable 0619" and instead scope
the suppression narrowly around the exact call to GetEntityId() in the
UnityTypeConverters class: locate the serialization code where GetEntityId() is
invoked, add "#pragma warning disable 0619" immediately before that
GetEntityId() call and "#pragma warning restore 0619" immediately after so only
that transition call is suppressed and other obsolete-API uses remain
detectable.
🪄 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: c63284ea-3109-4641-ac9a-2ece7fe0d017
📒 Files selected for processing (32)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/GameObjectLookup.csMCPForUnity/Editor/Helpers/GameObjectSerializer.csMCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.csMCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs.metaMCPForUnity/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/GameObjects/GameObjectDelete.csMCPForUnity/Editor/Tools/GameObjects/GameObjectDuplicate.csMCPForUnity/Editor/Tools/GameObjects/GameObjectLookAt.csMCPForUnity/Editor/Tools/GameObjects/ManageGameObjectCommon.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/Serialization/UnityTypeConverters.cs
| matching = matching.Take(maxResults); | ||
|
|
||
| return matching.Select(go => go.GetInstanceID()); | ||
| return matching.Select(go => go.GetInstanceIDCompat()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the fallback paths in GetInstanceIDCompat to understand when hash codes are returned
cat MCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs
# Search for any existing logging or documentation about the fallback behavior
rg -n "GetHashCode|fallback" MCPForUnity/Editor/Helpers/Repository: CoplayDev/unity-mcp
Length of output: 3271
🏁 Script executed:
#!/bin/bash
# Check the search methods at the specified lines to confirm they use GetInstanceIDCompat()
sed -n '155,165p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
sed -n '170,180p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
sed -n '187,197p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
sed -n '197,207p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
sed -n '230,240p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
sed -n '254,264p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
sed -n '274,284p' MCPForUnity/Editor/Helpers/GameObjectLookup.cs
# Check the ResolveInstanceID implementation
rg -n -A 15 "ResolveInstanceID" MCPForUnity/Editor/Helpers/GameObjectLookup.cs | head -50Repository: CoplayDev/unity-mcp
Length of output: 4806
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or documentation about round-trip ID handling
fd -i test MCPForUnity/Editor/Helpers/ -type f | head -20
# Search for any comments in GameObjectLookup.cs about instance ID handling
rg -n "round|trip|hash|fallback" MCPForUnity/Editor/Helpers/GameObjectLookup.cs
# Check if there's any defensive code around ID resolution
sed -n '65,90p' MCPForUnity/Editor/Helpers/GameObjectLookup.csRepository: CoplayDev/unity-mcp
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Check what triggers the hash code fallback - when would GetEntityId return non-parseable value?
rg -n "GetEntityId\|entity" MCPForUnity/Editor/Helpers/GameObjectLookup.cs
# Check if there are any warnings or documentation about ID limitations
rg -n "pragma warning|deprecated|obsolete" MCPForUnity/Editor/Helpers/GameObjectLookup.cs
# Check if SearchGameObjects ById case handles resolution differently
sed -n '110,130p' MCPForUnity/Editor/Helpers/GameObjectLookup.csRepository: CoplayDev/unity-mcp
Length of output: 945
Round-trip ID mismatch risk: search methods emit IDs via GetInstanceIDCompat(), which can return hash codes instead of native instance IDs.
The search methods at these lines emit instance IDs via GetInstanceIDCompat(), which falls back to GetHashCode() when GetEntityId() returns a non-parseable value or is unavailable. However, ResolveInstanceID() and FindById() expect native Unity instance IDs for EditorUtility.EntityIdToObject() / InstanceIDToObject() to work. If a hash code is emitted and later passed to FindById(), the lookup will fail silently.
This may be acceptable if the hash code fallback is rare in practice (e.g., only on unusual ECS configurations), but add debug logging to the fallback paths in GetInstanceIDCompat() to surface when this occurs.
Also applies to: 174-174, 191-191, 201-201, 234-234, 258-258, 278-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/GameObjectLookup.cs` at line 159, Search and
update the fallback path inside GetInstanceIDCompat() so that whenever it
returns a hash code (i.e., when GetEntityId() is missing/unparseable) it emits a
clear debug/warning log including the object context (name/type), the original
entity id string, and the produced hash code; this will make round-trip failures
visible when ResolveInstanceID()/FindById() later call
EditorUtility.EntityIdToObject() / InstanceIDToObject() with a non-native id.
Ensure the log is concise and only triggers on the fallback branch to avoid
noise, and update any related search call sites (those that currently return
matching.Select(go => go.GetInstanceIDCompat())) to rely on this improved
logging for diagnostics.
Comment on lines
+4
to
+5
| public static class UnityObjectIdCompatExtensions | ||
| { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add namespace to avoid polluting the global namespace.
The class is declared without a namespace, which pollutes the global namespace and is inconsistent with other helpers in MCPForUnity.Editor.Helpers.
♻️ Proposed fix
using System.Reflection;
using UnityEngine;
+namespace MCPForUnity.Editor.Helpers
+{
public static class UnityObjectIdCompatExtensions
{
public static int GetInstanceIDCompat(this Object obj)
{
// ... existing code ...
}
}
+}📝 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
| public static class UnityObjectIdCompatExtensions | |
| { | |
| using System.Reflection; | |
| using UnityEngine; | |
| namespace MCPForUnity.Editor.Helpers | |
| { | |
| public static class UnityObjectIdCompatExtensions | |
| { | |
| public static int GetInstanceIDCompat(this Object obj) | |
| { | |
| // ... existing code ... | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/UnityObjectIdCompatExtensions.cs` around lines 4 -
5, The UnityObjectIdCompatExtensions class is declared in the global namespace;
wrap it in the appropriate namespace (e.g., MCPForUnity.Editor.Helpers) to match
other helper types and avoid polluting the global namespace—update the file so
the class UnityObjectIdCompatExtensions is declared inside the
MCPForUnity.Editor.Helpers namespace (and adjust any using/imports or internal
references if needed).
Place the compatibility extension in MCPForUnity.Runtime so Runtime/Serialization can resolve GetInstanceIDCompat, fixing Unity 2022 compile errors from assembly visibility.
Author
|
Added follow-up fix: moved from Editor to Runtime assembly so can resolve in Unity 2022 projects as well. |
Author
|
Follow-up fix added: moved UnityObjectIdCompatExtensions from the Editor assembly to the Runtime assembly, so Runtime Serialization UnityTypeConverters can resolve GetInstanceIDCompat in Unity 2022 projects as well. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs (1)
23-38:⚠️ Potential issue | 🟠 MajorAvoid lossy EntityId conversion and hash-based ID fallbacks.
Line 29 (
int.TryParse(entity.ToString())) plus Lines 34/38 hash fallbacks can produce IDs that are not reliable for identity round-trips, especially when compared with serialization/object-resolution flows (e.g.,MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs:500-509). This should use a dedicated EntityId-safe path and avoid silently returning hash codes.#!/bin/bash # Read-only verification: inspect current ID write/read paths and lossy fallbacks. fd "UnityObjectIdCompatExtensions.cs" -x cat -n {} fd "UnityTypeConverters.cs" -x sed -n '470,560p' {} rg -n -C3 'TryParse\(.*ToString\(|GetHashCode\(\)|EntityIdToObject|InstanceIDToObject|entityID|instanceID'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs` around lines 23 - 38, The current code converts entity via entity.ToString() + int.TryParse and falls back to GetHashCode(), which can produce lossy/unstable IDs; instead, use the project's EntityId-safe conversion utilities and Unity instance IDs: when getEntityId.Invoke(obj) returns a value, check if it's a numeric/integral type (use type checks for int/long/short/uint/ulong/etc.) and convert to int with proper range checking (avoid parsing ToString); if it's not a numeric type, call the project's canonical mapping/resolution path in UnityTypeConverters (the EntityId/InstanceID conversion helpers such as EntityIdToObject/InstanceIDToObject or equivalent) to obtain a stable ID; never return entity.GetHashCode() — if no stable conversion is possible, use obj.GetInstanceID() as the deterministic fallback and/or surface an explicit error/log instead of silently using hash codes. Ensure changes target the method using getEntityId.Invoke in UnityObjectIdCompatExtensions and reference UnityTypeConverters helpers for resolution.
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs (1)
13-24: Cache reflectionMethodInfolookups for hot-path usage.Lines 13 and 23 call
GetMethod(...)on every invocation. Cache these as static readonly fields to reduce repeated reflection overhead.♻️ Proposed refactor
public static class UnityObjectIdCompatExtensions { + private static readonly MethodInfo GetInstanceIdMethod = + typeof(Object).GetMethod("GetInstanceID", BindingFlags.Public | BindingFlags.Instance); + private static readonly MethodInfo GetEntityIdMethod = + typeof(Object).GetMethod("GetEntityId", BindingFlags.Public | BindingFlags.Instance); + public static int GetInstanceIDCompat(this Object obj) { if (obj == null) { return 0; } - MethodInfo getInstanceId = typeof(Object).GetMethod("GetInstanceID", BindingFlags.Public | BindingFlags.Instance); - if (getInstanceId != null) + if (GetInstanceIdMethod != null) { - object result = getInstanceId.Invoke(obj, null); + object result = GetInstanceIdMethod.Invoke(obj, null); if (result is int id) { return id; } } - MethodInfo getEntityId = typeof(Object).GetMethod("GetEntityId", BindingFlags.Public | BindingFlags.Instance); - if (getEntityId != null) + if (GetEntityIdMethod != null) { - object entity = getEntityId.Invoke(obj, null); + object entity = GetEntityIdMethod.Invoke(obj, null); if (entity != null) { if (int.TryParse(entity.ToString(), out int parsed)) { return parsed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs` around lines 13 - 24, Cache reflection lookups by adding static readonly MethodInfo fields for the Object.GetInstanceID and Object.GetEntityId results (e.g., private static readonly MethodInfo s_getInstanceId and s_getEntityId) in the UnityObjectIdCompatExtensions type, initialize them once (using typeof(Object).GetMethod(...)) and then replace the per-call GetMethod(...) usages in the method that currently invokes getInstanceId.Invoke and getEntityId.Invoke to use these cached fields; ensure null checks still guard before invoking and that the existing logic (casting result to int) is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs`:
- Around line 23-38: The current code converts entity via entity.ToString() +
int.TryParse and falls back to GetHashCode(), which can produce lossy/unstable
IDs; instead, use the project's EntityId-safe conversion utilities and Unity
instance IDs: when getEntityId.Invoke(obj) returns a value, check if it's a
numeric/integral type (use type checks for int/long/short/uint/ulong/etc.) and
convert to int with proper range checking (avoid parsing ToString); if it's not
a numeric type, call the project's canonical mapping/resolution path in
UnityTypeConverters (the EntityId/InstanceID conversion helpers such as
EntityIdToObject/InstanceIDToObject or equivalent) to obtain a stable ID; never
return entity.GetHashCode() — if no stable conversion is possible, use
obj.GetInstanceID() as the deterministic fallback and/or surface an explicit
error/log instead of silently using hash codes. Ensure changes target the method
using getEntityId.Invoke in UnityObjectIdCompatExtensions and reference
UnityTypeConverters helpers for resolution.
---
Nitpick comments:
In `@MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs`:
- Around line 13-24: Cache reflection lookups by adding static readonly
MethodInfo fields for the Object.GetInstanceID and Object.GetEntityId results
(e.g., private static readonly MethodInfo s_getInstanceId and s_getEntityId) in
the UnityObjectIdCompatExtensions type, initialize them once (using
typeof(Object).GetMethod(...)) and then replace the per-call GetMethod(...)
usages in the method that currently invokes getInstanceId.Invoke and
getEntityId.Invoke to use these cached fields; ensure null checks still guard
before invoking and that the existing logic (casting result to int) is
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5e05d3f-9bb6-4e5b-a69b-111a828a440c
📒 Files selected for processing (1)
MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs
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.
Author
|
Implemented the review follow-ups and pushed updates:\n\n- Preserved wire compatibility by always emitting and additionally emitting on newer Unity.\n- Updated deserialization so unresolved no longer returns early; it now falls back to existing // logic.\n- Added exception guards around reflective parse/invoke paths in entity resolution.\n- Hardened to avoid hash-based fallback IDs and cache reflective method lookups.\n- Moved compatibility helper to Runtime assembly and included missing .\n\nValidation:\n- Unity 6.5 project compiles and opens.\n- Unity 2022.3 project compiles and opens. |
Author
|
Implemented the review follow-ups and pushed updates:\n\n- Preserved wire compatibility by always emitting instanceID and additionally emitting entityID on newer Unity.\n- Updated deserialization so unresolved entityID no longer returns early; it now falls back to existing instanceID/guid/path logic.\n- Added exception guards around reflective parse/invoke paths in entity resolution.\n- Hardened GetInstanceIDCompat to avoid hash-based fallback IDs and cache reflective method lookups.\n- Moved compatibility helper to Runtime assembly and included missing meta file.\n\nValidation:\n- Unity 6.5 project compiles and opens.\n- Unity 2022.3 project compiles and opens. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Serialization/UnityTypeConverters.cs (1)
508-580: Exception handling correctly implemented.The try-catch blocks around the reflective
ParseandInvokecalls properly address the previous review feedback, ensuring exceptions don't break deserialization and the method gracefully continues to the next candidate.One minor consideration: the method iterates through all
EditorUtilitymethods on each call. If deserialization becomes a hot path, caching the resolvedMethodInfocould improve performance. However, this is a deserialization routine that runs infrequently, so the current approach is acceptable.,
♻️ Optional: Cache method lookup for better performance
+ private static MethodInfo _cachedEntityIdToObjectMethod; + private static Type _cachedEntityIdParamType; + private static bool _entityIdMethodCacheInitialized; + private static bool TryResolveEntityIdToObject(string serializedEntityId, out UnityEngine.Object obj) { obj = null; `#if` UNITY_EDITOR && UNITY_6000_5_OR_NEWER if (string.IsNullOrWhiteSpace(serializedEntityId)) { return false; } - MethodInfo[] methods = typeof(UnityEditor.EditorUtility).GetMethods(BindingFlags.Public | BindingFlags.Static); - foreach (MethodInfo method in methods) + if (!_entityIdMethodCacheInitialized) { - if (method.Name != "EntityIdToObject") + _entityIdMethodCacheInitialized = true; + foreach (MethodInfo m in typeof(UnityEditor.EditorUtility).GetMethods(BindingFlags.Public | BindingFlags.Static)) { - continue; + if (m.Name == "EntityIdToObject" && m.GetParameters().Length == 1) + { + _cachedEntityIdToObjectMethod = m; + _cachedEntityIdParamType = m.GetParameters()[0].ParameterType; + break; + } } + } - ParameterInfo[] parameters = method.GetParameters(); - if (parameters.Length != 1) - { - continue; - } + if (_cachedEntityIdToObjectMethod == null) + { + return false; + } - Type paramType = parameters[0].ParameterType; - object arg = null; + object arg = null; + Type paramType = _cachedEntityIdParamType; - if (paramType == typeof(int)) + if (paramType == typeof(int)) + { + if (!int.TryParse(serializedEntityId, out int parsedInt)) { - if (!int.TryParse(serializedEntityId, out int parsedInt)) - { - continue; - } - arg = parsedInt; + return false; } - else + arg = parsedInt; + } + else + { + MethodInfo parseMethod = paramType.GetMethod("Parse", BindingFlags.Public | BindingFlags.Static, null, new[] { typeof(string) }, null); + if (parseMethod != null) { - MethodInfo parseMethod = paramType.GetMethod("Parse", BindingFlags.Public | BindingFlags.Static, null, new[] { typeof(string) }, null); - if (parseMethod != null) + try { - try - { - arg = parseMethod.Invoke(null, new object[] { serializedEntityId }); - } - catch - { - continue; - } + arg = parseMethod.Invoke(null, new object[] { serializedEntityId }); + } + catch + { + return false; } } + } - if (arg == null) - { - continue; - } + if (arg == null) + { + return false; + } - object result = null; - try - { - result = method.Invoke(null, new[] { arg }); - } - catch - { - continue; - } - if (result is UnityEngine.Object resolved) - { - obj = resolved; - return true; - } + object result = null; + try + { + result = _cachedEntityIdToObjectMethod.Invoke(null, new[] { arg }); } + catch + { + return false; + } + if (result is UnityEngine.Object resolved) + { + obj = resolved; + return true; + } `#endif` return false; }🤖 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 508 - 580, The current TryResolveEntityIdToObject reflects over UnityEditor.EditorUtility methods on every call; to optimize, cache the resolved MethodInfo (the EditorUtility method named "EntityIdToObject" with a single parameter) in a static field (e.g., a private static MethodInfo cachedEntityIdToObject) and reuse it: on first invocation locate and store the MethodInfo (and possibly the parameter Type), then use the cached MethodInfo for parsing/invocation in subsequent calls, falling back to the existing reflection search only if the cache is null.
🤖 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 508-580: The current TryResolveEntityIdToObject reflects over
UnityEditor.EditorUtility methods on every call; to optimize, cache the resolved
MethodInfo (the EditorUtility method named "EntityIdToObject" with a single
parameter) in a static field (e.g., a private static MethodInfo
cachedEntityIdToObject) and reuse it: on first invocation locate and store the
MethodInfo (and possibly the parameter Type), then use the cached MethodInfo for
parsing/invocation in subsequent calls, falling back to the existing reflection
search only if the cache is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7ae51c2-bb51-4d46-95d3-7e005541f0a0
📒 Files selected for processing (3)
MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.csMCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.metaMCPForUnity/Runtime/Serialization/UnityTypeConverters.cs
✅ Files skipped from review due to trivial changes (1)
- MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs.meta
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Runtime/Helpers/UnityObjectIdCompatExtensions.cs
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
1 participant
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.
Summary
Object.GetInstanceID()deprecation (CS0619treated as error).GetInstanceID()call sites with a compatibility helper (GetInstanceIDCompat) to avoid hard compile breaks across the package.UnityTypeConvertersto support EntityId-era behavior while preserving compatibility with existing payload expectations.Context
Unity's API transition around object identifiers moved from warning-level obsolescence in 6.4 to hard errors in 6.5 for many int/InstanceID call paths. This PR addresses those failures in MCP for Unity package code so projects can compile and open on Unity 6.5.
Test plan
GetInstanceID().GetInstanceID()callsites.Notes
beta).Made with Cursor
Summary by Sourcery
Update Unity object identification and serialization to remain compatible with Unity 6.5 while preserving existing instance-based workflows.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Improvements