refactor: drop defensive scaffolding made obsolete by module dep declarations#1168
Merged
Scriptwonder merged 1 commit intoMay 26, 2026
Conversation
…arations (CoplayDev#1122) Since CoplayDev#1122 declared the Physics 2D, Screen Capture, Image Conversion, etc. built-in modules as required deps in package.json, Unity Package Manager now keeps them enabled while MCP for Unity is installed. Two defensive layers that protected against the now-impossible "module disabled" case become dead code, plus one sub-floor preprocessor gate that was always-true on our declared `unity: 2021.3` floor. Tier 1A — UnityPhysicsCompat.cs - Revert `Type.GetType("UnityEngine.Physics2D, UnityEngine.Physics2DModule")` to `typeof(Physics2D)`. The property-level reflection (for `autoSyncTransforms` deprecation in Unity 6.x) stays — that's a different concern. Tier 1B — TrailControl.cs - Strip `#if UNITY_2021_1_OR_NEWER` gate. Package floor is 2021.3 per package.json, so the `#else` branch ("AddPosition requires Unity 2021.1+") is unreachable. Tier 2 — ScreenshotUtility.cs + callers - Remove `IsScreenCaptureModuleAvailable` property, `ScreenCaptureModuleNotAvailableError` constant, `InvokeCaptureScreenshotAsTexture` helper, and the `s_captureScreenshotMethod` / `s_captureScreenshotAsTextureMethod` / `s_screenCaptureModuleAvailable` reflection caches. - Replace reflective `MethodInfo.Invoke` calls with direct `ScreenCapture.X` calls in `CaptureToProjectFolder`, `CaptureComposited`, and the `ScreenshotCapturer` MonoBehaviour. - Camera-based fallback path (`FindAvailableCamera` + `CaptureFromCameraToProjectFolder`) is preserved — it still handles transient null returns from `ScreenCapture` inside `CaptureComposited`. - Drop pre-flight `if (!IsScreenCaptureModuleAvailable)` gates in `ManageUI.cs` (UI render) and `ManageScene.cs` (screenshot path). - `ProjectInfo` resource: `screenCapture` field is now an invariant `true` (preserves API shape for `mcpforunity://project/info` consumers). Net: 144 lines removed, 15 added. No new tests — the changes are pure removal of code paths that cannot fire with the current deps declared. Related: CoplayDev#1160, CoplayDev#1122.
Contributor
📝 WalkthroughWalkthroughThis PR removes runtime Screen Capture module availability checks and reflection-based fallback paths throughout the screenshot pipeline. ScreenshotUtility is simplified to call Unity's direct ScreenCapture APIs, and dependent code in ManageScene, ManageUI, and ProjectInfo is updated accordingly. Minor version-gating removals in TrailControl and UnityPhysicsCompat follow the same simplification pattern. ChangesScreenshot capture pipeline and module availability consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes defensive “module disabled” scaffolding that became obsolete after #1122 declared the relevant Unity built-in modules (Physics2D / ScreenCapture / ImageConversion, etc.) as required dependencies in MCPForUnity/package.json. It simplifies Runtime and Editor code paths by replacing reflection-based module detection with direct API usage, while keeping the existing cross-version property-level reflection in UnityPhysicsCompat.
Changes:
- Runtime: revert Physics2D type resolution back to
typeof(Physics2D)and simplifyUnityPhysicsCompatdocs accordingly. - Runtime: remove ScreenCapture module availability checks/reflection caches and call
ScreenCapture.CaptureScreenshot*directly; preserve camera fallback only for the “ScreenCapture returned null” case. - Editor: remove unreachable Unity-version gate in
TrailControl, and remove preflight ScreenCapture-module checks in UI/scene tools; hardcodeProjectInfo.packages.screenCapture = true.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| MCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs | Drops module-disabled type lookup and uses typeof(Physics2D) while keeping property reflection for deprecation/removal resilience. |
| MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | Removes ScreenCapture module detection/reflection and uses direct ScreenCapture calls; keeps runtime null-return fallback to camera capture. |
| MCPForUnity/Editor/Tools/Vfx/TrailControl.cs | Removes an unreachable #if UNITY_2021_1_OR_NEWER branch given the package floor (2021.3). |
| MCPForUnity/Editor/Tools/ManageUI.cs | Removes now-obsolete preflight rejection based on ScreenCapture module availability. |
| MCPForUnity/Editor/Tools/ManageScene.cs | Removes ScreenCapture module availability branching; retains version-specific camera requirement behavior for older Unity versions. |
| MCPForUnity/Editor/Resources/Project/ProjectInfo.cs | Keeps API shape but makes screenCapture capability an invariant true. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/ManageScene.cs (1)
681-690: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove Unity version gating into
UnityCompatShimsLine 682 adds a call-site
#if UNITY_*branch inManageScene. Please centralize this inMCPForUnity/Runtime/Helpers/UnityCompatShims.csand call a shim method here.♻️ Proposed refactor
- // Default path: ScreenCapture API for 2022.1+, camera fallback required on older versions. -#if !UNITY_2022_1_OR_NEWER - bool hasCameraFallback = Camera.main != null || UnityFindObjectsCompat.FindAll<Camera>().Length > 0; - if (!hasCameraFallback) - { - return new ErrorResponse( - "No camera found in the scene. Screenshot capture on Unity versions before 2022.1 requires a Camera in the scene." - ); - } -#endif + // Default path: ScreenCapture API for 2022.1+, camera fallback required on older versions. + if (UnityCompatShims.RequiresCameraFallbackForScreenCapture()) + { + bool hasCameraFallback = Camera.main != null || UnityFindObjectsCompat.FindAll<Camera>().Length > 0; + if (!hasCameraFallback) + { + return new ErrorResponse( + "No camera found in the scene. Screenshot capture on Unity versions before 2022.1 requires a Camera in the scene." + ); + } + }// MCPForUnity/Runtime/Helpers/UnityCompatShims.cs public static bool RequiresCameraFallbackForScreenCapture() { `#if` !UNITY_2022_1_OR_NEWER return true; `#else` return false; `#endif` }As per coding guidelines:
MCPForUnity/**/*.cs: Unity API compatibility shims must be added toMCPForUnity/Runtime/Helpers/UnityCompatShims.csrather than using#if UNITY_*_OR_NEWERat call sites across the codebase.🤖 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/ManageScene.cs` around lines 681 - 690, Replace the call-site preprocessor check in ManageScene (around the camera-fallback logic) with a call to a new shim method: UnityCompatShims.RequiresCameraFallbackForScreenCapture(); implement that shim in MCPForUnity/Runtime/Helpers/UnityCompatShims.cs using the same `#if` !UNITY_2022_1_OR_NEWER / else compile-time branches to return true/false. In ManageScene.cs, call the shim and, if it returns true, perform the existing Camera.main / UnityFindObjectsCompat.FindAll<Camera>().Length check and return the same ErrorResponse when no camera is found; remove the local `#if` UNITY_* block so the version gating lives solely in UnityCompatShims.
🤖 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/ManageScene.cs`:
- Around line 681-690: Replace the call-site preprocessor check in ManageScene
(around the camera-fallback logic) with a call to a new shim method:
UnityCompatShims.RequiresCameraFallbackForScreenCapture(); implement that shim
in MCPForUnity/Runtime/Helpers/UnityCompatShims.cs using the same `#if`
!UNITY_2022_1_OR_NEWER / else compile-time branches to return true/false. In
ManageScene.cs, call the shim and, if it returns true, perform the existing
Camera.main / UnityFindObjectsCompat.FindAll<Camera>().Length check and return
the same ErrorResponse when no camera is found; remove the local `#if` UNITY_*
block so the version gating lives solely in UnityCompatShims.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 653d5d3e-b664-4ff0-ae31-ab6e7772a28d
📒 Files selected for processing (6)
MCPForUnity/Editor/Resources/Project/ProjectInfo.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Editor/Tools/Vfx/TrailControl.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs
💤 Files with no reviewable changes (2)
- MCPForUnity/Editor/Tools/Vfx/TrailControl.cs
- MCPForUnity/Editor/Tools/ManageUI.cs
Scriptwonder
added
safe-to-test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #1122 (declared the Physics 2D / Screen Capture / Image Conversion / etc. modules as required deps in
package.json). Now that Unity Package Manager keeps those modules enabled while MCP for Unity is installed, two defensive layers that protected against the now-impossible "module disabled" case are dead code. Plus one sub-floor#ifgate that was always-true on ourunity: 2021.3floor.Net: +15 / -144 across 6 files.
Premise check
What needs to stay constant for the cleanup to be safe (validated against the CI matrix in
tools/unity-versions.json: 2021.3.45f2 → 2022.3.62f1 → 6000.0.75f1 → 6000.4.8f1):typeof(Physics2D)UnityEnginefor ~15 yearsScreenCapture.CaptureScreenshot(string, int)ScreenCapture.CaptureScreenshotAsTexture(int)com.unity.modules.physics2dbuilt-incom.unity.modules.screencapturebuilt-inAll five span the entire supported matrix. The dep declarations from #1122 force the modules to be present, which makes
typeof(...)and directScreenCapture.Xcalls safe.What stays reflective (and should): the property lookups inside
UnityPhysicsCompat.cs(autoSyncTransforms,simulationMode) — those are for property-level deprecations in Unity 6.x, a different concern from module presence.Changes
Tier 1A —
UnityPhysicsCompat.csRevert
Type.GetType(\"UnityEngine.Physics2D, UnityEngine.Physics2DModule\")totypeof(Physics2D). Drop thePhysics2DTypeNameconst and the file-level doc paragraph claiming the file compiles when the module is disabled.Tier 1B —
TrailControl.csStrip the
#if UNITY_2021_1_OR_NEWER/#elsegate aroundtr.AddPosition(pos). Package floor is2021.3, so the#elsebranch ("AddPosition requires Unity 2021.1+") was unreachable.Tier 2 —
ScreenshotUtility.cs+ callersIsScreenCaptureModuleAvailablepublic property,ScreenCaptureModuleNotAvailableErrorconstant,InvokeCaptureScreenshotAsTexturehelper, and the three reflection caches (s_screenCaptureModuleAvailable,s_captureScreenshotMethod,s_captureScreenshotAsTextureMethod).MethodInfo.Invokecalls with directScreenCapture.CaptureScreenshot(...)/ScreenCapture.CaptureScreenshotAsTexture(...)inCaptureToProjectFolder,CaptureComposited, and theScreenshotCapturerMonoBehaviour.FindAvailableCamera+CaptureFromCameraToProjectFolder) — it still handles transientnullreturns fromScreenCaptureinsideCaptureComposited.if (!IsScreenCaptureModuleAvailable)gates inManageUI.cs(UI render flow) andManageScene.cs(screenshot flow). The 2022.1+/2021.3 split inManageScene.csis preserved — that's aboutScreenCapturenot behaving identically on the floor version, separate concern.ProjectInfo.cs(mcpforunity://project/inforesource):screenCapturefield is now hardcodedtrue. Preserves the resource shape for any consumer that reads it.Explicitly out of scope
CompilationPipeline.isCompilinglookups in 4 bridge/service files (originally my proposed "Tier 1C") are not touched here — there are open PRs in that area, so leaving for a separate cleanup pass.Test plan
full-matrixso the cleanup runs against 2021.3.45f2 + 2022.3.62f1 + 6000.0.75f1 + 6000.4.8f1.manage_camera action=screenshotstill produces a valid PNG (camera-based path +ScreenCapturepath).manage_ui action=render_uistill queues + returns the play-mode capture.Related
UnityPhysicsCompat.csis kept; only the module-detect reflection is reverted)Summary by CodeRabbit