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 (
|
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?
…CoplayDev#414) * Update .Bat file and Bug fix on ManageScript * Update the .Bat file to include runtime folder * Fix the inconsistent EditorPrefs variable so the GUI change on Script Validation could cause real change. * Further changes String to Int for consistency * [Custom Tool] Roslyn Runtime Compilation Allows users to generate/compile codes during Playmode * Fix based on CR * Create claude_skill_unity.zip Upload the unity_claude_skill that can be uploaded to Claude for a combo of unity-mcp-skill. * Update for Custom_Tool Fix and Detection 1. Fix Original Roslyn Compilation Custom Tool to fit the V8 standard 2. Add a new panel in the GUI to see and toggle/untoggle the tools. The toggle feature will be implemented in the future, right now its implemented here to discuss with the team if this is a good feature to add; 3. Add few missing summary in certain tools * Revert "Update for Custom_Tool Fix and Detection" This reverts commit ae8cfe5. * Update README.md * Reapply "Update for Custom_Tool Fix and Detection" This reverts commit f423c2f. * Update ManageScript.cs Fix the layout problem of manage_script in the panel * Update To comply with the current server setting * Update on Batch Tested object generation/modification with batch and it works perfectly! We should push and let users test for a while and see PS: I tried both VS Copilot and Claude Desktop. Claude Desktop works but VS Copilot does not due to the nested structure of batch. Will look into it more. * Revert "Merge pull request #1 from Scriptwonder/batching" This reverts commit 55ee768, reversing changes made to ae2eedd.
…sults - manage_asset, manage_gameobject, manage_scene now check preflight return value and propagate busy/retry signals to clients (fixes Sourcery #1) - TestJobManager.FinalizeCurrentJobFromRunFinished now sets job status to Failed when resultPayload.Failed > 0, not always Succeeded (fixes Sourcery #2)
…ty tool (CoplayDev#507) * Add editor readiness v2, refresh tool, and preflight guards * Detect external package changes and harden refresh retry * feat: add TestRunnerNoThrottle and async test running with background stall prevention - Add TestRunnerNoThrottle.cs: Sets editor to 'No Throttling' mode during test runs with SessionState persistence across domain reload - Add run_tests_async and get_test_job tools for non-blocking test execution - Add TestJobManager for async test job tracking with progress monitoring - Add ForceSynchronousImport to all AssetDatabase.Refresh() calls to prevent stalls - Mark DomainReloadResilienceTests as [Explicit] with documentation explaining the test infrastructure limitation (internal coroutine waits vs MCP socket polling) - MCP workflow is unaffected - socket messages provide external stimulus that keeps Unity responsive even when backgrounded * refactor: simplify and clean up code - Remove unused Newtonsoft.Json.Linq import from TestJobManager - Add throttling to SessionState persistence (once per second) to reduce overhead - Critical job state changes (start/finish) still persist immediately - Fix duplicate XML summary tag in DomainReloadResilienceTests * docs: add async test tools to README, document domain reload limitation - Add run_tests_async and get_test_job to main README tools list - Document background stall limitation for domain reload tests in DEV readme * ci: add separate job for domain reload tests Run [Explicit] domain_reload tests in their own job using -testCategory * ci: run domain reload tests in same job as regular tests Combines into single job with two test steps to reuse cached Library * fix: address coderabbit review issues - Fix TOCTOU race in TestJobManager.StartJob (single lock scope for check-and-set) - Store TestRunnerApi reference with HideAndDontSave to prevent GC/serialization issues * docs: update tool descriptions to prefer run_tests_async - run_tests_async is now marked as preferred for long-running suites - run_tests description notes it blocks and suggests async alternative * docs: update README screenshot to v8.6 UI * docs: add v8.6 UI screenshot * Update README for MCP version and instructions for v8.7 * fix: handle preflight busy signals and derive job status from test results - manage_asset, manage_gameobject, manage_scene now check preflight return value and propagate busy/retry signals to clients (fixes Sourcery #1) - TestJobManager.FinalizeCurrentJobFromRunFinished now sets job status to Failed when resultPayload.Failed > 0, not always Succeeded (fixes Sourcery #2) * fix: increase HTTP server startup timeout for dev mode When 'Force fresh server install' is enabled, uvx uses --no-cache --refresh which rebuilds the package and takes significantly longer to start. - Increase timeout from 10s to 45s when dev mode is enabled - Add informative log message explaining the longer startup time - Show actual timeout value in warning message * fix: derive job status from test results in FinalizeFromTask fallback Apply same logic as FinalizeCurrentJobFromRunFinished: check result.Failed > 0 to correctly mark jobs as Failed when tests fail, even in the fallback path when RunFinished callback is not delivered.
* Add editor readiness v2, refresh tool, and preflight guards * Detect external package changes and harden refresh retry * feat: add TestRunnerNoThrottle and async test running with background stall prevention - Add TestRunnerNoThrottle.cs: Sets editor to 'No Throttling' mode during test runs with SessionState persistence across domain reload - Add run_tests_async and get_test_job tools for non-blocking test execution - Add TestJobManager for async test job tracking with progress monitoring - Add ForceSynchronousImport to all AssetDatabase.Refresh() calls to prevent stalls - Mark DomainReloadResilienceTests as [Explicit] with documentation explaining the test infrastructure limitation (internal coroutine waits vs MCP socket polling) - MCP workflow is unaffected - socket messages provide external stimulus that keeps Unity responsive even when backgrounded * refactor: simplify and clean up code - Remove unused Newtonsoft.Json.Linq import from TestJobManager - Add throttling to SessionState persistence (once per second) to reduce overhead - Critical job state changes (start/finish) still persist immediately - Fix duplicate XML summary tag in DomainReloadResilienceTests * docs: add async test tools to README, document domain reload limitation - Add run_tests_async and get_test_job to main README tools list - Document background stall limitation for domain reload tests in DEV readme * ci: add separate job for domain reload tests Run [Explicit] domain_reload tests in their own job using -testCategory * ci: run domain reload tests in same job as regular tests Combines into single job with two test steps to reuse cached Library * fix: address coderabbit review issues - Fix TOCTOU race in TestJobManager.StartJob (single lock scope for check-and-set) - Store TestRunnerApi reference with HideAndDontSave to prevent GC/serialization issues * docs: update tool descriptions to prefer run_tests_async - run_tests_async is now marked as preferred for long-running suites - run_tests description notes it blocks and suggests async alternative * docs: update README screenshot to v8.6 UI * docs: add v8.6 UI screenshot * docs: update v8.6 UI screenshot * docs: update v8.6 UI screenshot * docs: update v8.6 UI screenshot * Update README for MCP version and instructions for v8.7 * fix: handle preflight busy signals and derive job status from test results - manage_asset, manage_gameobject, manage_scene now check preflight return value and propagate busy/retry signals to clients (fixes Sourcery #1) - TestJobManager.FinalizeCurrentJobFromRunFinished now sets job status to Failed when resultPayload.Failed > 0, not always Succeeded (fixes Sourcery #2) * fix: increase HTTP server startup timeout for dev mode When 'Force fresh server install' is enabled, uvx uses --no-cache --refresh which rebuilds the package and takes significantly longer to start. - Increase timeout from 10s to 45s when dev mode is enabled - Add informative log message explaining the longer startup time - Show actual timeout value in warning message * fix: derive job status from test results in FinalizeFromTask fallback Apply same logic as FinalizeCurrentJobFromRunFinished: check result.Failed > 0 to correctly mark jobs as Failed when tests fail, even in the fallback path when RunFinished callback is not delivered. * Bound Unity reload/session waits * refactor: improve env var parsing and reason extraction Address code review feedback: - Catch ValueError specifically (instead of broad Exception) when parsing UNITY_MCP_RELOAD_MAX_WAIT_S, UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S, and UNITY_MCP_SESSION_READY_WAIT_SECONDS, with logging for easier diagnosis of misconfiguration - Normalize reason values to lowercase in _extract_response_reason() to avoid case-sensitive mismatches in comparisons - Simplify refresh_unity.py by removing redundant isinstance check and reusing _extract_response_reason instead of duplicating reason parsing * Add upper bound clamp and custom exception for bounded retry - Add upper bound (30s) to UNITY_MCP_RELOAD_MAX_WAIT_S to prevent misconfiguration from causing excessive waits - Add upper bound (30s) to UNITY_MCP_SESSION_RESOLVE_MAX_WAIT_S for consistency with readiness probe - Introduce NoUnitySessionError custom exception to replace fragile string matching in send_command_for_instance Addresses code review feedback for bounded retry policy PR.
Testing the PR
Summary by CodeRabbit