feat(mcpforunity): support atlas sprite resolution by guid+spriteName/fileID#873
Conversation
Reviewer's GuideAdds atlas sprite resolution support for Unity object references by detecting JObject values and deferring to SerializedProperty for UnityEngine.Object targets, and extends SetObjectReference to resolve sprites within atlases using guid+spriteName or guid+fileID, including a helper for fileID lookup. Sequence diagram for atlas sprite resolution via guid+spriteName or guid+fileIDsequenceDiagram
participant Caller
participant ComponentOps
participant SerializedProperty
participant AssetDatabase
participant Sprite
participant GlobalObjectId
participant ParamCoercion
Caller->>ComponentOps: SetObjectReference(prop, jObj, out_error)
ComponentOps->>jObj: read guid
alt guid is valid
ComponentOps->>AssetDatabase: LoadAssetAtPath~Object~(path from guid)
AssetDatabase-->>ComponentOps: mainAssetOrNull
ComponentOps->>jObj: read spriteName
alt spriteName present
ComponentOps->>AssetDatabase: LoadAllAssetsAtPath(path)
AssetDatabase-->>ComponentOps: allAssets
loop each asset
ComponentOps->>Sprite: check is Sprite and name == spriteName
end
alt matching sprite found
ComponentOps->>SerializedProperty: set objectReferenceValue = sprite
ComponentOps-->>Caller: true
else no matching sprite
ComponentOps-->>Caller: false with error "Sprite not found in atlas"
end
else spriteName missing
ComponentOps->>jObj: read fileID
alt fileID present
ComponentOps->>ParamCoercion: CoerceLong(fileID, 0)
ParamCoercion-->>ComponentOps: targetFileId
ComponentOps->>AssetDatabase: LoadAllAssetsAtPath(path)
AssetDatabase-->>ComponentOps: allAssets
loop each asset
ComponentOps->>Sprite: check is Sprite
alt is Sprite
ComponentOps->>GlobalObjectId: GetGlobalObjectIdSlow(sprite)
GlobalObjectId-->>ComponentOps: globalId
ComponentOps->>ComponentOps: compare globalId.targetObjectId with targetFileId
end
end
alt matching fileID sprite found
ComponentOps->>SerializedProperty: set objectReferenceValue = sprite
ComponentOps-->>Caller: true
else no matching fileID
ComponentOps->>SerializedProperty: set objectReferenceValue = LoadAssetAtPath(path)
ComponentOps-->>Caller: true
end
else no fileID
ComponentOps->>SerializedProperty: set objectReferenceValue = LoadAssetAtPath(path)
ComponentOps-->>Caller: true
end
end
else guid invalid
ComponentOps-->>Caller: false with error
end
Class diagram for updated ComponentOps object reference handlingclassDiagram
class ComponentOps {
+bool TrySetViaReflection(component type propertyName normalizedName value out_error)
+bool SetObjectReference(prop value out_error)
-long GetSpriteFileId(sprite)
}
class SerializedProperty {
+object objectReferenceValue
}
class AssetDatabase {
+Object[] LoadAllAssetsAtPath(path)
+Object LoadAssetAtPath~Object~(path)
}
class Sprite {
+string name
}
class GlobalObjectId {
+static GlobalObjectId GetGlobalObjectIdSlow(obj)
+long targetObjectId
}
class ParamCoercion {
+static long CoerceLong(token defaultValue)
}
class PropertyConversion {
+static object ConvertToType(value targetType)
}
ComponentOps --> SerializedProperty : uses
ComponentOps --> AssetDatabase : uses
ComponentOps --> Sprite : resolves
ComponentOps --> GlobalObjectId : uses
ComponentOps --> ParamCoercion : uses
ComponentOps --> PropertyConversion : uses
note for ComponentOps "TrySetViaReflection now skips UnityEngine.Object targets when value is a JObject, deferring to SerializedProperty-based resolution. SetObjectReference can now resolve atlas sprites via guid+spriteName or guid+fileID using GetSpriteFileId."
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}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the
SetObjectReferencepath forguid + spriteName/fileID, you callAssetDatabase.LoadAllAssetsAtPath(path)separately for thespriteNameandfileIDbranches; consider loading once and reusing the result to avoid redundant asset database calls. - When a non-zero
fileIDis provided but no matching sprite is found, the code silently falls back toLoadAssetAtPathinstead of surfacing an error like thespriteNamebranch; consider returning a specific error in this case to make reference resolution failures easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `SetObjectReference` path for `guid + spriteName/fileID`, you call `AssetDatabase.LoadAllAssetsAtPath(path)` separately for the `spriteName` and `fileID` branches; consider loading once and reusing the result to avoid redundant asset database calls.
- When a non-zero `fileID` is provided but no matching sprite is found, the code silently falls back to `LoadAssetAtPath` instead of surfacing an error like the `spriteName` branch; consider returning a specific error in this case to make reference resolution failures easier to diagnose.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Helpers/ComponentOps.cs" line_range="625-634" />
<code_context>
return false;
}
+
+ var spriteNameToken = jObj["spriteName"];
+ if (spriteNameToken != null)
+ {
+ string spriteName = spriteNameToken.ToString();
+ var allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
+ foreach (var asset in allAssets)
+ {
+ if (asset is Sprite sprite && sprite.name == spriteName)
+ {
+ prop.objectReferenceValue = sprite;
+ return true;
+ }
+ }
+
+ error = $"Sprite '{spriteName}' not found in atlas '{path}'.";
+ return false;
+ }
</code_context>
<issue_to_address>
**issue:** Sprite lookup via `spriteName` prevents `fileID`-based fallback when both are present.
When both `spriteName` and `fileID` exist, the `spriteName` path fails fast and prevents the `fileID` lookup from running. If `fileID` is intended to be more stable or authoritative, consider either preferring `fileID` when both are present, or only returning the `spriteName`-not-found error when no `fileID` is available so the `fileID` branch can still execute.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Editor/Helpers/ComponentOps.cs" line_range="629" />
<code_context>
+ var allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid repeated `AssetDatabase.LoadAllAssetsAtPath` calls for the same path.
Suggested implementation:
```csharp
string spriteName = spriteNameToken.ToString();
if (!_allAssetsByPath.TryGetValue(path, out var allAssets))
{
allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
_allAssetsByPath[path] = allAssets;
}
foreach (var asset in allAssets)
```
To fully support this change, you also need to:
1. Add (or reuse, if already present) a cache field on the containing class, for example:
`private static readonly System.Collections.Generic.Dictionary<string, UnityEngine.Object[]> _allAssetsByPath = new System.Collections.Generic.Dictionary<string, UnityEngine.Object[]>();`
2. If `System.Collections.Generic` is not already imported at the top of the file, add:
`using System.Collections.Generic;`
and then you can simplify the field to:
`private static readonly Dictionary<string, UnityEngine.Object[]> _allAssetsByPath = new Dictionary<string, UnityEngine.Object[]>();`
</issue_to_address>
### Comment 3
<location path="MCPForUnity/Editor/Helpers/ComponentOps.cs" line_range="835" />
<code_context>
}
+
+
+ private static long GetSpriteFileId(Sprite sprite)
+ {
+ if (sprite == null)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing all exceptions in `GetSpriteFileId` may make failures hard to diagnose.
The `catch` in `GetSpriteFileId` returns `0` for any exception without logging, and `0` is also the sentinel for “no fileID,” which can mask real issues (e.g., misuse of the API or bad editor state). Please either log the exception (e.g., `Debug.LogWarning`/`LogError`) or narrow the `catch` to specific, expected exception types so unexpected failures remain visible.
Suggested implementation:
```csharp
private static long GetSpriteFileId(Sprite sprite)
{
if (sprite == null)
return 0;
try
{
var globalId = GlobalObjectId.GetGlobalObjectIdSlow(sprite);
return (long)globalId.targetObjectId;
}
catch (Exception ex)
{
Debug.LogWarning($"GetSpriteFileId: failed to get GlobalObjectId for sprite '{(sprite != null ? sprite.name : \"<null>\")}'. Returning 0. Exception: {ex}");
return 0;
```
If `System` or `UnityEngine` are not already imported at the top of this file, you will also need:
1. `using System;`
2. `using UnityEngine;`
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var spriteNameToken = jObj["spriteName"]; | ||
| if (spriteNameToken != null) | ||
| { | ||
| string spriteName = spriteNameToken.ToString(); | ||
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | ||
| foreach (var asset in allAssets) | ||
| { | ||
| if (asset is Sprite sprite && sprite.name == spriteName) | ||
| { | ||
| prop.objectReferenceValue = sprite; |
There was a problem hiding this comment.
issue: Sprite lookup via spriteName prevents fileID-based fallback when both are present.
When both spriteName and fileID exist, the spriteName path fails fast and prevents the fileID lookup from running. If fileID is intended to be more stable or authoritative, consider either preferring fileID when both are present, or only returning the spriteName-not-found error when no fileID is available so the fileID branch can still execute.
| if (spriteNameToken != null) | ||
| { | ||
| string spriteName = spriteNameToken.ToString(); | ||
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); |
There was a problem hiding this comment.
suggestion (performance): Avoid repeated AssetDatabase.LoadAllAssetsAtPath calls for the same path.
Suggested implementation:
string spriteName = spriteNameToken.ToString();
if (!_allAssetsByPath.TryGetValue(path, out var allAssets))
{
allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
_allAssetsByPath[path] = allAssets;
}
foreach (var asset in allAssets)To fully support this change, you also need to:
-
Add (or reuse, if already present) a cache field on the containing class, for example:
private static readonly System.Collections.Generic.Dictionary<string, UnityEngine.Object[]> _allAssetsByPath = new System.Collections.Generic.Dictionary<string, UnityEngine.Object[]>(); -
If
System.Collections.Genericis not already imported at the top of the file, add:
using System.Collections.Generic;
and then you can simplify the field to:
private static readonly Dictionary<string, UnityEngine.Object[]> _allAssetsByPath = new Dictionary<string, UnityEngine.Object[]>();
| } | ||
|
|
||
|
|
||
| private static long GetSpriteFileId(Sprite sprite) |
There was a problem hiding this comment.
suggestion (bug_risk): Swallowing all exceptions in GetSpriteFileId may make failures hard to diagnose.
The catch in GetSpriteFileId returns 0 for any exception without logging, and 0 is also the sentinel for “no fileID,” which can mask real issues (e.g., misuse of the API or bad editor state). Please either log the exception (e.g., Debug.LogWarning/LogError) or narrow the catch to specific, expected exception types so unexpected failures remain visible.
Suggested implementation:
private static long GetSpriteFileId(Sprite sprite)
{
if (sprite == null)
return 0;
try
{
var globalId = GlobalObjectId.GetGlobalObjectIdSlow(sprite);
return (long)globalId.targetObjectId;
}
catch (Exception ex)
{
Debug.LogWarning($"GetSpriteFileId: failed to get GlobalObjectId for sprite '{(sprite != null ? sprite.name : \"<null>\")}'. Returning 0. Exception: {ex}");
return 0;If System or UnityEngine are not already imported at the top of this file, you will also need:
using System;using UnityEngine;
… resolution - Replace non-existent ParamCoercion.CoerceLong with fileIdToken.Value<long>() - Add error return when fileID matches no sprite (consistent with spriteName branch) - Remove extra blank line before GetSpriteFileId Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded JObject early-exit in reflection paths to defer UnityEngine.Object assignments to SerializedProperty; extended SerializedProperty GUID-based resolution to locate atlas Sprites by spriteName or internal fileID using a new GetSpriteFileId helper and improved error messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ComponentOps.cs (2)
624-641: Consider a more accurate error message.The error at line 639 assumes the asset is an "atlas," but the GUID could point to any asset type (e.g., a single texture with sub-sprites, a texture that's actually not sliced, etc.). A more generic message would be clearer:
- error = $"Sprite '{spriteName}' not found in atlas '{path}'."; + error = $"Sprite '{spriteName}' not found in asset '{path}'.";Also, the sprite name comparison is case-sensitive (
sprite.name == spriteName). This matches Unity's behavior, so it's correct—just noting for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 624 - 641, The error message produced when a matching Sprite is not found in the loaded assets assumes the asset is an "atlas"; update the message in the block handling spriteNameToken (symbols: spriteNameToken, spriteName, allAssets, prop.objectReferenceValue, path, jObj) to use a generic wording like "Sprite '<name>' not found in asset '<path>'." Keep the sprite name comparison as-is (case-sensitive) and ensure the new message includes spriteName and path so callers know which lookup failed.
643-666: Clarify behavior whenfileIDis 0.When
targetFileId == 0(line 647), the code skips the sprite search and falls through to line 668, loading the main asset. This might be intentional (fileID=0 often means "main asset" in Unity), but it creates an asymmetry:
fileID: 0→ falls through silently, loads main assetfileID: 12345(not found) → returns errorIf
fileID: 0should explicitly mean "load main asset," consider adding a comment. If it's unintentional, you may want to handle it explicitly.Also, same suggestion as above for the error message:
- error = $"Sprite with fileID '{targetFileId}' not found in atlas '{path}'."; + error = $"Sprite with fileID '{targetFileId}' not found in asset '{path}'.";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 643 - 666, The code silently falls through when targetFileId == 0; make this explicit by handling the zero case for fileID (either add a clear comment that fileID==0 means "main asset" or explicitly load the main asset and assign it to prop.objectReferenceValue when targetFileId == 0), and ensure the logic around GetSpriteFileId and the sprite search only runs for non-zero targetFileId; also improve the failure path (the error returned when a non-zero targetFileId isn’t found) to include the targetFileId and atlas path contextual details so the error is unambiguous (refer to fileIdToken, targetFileId, GetSpriteFileId, prop.objectReferenceValue).
🤖 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/ComponentOps.cs`:
- Around line 837-851: In GetSpriteFileId(Sprite sprite) change the
ulong-to-long conversion to an explicit unchecked cast — use
unchecked((long)globalId.targetObjectId) when returning — and replace the silent
catch with a minimal debug log (e.g., using
UnityEngine.Debug.LogWarning/LogException or existing logger) that records the
exception and context (sprite name/instance) before returning 0 so failures
aren’t swallowed; update references in GetSpriteFileId and
GlobalObjectId.targetObjectId accordingly.
---
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 624-641: The error message produced when a matching Sprite is not
found in the loaded assets assumes the asset is an "atlas"; update the message
in the block handling spriteNameToken (symbols: spriteNameToken, spriteName,
allAssets, prop.objectReferenceValue, path, jObj) to use a generic wording like
"Sprite '<name>' not found in asset '<path>'." Keep the sprite name comparison
as-is (case-sensitive) and ensure the new message includes spriteName and path
so callers know which lookup failed.
- Around line 643-666: The code silently falls through when targetFileId == 0;
make this explicit by handling the zero case for fileID (either add a clear
comment that fileID==0 means "main asset" or explicitly load the main asset and
assign it to prop.objectReferenceValue when targetFileId == 0), and ensure the
logic around GetSpriteFileId and the sprite search only runs for non-zero
targetFileId; also improve the failure path (the error returned when a non-zero
targetFileId isn’t found) to include the targetFileId and atlas path contextual
details so the error is unambiguous (refer to fileIdToken, targetFileId,
GetSpriteFileId, prop.objectReferenceValue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2eeda4b-3925-4b21-9c8b-4ca56f084d59
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ComponentOps.cs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ComponentOps.cs`:
- Around line 205-213: The current early return in ComponentOps (when propInfo
!= null, propInfo.CanWrite, isJObjectValue and the property type is a
UnityEngine.Object) skips invoking the property's setter and prevents
SetViaSerializedProperty from running; instead, check whether a
SerializedProperty exists before bailing and, if not found, use reflection to
set the property via the PropertyInfo setter. Concretely: in the branch that
checks propInfo, isJObjectValue and
typeof(UnityEngine.Object).IsAssignableFrom(propInfo.PropertyType), call
SerializedObject.FindProperty(...) /
SerializedProperty.FindPropertyRelative(...) (the same lookup used by
SetViaSerializedProperty) and only return false if that lookup succeeds;
otherwise keep reflection enabled and invoke propInfo.SetValue(...) (or call the
existing codepath that uses PropertyInfo) to set the value so writable
UnityEngine.Object properties like sprite/material still get their setters
invoked.
- Around line 625-665: The method assigns a Sprite to prop.objectReferenceValue
and returns true immediately, which can hide failed assignments; update both
branches (the spriteName branch and the fileID branch in
ComponentOps.ResolveAtlasSprite) to verify the assignment stuck by checking
prop.objectReferenceValue != null (or equals the assigned sprite) after setting
it, and only return true if the reference is non-null; otherwise set the error
message (as done currently) and return false—follow the same verification
pattern used in ResolveSceneObjectByName and continue using GetSpriteFileId for
the fileID branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 622da504-f584-49dd-a1fc-86111010a5b0
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ComponentOps.cs
| PropertyInfo propInfo = type.GetProperty(propertyName, flags) | ||
| ?? type.GetProperty(normalizedName, flags); | ||
| if (propInfo != null && propInfo.CanWrite) | ||
| { | ||
| if (isJObjectValue && typeof(UnityEngine.Object).IsAssignableFrom(propInfo.PropertyType)) | ||
| { | ||
| // Let SerializedProperty path handle complex object references. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does UnityEditor.SerializedObject.FindProperty resolve public C# properties like SpriteRenderer.sprite, or only serialized field/property paths?
💡 Result:
SerializedObject.FindProperty(string propertyPath) looks up Unity-serialized data by serialized “propertyPath”, not arbitrary runtime C# properties.
- It does not resolve C# properties like
SpriteRenderer.spriteby name; if you callFindProperty("sprite")you should expectnullunless there’s actually a serialized field with that exact path/name. Unity’s serialization/Inspector pipeline works on serialized fields and does not invoke C# property getters/setters. [1], [2] - If the data is serialized via a field (public field or
[SerializeField]),FindPropertycan find it by its serialized field path. For auto-properties serialized via[field: SerializeField], the serialized name is the compiler-generated backing field (e.g."<MyProp>k__BackingField"), notMyProp. [3], [4] - For built-in components, you typically must use Unity’s internal serialized field names (often
m_...) rather than the public API property name; e.g. many UI components expose sprites via a serializedm_Spritefield (soFindProperty("m_Sprite")works there, not"sprite"). [5]
So: FindProperty is for serialized field/property paths, not for resolving public C# properties like SpriteRenderer.sprite by their API name.
Sources: [1] Unity Manual: Script Serialization, [2] Unity Scripting API: SerializedObject (notes about setters/validation), [3] Unity serialization note about backing fields for auto-properties, [4] Stack Overflow discussion of FindProperty returning null for C# properties, [5] example using FindProperty("m_Sprite") in editor code.
Don't bypass writable UnityEngine.Object properties unconditionally.
SerializedProperty.FindProperty() resolves only serialized field/property paths, not arbitrary C# properties. When this early return is hit, a writable property like sprite or material never reaches its setter, and the fallback to SetViaSerializedProperty(...) fails with "SerializedProperty ... not found" because public C# properties have no serialized path. Gate this fallback on FindProperty(...) actually succeeding, or keep reflection enabled for the PropertyInfo case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 205 - 213, The
current early return in ComponentOps (when propInfo != null, propInfo.CanWrite,
isJObjectValue and the property type is a UnityEngine.Object) skips invoking the
property's setter and prevents SetViaSerializedProperty from running; instead,
check whether a SerializedProperty exists before bailing and, if not found, use
reflection to set the property via the PropertyInfo setter. Concretely: in the
branch that checks propInfo, isJObjectValue and
typeof(UnityEngine.Object).IsAssignableFrom(propInfo.PropertyType), call
SerializedObject.FindProperty(...) /
SerializedProperty.FindPropertyRelative(...) (the same lookup used by
SetViaSerializedProperty) and only return false if that lookup succeeds;
otherwise keep reflection enabled and invoke propInfo.SetValue(...) (or call the
existing codepath that uses PropertyInfo) to set the value so writable
UnityEngine.Object properties like sprite/material still get their setters
invoked.
| var spriteNameToken = jObj["spriteName"]; | ||
| if (spriteNameToken != null) | ||
| { | ||
| string spriteName = spriteNameToken.ToString(); | ||
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | ||
| foreach (var asset in allAssets) | ||
| { | ||
| if (asset is Sprite sprite && sprite.name == spriteName) | ||
| { | ||
| prop.objectReferenceValue = sprite; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| error = $"Sprite '{spriteName}' not found in atlas '{path}'."; | ||
| return false; | ||
| } | ||
|
|
||
| var fileIdToken = jObj["fileID"]; | ||
| if (fileIdToken != null) | ||
| { | ||
| long targetFileId = fileIdToken.Value<long>(); | ||
| if (targetFileId != 0) | ||
| { | ||
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | ||
| foreach (var asset in allAssets) | ||
| { | ||
| if (asset is Sprite sprite) | ||
| { | ||
| long spriteFileId = GetSpriteFileId(sprite); | ||
| if (spriteFileId == targetFileId) | ||
| { | ||
| prop.objectReferenceValue = sprite; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| error = $"Sprite with fileID '{targetFileId}' not found in atlas '{path}'."; | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When assigning UnityEditor.SerializedProperty.objectReferenceValue to an incompatible UnityEngine.Object type, does Unity leave the property null?
💡 Result:
Yes—Unity generally rejects incompatible assignments to SerializedProperty.objectReferenceValue, and the serialized reference ends up cleared (null) (often with the Inspector showing “Type mismatch”). This behavior is commonly observed when code assigns a UnityEngine.Object that doesn’t match what the field/ObjectField expects. [1]
Be aware that “Type mismatch” doesn’t always mean the underlying reference is null: Unity has reported cases where the Inspector displays “Type mismatch” even though the reference is still present/usable (a visual/serialization-state issue, especially around Play Mode). [2]
Sources: [1] [2]
🏁 Script executed:
# Find the ResolveSceneObjectByName method in ComponentOps.cs and check for null-check pattern
fd -e cs -x grep -l "ResolveSceneObjectByName" {} \;Repository: CoplayDev/unity-mcp
Length of output: 107
🏁 Script executed:
# Also search for SetObjectReference to understand the full context
fd ComponentOps.cs -x wc -l {} \;Repository: CoplayDev/unity-mcp
Length of output: 164
🏁 Script executed:
# Look at the GameObjectComponentHelpers.cs caller mentioned in the review
fd GameObjectComponentHelpers.csRepository: CoplayDev/unity-mcp
Length of output: 201
🏁 Script executed:
# Search for ResolveSceneObjectByName in ComponentOps.cs
grep -n "ResolveSceneObjectByName" MCPForUnity/Editor/Helpers/ComponentOps.csRepository: CoplayDev/unity-mcp
Length of output: 349
🏁 Script executed:
# Look at the GameObjectComponentHelpers.cs file around lines 180-203 and search for calls to SetProperty/SetObjectReference
sed -n '170,210p' MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 2503
🏁 Script executed:
# Get more context from GameObjectComponentHelpers.cs to understand what SetProperty is
grep -n "SetProperty" MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs | head -20Repository: CoplayDev/unity-mcp
Length of output: 180
🏁 Script executed:
# View the ResolveSceneObjectByName method implementation
sed -n '725,760p' MCPForUnity/Editor/Helpers/ComponentOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 1716
🏁 Script executed:
# Get the full SetObjectReference method to see the exact lines 625-665
sed -n '625,665p' MCPForUnity/Editor/Helpers/ComponentOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 2272
🏁 Script executed:
# Verify the full context of ResolveSceneObjectByName to see if it continues with component fallback
sed -n '725,770p' MCPForUnity/Editor/Helpers/ComponentOps.cs | cat -nRepository: CoplayDev/unity-mcp
Length of output: 2095
Verify the sprite assignment actually sticks before returning success.
Both atlas sprite resolution branches return true immediately after assigning prop.objectReferenceValue = sprite without verifying the assignment succeeded. If the target field type does not accept Sprite, Unity rejects the assignment and leaves the reference null, yet this method still reports success. The caller (GameObjectComponentHelpers.cs:187) treats true as final success and does not retry, making this a silent no-op. The same file already uses the safer pattern in ResolveSceneObjectByName (lines 725-770): verify the reference is non-null before returning success.
Proposed fix
if (spriteNameToken != null)
{
string spriteName = spriteNameToken.ToString();
var allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
foreach (var asset in allAssets)
{
if (asset is Sprite sprite && sprite.name == spriteName)
{
prop.objectReferenceValue = sprite;
- return true;
+ if (prop.objectReferenceValue != null)
+ return true;
+
+ error = $"Resolved sprite '{spriteName}' is not compatible with '{prop.propertyPath}'.";
+ return false;
}
}
@@
foreach (var asset in allAssets)
{
if (asset is Sprite sprite)
{
long spriteFileId = GetSpriteFileId(sprite);
if (spriteFileId == targetFileId)
{
prop.objectReferenceValue = sprite;
- return true;
+ if (prop.objectReferenceValue != null)
+ return true;
+
+ error = $"Resolved sprite with fileID '{targetFileId}' is not compatible with '{prop.propertyPath}'.";
+ return false;
}
}
}📝 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.
| var spriteNameToken = jObj["spriteName"]; | |
| if (spriteNameToken != null) | |
| { | |
| string spriteName = spriteNameToken.ToString(); | |
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | |
| foreach (var asset in allAssets) | |
| { | |
| if (asset is Sprite sprite && sprite.name == spriteName) | |
| { | |
| prop.objectReferenceValue = sprite; | |
| return true; | |
| } | |
| } | |
| error = $"Sprite '{spriteName}' not found in atlas '{path}'."; | |
| return false; | |
| } | |
| var fileIdToken = jObj["fileID"]; | |
| if (fileIdToken != null) | |
| { | |
| long targetFileId = fileIdToken.Value<long>(); | |
| if (targetFileId != 0) | |
| { | |
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | |
| foreach (var asset in allAssets) | |
| { | |
| if (asset is Sprite sprite) | |
| { | |
| long spriteFileId = GetSpriteFileId(sprite); | |
| if (spriteFileId == targetFileId) | |
| { | |
| prop.objectReferenceValue = sprite; | |
| return true; | |
| } | |
| } | |
| } | |
| } | |
| error = $"Sprite with fileID '{targetFileId}' not found in atlas '{path}'."; | |
| return false; | |
| var spriteNameToken = jObj["spriteName"]; | |
| if (spriteNameToken != null) | |
| { | |
| string spriteName = spriteNameToken.ToString(); | |
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | |
| foreach (var asset in allAssets) | |
| { | |
| if (asset is Sprite sprite && sprite.name == spriteName) | |
| { | |
| prop.objectReferenceValue = sprite; | |
| if (prop.objectReferenceValue != null) | |
| return true; | |
| error = $"Resolved sprite '{spriteName}' is not compatible with '{prop.propertyPath}'."; | |
| return false; | |
| } | |
| } | |
| error = $"Sprite '{spriteName}' not found in atlas '{path}'."; | |
| return false; | |
| } | |
| var fileIdToken = jObj["fileID"]; | |
| if (fileIdToken != null) | |
| { | |
| long targetFileId = fileIdToken.Value<long>(); | |
| if (targetFileId != 0) | |
| { | |
| var allAssets = AssetDatabase.LoadAllAssetsAtPath(path); | |
| foreach (var asset in allAssets) | |
| { | |
| if (asset is Sprite sprite) | |
| { | |
| long spriteFileId = GetSpriteFileId(sprite); | |
| if (spriteFileId == targetFileId) | |
| { | |
| prop.objectReferenceValue = sprite; | |
| if (prop.objectReferenceValue != null) | |
| return true; | |
| error = $"Resolved sprite with fileID '{targetFileId}' is not compatible with '{prop.propertyPath}'."; | |
| return false; | |
| } | |
| } | |
| } | |
| } | |
| error = $"Sprite with fileID '{targetFileId}' not found in atlas '{path}'."; | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs` around lines 625 - 665, The
method assigns a Sprite to prop.objectReferenceValue and returns true
immediately, which can hide failed assignments; update both branches (the
spriteName branch and the fileID branch in ComponentOps.ResolveAtlasSprite) to
verify the assignment stuck by checking prop.objectReferenceValue != null (or
equals the assigned sprite) after setting it, and only return true if the
reference is non-null; otherwise set the error message (as done currently) and
return false—follow the same verification pattern used in
ResolveSceneObjectByName and continue using GetSpriteFileId for the fileID
branch.
- unchecked((long)targetObjectId) prevents OverflowException for large ulong fileIDs - Replace silent catch with McpLog.Warn including sprite name and instanceID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Adds atlas sprite resolution support for object references using
guid + spriteNameandfileIDforms.ComponentOpsValidation:
CoplayDev/unity-mcp:betaSummary by Sourcery
Improve handling of Unity object references in component serialization to better resolve atlased sprites from complex reference forms.
New Features:
Enhancements:
Summary by CodeRabbit