Feature/generalize getcomponentdata #1
Conversation
WalkthroughThis pull request updates the serialization and command processing logic across several modules. In the UnityMcpBridge C# files, an optional parameter ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CmdHandler as ProcessCommands
participant Serializer as JSON Serializer
Client->>CmdHandler: Send JSON command
CmdHandler->>Serializer: Deserialize command JSON
CmdHandler->>CmdHandler: Check if command is null
alt Command is null
CmdHandler->>CmdHandler: Create error message
CmdHandler->>Client: Return error response
else Command is valid
CmdHandler->>...: Process command normally
end
sequenceDiagram
participant UserCmd as Caller
participant Handler as HandleCommand
participant CompGetter as GetComponentsFromTarget
participant CompData as GetComponentData
participant Cache as Metadata Cache
participant Serializer as AddSerializableValue
UserCmd->>Handler: Invoke command with includeNonPublicSerialized flag
Handler->>CompGetter: Request component data (passes flag)
CompGetter->>CompData: Retrieve component data with flag
alt Metadata available in cache?
CompData->>Cache: Retrieve cached metadata
else
CompData->>Cache: Store new reflection metadata
end
CompData->>Serializer: Serialize properties/fields
Serializer-->>CompData: Return serialized value
CompData-->>CompGetter: Return complete component data
CompGetter-->>Handler: Return data to caller
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (5)
25-28: Remove or gate commented-out debug logs.
These lines are currently commented out. Consider removing them or converting them into a conditional/log-level-based debug mechanism to reduce clutter.- // --- DEBUG --- Log the raw parameter value --- - // JToken rawIncludeFlag = @params["includeNonPublicSerialized"]; - // Debug.Log($"[HandleCommand Debug] Raw includeNonPublicSerialized parameter: Type={rawIncludeFlag?.Type.ToString() ?? "Null"}, Value={rawIncludeFlag?.ToString() ?? "N/A"}"); - // --- END DEBUG ---
46-50: Clean up commented-out parameter logic.
Several lines for toggling howincludeNonPublicSerializedis handled remain commented out. If the logic is no longer needed, removing them will improve readability.- // Reverting to original logic, assuming external system will be fixed... - bool includeNonPublicSerialized = @params["includeNonPublicSerialized"]?.ToObject<bool>() ?? true; // Default - // Revised: Explicitly check for null, default to false if null/missing. -- REMOVED - // bool includeNonPublicSerialized = @params["includeNonPublicSerialized"] != null && ...
2210-2230: Introducing reflection metadata caching.
The newCachedMetadataclass and_metadataCachedictionary greatly optimize repeated reflection. However, if any multi-threaded usage is foreseen, consider adding locking or concurrency safeguards around_metadataCache.
2230-2382: Hierarchical reflection inGetComponentData.
Traversing the inheritance chain to gather properties and fields is a robust approach. It neatly avoids repeated reflection logic at each level. Keep an eye on performance for large hierarchies, and consider a maximum depth or explicit skip logic if future expansions risk complexity.
2382-2475: Serialization approach inAddSerializableValue.
Rich handling of primitives, Unity structs, andUnityEngine.Objectreferences ensures thorough coverage. Consider splitting overly complex conditions (e.g. for Materials) into helper methods to keep code maintainable and reduce branching complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
UnityMcpServer/src/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs(11 hunks)UnityMcpBridge/Editor/UnityMcpBridge.cs(1 hunks)UnityMcpServer/src/tools/manage_gameobject.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
Type(1116-1151)UnityMcpBridge/Editor/Helpers/Vector3Helper.cs (1)
Vector3(17-22)UnityMcpBridge/Editor/Windows/UnityMcpEditorWindow.cs (1)
Color(40-53)
🔇 Additional comments (8)
UnityMcpBridge/Editor/UnityMcpBridge.cs (1)
270-280: Good addition for null command handling.
This new block properly handles the scenario where a valid JSON string deserializes to a null command, providing a clear error response. This significantly improves robustness and debugging clarity.UnityMcpServer/src/tools/manage_gameobject.py (3)
38-38: Parameter added for private serialized fields control.
AddingincludeNonPublicSerializedas an optional boolean parameter is a solid improvement, enabling more flexible serialization of private[SerializeField]fields.
63-63: Docstring updated for new parameter.
The additional docstring clarifies behavior and usage ofincludeNonPublicSerialized, ensuring maintainability and discoverability.
96-97: Inclusion of new parameter in the command dictionary.
Correctly addingincludeNonPublicSerializedto the parameters ensures the Python tool mirrors the Unity side, maintaining consistent behavior across systems.UnityMcpBridge/Editor/Tools/ManageGameObject.cs (4)
5-5: New import aligns with usage of Json classes.
ImportingNewtonsoft.Jsonis consistent with the rest of the file’s approach to JSON handling.
142-142: Parameter forwarding to GetComponentsFromTarget.
ForwardingincludeNonPublicSerializedensures that the serialization logic downstream is consistent with the user’s request.
882-882: Enhanced signature for component retrieval.
Introducing thebool includeNonPublicSerializedparameter inGetComponentsFromTargetis aligned with the broader goal of selectively including non-public serialized fields.
895-896: Passing the flag to GetComponentData.
This continues the pipeline for handling non-public serialized fields. The consistency between parameter reading and usage is well maintained.
…ializer helper Moved serialization logic (GetGameObjectData, GetComponentData, metadata caching, JSON conversion helpers) from ManageGameObject tool to a dedicated GameObjectSerializer class in the Helpers namespace. This improves separation of concerns and reduces the size/complexity of ManageGameObject.cs. Updated ManageGameObject to use the new helper class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
26-29: Optional removal of debug logs in production.Debug statements for
includeNonPublicSerializedare commented out, which is acceptable for now. Consider removing these stubs if they are no longer necessary to avoid clutter in future maintenance.
1424-1441: Custom input serializer for property assignment.Defining a separate serializer (with relevant converters) specifically for reading incoming property values is a practical design choice. This approach provides a clearer distinction between read- vs. write-serialization logic and helps avoid confusion with your output-oriented serializer. Minor performance overhead from re-instantiating
JsonSerializerSettingseach time is acceptable, though you could store it statically if needed.
1605-1643: Flexible handling of material shader properties.Your approach to interpret arrays for color or vector sets is creative. However, note that ambiguous array lengths (e.g., 2 vs. 3 vs. 4 elements) could lead to partial misinterpretations (e.g., a 4-element array might be incorrectly used). Consider logging more detail if multiple conversions fail, so users know which fallback was triggered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs(1 hunks)UnityMcpBridge/Editor/Tools/ManageGameObject.cs(19 hunks)
🔇 Additional comments (9)
UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs (5)
21-99: Commendable thoroughness inGetGameObjectData.This method offers a detailed snapshot of the
GameObjectstate, carefully handling transform details to avoid reference loops. The explicit approach for serializing position, rotation, etc., is verbose but ensures clear, cycle-free data, suitable for external consumers. The optional component serialization is also well-structured and commented out to reduce overhead. No immediate corrections are needed.
101-115: Efficient reflection metadata caching strategy.By storing computed properties/fields in
_metadataCachekeyed by(Type, bool), you reduce reflection overhead significantly. This is especially beneficial for large-scale and repeated serialization tasks. Implementation appears robust; no concerns here.
117-195: Solid reflection-based serialization inGetComponentData.Your logic to traverse inheritance hierarchy up to
MonoBehaviouris appropriate for most Unity components. Filtering out backing fields, skipping unwanted property names, and conditionally including non-public serialized fields help maintain a safe and consistent representation. One small caution: for extremely large or complex components, reflection might still be slow. However, your caching and property-skipping steps minimize this risk well.
252-335: Prudent JSON token handling and fallback conversions.The
AddSerializableValuepipeline ensures that problematic fields/properties are gracefully skipped. ConvertingJTokenback to plain objects inConvertJTokenToPlainObjectis a neat approach for returning raw serializable data. The mix of logging warnings and ignoring reference loops helps minimize unexpected breaks. Great error resilience here.
337-375: Robust custom serializer configuration.You have nicely consolidated Unity-specific converters (Vector3Converter, ColorConverter, etc.) into
_outputSerializerSettings. The try/catch inCreateTokenFromValuehandles serialization failures gracefully. Overall, this design is solid for Editor usage where reflection-based approaches are common.UnityMcpBridge/Editor/Tools/ManageGameObject.cs (4)
47-49: Good addition ofincludeNonPublicSerializedparameter.Defaulting to
truemaintains backward compatibility and ensures feature parity with prior reflection usage. Keep in mind that this allows external requests to read private fields (if[SerializeField]is present), so confirm that no sensitive data is exposed inadvertently.
139-140: Passing the new flag downstream is consistent.Forwarding
includeNonPublicSerializedtoGetComponentsFromTargetensures correct usage throughout the pipeline. The naming is self-explanatory, positively impacting maintainability. No issues spotted.
532-533: Neat usage ofGameObjectSerializer.GetGameObjectData.Returning serialized object data in success responses dramatically improves debugging and remote inspection. This centralizes serialization logic in the helper class and keeps this method clean.
1982-2025: Comprehensive type resolution logic.The fallback approach scanning assemblies is thorough for a reflection-heavy editor utility. The incremental approach—checking known assemblies first, then searching the rest—should mitigate performance hits. Logging warnings on ambiguities is also appreciated. Well done.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (5)
36-56: Similar concern about missing properties inVector2Converter.
58-84: Similar concern about missing properties inQuaternionConverter.
86-112: Similar concern about missing properties inColorConverter.
114-140: Similar concern about missing properties inRectConverter.
142-161: Similar concern about missing properties inBoundsConverter.
🧹 Nitpick comments (1)
UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (1)
11-34: Strengthen error handling for incomplete or malformed JSON.In
Vector3Converter.ReadJson,(float)jo["x"]may throw if the tokens are missing or of an unexpected type. Consider verifying the presence of these properties or falling back to default values when tokens are missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
UnityMcpBridge/Editor/UnityMcpBridge.Editor.asmdef(1 hunks)UnityMcpBridge/Editor/UnityMcpBridge.Editor.asmdef.meta(1 hunks)UnityMcpBridge/Runtime.meta(1 hunks)UnityMcpBridge/Runtime/Serialization.meta(1 hunks)UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs(1 hunks)UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs.meta(1 hunks)UnityMcpBridge/Runtime/UnityMcpBridge.Runtime.asmdef(1 hunks)UnityMcpBridge/Runtime/UnityMcpBridge.Runtime.asmdef.meta(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- UnityMcpBridge/Runtime/UnityMcpBridge.Runtime.asmdef.meta
- UnityMcpBridge/Runtime/UnityMcpBridge.Runtime.asmdef
- UnityMcpBridge/Runtime/Serialization.meta
- UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs.meta
- UnityMcpBridge/Editor/UnityMcpBridge.Editor.asmdef
- UnityMcpBridge/Editor/UnityMcpBridge.Editor.asmdef.meta
- UnityMcpBridge/Runtime.meta
🔇 Additional comments (2)
UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (2)
1-10: No issues found in the initial setup.The using statements and namespace declaration appear correct, and there are no obvious concerns here.
163-266: Assess potential security implications of loading assets via user-supplied paths.If portions of the JSON come from untrusted sources, malicious paths could be supplied. Consider validating or sanitizing asset paths before calling
LoadAssetAtPathto reduce any security risk.Would you please confirm whether the JSON input is fully controlled? If external, do you need a safeguard to restrict possible paths?
1 participant
Testing the PR
Summary by CodeRabbit