Fix on Issue #465 #477
Fix on Issue #465 #477
Conversation
* Update the .Bat file to include runtime folder * Fix the inconsistent EditorPrefs variable so the GUI change on Script Validation could cause real change.
String to Int for consistency
Allows users to generate/compile codes during Playmode
Upload the unity_claude_skill that can be uploaded to Claude for a combo of unity-mcp-skill.
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
This reverts commit ae8cfe5.
This reverts commit f423c2f.
Merge and use the previous PR's solution to solve the windows issue.
Fix the layout problem of manage_script in the panel
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.
Update on Batch
* Enable Camera Capture through both play and editor mode Notes: Because the standard ScreenCapture.CaptureScreenshot does not work in editor mode, so we use ScreenCapture.CaptureScreenshotIntoRenderTexture to enable it during play mode. * user can access the camera access through the tool menu, or through direct LLM calling. Both tested on Windows with Claude Desktop.
nitpicking changes
Similar to deploy.bat, but sideload it to MCP For Unity for easier deployment inside Unity menu.
Add a browse for server and make the source input more than readonly
Fix issue in issue#465
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts screenshot capture in the runtime helper so that ScreenCapture writes into the Assets folder (using the normalized full path) instead of just the filename, and performs some minor namespace formatting cleanup. Sequence diagram for updated CaptureToAssetsFolder screenshot write pathsequenceDiagram
actor Developer
participant ScreenshotUtility
participant Path
participant FileSystem
participant ScreenCapture
Developer->>ScreenshotUtility: CaptureToAssetsFolder(fileName, size)
activate ScreenshotUtility
ScreenshotUtility->>Path: GetFullPath(fileName)
Path-->>ScreenshotUtility: normalizedFullPath
ScreenshotUtility->>ScreenshotUtility: GetProjectRootPath()
ScreenshotUtility-->>ScreenshotUtility: projectRoot
ScreenshotUtility->>ScreenshotUtility: assetsRelativePath = normalizedFullPath
alt unity_2022_1_or_newer
ScreenshotUtility->>ScreenCapture: CaptureScreenshot(assetsRelativePath, size)
else older_unity
ScreenshotUtility->>ScreenshotUtility: CaptureFromCameraToAssetsFolder(Camera.main, fileName, size, false)
end
ScreenshotUtility->>ScreenshotUtility: strip projectRoot from assetsRelativePath
ScreenshotUtility->>FileSystem: ensure path under Assets folder
FileSystem-->>ScreenshotUtility: write_result
ScreenshotUtility-->>Developer: ScreenshotCaptureResult
deactivate ScreenshotUtility
Class diagram for ScreenshotUtility runtime helper changesclassDiagram
namespace MCPForUnity_Runtime_Helpers {
class ScreenshotCaptureResult {
+bool success
+string filePath
+string errorMessage
}
class ScreenshotUtility {
<<static>> ScreenshotCaptureResult CaptureToAssetsFolder(string fileName, int size)
<<static>> string GetProjectRootPath()
<<static>> ScreenshotCaptureResult CaptureFromCameraToAssetsFolder(Camera camera, string fileName, int size, bool logWarnings)
}
}
ScreenshotUtility ..> ScreenshotCaptureResult : returns
ScreenshotUtility ..> Camera : uses
ScreenshotUtility ..> ScreenCapture : uses
ScreenshotUtility ..> Debug : uses
ScreenshotUtility ..> Path : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughPre-computes a project-relative Assets path and uses it when calling ScreenCapture.CaptureScreenshot for UNITY_2022_1_OR_NEWER, moving path normalization earlier and removing duplicated path logic in the screenshot helper. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
assetsRelativePathpassed toScreenCapture.CaptureScreenshotis still the full normalized path at that point, which may not match Unity’s expected path semantics (typically relative to the project root); consider ensuring the value you pass toCaptureScreenshotis the same relative path you compute later after strippingprojectRoot. - The
projectRootandassetsRelativePathvariables are now defined before the#if UNITY_2022_1_OR_NEWERblock but only used afterward; you could simplify the flow and avoid confusion by computing the final relativeassetsRelativePathonce and reusing it both forCaptureScreenshotand for the return value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `assetsRelativePath` passed to `ScreenCapture.CaptureScreenshot` is still the full normalized path at that point, which may not match Unity’s expected path semantics (typically relative to the project root); consider ensuring the value you pass to `CaptureScreenshot` is the same relative path you compute later after stripping `projectRoot`.
- The `projectRoot` and `assetsRelativePath` variables are now defined before the `#if UNITY_2022_1_OR_NEWER` block but only used afterward; you could simplify the flow and avoid confusion by computing the final relative `assetsRelativePath` once and reusing it both for `CaptureScreenshot` and for the return value.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs:51-57` </location>
<code_context>
#if UNITY_2022_1_OR_NEWER
- ScreenCapture.CaptureScreenshot(captureName, size);
+ ScreenCapture.CaptureScreenshot(assetsRelativePath, size);
#else
Debug.LogWarning("ScreenCapture is supported after Unity 2022.1. Using main camera capture as fallback.");
CaptureFromCameraToAssetsFolder(Camera.main, captureName, size, false);
#endif
- string projectRoot = GetProjectRootPath();
- string assetsRelativePath = normalizedFullPath;
if (assetsRelativePath.StartsWith(projectRoot, StringComparison.OrdinalIgnoreCase))
{
assetsRelativePath = assetsRelativePath.Substring(projectRoot.Length).TrimStart('/');
</code_context>
<issue_to_address>
**issue (bug_risk):** CaptureScreenshot uses the absolute path before it is converted to a project-relative assets path.
`assetsRelativePath` is still an absolute `normalizedFullPath` when passed to `ScreenCapture.CaptureScreenshot`, and only afterward is `projectRoot` stripped off. If this API expects a project-relative path under Assets, we should compute the relative path before the call. Otherwise, if an absolute path is intended, we should rename the variable and update the comment so it doesn’t imply a Unity-style relative Assets path while actually using a full filesystem path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
48-48: Remove unnecessary blank line.Extra blank line at line 48 should be removed for consistency.
🔎 Proposed fix
string projectRoot = GetProjectRootPath(); string assetsRelativePath = normalizedFullPath; -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (1)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
CaptureScreenshot(349-396)
| // Use Asset folder for ScreenCapture.CaptureScreenshot to ensure write to asset rather than project root | ||
| string projectRoot = GetProjectRootPath(); | ||
| string assetsRelativePath = normalizedFullPath; | ||
|
|
There was a problem hiding this comment.
Variable naming is misleading: assetsRelativePath contains an absolute path when passed to ScreenCapture.CaptureScreenshot.
Unity's ScreenCapture.CaptureScreenshot API expects a filepath relative to the project directory in the Editor, but the code passes normalizedFullPath (an absolute path) before converting it to relative at lines 57-60. Rename the variable to reflect its actual content, and move the path normalization logic before the screenshot call:
- // Use Asset folder for ScreenCapture.CaptureScreenshot to ensure write to asset rather than project root
string projectRoot = GetProjectRootPath();
- string assetsRelativePath = normalizedFullPath;
-
+ string assetsRelativePath = normalizedFullPath;
+ if (assetsRelativePath.StartsWith(projectRoot, StringComparison.OrdinalIgnoreCase))
+ {
+ assetsRelativePath = assetsRelativePath.Substring(projectRoot.Length).TrimStart('/');
+ }
#if UNITY_2022_1_OR_NEWER
ScreenCapture.CaptureScreenshot(assetsRelativePath, size);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use Asset folder for ScreenCapture.CaptureScreenshot to ensure write to asset rather than project root | |
| string projectRoot = GetProjectRootPath(); | |
| string assetsRelativePath = normalizedFullPath; | |
| string projectRoot = GetProjectRootPath(); | |
| string assetsRelativePath = normalizedFullPath; | |
| if (assetsRelativePath.StartsWith(projectRoot, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| assetsRelativePath = assetsRelativePath.Substring(projectRoot.Length).TrimStart('/'); | |
| } |
🤖 Prompt for AI Agents
In MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs around lines 45-48, the
variable named assetsRelativePath actually holds an absolute path and is passed
to ScreenCapture.CaptureScreenshot; rename it to something like assetsFullPath
(or absoluteAssetsPath) to reflect its content, move the path normalization
logic so normalizedFullPath (absolute path) is computed before the
CaptureScreenshot call, then convert that normalized absolute path to a
project-relative path immediately before calling CaptureScreenshot (or pass the
correctly relative path there) so the API receives a true project-relative
filepath.
| ScreenCapture.CaptureScreenshot(assetsRelativePath, size); | ||
| #else | ||
| Debug.LogWarning("ScreenCapture is supported after Unity 2022.1. Using main camera capture as fallback."); | ||
| CaptureFromCameraToAssetsFolder(Camera.main, captureName, size, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 10 "public static ScreenshotCaptureResult CaptureFromCameraToAssetsFolder"Repository: CoplayDev/unity-mcp
Length of output: 1181
🏁 Script executed:
rg -n -A 15 "private static string BuildFileName"Repository: CoplayDev/unity-mcp
Length of output: 1505
🏁 Script executed:
rg -n -A 20 "private static string SanitizeFileName"Repository: CoplayDev/unity-mcp
Length of output: 1938
🏁 Script executed:
cat -n MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | sed -n '30,80p'Repository: CoplayDev/unity-mcp
Length of output: 2644
Fallback uses filename-only parameter, while 2022.1+ uses full path—verify consistent output location.
At line 43, captureName is extracted as filename-only via Path.GetFileName(), while line 47 stores the full assetsRelativePath. The 2022.1+ path (line 51) passes the full path to ScreenCapture.CaptureScreenshot, but the pre-2022.1 fallback (line 54) passes only the filename to CaptureFromCameraToAssetsFolder.
Since CaptureFromCameraToAssetsFolder hardcodes the target folder to Path.Combine(Application.dataPath, ScreenshotsFolderName) and expects a filename-only input (line 80), the current approach appears correct. However, document why the two paths have different semantics to prevent future confusion about which parameter format is expected.
🤖 Prompt for AI Agents
In MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs around lines 43 to 54,
document the differing parameter semantics: add a concise comment explaining
that Unity 2022.1+ ScreenCapture.CaptureScreenshot is passed the full
assets-relative path (assetsRelativePath) while the pre-2022.1 fallback calls
CaptureFromCameraToAssetsFolder with a filename-only (captureName) because that
helper writes to Application.dataPath/ScreenshotsFolderName and expects only a
filename; also clarify in the CaptureFromCameraToAssetsFolder method comment
that it hardcodes the target folder and requires a filename-only argument so
future maintainers understand why two different values are used.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (2)
47-51: Consider logging when relative path conversion is skipped.If
normalizedFullPathdoesn't start withprojectRoot(lines 48-51), the code falls back to passing an absolute path toScreenCapture.CaptureScreenshot. While this is unlikely given that paths are constructed fromApplication.dataPath, adding a debug log when the conversion is skipped could help diagnose unexpected behavior in edge cases.🔎 Optional logging enhancement
string assetsRelativePath = normalizedFullPath; if (assetsRelativePath.StartsWith(projectRoot, StringComparison.OrdinalIgnoreCase)) { assetsRelativePath = assetsRelativePath.Substring(projectRoot.Length).TrimStart('/'); } +else +{ + Debug.LogWarning($"Screenshot path '{normalizedFullPath}' is outside project root '{projectRoot}'. Using absolute path."); +}
53-58: Different parameter semantics between Unity versions are handled correctly but could be better documented.The code correctly passes:
assetsRelativePath(project-relative path) toScreenCapture.CaptureScreenshotfor Unity 2022.1+captureName(filename only) toCaptureFromCameraToAssetsFolderfor older versionsWhile the comment at line 45 explains the 2022.1+ behavior, consider adding a brief inline comment at line 57 explaining why the fallback uses only the filename, since
CaptureFromCameraToAssetsFolderhardcodes the target folder.🔎 Suggested documentation enhancement
#else Debug.LogWarning("ScreenCapture is supported after Unity 2022.1. Using main camera capture as fallback."); + // Fallback expects filename only; it constructs the full path internally CaptureFromCameraToAssetsFolder(Camera.main, captureName, size, false); #endif
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs
🧬 Code graph analysis (1)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (2)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
CaptureScreenshot(349-396)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
🔇 Additional comments (2)
MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs (2)
6-6: Minor formatting change with no functional impact.The spacing adjustment in the namespace declaration has no effect on functionality.
45-54: Path conversion correctly implements required API behavior.The
ScreenCapture.CaptureScreenshotAPI in the Editor expects a filepath relative to the project directory, and the code correctly converts the absolute path to a relative path before calling the API. The conversion logic at lines 48–51 properly strips the project root and trims any leading slashes, ensuring the relative path is passed as expected.The fix addresses the previous review concern by computing the relative path before the API call, not after.
1 participant
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.