Merged
Conversation
Contributor
Reviewer's GuideRefactors object reference handling to use a shared helper with enhanced Sprite/sub-asset support, and adds nested prefab creation support in prefab editing APIs while updating server-side tool docs and validation accordingly. Sequence diagram for shared object reference resolution with sub-asset fallbacksequenceDiagram
actor Client
participant Server_manage_scriptable_object
participant ManageScriptableObject
participant ComponentOps
participant AssetDatabase
Client->>Server_manage_scriptable_object: HTTP request patches with ref/value
Server_manage_scriptable_object->>ManageScriptableObject: ApplySet(so, propertyPath, patchObj)
ManageScriptableObject->>ComponentOps: SetObjectReference(prop, resolveToken, out refError)
ComponentOps->>AssetDatabase: Resolve guid/path to asset
ComponentOps->>ComponentOps: AssignObjectReference(prop, resolved, out error)
alt property accepts resolved directly
ComponentOps-->>ManageScriptableObject: success, prop.objectReferenceValue set
else needs sub-asset (e.g. Texture2D to Sprite)
ComponentOps->>AssetDatabase: LoadAllAssetsAtPath(subAssetPath)
alt single compatible sub-asset
ComponentOps-->>ManageScriptableObject: success, prop.objectReferenceValue set to Sprite
else multiple compatible sub-assets
ComponentOps-->>ManageScriptableObject: failure, error "Multiple compatible sub-assets found"
end
end
alt success
ManageScriptableObject-->>Server_manage_scriptable_object: ok=true, message "Set reference to 'name'"
else failure
ManageScriptableObject-->>Server_manage_scriptable_object: ok=false, message=refError
end
Updated class diagram for Unity object reference and prefab managementclassDiagram
class ManageScriptableObject {
+ApplySet(SerializedObject so, string propertyPath, JObject patchObj) object
+TrySetValueRecursive(SerializedProperty prop, JToken valueToken, out string message) bool
}
class ComponentOps {
+SetSerializedPropertyRecursive(SerializedProperty prop, JToken valueToken, out string error) bool
+SetObjectReference(SerializedProperty prop, JToken value, out string error) bool
-AssignObjectReference(SerializedProperty prop, UnityEngine.Object resolved, out string error) bool
}
class ManagePrefabs {
-ApplyModificationsToPrefabObject(GameObject targetGo, JObject paramsObj, GameObject prefabRoot, string editingPrefabPath) (bool, ErrorResponse)
-CreateSingleChildInPrefab(JToken createChildToken, GameObject defaultParent, GameObject prefabRoot, string editingPrefabPath) (bool, ErrorResponse)
}
class ErrorResponse {
+ErrorResponse(string message)
}
ManageScriptableObject ..> ComponentOps : uses
ManagePrefabs ..> ComponentOps : uses
ManagePrefabs ..> ErrorResponse : returns
Flow diagram for nested prefab creation in CreateSingleChildInPrefabflowchart TD
A[CreateSingleChildInPrefab] --> B[Read sourcePrefabPath and primitiveType]
B --> C{Both sourcePrefabPath and primitiveType set?}
C -->|Yes| E[Return error: mutually exclusive]
C -->|No| D{sourcePrefabPath set?}
D -->|Yes| F[Sanitize sourcePrefabPath]
F --> G{Sanitized path valid?}
G -->|No| H[Error: invalid source_prefab_path]
G -->|Yes| I{editingPrefabPath equals sourcePrefabPath?}
I -->|Yes| J[Error: cannot nest prefab inside itself]
I -->|No| K[Load prefab asset at path]
K --> L{Prefab asset found?}
L -->|No| M[Error: source prefab not found]
L -->|Yes| N[Instantiate nested prefab under parentTransform]
N --> O{Instantiation succeeded?}
O -->|No| P[Error: failed to instantiate nested prefab]
O -->|Yes| Q[Set newChild.name and parentTransform]
D -->|No| R{primitiveType set?}
R -->|Yes| S[Create primitive GameObject of type]
R -->|No| T[Create empty GameObject with name]
S --> U[Set parentTransform worldPositionStays=false]
T --> U
Q --> U
U --> V[Apply transform, components, tag, layer, active state]
V --> W[Return created=true]
File-Level Changes
Possibly linked issues
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.
Pull request overview
This PR addresses Unity MCP issues #949/#950/#953 by improving object reference resolution (especially Sprite sub-assets), aligning manage_scriptable_object behavior with the shared resolver, and adding nested prefab instantiation support for prefab editing workflows.
Changes:
- Document and support Sprite sub-asset resolution (including “single-sprite texture” auto-resolve) in tool parameter docs.
- Update
ManageScriptableObjectto delegate ObjectReference patching to the sharedComponentOps.SetObjectReference. - Add nested prefab support to
manage_prefabs.modify_contentsviacreate_child.source_prefab_path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/src/services/tools/manage_scriptable_object.py | Expands patches parameter docs to include object refs and Sprite sub-asset guidance. |
| Server/src/services/tools/manage_prefabs.py | Updates tool docs to describe nested prefab child creation and Sprite sub-asset reference format. |
| Server/src/services/tools/manage_components.py | Updates value docs to describe object refs and Sprite sub-asset formats (spriteName/fileID). |
| MCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.cs | Implements source_prefab_path nested prefab instantiation inside prefab contents editing. |
| MCPForUnity/Editor/Tools/ManageScriptableObject.cs | Switches ObjectReference patching to use ComponentOps.SetObjectReference. |
| MCPForUnity/Editor/Helpers/ComponentOps.cs | Exposes SetObjectReference for reuse and adds sub-asset fallback behavior for compatible sub-assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
740
to
746
| // Legacy "ref" key takes precedence for backward compatibility | ||
| var refObj = patchObj["ref"] as JObject; | ||
| var objRefValue = patchObj["value"]; | ||
| UnityEngine.Object newRef = null; | ||
| string refGuid = refObj?["guid"]?.ToString(); | ||
| string refPath = refObj?["path"]?.ToString(); | ||
| string resolveMethod = "explicit"; | ||
| JToken resolveToken = refObj ?? objRefValue; | ||
|
|
||
| if (refObj == null && objRefValue?.Type == JTokenType.Null) | ||
| if (!ComponentOps.SetObjectReference(prop, resolveToken, out string refError)) | ||
| { |
There was a problem hiding this comment.
In ApplySet for ObjectReference, resolveToken becomes null when neither ref nor value is provided. Passing null into ComponentOps.SetObjectReference clears the reference and returns success, which is inconsistent with the non-ObjectReference branch (it errors on missing value) and can cause accidental data loss. Treat missing ref/value as an error instead of clearing the field.
Comment on lines
749
to
754
|
|
||
| if (prop.objectReferenceValue != newRef) | ||
| { | ||
| prop.objectReferenceValue = newRef; | ||
| changed = true; | ||
| } | ||
|
|
||
| string refMessage = newRef == null ? "Cleared reference." : $"Set reference ({resolveMethod})."; | ||
| changed = true; | ||
| string refMessage = prop.objectReferenceValue == null | ||
| ? "Cleared reference." | ||
| : $"Set reference to '{prop.objectReferenceValue.name}'."; | ||
| return new { propertyPath, op = "set", ok = true, resolvedPropertyType = prop.propertyType.ToString(), message = refMessage }; |
There was a problem hiding this comment.
changed is set to true unconditionally after a successful object reference resolution. This will mark the asset dirty and trigger SaveAssets() even for no-op updates (setting the same reference or clearing an already-null field). Consider computing changed by comparing the pre/post objectReferenceValue (or instanceID) so Modify only saves when something actually changed.
Comment on lines
783
to
+821
| // Try direct assignment first | ||
| prop.objectReferenceValue = resolved; | ||
| if (prop.objectReferenceValue != null) | ||
| return true; | ||
|
|
||
| // Sub-asset fallback: e.g., Texture2D → Sprite | ||
| string subAssetPath = AssetDatabase.GetAssetPath(resolved); | ||
| if (!string.IsNullOrEmpty(subAssetPath)) | ||
| { | ||
| var subAssets = AssetDatabase.LoadAllAssetsAtPath(subAssetPath); | ||
| UnityEngine.Object match = null; | ||
| int matchCount = 0; | ||
| foreach (var sub in subAssets) | ||
| { | ||
| if (sub == null || sub == resolved) continue; | ||
| prop.objectReferenceValue = sub; | ||
| if (prop.objectReferenceValue != null) | ||
| { | ||
| match = sub; | ||
| matchCount++; | ||
| if (matchCount > 1) break; | ||
| } | ||
| } | ||
|
|
||
| if (matchCount == 1) | ||
| { | ||
| prop.objectReferenceValue = match; | ||
| return true; | ||
| } | ||
|
|
||
| // Clean up: probing may have left the property dirty | ||
| prop.objectReferenceValue = null; | ||
|
|
||
| if (matchCount > 1) | ||
| { | ||
| error = $"Multiple compatible sub-assets found in '{subAssetPath}'. " + | ||
| "Use {\"guid\": \"...\", \"spriteName\": \"<name>\"} or " + | ||
| "{\"guid\": \"...\", \"fileID\": <id>} for precise selection."; | ||
| return false; |
There was a problem hiding this comment.
The new sub-asset fallback probes compatibility by assigning candidates into prop.objectReferenceValue, and on failure it sets the property to null. Because the previous value isn’t restored, a failed resolution (e.g. multiple compatible sub-assets) can still clear/modify the property inside the SerializedObject, and those unintended changes can be persisted later if any other patch succeeds. Preserve and restore the original reference on any failure path (including after probing) so SetObjectReference is side-effect free when returning false.
Comment on lines
1022
to
1049
| @@ -1010,7 +1044,7 @@ private static (bool created, ErrorResponse error) CreateSingleChildInPrefab(JTo | |||
| newChild = new GameObject(childName); | |||
| } | |||
|
|
|||
| // Set parent | |||
| // Ensure local-space transform (worldPositionStays=false) for all creation modes | |||
| newChild.transform.SetParent(parentTransform, false); | |||
|
|
|||
There was a problem hiding this comment.
For nested prefab creation, PrefabUtility.InstantiatePrefab(sourcePrefabAsset, parentTransform) already parents the instance, and then you call newChild.transform.SetParent(parentTransform, false) again. This can unexpectedly alter the instantiated prefab’s transform (depending on how InstantiatePrefab handles world/local space) and is redundant work. Consider instantiating without a parent and then doing a single SetParent(parentTransform, false), or skip the second parenting step for the nested-prefab branch.
Comment on lines
+52
to
+53
| "For Sprite sub-assets: include \"spriteName\" in the ref/value object. " | ||
| "Single-sprite textures auto-resolve from guid/path alone."] = None, |
There was a problem hiding this comment.
The new patches help text suggests adding spriteName to the ref/value object, but on the Unity side spriteName selection is only implemented for {guid: ..., spriteName: ...} (not {path: ..., spriteName: ...}). To avoid misleading callers, clarify in this docstring that spriteName/fileID disambiguation requires guid (path-only supports only the single-subasset auto-resolve fallback).
Suggested change
| "For Sprite sub-assets: include \"spriteName\" in the ref/value object. " | |
| "Single-sprite textures auto-resolve from guid/path alone."] = None, | |
| "For Sprite sub-assets with GUID references, include \"spriteName\" (or fileID) in the ref/value object; " | |
| "path-only references do not support spriteName/fileID disambiguation and only auto-resolve single-sprite textures."] = None, |
Comment on lines
+997
to
+1000
| if (!string.IsNullOrEmpty(sourcePrefabPath) && !string.IsNullOrEmpty(primitiveType)) | ||
| { | ||
| return (false, new ErrorResponse("'source_prefab_path' and 'primitive_type' are mutually exclusive in create_child.")); | ||
| } |
There was a problem hiding this comment.
The mutually-exclusive validation error mentions only the snake_case keys (source_prefab_path / primitive_type), but the API also accepts camelCase (sourcePrefabPath / primitiveType). Consider wording the error message to reference both variants (or the conceptual fields) so callers using camelCase aren’t confused.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCentralized object-reference assignment into ComponentOps.SetObjectReference, added sub-asset fallback for asset-sub-asset matching, routed ManageScriptableObject to use that method, enabled nested prefab creation via source_prefab_path, and expanded server tool parameter docs for reference and sprite formats. Changes
Sequence DiagramssequenceDiagram
participant ManageSO as ManageScriptableObject
participant ComponentOps
participant SerializedProp as SerializedProperty
participant AssetDB as AssetDatabase
ManageSO->>ComponentOps: SetObjectReference(prop, jtoken)
ComponentOps->>SerializedProp: objectReferenceValue = resolved_obj
alt Direct assignment accepted
ComponentOps-->>ManageSO: success
else Assignment rejected by Unity (null)
ComponentOps->>AssetDB: GetAssetPath(resolved_obj)
AssetDB-->>ComponentOps: asset_path
ComponentOps->>AssetDB: LoadAllAssetsAtPath(asset_path)
AssetDB-->>ComponentOps: sub_assets[]
loop Probe sub-assets
ComponentOps->>SerializedProp: objectReferenceValue = sub_asset
ComponentOps->>ComponentOps: tally compatible matches
end
alt Exactly one match
ComponentOps->>SerializedProp: objectReferenceValue = matched_sub_asset
ComponentOps-->>ManageSO: success
else Multiple matches
ComponentOps->>SerializedProp: objectReferenceValue = null
ComponentOps-->>ManageSO: failure (ambiguity)
else No matches
ComponentOps-->>ManageSO: failure (continue fallback)
end
end
sequenceDiagram
participant ManagePrefabs
participant CreateSingle as CreateSingleChildInPrefab
participant AssetDB as AssetDatabase
participant PrefabUtil as PrefabUtility
participant Transform
ManagePrefabs->>CreateSingle: CreateSingleChildInPrefab(..., editingPrefabPath, sourcePrefabPath)
alt source_prefab_path provided
CreateSingle->>CreateSingle: Sanitize and validate sourcePrefabPath
CreateSingle->>CreateSingle: Check source != editingPrefabPath
CreateSingle->>AssetDB: LoadAssetAtPath(sanitized_path)
AssetDB-->>CreateSingle: source_prefab_asset
CreateSingle->>PrefabUtil: InstantiatePrefab(source_prefab_asset)
PrefabUtil-->>CreateSingle: instantiated_prefab
CreateSingle->>Transform: SetParent(instantiated_prefab, parent, false)
CreateSingle-->>ManagePrefabs: return instantiated_prefab
else primitive or empty creation
CreateSingle->>CreateSingle: Create primitive/empty GameObject
CreateSingle->>Transform: SetParent(created_obj, parent, false)
CreateSingle-->>ManagePrefabs: return created_obj
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Helpers/ComponentOps.cs (1)
645-685:⚠️ Potential issue | 🔴 CriticalDon't report success or clear the old value while probing sprite/sub-asset candidates.
Lines 654 and 677 return success immediately after
prop.objectReferenceValue = sprite, but Unity leaves that fieldnullwhen the property type is incompatible. Then lines 798–815 probe more candidates by mutating the same property and restorenullinstead of the original reference. In batched patch operations, a prior successful patch can still cause that unintended clear to be persisted byApplyModifiedProperties().Suggested fix
@@ - if (asset is Sprite sprite && sprite.name == spriteName) - { - prop.objectReferenceValue = sprite; - return true; - } + if (asset is Sprite sprite && sprite.name == spriteName) + { + return AssignObjectReference(prop, sprite, componentFilter, out error); + } @@ - if (spriteFileId == targetFileId) - { - prop.objectReferenceValue = sprite; - return true; - } + if (spriteFileId == targetFileId) + { + return AssignObjectReference(prop, sprite, componentFilter, out error); + } @@ private static bool AssignObjectReference(SerializedProperty prop, UnityEngine.Object resolved, string componentFilter, out string error) { error = null; + UnityEngine.Object originalValue = prop.objectReferenceValue; if (resolved == null) { error = "Resolved object is null."; return false; } @@ foreach (var comp in components) { if (comp == null) continue; prop.objectReferenceValue = comp; if (prop.objectReferenceValue != null) return true; } + prop.objectReferenceValue = originalValue; error = $"Component '{componentFilter}' not found on GameObject '{filterGo.name}'."; return false; } @@ - prop.objectReferenceValue = null; + prop.objectReferenceValue = originalValue; @@ foreach (var comp in components) { if (comp == null) continue; prop.objectReferenceValue = comp; if (prop.objectReferenceValue != null) return true; } + prop.objectReferenceValue = originalValue; error = $"GameObject '{go.name}' found but no compatible component for the property type."; return false; } + prop.objectReferenceValue = originalValue; error = $"Object '{resolved.name}' (type: {resolved.GetType().Name}) is not compatible with the property type."; return false; }Add editor regression tests for both cases:
spriteName/fileIDagainst a non-Spriteproperty field, and a failed reference patch after an earlier successful patch in the same batch.🤖 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 645 - 685, The probe for sprite sub-assets mutates prop.objectReferenceValue and returns true early (in the spriteName and fileID branches) which can leave the property null if Unity rejects the assigned type; instead, do not persist assignments until you've verified the candidate is actually compatible and the operation will succeed: capture the original prop.objectReferenceValue, test candidates by only checking types/IDs (via AssetDatabase.LoadAllAssetsAtPath, GetSpriteFileId, sprite.name, jObj["spriteName"], jObj["fileID"]) and only set prop.objectReferenceValue and return true after confirming compatibility; on any failure restore the original value and return false so batched ApplyModifiedProperties() cannot clear a previous valid reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/ManageScriptableObject.cs`:
- Around line 740-745: The code narrows patchObj["ref"] to JObject losing
non-object tokens and can silently clear references; change the handling in
ManageScriptableObject to use patchObj.TryGetValue("ref", out JToken refToken)
(keep refToken as JToken), set resolveToken = refToken ?? patchObj["value"], and
then call ComponentOps.SetObjectReference(prop, resolveToken, out string
refError); additionally add validation to reject the payload (return/report an
error) when both refToken and patchObj["value"] are missing or null so invalid
inputs are not treated as a deliberate clear.
---
Outside diff comments:
In `@MCPForUnity/Editor/Helpers/ComponentOps.cs`:
- Around line 645-685: The probe for sprite sub-assets mutates
prop.objectReferenceValue and returns true early (in the spriteName and fileID
branches) which can leave the property null if Unity rejects the assigned type;
instead, do not persist assignments until you've verified the candidate is
actually compatible and the operation will succeed: capture the original
prop.objectReferenceValue, test candidates by only checking types/IDs (via
AssetDatabase.LoadAllAssetsAtPath, GetSpriteFileId, sprite.name,
jObj["spriteName"], jObj["fileID"]) and only set prop.objectReferenceValue and
return true after confirming compatibility; on any failure restore the original
value and return false so batched ApplyModifiedProperties() cannot clear a
previous valid reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96bada40-ff2b-4a32-88fb-8a68bdc29f9a
📒 Files selected for processing (6)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Tools/ManageScriptableObject.csMCPForUnity/Editor/Tools/Prefabs/ManagePrefabs.csServer/src/services/tools/manage_components.pyServer/src/services/tools/manage_prefabs.pyServer/src/services/tools/manage_scriptable_object.py
ComponentOps: preserve the original objectReferenceValue before trying to assign found sprites; if Unity rejects the assignment (resulting in a null reference), restore the original value and return a clear error indicating the sprite name or fileID was incompatible with the property type. ManageScriptableObject: use TryGetValue to read the legacy "ref" token so non-JObject tokens (e.g. string GUIDs) are preserved; treat ref or value as the resolve token and return a structured error when neither is present. These changes improve robustness when object references are incompatible or when legacy ref formats are used.
Scriptwonder
deleted the
fix/949-950-953-object-ref-sprite-nested-prefab
branch
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A fix patch that is dedicated on issue #949, #950, #953.
Summary by Sourcery
Unify object reference handling across components and scriptable-object patches, add Sprite sub-asset resolution, and extend prefab modification to support nested prefab instances with updated API docs.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation