Feat/optional physics screencapture modules#1208
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds compile-time module symbols for optional Unity Physics, Physics2D, and Screen Capture support, gates editor/runtime physics and capture code on those symbols, switches runtime physics type lookup to reflection, and updates tool docs to describe module-dependent behavior and errors. Optional Unity Module Compile Guards
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b1eb3bd69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
242-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify exception text for module-absent path.
When
MCP_HAS_SCREEN_CAPTUREis undefined, this branch can throw"ScreenCapture.CaptureScreenshotAsTexture returned null...", which is misleading because ScreenCapture was never called. Please make the message include the module-missing case explicitly to improve operator diagnostics.Proposed fix
- throw new InvalidOperationException("ScreenCapture.CaptureScreenshotAsTexture returned null and no fallback camera available."); + throw new InvalidOperationException( + "Cannot capture composited screenshot: Screen Capture module is unavailable (or capture returned null) and no fallback camera is available.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs` around lines 242 - 248, The exception message in the fallback block after FindAvailableCamera() is called is misleading when MCP_HAS_SCREEN_CAPTURE is undefined, as it references ScreenCapture.CaptureScreenshotAsTexture even though ScreenCapture was never invoked in that code path. Update the InvalidOperationException message to explicitly clarify that the error occurs when either the ScreenCapture module is not installed or no fallback camera is available, improving clarity for operator diagnostics. This will help distinguish between the cases where ScreenCapture was actually called versus when the module was not present.MCPForUnity/Editor/Tools/Physics/JointOps.cs (1)
71-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle explicit-dimension module absence before Rigidbody existence checks.
For explicit
dimensionrequests, Line 73/Line 79 can return “has no Rigidbody/Rigidbody2D” even when that module is not installed. This should return the module-missing error first for accurate remediation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/Physics/JointOps.cs` around lines 71 - 95, When the dimensionParam is explicitly set to "2d" or "3d", the code checks for Rigidbody existence before verifying that the required physics module is installed. For explicit dimension requests, add module availability checks before the existing Rigidbody existence checks in both the dimensionParam == "2d" branch and the dimensionParam == "3d" branch. If the required module is not installed, return a module-missing error instead of a Rigidbody-missing error, so users get accurate guidance on what to install first.MCPForUnity/Editor/Tools/Physics/PhysicsForceOps.cs (1)
39-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn module-missing errors before Rigidbody presence checks for explicit dimensions.
With
dimension: "2d"or"3d", Line 47 and Line 50 can return “add Rigidbody” even when that physics module is compiled out, which is impossible for users to fix. Please gate explicit dimension requests with module-availability checks first, then run component validation.💡 Suggested fix
- if (dimensionParam == "2d") - is2D = true; - else if (dimensionParam == "3d") - is2D = false; + if (dimensionParam == "2d") + { +#if !MCP_HAS_PHYSICS_2D + return new ErrorResponse("Physics 2D module (com.unity.modules.physics2d) is not installed."); +#endif + is2D = true; + } + else if (dimensionParam == "3d") + { +#if !MCP_HAS_PHYSICS + return new ErrorResponse("Physics module (com.unity.modules.physics) is not installed."); +#endif + is2D = false; + } else is2D = has2DRb && !has3DRb;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/Physics/PhysicsForceOps.cs` around lines 39 - 50, The code validates Rigidbody presence after determining the dimension, but this order causes misleading errors when explicit dimensions are requested for unavailable physics modules. When dimensionParam is explicitly "2d" or "3d", add module-availability checks before the Rigidbody validation logic at lines 47 and 50. Verify that the 2D physics module is compiled in when dimensionParam equals "2d", and verify the 3D physics module is compiled in when dimensionParam equals "3d", returning a module-unavailable error immediately if either check fails. Only after confirming the required module exists should the code proceed to validate that the appropriate Rigidbody component (has2DRb or has3DRb) is present.MCPForUnity/Editor/Tools/Physics/PhysicsRigidbodyOps.cs (1)
51-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid generic “No Rigidbody” fallback before module-specific routing in
GetRigidbody.The check at Line 51 can short-circuit to a component-missing message even when the requested dimension’s module is unavailable. Let the dimension branch return module-specific errors first (or move this check after module/dimension resolution).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/Physics/PhysicsRigidbodyOps.cs` around lines 51 - 75, In the GetRigidbody method, the early check for missing rigidbodies (when !has3D && !has2D) returns a generic error before the code determines which physics dimension/module is needed. Relocate this check to occur after the is2D logic is determined and after the conditional compilation checks for module availability, so that module-specific errors (MCP_HAS_PHYSICS_2D and MCP_HAS_PHYSICS) are returned first when appropriate, providing users with more helpful error messages. Only return the generic "No Rigidbody" error if the required module is actually available but the component is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Tools/ManageAsset.cs`:
- Around line 13-21: Move the Unity version-specific conditional compilation
logic from ManageAsset.cs into a compatibility shim file. Specifically, the `#if`
UNITY_6000_0_OR_NEWER preprocessor directives that define PhysicsMaterialType
and PhysicsMaterialCombine type aliases should be moved to
MCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs (or similar shim file). In
that shim file, define these type aliases with the appropriate conditional
branches for different Unity versions. Then in ManageAsset.cs, replace the
entire conditional block (lines 13-21) with a single using statement that
imports these type aliases from the compat shim, eliminating all
version-specific branching from the call site.
---
Outside diff comments:
In `@MCPForUnity/Editor/Tools/Physics/JointOps.cs`:
- Around line 71-95: When the dimensionParam is explicitly set to "2d" or "3d",
the code checks for Rigidbody existence before verifying that the required
physics module is installed. For explicit dimension requests, add module
availability checks before the existing Rigidbody existence checks in both the
dimensionParam == "2d" branch and the dimensionParam == "3d" branch. If the
required module is not installed, return a module-missing error instead of a
Rigidbody-missing error, so users get accurate guidance on what to install
first.
In `@MCPForUnity/Editor/Tools/Physics/PhysicsForceOps.cs`:
- Around line 39-50: The code validates Rigidbody presence after determining the
dimension, but this order causes misleading errors when explicit dimensions are
requested for unavailable physics modules. When dimensionParam is explicitly
"2d" or "3d", add module-availability checks before the Rigidbody validation
logic at lines 47 and 50. Verify that the 2D physics module is compiled in when
dimensionParam equals "2d", and verify the 3D physics module is compiled in when
dimensionParam equals "3d", returning a module-unavailable error immediately if
either check fails. Only after confirming the required module exists should the
code proceed to validate that the appropriate Rigidbody component (has2DRb or
has3DRb) is present.
In `@MCPForUnity/Editor/Tools/Physics/PhysicsRigidbodyOps.cs`:
- Around line 51-75: In the GetRigidbody method, the early check for missing
rigidbodies (when !has3D && !has2D) returns a generic error before the code
determines which physics dimension/module is needed. Relocate this check to
occur after the is2D logic is determined and after the conditional compilation
checks for module availability, so that module-specific errors
(MCP_HAS_PHYSICS_2D and MCP_HAS_PHYSICS) are returned first when appropriate,
providing users with more helpful error messages. Only return the generic "No
Rigidbody" error if the required module is actually available but the component
is missing.
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 242-248: The exception message in the fallback block after
FindAvailableCamera() is called is misleading when MCP_HAS_SCREEN_CAPTURE is
undefined, as it references ScreenCapture.CaptureScreenshotAsTexture even though
ScreenCapture was never invoked in that code path. Update the
InvalidOperationException message to explicitly clarify that the error occurs
when either the ScreenCapture module is not installed or no fallback camera is
available, improving clarity for operator diagnostics. This will help
distinguish between the cases where ScreenCapture was actually called versus
when the module was not present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4612f8a0-640e-44af-9530-b3cc40072327
📒 Files selected for processing (20)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Helpers/GameObjectSerializer.csMCPForUnity/Editor/MCPForUnity.Editor.asmdefMCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.csMCPForUnity/Editor/Tools/ManageAsset.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/Physics/CollisionMatrixOps.csMCPForUnity/Editor/Tools/Physics/JointOps.csMCPForUnity/Editor/Tools/Physics/ManagePhysics.csMCPForUnity/Editor/Tools/Physics/PhysicsForceOps.csMCPForUnity/Editor/Tools/Physics/PhysicsMaterialOps.csMCPForUnity/Editor/Tools/Physics/PhysicsQueryOps.csMCPForUnity/Editor/Tools/Physics/PhysicsRigidbodyOps.csMCPForUnity/Editor/Tools/Physics/PhysicsSettingsOps.csMCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.csMCPForUnity/Editor/Tools/Physics/PhysicsValidationOps.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Helpers/UnityPhysicsCompat.csMCPForUnity/Runtime/MCPForUnity.Runtime.asmdefMCPForUnity/package.json
💤 Files with no reviewable changes (1)
- MCPForUnity/package.json
| #if MCP_HAS_PHYSICS | ||
| #if UNITY_6000_0_OR_NEWER | ||
| using PhysicsMaterialType = UnityEngine.PhysicsMaterial; | ||
| using PhysicsMaterialCombine = UnityEngine.PhysicsMaterialCombine; | ||
| #else | ||
| using PhysicsMaterialType = UnityEngine.PhysicMaterial; | ||
| using PhysicsMaterialCombine = UnityEngine.PhysicMaterialCombine; | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move Unity version branching into a compat shim.
This #if UNITY_6000_0_OR_NEWER gate is in a tool call site. Please move the API-version split into MCPForUnity/Runtime/Helpers/Unity*Compat.cs and consume a single shim type/API from ManageAsset.
As per coding guidelines, "MCPForUnity/**/*.cs: Unity API version compatibility issues must use shims in MCPForUnity/Runtime/Helpers/Unity*Compat.cs rather than sprinkling #if UNITY_x_y_OR_NEWER at call sites."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MCPForUnity/Editor/Tools/ManageAsset.cs` around lines 13 - 21, Move the Unity
version-specific conditional compilation logic from ManageAsset.cs into a
compatibility shim file. Specifically, the `#if` UNITY_6000_0_OR_NEWER
preprocessor directives that define PhysicsMaterialType and
PhysicsMaterialCombine type aliases should be moved to
MCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs (or similar shim file). In
that shim file, define these type aliases with the appropriate conditional
branches for different Unity versions. Then in ManageAsset.cs, replace the
entire conditional block (lines 13-21) with a single using statement that
imports these type aliases from the compat shim, eliminating all
version-specific branching from the call site.
Source: Coding guidelines
LuuOW
left a comment
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: code patterns and architectural layout verified for system reliability.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Tools/ManageUI.cs (1)
944-950:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset pending-capture state when Begin fails synchronously.
At Line 944-Line 950,
s_pendingCaptureStartedis set beforeScreenshotCapturer.Begin(...). IfBeginthrows before callback registration/completion, the flag staystrueand all later calls are blocked as “capture_in_progress”.Suggested fix
s_pendingCaptureDone = false; s_pendingCaptureTex = null; s_pendingCaptureStarted = true; - ScreenshotCapturer.Begin(1, tex => - { - s_pendingCaptureTex = tex; - s_pendingCaptureDone = true; - s_pendingCaptureStarted = false; - }); + try + { + ScreenshotCapturer.Begin(1, tex => + { + s_pendingCaptureTex = tex; + s_pendingCaptureDone = true; + s_pendingCaptureStarted = false; + }); + } + catch + { + s_pendingCaptureStarted = false; + throw; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/ManageUI.cs` around lines 944 - 950, The flag s_pendingCaptureStarted is set to true before calling ScreenshotCapturer.Begin(), but if Begin() throws a synchronous exception, the flag remains true and blocks all subsequent capture attempts. Wrap the ScreenshotCapturer.Begin() call in a try-catch block to reset s_pendingCaptureStarted back to false in the catch handler, ensuring the pending capture state is properly cleaned up if an exception occurs during the Begin call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 944-950: The flag s_pendingCaptureStarted is set to true before
calling ScreenshotCapturer.Begin(), but if Begin() throws a synchronous
exception, the flag remains true and blocks all subsequent capture attempts.
Wrap the ScreenshotCapturer.Begin() call in a try-catch block to reset
s_pendingCaptureStarted back to false in the catch handler, ensuring the pending
capture state is properly cleaned up if an exception occurs during the Begin
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d80c1f43-9ec7-41bd-b63b-0480ecbd06a0
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
|
Addressed the review + documented the optional-module behavior. Screen Capture fix (0878bba): play-mode Docs (d30bbc4): tool descriptions now state the requirements — Both modules stay optional — the package compiles/runs with any subset and degrades gracefully. |
jordandunmire97-ai
left a comment
jordandunmire97-ai
left a comment
There was a problem hiding this comment.
Please understand I am reading it and the very much beautiful alternatives . Please understand I know it needs to be reviewed i am just approving for myself as I can barely type this .
…cs-screencapture-modules
3 participants
Description
9.7.1 made MCP for Unity hard-depend on three built-in modules (Physics, Physics 2D,
Screen Capture):
UnityPhysicsCompat.csusestypeof(Physics)/typeof(Physics2D)andseveral editor tools reference Physics/Physics2D/ScreenCapture types directly, so the
package fails to compile when any of those modules is disabled (#1172). They were also
added as hard
package.jsondependencies, which force-pulls them into every consumingproject — undesirable for an editor-focused package. This PR makes all three modules
optional via
versionDefines(the approach proposed in the issue) and removes the forceddependencies.
Type of Change
Changes Made
versionDefinestoMCPForUnity.Editor.asmdefandMCPForUnity.Runtime.asmdef(mirroring the existing
UNITY_VFX_GRAPH):MCP_HAS_PHYSICS,MCP_HAS_PHYSICS_2D,MCP_HAS_SCREEN_CAPTURE, resolved fromcom.unity.modules.physics,com.unity.modules.physics2d,com.unity.modules.screencapture.Runtime/Helpers/UnityPhysicsCompat.cs: resolvePhysics/Physics2Dvia reflection(
Type.GetType(...)) instead oftypeof, so the shim compiles with the modules absent(no
#if; consistent with the file's existing reflection design).Runtime/Helpers/ScreenshotUtility.cs: guarded allScreenCapturecall sites behindMCP_HAS_SCREEN_CAPTURE, falling back to camera-based capture when absent.Editor/Tools/Physics/*(incl.ManagePhysicsreturning a clear "module not installed"message), plus incidental usages in
ManageScene,ManageAsset,Helpers/ComponentOps,Helpers/GameObjectSerializer,Tools/GameObjects/GameObjectComponentHelpers.3D and 2D physics are handled independently.
com.unity.modules.physics,com.unity.modules.physics2dandcom.unity.modules.screencapturefrompackage.jsondependenciesso they are nolonger force-pulled. (
imageconversion,uielements,unitywebrequest,animationare genuinely used and kept.)
Compatibility / Package Source
TestProjects/UnityMCPTestsin all four modulecombinations (both / none / 3D-only / 2D-only): 0 compile errors, both package
assemblies build in every case.
screencapture absent): the package no longer pulls the modules in and compiles cleanly.
file:(local checkout of this branch). Reviewers can also point athttps://github.com/CoplayDev/unity-mcp.git?path=/MCPForUnity#feat/optional-physics-screencapture-modules.Packages/packages-lock.json(if using a Git package URL):6b1eb3bd6906d480aeca36353a8b20e3a7309fe3Testing/Screenshots/Recordings
cd Server && uv run pytest tests/ -v)Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)Related Issues
Fixes #1172
Additional Notes
UnityPhysicsCompatreflection resolves to the same types, somanage_physicsandscreenshots behave exactly as before. Graceful degradation only kicks in when a module
is actually disabled.
touched.
configuration), and correctness with modules absent is covered by the compile matrix
above. CI will exercise the full EditMode suite.
ProfilerCategory.Physics/Physics2D(Profiling module) and a"Rigidbody"string-literal check inManageScript.Summary by CodeRabbit
Release Notes
Chores / Improvements
Bug Fixes
dimension: "2d"when 3D physics isn’t installed.