Skip to content

Conversation

@brunomikoski
Copy link
Owner

No description provided.

@brunomikoski brunomikoski requested a review from Copilot May 24, 2025 11:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for setting generic and managed-reference values on SerializedProperty by using a reflective setter for paths not covered by built-in property types.

  • Introduced SetValueReflective to walk a property path, handle arrays/lists, and set field values via reflection
  • Extended the original SetValue to handle SerializedPropertyType.ManagedReference and SerializedPropertyType.Generic
  • Ensures changes are applied via Update/ApplyModifiedProperties in the reflective path
Comments suppressed due to low confidence (2)

Scripts/Editor/Extensions/SerializedPropertyExtensions.cs:32

  • [nitpick] The parameter name prop is inconsistent with other extension methods that use serializedProperty. Consider renaming it for consistency (e.g., serializedProperty).
public static void SetValueReflective(this SerializedProperty prop, object value)

Scripts/Editor/Extensions/SerializedPropertyExtensions.cs:34

  • The new reflective setter logic covers nested arrays and fields but lacks dedicated unit tests. Consider adding tests for common and edge-case property paths.
object root = prop.serializedObject.targetObject;

Comment on lines +36 to +38
string[] parts = prop.propertyPath
.Replace(".Array.data[", "[")
.Split('.');
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

Parsing the propertyPath using hard-coded replace/split logic can be brittle. Consider extracting the path parsing into a helper or adding comments to clarify the expected formats.

Suggested change
string[] parts = prop.propertyPath
.Replace(".Array.data[", "[")
.Split('.');
string[] parts = ParsePropertyPath(prop.propertyPath);

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +76
prop.serializedObject.Update();
prop.serializedObject.ApplyModifiedProperties();
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

[nitpick] Calling Update() and ApplyModifiedProperties() inside the reflective setter may lead to repeated serialization passes when setting multiple properties. It might be more efficient to push these calls to the caller.

Suggested change
prop.serializedObject.Update();
prop.serializedObject.ApplyModifiedProperties();
// Note: The caller is responsible for calling `Update()` and `ApplyModifiedProperties()`
// on the `SerializedObject` to apply changes and update the serialized state.

Copilot uses AI. Check for mistakes.
}
public static void SetValue(this SerializedProperty serializedProperty, object value)

Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

Public extension methods should have XML doc comments. Please add a summary and parameter descriptions for SetValueReflective.

Suggested change
/// <summary>
/// Sets the value of a serialized property reflectively by traversing its property path.
/// </summary>
/// <param name="prop">The serialized property whose value is to be set.</param>
/// <param name="value">The new value to assign to the property.</param>

Copilot uses AI. Check for mistakes.
@brunomikoski brunomikoski merged commit f6a6c70 into master May 24, 2025
1 check passed
@brunomikoski brunomikoski deleted the feature/add-support-for-reflection-value-assign-for-generic branch May 24, 2025 11:46
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.

2 participants