Fix UnityEvent wiring via manage_components set_property#757
Conversation
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Reviewer's GuideRoutes 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 SerializedPropertysequenceDiagram
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
Updated class diagram for ComponentOps UnityEvent handlingclassDiagram
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
Flow diagram for SetSerializedPropertyRecursive logicgraph 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
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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>
* 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>
* 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>
Summary
Closes #631
Closes #756
Test plan
🤖 Generated with Claude Code