Skip to content

Fix UnityEvent wiring via manage_components set_property#757

Merged
dsarno merged 3 commits into
CoplayDev:betafrom
dsarno:fix/unity-event-serialized-property-631
Feb 16, 2026
Merged

Fix UnityEvent wiring via manage_components set_property#757
dsarno merged 3 commits into
CoplayDev:betafrom
dsarno:fix/unity-event-serialized-property-631

Conversation

@dsarno

@dsarno dsarno commented Feb 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Routes UnityEventBase-derived fields through SerializedObject/SerializedProperty instead of reflection, fixing silent data loss where m_PersistentCalls was empty on save
  • Adds recursive SerializedProperty setter handling Generic structs, arrays, ObjectReference, and leaf types (int, bool, float, string, enum)
  • Fixes batch_execute recursive key normalization that mangled nested value keys like m_PersistentCalls → mPersistentCalls (batch_execute normalizes JSON value keys, stripping underscores from nested payloads #756)

Closes #631
Closes #756

Test plan

  • 6 unit tests for UnityEvent wiring (single call, multi-call, clear, private field, regression, end-to-end)
  • 3 unit tests for batch_execute key preservation (nested keys preserved, top-level normalized, regression)
  • Live smoke test: batch_execute 3 UnityEvent set_property calls — all succeed
  • Manual verification: wire a Button onClick via MCP, save scene, reopen, confirm m_Calls persists in Inspector

🤖 Generated with Claude Code

UnityEventBase-derived fields set via reflection create disconnected
objects that Unity's serialization layer doesn't track, causing
m_PersistentCalls to be empty on save. Route these types through
SerializedObject/SerializedProperty instead.

Adds recursive SerializedProperty setter supporting Generic structs,
arrays, ObjectReference, and leaf types (int, bool, float, string, enum).
Includes fuzzy child property matching to handle batch_execute stripping
underscores from nested JSON keys (tracked in CoplayDev#756).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Feb 15, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sourcery-ai

sourcery-ai Bot commented Feb 15, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Routes UnityEvent properties through Unity’s SerializedObject/SerializedProperty pipeline instead of reflection, adds a recursive JSON→SerializedProperty setter (including fuzzy child lookup), and introduces focused Unity tests around UnityEvent wiring and end-to-end manage_components behavior.

Sequence diagram for UnityEvent SetProperty via SerializedProperty

sequenceDiagram
    actor Caller
    participant ComponentOps
    participant SerializedObject
    participant SerializedProperty
    participant EditorUtility
    participant AssetDatabase

    Caller->>ComponentOps: SetProperty(component, propertyName, value, out error)
    activate ComponentOps
    ComponentOps->>ComponentOps: ResolveMemberType(componentType, propertyName, normalizedName)
    ComponentOps-->>ComponentOps: memberType
    alt memberType is UnityEventBase derived
        ComponentOps->>ComponentOps: SetViaSerializedProperty(component, propertyName, normalizedName, value, out error)
        activate SerializedObject
        ComponentOps->>SerializedObject: new SerializedObject(component)
        SerializedObject-->>ComponentOps: so
        ComponentOps->>SerializedObject: FindProperty(propertyName or normalizedName)
        SerializedObject-->>ComponentOps: prop
        alt prop found
            ComponentOps->>ComponentOps: SetSerializedPropertyRecursive(prop, value, out error, depth=0)
            loop recursion over children
                ComponentOps->>ComponentOps: handle arrays, generics, leaf types
                alt prop.propertyType == ObjectReference
                    ComponentOps->>ComponentOps: SetObjectReference(prop, value, out error)
                    alt value is instanceID
                        ComponentOps->>EditorUtility: InstanceIDToObject(id)
                        EditorUtility-->>ComponentOps: UnityEngine.Object
                    else value has guid or path
                        ComponentOps->>AssetDatabase: GUIDToAssetPath or LoadAssetAtPath
                        AssetDatabase-->>ComponentOps: UnityEngine.Object
                    end
                else prop.propertyType == Enum
                    ComponentOps->>ComponentOps: SetEnum(prop, value, out error)
                end
            end
            ComponentOps->>SerializedObject: ApplyModifiedProperties()
            SerializedObject-->>ComponentOps: properties applied
            ComponentOps-->>Caller: true, error = null
        else prop not found
            ComponentOps-->>Caller: false, error
        end
    else memberType is not UnityEventBase derived
        ComponentOps-->>Caller: fall back to reflection-based SetProperty path
    end
    deactivate ComponentOps
Loading

Updated class diagram for ComponentOps UnityEvent handling

classDiagram
    class ComponentOps {
        <<static>>
        +bool SetProperty(Component component, string propertyName, JToken value, out string error)
        +Type ResolveMemberType(Type componentType, string propertyName, string normalizedName)
        +bool SetViaSerializedProperty(Component component, string propertyName, string normalizedName, JToken value, out string error)
        +bool SetSerializedPropertyRecursive(SerializedProperty prop, JToken value, out string error, int depth)
        +bool SetObjectReference(SerializedProperty prop, JToken value, out string error)
        +SerializedProperty FindPropertyRelativeFuzzy(SerializedProperty parent, string key)
        +bool SetEnum(SerializedProperty prop, JToken value, out string error)
    }

    class UnityEventBase {
    }

    class UnityEvent {
    }

    class UnityEvent_float {
        <<generic UnityEvent<float>>
    }

    class UnityEventTestComponent {
        +UnityEvent onSimpleEvent
        +UnityEvent_float onFloatEvent
        -UnityEvent _onPrivateEvent
    }

    class MonoBehaviour {
    }

    class SerializedObject {
    }

    class SerializedProperty {
        +bool isArray
        +SerializedPropertyType propertyType
        +int arraySize
        +SerializedObject serializedObject
        +string propertyPath
        +string name
        +int depth
        +int intValue
        +bool boolValue
        +float floatValue
        +string stringValue
        +string[] enumNames
        +int enumValueIndex
        +UnityEngine_Object objectReferenceValue
        +SerializedProperty GetArrayElementAtIndex(int index)
        +SerializedProperty FindPropertyRelative(string relativePropertyPath)
        +SerializedProperty GetEndProperty()
        +SerializedProperty Copy()
        +bool Next(bool enterChildren)
        +static bool EqualContents(SerializedProperty x, SerializedProperty y)
    }

    class SerializedPropertyType {
        <<enum>>
        Integer
        Boolean
        Float
        String
        Enum
        ObjectReference
        Generic
    }

    class EditorUtility {
        +UnityEngine_Object InstanceIDToObject(int id)
    }

    class AssetDatabase {
        +string GUIDToAssetPath(string guid)
        +UnityEngine_Object LoadAssetAtPath~UnityEngine_Object~(string path)
    }

    class AssetPathUtility {
        +string SanitizeAssetPath(string path)
    }

    class ParamCoercion {
        +int CoerceInt(JToken value, int defaultValue)
        +bool CoerceBool(JToken value, bool defaultValue)
        +float CoerceFloat(JToken value, float defaultValue)
        +string NormalizePropertyName(string name)
    }

    class JToken {
        +JTokenType Type
        +string ToString()
    }

    class JObject {
    }

    class JArray {
        +int Count
        +JToken this[int index]
    }

    class JTokenType {
        <<enum>>
        Integer
        Boolean
        Float
        String
        Null
    }

    class UnityEngine_Object {
    }

    ComponentOps ..> UnityEventBase
    ComponentOps ..> SerializedObject
    ComponentOps ..> SerializedProperty
    ComponentOps ..> EditorUtility
    ComponentOps ..> AssetDatabase
    ComponentOps ..> AssetPathUtility
    ComponentOps ..> ParamCoercion
    ComponentOps ..> JToken
    ComponentOps ..> JObject
    ComponentOps ..> JArray

    UnityEventTestComponent --|> MonoBehaviour
    UnityEventTestComponent o--> UnityEvent : onSimpleEvent
    UnityEventTestComponent o--> UnityEvent_float : onFloatEvent
    UnityEventTestComponent o--> UnityEvent : _onPrivateEvent

    UnityEvent --|> UnityEventBase
    UnityEvent_float --|> UnityEventBase
Loading

Flow diagram for SetSerializedPropertyRecursive logic

graph TD
    A["SetSerializedPropertyRecursive(prop, value, error, depth)"] --> B{depth > MaxDepth?}
    B -- Yes --> C["Set error: Maximum recursion depth exceeded<br>return false"]
    B -- No --> D{prop.isArray<br>and prop.propertyType != String<br>and value is JArray?}
    D -- Yes --> E["Set prop.arraySize = jArray.Count<br>ApplyModifiedProperties<br>Update"]
    E --> F["For each index i in jArray"]
    F --> G["Get element = prop.GetArrayElementAtIndex(i)<br>Recurse SetSerializedPropertyRecursive(element, jArray[i], ...)"]
    G --> H["On any failure<br>return false"]
    G --> I["All elements set<br>return true"]

    D -- No --> J{prop.propertyType == Generic<br>and not prop.isArray<br>and value is JObject?}
    J -- Yes --> K["For each kvp in JObject"]
    K --> L["child = FindPropertyRelativeFuzzy(prop, kvp.Key)"]
    L --> M{child == null?}
    M -- Yes --> N["Set error: Sub-property not found<br>return false"]
    M -- No --> O["Recurse SetSerializedPropertyRecursive(child, kvp.Value, ...)"]
    O --> P["On any failure<br>return false"]
    K --> Q["After all children<br>return true"]

    J -- No --> R{prop.propertyType == ObjectReference?}
    R -- Yes --> S["Call SetObjectReference(prop, value, out error)<br>return"]

    R -- No --> T{Leaf propertyType?}
    T -- Yes --> U{Integer?}
    U -- Yes --> V["CoerceInt(value)<br>validate<br>on error set error and return false<br>else set prop.intValue and return true"]
    U -- No --> W{Boolean?}
    W -- Yes --> X["Validate non-null<br>CoerceBool(value)<br>set prop.boolValue<br>return true"]
    W -- No --> Y{Float?}
    Y -- Yes --> Z["CoerceFloat(value)<br>if NaN set error and return false<br>else set prop.floatValue and return true"]
    Y -- No --> AA{String?}
    AA -- Yes --> AB["If value is Null set prop.stringValue = null<br>else prop.stringValue = value.ToString()<br>return true"]
    AA -- No --> AC{Enum?}
    AC -- Yes --> AD["Call SetEnum(prop, value, out error)<br>return"]

    AC -- No --> AE["Set error: Unsupported SerializedPropertyType at path<br>return false"]

    T -- No --> AE

    subgraph ExceptionHandling
        AF["Any exception thrown"] --> AG["Set error: Error setting path with exception message<br>return false"]
    end
Loading

File-Level Changes

Change Details Files
Route UnityEventBase-derived properties through SerializedProperty with recursive JSON mapping instead of reflection to avoid losing persistent calls.
  • Detect member type for the requested property/field and short-circuit to SerializedProperty handling when it derives from UnityEventBase.
  • Resolve member type via properties, fields, or serialized backing fields using a helper that searches the type hierarchy.
  • Use SerializedObject.FindProperty on both raw and normalized names, apply changes via SerializedObject.ApplyModifiedProperties.
MCPForUnity/Editor/Helpers/ComponentOps.cs
Implement a general-purpose recursive setter from JToken to SerializedProperty that supports arrays, generic structs/classes, object references, enums, and leaf primitives, including fuzzy child lookup for keys with stripped underscores.
  • Add SetSerializedPropertyRecursive with depth limiting to walk SerializedProperty trees and map JObject/JArray structures to Unity’s serialized layout.
  • Handle arrays by resizing and iterating elements, generic/struct properties by iterating JObject fields and matching child properties (with fuzzy underscore-insensitive matching), and leaf types (int, bool, float, string, enum) with existing ParamCoercion helpers and validation.
  • Implement SetObjectReference to resolve references from instanceID, guid, or asset path, and SetEnum to map enum indices or names safely, returning structured error messages on failure.
  • Introduce FindPropertyRelativeFuzzy to fall back to underscore-insensitive name matching so JSON keys like mPersistentCalls still resolve to m_PersistentCalls under a parent property.
MCPForUnity/Editor/Helpers/ComponentOps.cs
Add focused edit-mode tests and a test component to validate UnityEvent wiring via manage_components, including regression coverage for persistent calls and private serialized events.
  • Create UnityEventTestComponent with public and private UnityEvent fields used as a target for tests.
  • Add ComponentOpsUnityEventTests to cover single/multiple/cleared persistent calls, private [SerializeField] UnityEvent routing, non-UnityEvent properties still using reflection, and an end-to-end ManageComponents.HandleCommand path.
  • Add corresponding Unity meta files for new test scripts.
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/UnityEventTestComponent.cs
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/UnityEventTestComponent.cs.meta
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentOpsUnityEventTests.cs
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentOpsUnityEventTests.cs.meta

Possibly linked issues

  • Bug: UnityEvent Wiring Clears m_Calls Instead of Setting Them #631: PR switches UnityEvent handling to SerializedProperty so m_PersistentCalls.m_Calls persist, directly fixing the clearing bug.
  • #N/A: PR implements SerializedProperty-based UnityEvent handling in manage_components, directly solving Inspector persistence and wiring limits described in issue.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `MCPForUnity/Editor/Helpers/ComponentOps.cs:517-518` </location>
<code_context>
+                        prop.floatValue = floatVal;
+                        return true;
+
+                    case SerializedPropertyType.String:
+                        prop.stringValue = value.Type == JTokenType.Null ? null : value.ToString();
+                        return true;
+
</code_context>

<issue_to_address>
**issue (bug_risk):** Guard against null `value` before accessing `value.Type` for string properties.

Here `value.Type` is used without checking for `value == null`, unlike the other branches. A null `value` would throw a `NullReferenceException`. Consider mirroring the boolean handling, e.g. `if (value == null || value.Type == JTokenType.Null) prop.stringValue = null; else prop.stringValue = value.ToString();`.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread MCPForUnity/Editor/Helpers/ComponentOps.cs Outdated
dsarno and others added 2 commits February 15, 2026 16:03
batch_execute NormalizeParameterKeys recursively converted all JSON
keys to camelCase, including nested value data like Unity serialized
property paths (m_PersistentCalls to mPersistentCalls). This broke
UnityEvent wiring and any tool receiving structured nested data through
batch_execute. Stop recursing into values -- only normalize top-level
parameter keys.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address Sourcery review: check value == null before accessing
value.Type to prevent NullReferenceException in the string case of
SetSerializedPropertyRecursive.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dsarno dsarno changed the base branch from main to beta February 16, 2026 00:21
@dsarno dsarno merged commit 18c3e03 into CoplayDev:beta Feb 16, 2026
2 checks passed
msanatan pushed a commit to msanatan/unity-mcp that referenced this pull request Feb 25, 2026
* Fix UnityEvent wiring via manage_components set_property (CoplayDev#631)

UnityEventBase-derived fields set via reflection create disconnected
objects that Unity's serialization layer doesn't track, causing
m_PersistentCalls to be empty on save. Route these types through
SerializedObject/SerializedProperty instead.

Adds recursive SerializedProperty setter supporting Generic structs,
arrays, ObjectReference, and leaf types (int, bool, float, string, enum).
Includes fuzzy child property matching to handle batch_execute stripping
underscores from nested JSON keys (tracked in CoplayDev#756).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix batch_execute mangling nested value keys (CoplayDev#756)

batch_execute NormalizeParameterKeys recursively converted all JSON
keys to camelCase, including nested value data like Unity serialized
property paths (m_PersistentCalls to mPersistentCalls). This broke
UnityEvent wiring and any tool receiving structured nested data through
batch_execute. Stop recursing into values -- only normalize top-level
parameter keys.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Guard against null value in SerializedProperty string setter

Address Sourcery review: check value == null before accessing
value.Type to prevent NullReferenceException in the string case of
SetSerializedPropertyRecursive.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
msanatan pushed a commit to msanatan/unity-mcp that referenced this pull request Feb 25, 2026
* Fix UnityEvent wiring via manage_components set_property (CoplayDev#631)

UnityEventBase-derived fields set via reflection create disconnected
objects that Unity's serialization layer doesn't track, causing
m_PersistentCalls to be empty on save. Route these types through
SerializedObject/SerializedProperty instead.

Adds recursive SerializedProperty setter supporting Generic structs,
arrays, ObjectReference, and leaf types (int, bool, float, string, enum).
Includes fuzzy child property matching to handle batch_execute stripping
underscores from nested JSON keys (tracked in CoplayDev#756).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix batch_execute mangling nested value keys (CoplayDev#756)

batch_execute NormalizeParameterKeys recursively converted all JSON
keys to camelCase, including nested value data like Unity serialized
property paths (m_PersistentCalls to mPersistentCalls). This broke
UnityEvent wiring and any tool receiving structured nested data through
batch_execute. Stop recursing into values -- only normalize top-level
parameter keys.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Guard against null value in SerializedProperty string setter

Address Sourcery review: check value == null before accessing
value.Type to prevent NullReferenceException in the string case of
SetSerializedPropertyRecursive.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
@dsarno dsarno deleted the fix/unity-event-serialized-property-631 branch April 6, 2026 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

batch_execute normalizes JSON value keys, stripping underscores from nested payloads Bug: UnityEvent Wiring Clears m_Calls Instead of Setting Them

1 participant