fix: read_console severity, screenshot color space, execute_code assembly leak, manage_scene package paths - #1353
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5129ed1 to
83bffb4
Compare
The constants never matched UnityEditor.ConsoleWindow.Mode. 1<<2 is Unity's Log bit but was labelled Warning, and every scripting bit was off by one (ScriptingError is 1<<8, not 1<<9). Logs surfaced as Warnings and Warnings as Errors; entries carrying only ScriptingError matched nothing in the table and were classified by InferTypeFromMessage finding "LogError" in the appended stack trace. Replaces the table with Unity's real values and rewrites GetLogTypeFromMode to check Exception and Assert before the error mask, since Unity sets the Error bit alongside both. Fixes CoplayDev#1348
DownscaleTexture called RenderTexture.GetTemporary without a RenderTextureReadWrite argument, so it defaulted to sRGB in a Linear colorspace project. Graphics.Blit then sampled the linear-flagged capture with no decode while the target applied the encode on store, turning 11 into 60 exactly as reported. The direct EncodeToPNG path never goes through DownscaleTexture, which is why the on-disk PNG was correct and only the inline preview was washed out. Fixes CoplayDev#1328
… call Every invocation emitted a fresh in-memory "MCPDynamic" assembly via Assembly.Load, and Mono cannot unload a non-collectible assembly, so the count only dropped at domain reload. The existing caches hold reference paths, not compiled output. Caches the compiled assembly keyed on compiler + wrapped source, cleared on domain reload alongside the path caches and capped at 64 entries so a stream of distinct snippets cannot become the leak itself. Distinct snippets still accumulate until the next reload; that is a Mono limitation. Fixes CoplayDev#1351
HandleCommand stripped any leading Assets/ and re-rooted the remainder under Application.dataPath, so Packages/com.foo/Samples/X.unity became Assets/Packages/com.foo/Samples/X.unity and could never resolve. Paths are now rooted at Assets or Packages, and the existence checks in LoadScene/LoadSceneAdditive move from File.Exists to the AssetDatabase -- the pattern ManageAsset.cs already documents, and the only one that works for packages resolved through Library/PackageCache. Fixes CoplayDev#1197
Two reported documentation gaps: - resources/read takes a server key separate from the mcpforunity:// URI. Codex exposes tools as mcp__unityMCP__* but wants server: "unityMCP" on a resource read, which the server instructions never mentioned. Distinct from the name-vs-URI mistake CoplayDev#1302 fixed. - com.unity.ai.assistant can livelock AssetDatabase::InitialRefresh on Unity 6000.5.x, before any MCP assembly loads, which presents as an MCP connection failure. Documents the packages-lock.json deletion people miss. Fixes CoplayDev#1220 Fixes CoplayDev#1219
The reference pages are generated from the Python annotations by tools/generate_docs_reference.py; the hand-edited row drifted from what the generator emits, failing the Docs -- Reference Drift Check.
Swapping File.Exists for an AssetDatabase lookup fixed Packages/ resolution but narrowed Assets/: the AssetDatabase does not know about a scene file until it is imported, so a scene written by an external tool before a refresh became unloadable where it previously opened. Accept either answer. The AssetDatabase is the only one that resolves Packages/... (embedded packages live in Library/PackageCache); File.Exists still covers the not-yet-imported case. The guard only exists to produce a clearer error than EditorSceneManager.OpenScene would, so accepting is the safe direction.
83bffb4 to
1a1566e
Compare
There was a problem hiding this comment.
Pull request overview
This PR batches several targeted fixes across the Unity Editor package, Python server tool metadata, tests, and website docs to improve correctness and usability of core MCP-for-Unity workflows (console reading, screenshots, execute_code, and scene path handling).
Changes:
- Fixes
read_consoleseverity classification by correctingLogEntry.modebit mappings and updating the mode→LogTypeprecedence; adds mapping tests. - Fixes downscaled inline screenshot previews in Linear color-space projects by matching RenderTexture/Texture2D color space to the source; adds downscale tests.
- Adds
Packages/support tomanage_scenepath handling (plus AssetDatabase-aware existence checks) and updates docs/tests accordingly; addsexecute_codecompiled-assembly caching and tests; updates troubleshooting/instructions docs for Codex resource server key naming and Unity AI Assistant import hang.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/reference/tools/core/manage_scene.md | Documents Packages/-rooted scene paths and adds a package-scene load example. |
| website/docs/guides/troubleshooting.md | Adds troubleshooting entries for Unity 6.5 AI Assistant import hang and Codex resources/read server-key mismatch. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ScreenshotUtilityDownscaleTests.cs.meta | New meta for downscale tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ScreenshotUtilityDownscaleTests.cs | Adds coverage for downscale color-space correctness and non-upscale behavior. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs | Adds test cases validating LogEntry.mode severity mapping and precedence. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScenePackagePathTests.cs.meta | New meta for manage_scene package path tests. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScenePackagePathTests.cs | Adds tests for Packages/ rooting and AssetDatabase/disk existence behavior. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs | Adds tests asserting assembly reuse for identical snippets and independence for different snippets. |
| Server/src/services/tools/manage_scene.py | Updates tool parameter description to mention Assets/ and Packages/ roots. |
| Server/src/main.py | Updates server instructions to clarify Codex’s resource server key vs mcpforunity:// resource URIs. |
| MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | Fixes downscale path to preserve the source texture’s color space when blitting/reading back. |
| MCPForUnity/Editor/Tools/ReadConsole.cs | Corrects mode-bit constants and improves severity inference precedence. |
| MCPForUnity/Editor/Tools/ManageScene.cs | Adds Packages/ rooting support and replaces file existence checks with AssetDatabase-aware logic. |
| MCPForUnity/Editor/Tools/ExecuteCode.cs | Adds a compiled-snippet cache to reduce repeated assembly loads for identical snippets. |
Files not reviewed (2)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScenePackagePathTests.cs.meta: Generated file
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ScreenshotUtilityDownscaleTests.cs.meta: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -180,6 +182,11 @@ public static object HandleCommand(JObject @params) | |||
| { | |||
| string wrappedSource = WrapUserCode(code); | ||
|
|
||
| string cacheKey = compiler + "\n" + wrappedSource; | ||
| if (_compiledCache.TryGetValue(cacheKey, out CompiledSnippet cached)) | ||
| return InvokeCompiled(cached.Assembly, cached.Compiler); |
Six triaged quick-fix issues, batched because each is small and they touch disjoint files. Happy to split into separate PRs if you'd rather review them independently — the commits are already separated one-per-issue, so
git cherry-pickis clean.Every issue below was verified against the current code rather than taken at the reporter's word; two reports turned out to have the wrong diagnosis (noted inline).
fix(read_console): correct the LogEntry.mode bit table — closes #1348The reporter suspected a Unity 6000.5-specific bit shift. It isn't version-specific — the table was simply wrong, and had been since it was written.
The constants never matched
UnityEditor.ConsoleWindow.Mode:1 << 2ScriptingError1 << 91 << 8ScriptingWarning1 << 101 << 9ScriptingLog1 << 111 << 10So Logs surfaced as Warnings and Warnings as Errors. Entries carrying only
ScriptingErrormatched nothing in the table and were classified correctly only by accident —InferTypeFromMessagefinding the literal"LogError"in the appended stack trace.The reporter's three reflected samples are each independently consistent with that off-by-one, which is what confirmed it.
Replaces the table with Unity's real values and rewrites
GetLogTypeFromModeto test Exception and Assert before the error mask, since Unity sets the Error bit alongside both. 15 mapping test cases added, including the two modes captured from a live 6000.5.4f1 console.fix(screenshot): match downscale RenderTexture color space to the source — closes #1328The reporter guessed this was server-side. There is no Python-side resize at all — no PIL anywhere in
Server/src.DownscaleTexturecalledRenderTexture.GetTemporary(...)without aRenderTextureReadWriteargument, so it defaulted toDefault= sRGB in a Linear-colorspace project.Graphics.Blitthen sampled the linear-flagged capture with no decode while the sRGB target applied the encode on store:which is exactly the value reported. The direct
EncodeToPNGpath never goes throughDownscaleTexture— precisely why the on-disk PNG was correct and only the inline preview was washed out.The temp RT and the destination
Texture2Dnow both follow the source's color space.GraphicsFormatUtility.IsSRGBFormatandTexture.graphicsFormatare both 2019.1+, so no compat shim is needed.fix(execute_code): cache compiled assemblies — closes #1351Every call
Assembly.Load()s a fresh in-memoryMCPDynamicimage, and Mono cannot unload a non-collectible assembly, so the count only dropped at domain reload. The existing caches (_cachedAssemblyPaths,RoslynCompiler.ResetCache) hold reference paths, not compiled output — #d2247e94 addressed paths, not this.Caches the compiled assembly keyed on
compiler + wrapped source, cleared in the existingOnDomainReload()alongside the path caches, and capped at 64 entries so a stream of genuinely distinct snippets cannot itself become the leak.Honest limit: distinct snippets still accumulate until the next domain reload. That's a Mono constraint, not something fixable in-process. This fixes the repeated-identical-snippet case, which is the one users actually hit.
feat(manage_scene): acceptPackages/paths — closes #1197HandleCommandstripped any leadingAssets/and re-rooted the remainder underApplication.dataPath, soPackages/com.foo/Samples/X.unitywas rewritten toAssets/Packages/com.foo/Samples/X.unityand could never resolve.Paths are now rooted at
AssetsorPackages. The existence checks inLoadScene/LoadSceneAdditivemove fromFile.Existsto the AssetDatabase —ManageAsset.cs:165already carries the comment "Virtual paths like 'Packages/...' don't work with File.Exists()", so the correct pattern was already in the codebase; this reuses it rather than inventing one.One correction found while self-reviewing: the first pass replaced
File.Existswith the AssetDatabase lookup, which fixedPackages/but narrowedAssets/— the AssetDatabase does not know about a scene file until it is imported, so a scene written by an external tool before a refresh would have become unloadable. The check now accepts either answer. The guard exists only to give a clearer error thanEditorSceneManager.OpenScenewould, so accepting is the safe direction;SceneAssetExists_FindsUnimportedFileOnDiskpins it.@camnewnham volunteered to test this on the issue.
docs: two reported documentation gaps — closes #1220, #1219resources/readtakes a server key separate from themcpforunity://URI. Codex exposes tools asmcp__unityMCP__*but wantsserver: "unityMCP"on a resource read. Distinct from the name-vs-URI mistake fix(server): address resources by URI in agent-facing prose #1302 fixed, which is why that fix didn't resolve it. Added to_build_instructions()and the troubleshooting FAQ.com.unity.ai.assistantcan livelockAssetDatabase::InitialRefreshon Unity 6000.5.x. The reporter's gdb backtrace sits entirely inside Unity's import path, before any Editor assembly of ours loads, so this presents as an MCP connection failure but no MCP code runs. Documents thepackages-lock.jsondeletion step people miss. Upstream UUM-132096.Verification
uv run pytest tests/— 1374 passed, 3 skipped.tools/check-unity-versions.shcompile-check passed.CSerrors,Cscconfirmed rebuilding bothMCPForUnity.Runtime.dllandMCPForUnity.Editor.dll.New tests: 15
read_consoleseverity-mapping cases, 3ScreenshotUtilitydownscale cases, 2execute_codecache cases, 11manage_scenepackage-path cases (all 11 pass on 6000.4.11f1).