fix: ScreenshotUtility.cs WebGL compilation #578
Conversation
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
ScreenCapture doesn't exist in WebGL scripting backend, causing CS0103 error when WebGL is the active build target. Editor scripts compile with desktop backend regardless of target platform. No functionality impact since MCP tools only run in Editor.
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMoves ScreenshotUtility into an Editor-only context to avoid WebGL compilation errors by changing its namespace and references, and removes the now-unneeded runtime helpers folder metadata. Class diagram for ScreenshotUtility editor migrationclassDiagram
namespace MCPForUnity_Runtime_Helpers {
class ScreenshotUtility
class ScreenshotCaptureResult
}
namespace MCPForUnity_Editor_Helpers {
class ScreenshotUtility
class ScreenshotCaptureResult
}
namespace MCPForUnity_Editor_Tools {
class ManageScene
}
ManageScene ..> ScreenshotUtility : uses
MCPForUnity_Runtime_Helpers_ScreenshotUtility <|.. MCPForUnity_Editor_Helpers_ScreenshotUtility
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughScreenshotUtility is relocated from the Runtime namespace to the Editor namespace to resolve WebGL compilation errors. This involves updating the namespace declaration in ScreenshotUtility.cs, removing the obsolete Runtime import from ManageScene.cs, and deleting the associated Runtime metadata file. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The comment above
ScreenshotCaptureResultstill refers to avoiding editor-only dependencies in runtime code, which no longer matches the class’s location/role and should be updated or removed for clarity. - In
ManageScene.cs, the inline comment// For ScreenshotUtilitywas removed along with the using, but the remaining// For Response classcomment could be clarified or generalized now that this file imports multiple helpers from the same namespace.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The comment above `ScreenshotCaptureResult` still refers to avoiding editor-only dependencies in runtime code, which no longer matches the class’s location/role and should be updated or removed for clarity.
- In `ManageScene.cs`, the inline comment `// For ScreenshotUtility` was removed along with the using, but the remaining `// For Response class` comment could be clarified or generalized now that this file imports multiple helpers from the same namespace.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}
Contributor
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 (1)
MCPForUnity/Editor/Helpers/ScreenshotUtility.cs (1)
97-115: Texture2D is never destroyed — memory leak.The
Texture2Dallocated at line 103 is not disposed after encoding to PNG. Since this is Editor code, useDestroyImmediateto release the texture memory.🐛 Proposed fix
try { camera.targetTexture = rt; camera.Render(); RenderTexture.active = rt; var tex = new Texture2D(width, height, TextureFormat.RGBA32, false); - tex.ReadPixels(new Rect(0, 0, width, height), 0, 0); - tex.Apply(); - - byte[] png = tex.EncodeToPNG(); - File.WriteAllBytes(normalizedFullPath, png); + try + { + tex.ReadPixels(new Rect(0, 0, width, height), 0, 0); + tex.Apply(); + + byte[] png = tex.EncodeToPNG(); + File.WriteAllBytes(normalizedFullPath, png); + } + finally + { + UnityEngine.Object.DestroyImmediate(tex); + } } finally {
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Helpers/ScreenshotUtility.cs`:
- Around line 6-7: The inline comment following the namespace
MCPForUnity.Editor.Helpers is stale—remove or rewrite it to reflect that this
file (ScreenshotUtility) now lives in the Editor assembly and is Editor-only;
update the comment text to mention "Editor-only utilities" or delete the comment
entirely so it no longer references "Runtime Utilities" or "runtime code", and
ensure the change is applied in the ScreenshotUtility.cs header near the
namespace declaration.
Comment on lines
+6
to
7
| namespace MCPForUnity.Editor.Helpers | ||
| //The reason for having another Runtime Utilities in additional to Editor Utilities is to avoid Editor-only dependencies in this runtime code. |
Contributor
There was a problem hiding this comment.
Stale comment references Runtime context.
The namespace update on line 6 is correct. However, line 7's comment still refers to "Runtime Utilities" and "runtime code," which is now misleading since this file has been relocated to the Editor assembly.
📝 Suggested fix
namespace MCPForUnity.Editor.Helpers
-//The reason for having another Runtime Utilities in additional to Editor Utilities is to avoid Editor-only dependencies in this runtime code.
{Or update the comment to reflect the new Editor-only purpose if documentation is still desired.
📝 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.
Suggested change
| namespace MCPForUnity.Editor.Helpers | |
| //The reason for having another Runtime Utilities in additional to Editor Utilities is to avoid Editor-only dependencies in this runtime code. | |
| namespace MCPForUnity.Editor.Helpers | |
| { |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Helpers/ScreenshotUtility.cs` around lines 6 - 7, The
inline comment following the namespace MCPForUnity.Editor.Helpers is
stale—remove or rewrite it to reflect that this file (ScreenshotUtility) now
lives in the Editor assembly and is Editor-only; update the comment text to
mention "Editor-only utilities" or delete the comment entirely so it no longer
references "Runtime Utilities" or "runtime code", and ensure the change is
applied in the ScreenshotUtility.cs header near the namespace declaration.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
When WebGL is the active build target, the project fails to compile:
error CS0103: The name 'ScreenCapture' does not exist in the current context
Solution
Move
ScreenshotUtility.csfromRuntime/Helpers/toEditor/Helpers/.Why this works
ScreenCapture doesn't exist in WebGL scripting backend. Editor scripts compile with desktop backend regardless of target platform. No functionality impact since MCP tools only run in Editor.
Fixes #574
Summary by Sourcery
Move screenshot capture utilities into the editor-only helpers namespace to avoid WebGL build errors.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.