Skip to content

feat(mcpforunity): support atlas sprite resolution by guid+spriteName/fileID #873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Scriptwonder merged 4 commits into from
Mar 11, 2026
Merged

feat(mcpforunity): support atlas sprite resolution by guid+spriteName/fileID #873

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8d4915
feat(mcpforunity): support atlas sprite resolution by guid+spriteName…
Mar 5, 2026
7fd9af2
fix: resolve compilation error and silent fallthrough in atlas sprite…
Scriptwonder Mar 11, 2026
3e1bc51
Merge remote-tracking branch 'origin/beta' into pr-873-update
Scriptwonder Mar 11, 2026
b61abdc
fix: use unchecked cast and log exceptions in GetSpriteFileId
Scriptwonder Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions MCPForUnity/Editor/Helpers/ComponentOps.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,21 @@ private static bool TrySetViaReflection(object component, Type type, string prop
{
error = null;

// Skip reflection for UnityEngine.Object types with JObject values
// so SerializedProperty can resolve guid/spriteName/fileID forms.
bool isJObjectValue = value != null && value.Type == JTokenType.Object;

// Try property first
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;
}
Comment on lines 205 to +213
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.sprite by name; if you call FindProperty("sprite") you should expect null unless 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]), FindProperty can 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"), not MyProp. [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 serialized m_Sprite field (so FindProperty("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.


try
{
object convertedValue = PropertyConversion.ConvertToType(value, propInfo.PropertyType);
Expand All @@ -225,6 +235,12 @@ private static bool TrySetViaReflection(object component, Type type, string prop
?? type.GetField(normalizedName, flags);
if (fieldInfo != null && !fieldInfo.IsInitOnly)
{
if (isJObjectValue && typeof(UnityEngine.Object).IsAssignableFrom(fieldInfo.FieldType))
{
// Let SerializedProperty path handle complex object references.
return false;
}

try
{
object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType);
Expand All @@ -248,6 +264,12 @@ private static bool TrySetViaReflection(object component, Type type, string prop
?? FindSerializedFieldInHierarchy(type, normalizedName);
if (fieldInfo != null)
{
if (isJObjectValue && typeof(UnityEngine.Object).IsAssignableFrom(fieldInfo.FieldType))
{
// Let SerializedProperty path handle complex object references.
return false;
}

try
{
object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType);
Expand Down Expand Up @@ -599,6 +621,50 @@ private static bool SetObjectReference(SerializedProperty prop, JToken value, ou
error = $"No asset found for GUID '{guidToken}'.";
return false;
}

var spriteNameToken = jObj["spriteName"];
if (spriteNameToken != null)
{
string spriteName = spriteNameToken.ToString();
var allAssets = AssetDatabase.LoadAllAssetsAtPath(path);
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  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[]>();

foreach (var asset in allAssets)
{
if (asset is Sprite sprite && sprite.name == spriteName)
{
prop.objectReferenceValue = sprite;
Comment on lines +625 to +634
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
Comment on lines +625 to +665
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.cs

Repository: CoplayDev/unity-mcp

Length of output: 201

🏁 Script executed:

# Search for ResolveSceneObjectByName in ComponentOps.cs
grep -n "ResolveSceneObjectByName" MCPForUnity/Editor/Helpers/ComponentOps.cs

Repository: 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 -n

Repository: 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 -20

Repository: CoplayDev/unity-mcp

Length of output: 180

🏁 Script executed:

# View the ResolveSceneObjectByName method implementation
sed -n '725,760p' MCPForUnity/Editor/Helpers/ComponentOps.cs | cat -n

Repository: 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 -n

Repository: 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 -n

Repository: 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.

Suggested change
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.

}

prop.objectReferenceValue = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path);
return true;
}
Expand Down Expand Up @@ -767,6 +833,23 @@ private static bool SetEnum(SerializedProperty prop, JToken value, out string er
error = $"Unknown enum name '{s}'.";
return false;
}

private static long GetSpriteFileId(Sprite sprite)
Copy link
Contributor

@sourcery-ai sourcery-ai bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. using System;
  2. using UnityEngine;

{
if (sprite == null)
return 0;

try
{
var globalId = GlobalObjectId.GetGlobalObjectIdSlow(sprite);
return unchecked((long)globalId.targetObjectId);
}
catch (Exception ex)
{
McpLog.Warn($"Failed to get fileID for sprite '{sprite.name}' (instanceID={sprite.GetInstanceID()}): {ex.Message}");
return 0;
}
}
}
}