fix: improve prefab stage save for automated workflows #1
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
* fix: improve manage_scene screenshot capture * fix: address PR review feedback for screenshot capture - Gate pre-2022 ScreenCapture fallback warning to log only once - Downgrade warning to Debug.Log to reduce log noise - Refactor path-building into shared PrepareCaptureResult() helper - Add conditional logging to catch blocks in BestEffortPrepareGameViewForScreenshot - Add timeout/failure logging to ScheduleAssetImportWhenFileExists - Fix grammar in README-DEV.md * fix(unity): resolve screenshot import callback type + FindObjectsOfType deprecation * chore: bump version to 9.2.0 * Update ManageScene.cs * Update ScreenshotUtility.cs * Update error logging in ManageScene.cs --------- Co-authored-by: Marcus Sanatan <msanatan@gmail.com> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com>
* Update for Texture2D/Sprite Generation Given the choice to generate Texture2D based on patterns and color, also introduce pipeline to turn Texture2D direct to Sprite. Update CLI command to include this too. * Texture Size Set Set texture size to 1024X1024 to avoid too large texture set * Add image input * Update to release direct error with large tex2d * Fix for AI advice * Update on action fetch line
Unity UI Toolkit does not support the :last-child pseudo-class. Replace it with a .section-last class that is applied programmatically to the last section in each .section-stack container. Also moves the Configure All Detected Clients button to the bottom of the Client Configuration section and makes it auto-width. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…atevertogo/unity-mcp into fix/last-child-pseudo-class-warnings
- Add force parameter to save_open_stage for automated workflows where isDirty may not be correctly set - Use PrefabUtility.SaveAsPrefabAsset for dialog-free saving - Mark prefab stage scene dirty when modifying GameObjects in prefab mode - Skip save when no changes and force=false (prevents false dirty flag) The force parameter ensures reliable saving in CI/automation scenarios where Unity dirty tracking may be inconsistent with programmatic changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reviewer's GuideExtends Unity MCP tooling with a new manage_texture pipeline (editor tool, server tool, CLI, docs, and tests), improves prefab save semantics for automated workflows, and enhances screenshot capture to be async-aware and more reliable across Unity versions, along with a few robustness and UX tweaks. Sequence diagram for manage_texture end-to-end pipelinesequenceDiagram
actor Developer
participant CLI_texture as CLI_texture_command
participant Server_tool as Server_manage_texture_tool
participant UnityTransport as Unity_transport
participant UnityTool as ManageTexture
participant TextureOps as TextureOps
participant AssetDB as AssetDatabase
Developer->>CLI_texture: unity_mcp texture create path width height color pattern palette
CLI_texture->>CLI_texture: normalize_color normalize_palette normalize_import_settings
CLI_texture->>Server_tool: manage_texture action=create path width height fill_color pattern palette import_settings
Server_tool->>Server_tool: preflight
Server_tool->>Server_tool: _normalize_color _normalize_palette _normalize_import_settings
Server_tool->>UnityTransport: async_send_command_with_retry("manage_texture", params)
UnityTransport->>UnityTool: HandleCommand(params JObject)
UnityTool->>UnityTool: action dispatch create
UnityTool->>UnityTool: AssetPathUtility.SanitizeAssetPath
UnityTool->>UnityTool: EnsureDirectoryExists
alt imagePath provided
UnityTool->>UnityTool: File.ReadAllBytes imagePath
UnityTool->>UnityTool: new Texture2D LoadImage
else generated texture
UnityTool->>TextureOps: FillTexture or ApplyPixelData or ApplyPatternToTexture
TextureOps-->>UnityTool: populated Texture2D
end
UnityTool->>TextureOps: EncodeTexture(texture assetPath)
TextureOps-->>UnityTool: byte[] imageData
UnityTool->>UnityTool: File.WriteAllBytes
UnityTool->>AssetDB: ImportAsset path ForceUpdate
opt importSettings or spriteSettings
UnityTool->>UnityTool: ConfigureTextureImporter or ConfigureAsSprite
UnityTool->>AssetDB: SaveAndReimport via TextureImporter
end
UnityTool-->>UnityTransport: SuccessResponse path width height warnings
UnityTransport-->>Server_tool: response dict
Server_tool-->>CLI_texture: response dict
CLI_texture-->>Developer: formatted output
Sequence diagram for forced prefab stage save workflowsequenceDiagram
actor CI
participant Server_manage_gameobject as Server_manage_gameobject_tool
participant Unity_Modify as GameObjectModify
participant PrefabStageUtil as PrefabStageUtility
participant SceneMgr as EditorSceneManager
participant Server_manage_prefabs as Server_manage_prefabs_tool
participant Unity_Prefabs as ManagePrefabs
participant PrefabUtil as PrefabUtility
participant AssetDB as AssetDatabase
CI->>Server_manage_gameobject: manage_gameobject action=modify target params
Server_manage_gameobject->>Unity_Modify: Handle params targetToken
Unity_Modify->>Unity_Modify: apply changes to GameObject
Unity_Modify->>Unity_Modify: EditorUtility.SetDirty(targetGo)
Unity_Modify->>PrefabStageUtil: GetCurrentPrefabStage
alt prefab stage open
Unity_Modify->>SceneMgr: MarkSceneDirty(prefabStage.scene)
else regular scene
Unity_Modify->>SceneMgr: MarkSceneDirty(GetActiveScene)
end
Unity_Modify-->>Server_manage_gameobject: SuccessResponse
CI->>Server_manage_prefabs: manage_prefabs action=save_open_stage force=true
Server_manage_prefabs->>Unity_Prefabs: SaveOpenStage JObject{force=true}
Unity_Prefabs->>PrefabStageUtil: GetCurrentPrefabStage
PrefabStageUtil-->>Unity_Prefabs: PrefabStage stage
Unity_Prefabs->>Unity_Prefabs: ValidateStage(stage)
Unity_Prefabs->>Unity_Prefabs: wasDirty = stage.scene.isDirty
Unity_Prefabs->>Unity_Prefabs: force = true
Unity_Prefabs->>Unity_Prefabs: SaveAndRefreshStage(stage force=true)
Unity_Prefabs->>Unity_Prefabs: validate prefabContentsRoot and assetPath
Unity_Prefabs->>Unity_Prefabs: EditorUtility.SetDirty(stage.prefabContentsRoot)
Unity_Prefabs->>SceneMgr: MarkSceneDirty(stage.scene)
Unity_Prefabs->>PrefabUtil: SaveAsPrefabAsset(stage.prefabContentsRoot stage.assetPath out success)
PrefabUtil-->>Unity_Prefabs: success=true
Unity_Prefabs->>AssetDB: SaveAssets
Unity_Prefabs-->>Server_manage_prefabs: SuccessResponse SerializeStage(stage)
Server_manage_prefabs-->>CI: success message
Sequence diagram for async screenshot capture workflowsequenceDiagram
actor User
participant ManageScene as ManageScene_CaptureScreenshot
participant ScreenshotUtil as ScreenshotUtility
participant GameView as EnsureGameView
participant ScreenCap as ScreenCapture_or_Camera
participant ImportScheduler as ScheduleAssetImportWhenFileExists
participant EditorApp as EditorApplication
participant AssetDB as AssetDatabase
User->>ManageScene: manage_scene action=screenshot fileName superSize
ManageScene->>ManageScene: resolvedSuperSize
alt not Application.isBatchMode
ManageScene->>GameView: EnsureGameView
GameView->>EditorApp: ExecuteMenuItem Window.General.Game
GameView->>GameView: EditorWindow.GetWindow GameView.Repaint
GameView->>SceneView: RepaintAll
GameView->>EditorApp: QueuePlayerLoopUpdate
end
ManageScene->>ScreenshotUtil: CaptureToAssetsFolder(fileName superSize ensureUniqueFileName=true)
alt UNITY_2022_1_OR_NEWER
ScreenshotUtil->>ScreenshotUtil: PrepareCaptureResult isAsync=true
ScreenshotUtil->>ScreenCap: ScreenCapture.CaptureScreenshot(assetsRelativePath superSize)
ScreenCap-->>ScreenshotUtil: async capture
ScreenshotUtil-->>ManageScene: ScreenshotCaptureResult IsAsync=true
else legacy Unity
ScreenshotUtil->>ScreenshotUtil: FindAvailableCamera
ScreenshotUtil->>ScreenshotUtil: PrepareCaptureResult isAsync=false
ScreenshotUtil->>ScreenCap: CaptureFromCameraToAssetsFolder(camera fileName superSize ensureUniqueFileName)
ScreenCap-->>ScreenshotUtil: sync file write
ScreenshotUtil-->>ManageScene: ScreenshotCaptureResult IsAsync=false
end
alt result.IsAsync
ManageScene->>ImportScheduler: ScheduleAssetImportWhenFileExists(assetsRelativePath fullPath timeoutSeconds)
ImportScheduler->>EditorApp: register update callback
loop until file exists or timeout
ImportScheduler->>ImportScheduler: File.Exists(fullPath)
alt file exists
ImportScheduler->>AssetDB: ImportAsset(assetsRelativePath ForceSynchronousImport)
ImportScheduler->>EditorApp: unregister callback
else timeout
ImportScheduler->>ImportScheduler: log warning
ImportScheduler->>EditorApp: unregister callback
end
end
else sync capture
ManageScene->>AssetDB: ImportAsset(assetsRelativePath ForceSynchronousImport)
end
ManageScene-->>User: SuccessResponse message path fullPath superSize isAsync
Updated class diagram for texture management, prefab saving, and screenshotsclassDiagram
class ManageTexture {
<<static>>
-int MaxTextureDimension
-int MaxTexturePixels
-int MaxNoiseWork
-List~string~ ValidActions
+object HandleCommand(JObject @params)
-object CreateTexture(JObject @params, bool asSprite)
-object ModifyTexture(JObject @params)
-object DeleteTexture(string path)
-object ApplyPattern(JObject @params)
-object ApplyGradient(JObject @params)
-object ApplyNoise(JObject @params)
-ErrorResponse ValidateDimensions(int width, int height, List~string~ warnings)
-void ApplyPatternToTexture(Texture2D texture, string pattern, List~Color32~ palette, int patternSize)
-Color32 GetPatternColor(int x, int y, string pattern, List~Color32~ palette, int size, int width, int height)
-void ApplyLinearGradient(Texture2D texture, List~Color32~ palette, float angle)
-void ApplyRadialGradient(Texture2D texture, List~Color32~ palette)
-Color32 LerpPalette(List~Color32~ palette, float t)
-void ApplyPerlinNoise(Texture2D texture, List~Color32~ palette, float scale, int octaves)
-void ConfigureAsSprite(string path, JToken spriteSettings)
-void ConfigureTextureImporter(string path, JToken importSettings)
-bool TryParseEnum~T~(string value, out T result)
-bool AssetExists(string path)
-void EnsureDirectoryExists(string assetPath)
-string GetAbsolutePath(string assetPath)
-string ResolveImagePath(string imagePath)
}
class TextureOps {
<<static>>
+byte[] EncodeTexture(Texture2D texture, string assetPath)
+void FillTexture(Texture2D texture, Color32 color)
+Color32 ParseColor32(JArray colorArray)
+List~Color32~ ParsePalette(JArray paletteArray)
+void ApplyPixelData(Texture2D texture, JToken pixelsToken, int width, int height)
+void ApplyPixelDataToRegion(Texture2D texture, JToken pixelsToken, int offsetX, int offsetY, int regionWidth, int regionHeight)
}
class ScreenshotCaptureResult {
+ScreenshotCaptureResult(string fullPath, string assetsRelativePath, int superSize)
+ScreenshotCaptureResult(string fullPath, string assetsRelativePath, int superSize, bool isAsync)
+string FullPath
+string AssetsRelativePath
+int SuperSize
+bool IsAsync
}
class ScreenshotUtility {
<<static>>
-string ScreenshotsFolderName
-bool s_loggedLegacyScreenCaptureFallback
-Camera FindAvailableCamera()
+ScreenshotCaptureResult CaptureToAssetsFolder(string fileName, int superSize, bool ensureUniqueFileName)
+ScreenshotCaptureResult CaptureFromCameraToAssetsFolder(Camera camera, string fileName, int superSize, bool ensureUniqueFileName)
-ScreenshotCaptureResult PrepareCaptureResult(string fileName, int superSize, bool ensureUniqueFileName, bool isAsync)
-string ToAssetsRelativePath(string normalizedFullPath)
-string BuildFileName(string fileName)
-string EnsureUnique(string fullPath)
-string GetProjectRootPath()
}
class ManageScene {
<<static>>
-object CaptureScreenshot(string fileName, int? superSize)
-void EnsureGameView()
-void ScheduleAssetImportWhenFileExists(string assetsRelativePath, string fullPath, double timeoutSeconds)
-object GetActiveSceneInfo()
-object BuildGameObjectSummary(GameObject go, bool includeTransform, int maxChildrenPerNode, int maxComponentsPerNode)
}
class ManagePrefabs {
<<static>>
+object HandleCommand(JObject @params)
-object SaveOpenStage(JObject @params)
-void SaveAndRefreshStage(PrefabStage stage, bool force)
-bool ValidateStage(PrefabStage stage)
-object SerializeStage(PrefabStage stage)
}
class GameObjectModify {
<<static>>
+object Handle(JObject @params, JToken targetToken, string searchContext)
}
class MCPForUnityEditorWindow {
+void CreateGUI()
-void EnsureToolsLoaded()
-void ApplySectionLastClasses()
}
ManageTexture ..> TextureOps : uses
ManageTexture ..> AssetDatabase : imports
ManageTexture ..> TextureImporter : configures
ManageTexture ..> McpLog : logs
ScreenshotUtility o--> ScreenshotCaptureResult
ManageScene ..> ScreenshotUtility : uses
ManageScene ..> AssetDatabase : imports
ManageScene ..> EditorApplication : schedules update
ManagePrefabs ..> PrefabStage : saves
ManagePrefabs ..> PrefabUtility : SaveAsPrefabAsset
ManagePrefabs ..> AssetDatabase : SaveAssets
GameObjectModify ..> PrefabStageUtility : GetCurrentPrefabStage
GameObjectModify ..> EditorSceneManager : MarkSceneDirty
MCPForUnityEditorWindow ..> VisualElement : UI Toolkit
MCPForUnityEditorWindow ..> McpLog : logs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 5 issues, and left some high level feedback:
- The new ManageTexture.cs is very large and mixes command routing, validation, IO, and multiple rendering algorithms in a single static class; consider extracting pattern/gradient/noise generation and importer configuration into smaller helper types or partial classes to keep the tool easier to navigate and maintain.
- TextureOps.ParseColor32 assumes incoming channel values are already in the 0–255 range; if there’s any future path that sends normalized 0.0–1.0 colors directly to Unity, it may be worth adding a safety check/conversion here similar to the Python-side normalization to guard against subtle color bugs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new ManageTexture.cs is very large and mixes command routing, validation, IO, and multiple rendering algorithms in a single static class; consider extracting pattern/gradient/noise generation and importer configuration into smaller helper types or partial classes to keep the tool easier to navigate and maintain.
- TextureOps.ParseColor32 assumes incoming channel values are already in the 0–255 range; if there’s any future path that sends normalized 0.0–1.0 colors directly to Unity, it may be worth adding a safety check/conversion here similar to the Python-side normalization to guard against subtle color bugs.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/ManageTexture.cs:1012-1014` </location>
<code_context>
+ }
+ }
+
+ private static string GetAbsolutePath(string assetPath)
+ {
+ return Path.Combine(Directory.GetCurrentDirectory(), assetPath);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on `Directory.GetCurrentDirectory()` for project paths can break in some editor contexts.
Methods like `GetAbsolutePath`, `EnsureDirectoryExists`, and the create/modify file writes implicitly treat `Directory.GetCurrentDirectory()` as the Unity project root, which may not hold in all editor or pipeline contexts. Prefer deriving paths from `Application.dataPath` (for example, `Path.GetFullPath(Path.Combine(Application.dataPath, "..", assetPath))`) so paths are anchored to the actual Unity project root instead of the process working directory.
Suggested implementation:
```csharp
private static string GetAbsolutePath(string assetPath)
{
// Resolve paths relative to the Unity project root (parent of Assets)
// This is more robust than relying on Directory.GetCurrentDirectory()
string projectRoot = Path.GetFullPath(Path.Combine(Application.dataPath, ".."));
// Normalize assetPath to avoid leading separators breaking Path.Combine
string normalizedAssetPath = assetPath.TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
return Path.GetFullPath(Path.Combine(projectRoot, normalizedAssetPath));
}
```
To compile successfully and use `Application.dataPath`, ensure this file has `using UnityEngine;` at the top. If it's missing, add:
- `using UnityEngine;` alongside the other using directives.
No other call sites of `GetAbsolutePath` need changes; they will now automatically resolve against the Unity project root.
</issue_to_address>
### Comment 2
<location> `Server/src/services/tools/manage_texture.py:557-558` </location>
<code_context>
+ if image_path is not None and action_lower not in ("create", "create_sprite"):
+ return {"success": False, "message": "image_path is only supported for create/create_sprite."}
+
+ if image_path is not None and (fill_color is not None or pattern is not None or pixels is not None):
+ return {"success": False, "message": "image_path cannot be combined with fill_color, pattern, or pixels."}
+
+ # Default to white for create action if nothing else specified
</code_context>
<issue_to_address>
**question (bug_risk):** Server-side defaults for `create` differ from Unity-side defaults, which can make behavior surprising.
For `action == "create"`, this code defaults to a white fill when no `fill_color`, `pattern`, `pixels`, or `image_path` are provided, but `ManageTexture.CreateTexture` defaults to a fully transparent texture in the same case. Unless this difference is intentional, consider aligning these defaults so the same call shape behaves consistently between the MCP tool and Unity.
</issue_to_address>
### Comment 3
<location> `Server/tests/test_cli.py:1230` </location>
<code_context>
+# Texture Command Tests
+# =============================================================================
+
+class TestTextureCommands:
+ """Tests for Texture CLI commands."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Texture CLI tests only cover happy paths; add failure-mode tests for argument validation (e.g. `image-path` conflicts and bad colors).
Current tests only cover success flows plus one invalid JSON case, but they miss key validation paths in `cli.commands.texture`, such as:
- Rejecting `--image-path` when combined with `--color`/`--pattern`/`--palette` for `create` and `sprite`.
- Failing `create` when dimensions are 0 or negative and exposing the validation message.
- Returning non‑zero and printing a clear error when `--color` or `--palette` are malformed for `sprite`/`create`.
Please add tests for these cases (asserting `exit_code == 1` and checking the error text) so changes to CLI parsing and normalization are safer.
Suggested implementation:
```python
class TestTextureCommands:
"""Tests for Texture CLI commands."""
def test_texture_create_rejects_image_path_with_color(self, cli_runner, cli):
"""--image-path must not be combined with --color on `texture create`."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--image-path",
"tests/data/texture.png",
"--color",
"#ff0000",
],
)
assert result.exit_code == 1
# Keep assertions loose to be resilient to wording changes
assert "image-path" in result.output
assert "color" in result.output
def test_texture_create_rejects_image_path_with_pattern(self, cli_runner, cli):
"""--image-path must not be combined with --pattern on `texture create`."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--image-path",
"tests/data/texture.png",
"--pattern",
"stripes",
],
)
assert result.exit_code == 1
assert "image-path" in result.output
assert "pattern" in result.output
def test_texture_create_rejects_image_path_with_palette(self, cli_runner, cli):
"""--image-path must not be combined with --palette on `texture create`."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--image-path",
"tests/data/texture.png",
"--palette",
'[ "#ff0000", "#00ff00" ]',
],
)
assert result.exit_code == 1
assert "image-path" in result.output
assert "palette" in result.output
def test_texture_create_rejects_zero_or_negative_dimensions(self, cli_runner, cli):
"""`texture create` should fail fast on zero/negative dimensions."""
# Width is zero
result_zero = cli_runner.invoke(
cli,
[
"texture",
"create",
"--width",
"0",
"--height",
"32",
"--color",
"#ff0000",
],
)
assert result_zero.exit_code == 1
assert "width" in result_zero.output
# Height is negative
result_negative = cli_runner.invoke(
cli,
[
"texture",
"create",
"--width",
"32",
"--height",
"-1",
"--color",
"#ff0000",
],
)
assert result_negative.exit_code == 1
assert "height" in result_negative.output
def test_texture_create_rejects_malformed_color(self, cli_runner, cli):
"""`texture create` should fail on malformed color values."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--width",
"32",
"--height",
"32",
"--color",
"not-a-color",
],
)
assert result.exit_code == 1
assert "color" in result.output
assert "invalid" in result.output.lower()
def test_texture_sprite_rejects_malformed_palette_colors(self, cli_runner, cli):
"""
`texture sprite` should fail when palette JSON is valid but contains
invalid color entries.
"""
result = cli_runner.invoke(
cli,
[
"texture",
"sprite",
"--palette",
'["#ff0000", "not-a-color"]',
],
)
assert result.exit_code == 1
assert "palette" in result.output
assert "color" in result.output
assert "invalid" in result.output.lower()
```
These edits assume:
1. There is a `cli_runner` fixture (likely wrapping `click.testing.CliRunner`) and a `cli` object used in other CLI tests. If your fixtures/names differ (e.g. `runner` instead of `cli_runner`, or `main` instead of `cli`), update the test signatures and `invoke` calls accordingly.
2. The root command exposes `texture` with subcommands `create` and `sprite`. If the command path is different (e.g. `["textures", "create"]` or no root `texture` group), adjust the argument lists.
3. The error messages include the option names (`image-path`, `color`, `pattern`, `palette`, `width`, `height`) and the word `invalid` for malformed values. If your error text differs, tweak the asserted substrings to match the actual messages while keeping them reasonably loose to avoid brittle tests.
</issue_to_address>
### Comment 4
<location> `Server/tests/integration/test_manage_texture.py:21` </location>
<code_context>
+async def noop_preflight(*args, **kwargs):
+ return None
+
+class TestManageTextureIntegration:
+ """Integration tests for texture management tool logic."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Integration tests cover create/modify/delete well, but `apply_pattern`, `apply_gradient`, and `apply_noise` paths are untested.
These actions introduce additional parameters (`gradient_type`, `gradient_angle`, `noise_scale`, `octaves`, palette handling, etc.). Please add at least one integration test for each that checks parameter normalization and the structure of the outgoing `params` dict to ensure ongoing compatibility with the Unity integration.
Suggested implementation:
```python
async def noop_preflight(*args, **kwargs):
return None
import pytest
import asyncio
from .test_helpers import DummyContext
import services.tools.manage_texture as manage_texture_mod
def run_async(coro):
```
```python
def run_async(coro):
"""Simple wrapper to run a coroutine synchronously."""
loop = asyncio.new_event_loop()
try:
asyncio.set_event_loop(loop)
class TestManageTextureApplyVariantsIntegration:
"""Integration tests for pattern/gradient/noise application parameter handling."""
def _extract_params(self, result):
"""
Helper to normalize access to the params dict from the manage_texture tool result.
The tool may return either:
- an object with a `.params` attribute, or
- a plain dict with a `"params"` key.
This helper keeps the tests resilient to either representation.
"""
if hasattr(result, "params"):
return result.params
if isinstance(result, dict) and "params" in result:
return result["params"]
pytest.fail(f"manage_texture result does not expose 'params': {result!r}")
def test_apply_pattern_params_normalization(self, tmp_path):
"""Ensure apply_pattern parameters are normalized and forwarded correctly."""
ctx = DummyContext(project_root=tmp_path)
request = {
"action": "apply_pattern",
"texture_path": "Assets/Textures/test_pattern.png",
"pattern_name": "checkerboard",
"palette": ["#ff0000", "#00ff00"],
"scale": 2.5,
}
result = run_async(
manage_texture_mod.manage_texture(
context=ctx,
request=request,
preflight=noop_preflight,
)
)
params = self._extract_params(result)
assert params["action"] == "apply_pattern"
assert params["texture_path"].endswith("Assets/Textures/test_pattern.png")
assert params["pattern_name"] == "checkerboard"
# scale should be a float and preserved/normalized
assert isinstance(params["scale"], (int, float))
assert params["scale"] == pytest.approx(2.5)
# palette should be a list of strings (Unity expects color strings)
assert isinstance(params["palette"], list)
assert all(isinstance(color, str) for color in params["palette"])
def test_apply_gradient_params_normalization(self, tmp_path):
"""Ensure apply_gradient parameters (type, angle, palette) are handled correctly."""
ctx = DummyContext(project_root=tmp_path)
request = {
"action": "apply_gradient",
"texture_path": "Assets/Textures/test_gradient.png",
"gradient_type": "linear",
"gradient_angle": 45,
"palette": ["#000000", "#ffffff"],
}
result = run_async(
manage_texture_mod.manage_texture(
context=ctx,
request=request,
preflight=noop_preflight,
)
)
params = self._extract_params(result)
assert params["action"] == "apply_gradient"
assert params["texture_path"].endswith("Assets/Textures/test_gradient.png")
assert params["gradient_type"] == "linear"
# angle should survive any normalization and be numeric
assert isinstance(params["gradient_angle"], (int, float))
assert params["gradient_angle"] == pytest.approx(45)
assert isinstance(params["palette"], list)
assert all(isinstance(color, str) for color in params["palette"])
def test_apply_noise_params_normalization(self, tmp_path):
"""Ensure apply_noise parameters (noise_scale, octaves, seed) are handled correctly."""
ctx = DummyContext(project_root=tmp_path)
request = {
"action": "apply_noise",
"texture_path": "Assets/Textures/test_noise.png",
"noise_scale": 0.75,
"octaves": 4,
"seed": 1234,
}
result = run_async(
manage_texture_mod.manage_texture(
context=ctx,
request=request,
preflight=noop_preflight,
)
)
params = self._extract_params(result)
assert params["action"] == "apply_noise"
assert params["texture_path"].endswith("Assets/Textures/test_noise.png")
# noise_scale should be a float and preserved/normalized
assert isinstance(params["noise_scale"], (int, float))
assert params["noise_scale"] == pytest.approx(0.75)
# octaves should be an integer
assert isinstance(params["octaves"], int)
assert params["octaves"] == 4
# seed should be present and stable
assert params["seed"] == 1234
```
These tests assume the following API, consistent with typical patterns in this codebase:
1. `manage_texture_mod.manage_texture` is an async function with signature similar to:
`async def manage_texture(context: DummyContext, request: dict, preflight: Callable = noop_preflight) -> ResultLike:`
2. The returned object exposes the outgoing Unity call parameters either via:
- a `.params` attribute, or
- a `"params"` key on a dict.
If your actual API differs, adjust:
- The call site inside each test (`manage_texture_mod.manage_texture(...)`) to match the real parameter names/order.
- The `_extract_params` helper if the result structure uses a different attribute/key for the Unity parameter dict.
- The request dict keys (`"gradient_type"`, `"gradient_angle"`, `"noise_scale"`, `"octaves"`, `"palette"`) to match the exact names expected by `manage_texture`.
Finally, if the existing integration tests use a shared fixture or factory for `DummyContext` or for building requests, you may want to:
- Replace direct `DummyContext(project_root=tmp_path)` with that fixture.
- Build the `request` dicts via the same helper to keep test style consistent.
</issue_to_address>
### Comment 5
<location> `Server/tests/integration/test_manage_texture.py:257` </location>
<code_context>
+ assert resp["success"] is True
+ assert captured["params"]["path"] == "Assets/Textures/Old.png"
+
+ def test_invalid_dimensions(self, monkeypatch):
+ """Test error handling for invalid dimensions."""
+ async def fake_send(func, instance, cmd, params, **kwargs):
</code_context>
<issue_to_address>
**nitpick (testing):** The fake `send_with_unity_instance` logic in `test_invalid_dimensions` is unused; consider simplifying to keep the test focused on local validation.
In `test_invalid_dimensions`, the `fake_send` branch for `w > 4096` is never reached because normalization fails earlier at `width=0`, so `send_with_unity_instance` is never called. This makes the test’s intent unclear. You could either simplify `fake_send` to just record calls (or skip the patch), or add a separate test for a too‑large positive width that verifies the Unity call and expected warnings.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1012
to
+1014
| private static string GetAbsolutePath(string assetPath) | ||
| { | ||
| return Path.Combine(Directory.GetCurrentDirectory(), assetPath); |
There was a problem hiding this comment.
suggestion (bug_risk): Relying on Directory.GetCurrentDirectory() for project paths can break in some editor contexts.
Methods like GetAbsolutePath, EnsureDirectoryExists, and the create/modify file writes implicitly treat Directory.GetCurrentDirectory() as the Unity project root, which may not hold in all editor or pipeline contexts. Prefer deriving paths from Application.dataPath (for example, Path.GetFullPath(Path.Combine(Application.dataPath, "..", assetPath))) so paths are anchored to the actual Unity project root instead of the process working directory.
Suggested implementation:
private static string GetAbsolutePath(string assetPath)
{
// Resolve paths relative to the Unity project root (parent of Assets)
// This is more robust than relying on Directory.GetCurrentDirectory()
string projectRoot = Path.GetFullPath(Path.Combine(Application.dataPath, ".."));
// Normalize assetPath to avoid leading separators breaking Path.Combine
string normalizedAssetPath = assetPath.TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
return Path.GetFullPath(Path.Combine(projectRoot, normalizedAssetPath));
}To compile successfully and use Application.dataPath, ensure this file has using UnityEngine; at the top. If it's missing, add:
using UnityEngine;alongside the other using directives.
No other call sites ofGetAbsolutePathneed changes; they will now automatically resolve against the Unity project root.
Comment on lines
+557
to
+558
| if image_path is not None and (fill_color is not None or pattern is not None or pixels is not None): | ||
| return {"success": False, "message": "image_path cannot be combined with fill_color, pattern, or pixels."} |
There was a problem hiding this comment.
question (bug_risk): Server-side defaults for create differ from Unity-side defaults, which can make behavior surprising.
For action == "create", this code defaults to a white fill when no fill_color, pattern, pixels, or image_path are provided, but ManageTexture.CreateTexture defaults to a fully transparent texture in the same case. Unless this difference is intentional, consider aligning these defaults so the same call shape behaves consistently between the MCP tool and Unity.
| # Texture Command Tests | ||
| # ============================================================================= | ||
|
|
||
| class TestTextureCommands: |
There was a problem hiding this comment.
suggestion (testing): Texture CLI tests only cover happy paths; add failure-mode tests for argument validation (e.g. image-path conflicts and bad colors).
Current tests only cover success flows plus one invalid JSON case, but they miss key validation paths in cli.commands.texture, such as:
- Rejecting
--image-pathwhen combined with--color/--pattern/--paletteforcreateandsprite. - Failing
createwhen dimensions are 0 or negative and exposing the validation message. - Returning non‑zero and printing a clear error when
--coloror--paletteare malformed forsprite/create.
Please add tests for these cases (asserting exit_code == 1 and checking the error text) so changes to CLI parsing and normalization are safer.
Suggested implementation:
class TestTextureCommands:
"""Tests for Texture CLI commands."""
def test_texture_create_rejects_image_path_with_color(self, cli_runner, cli):
"""--image-path must not be combined with --color on `texture create`."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--image-path",
"tests/data/texture.png",
"--color",
"#ff0000",
],
)
assert result.exit_code == 1
# Keep assertions loose to be resilient to wording changes
assert "image-path" in result.output
assert "color" in result.output
def test_texture_create_rejects_image_path_with_pattern(self, cli_runner, cli):
"""--image-path must not be combined with --pattern on `texture create`."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--image-path",
"tests/data/texture.png",
"--pattern",
"stripes",
],
)
assert result.exit_code == 1
assert "image-path" in result.output
assert "pattern" in result.output
def test_texture_create_rejects_image_path_with_palette(self, cli_runner, cli):
"""--image-path must not be combined with --palette on `texture create`."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--image-path",
"tests/data/texture.png",
"--palette",
'[ "#ff0000", "#00ff00" ]',
],
)
assert result.exit_code == 1
assert "image-path" in result.output
assert "palette" in result.output
def test_texture_create_rejects_zero_or_negative_dimensions(self, cli_runner, cli):
"""`texture create` should fail fast on zero/negative dimensions."""
# Width is zero
result_zero = cli_runner.invoke(
cli,
[
"texture",
"create",
"--width",
"0",
"--height",
"32",
"--color",
"#ff0000",
],
)
assert result_zero.exit_code == 1
assert "width" in result_zero.output
# Height is negative
result_negative = cli_runner.invoke(
cli,
[
"texture",
"create",
"--width",
"32",
"--height",
"-1",
"--color",
"#ff0000",
],
)
assert result_negative.exit_code == 1
assert "height" in result_negative.output
def test_texture_create_rejects_malformed_color(self, cli_runner, cli):
"""`texture create` should fail on malformed color values."""
result = cli_runner.invoke(
cli,
[
"texture",
"create",
"--width",
"32",
"--height",
"32",
"--color",
"not-a-color",
],
)
assert result.exit_code == 1
assert "color" in result.output
assert "invalid" in result.output.lower()
def test_texture_sprite_rejects_malformed_palette_colors(self, cli_runner, cli):
"""
`texture sprite` should fail when palette JSON is valid but contains
invalid color entries.
"""
result = cli_runner.invoke(
cli,
[
"texture",
"sprite",
"--palette",
'["#ff0000", "not-a-color"]',
],
)
assert result.exit_code == 1
assert "palette" in result.output
assert "color" in result.output
assert "invalid" in result.output.lower()These edits assume:
- There is a
cli_runnerfixture (likely wrappingclick.testing.CliRunner) and acliobject used in other CLI tests. If your fixtures/names differ (e.g.runnerinstead ofcli_runner, ormaininstead ofcli), update the test signatures andinvokecalls accordingly. - The root command exposes
texturewith subcommandscreateandsprite. If the command path is different (e.g.["textures", "create"]or no roottexturegroup), adjust the argument lists. - The error messages include the option names (
image-path,color,pattern,palette,width,height) and the wordinvalidfor malformed values. If your error text differs, tweak the asserted substrings to match the actual messages while keeping them reasonably loose to avoid brittle tests.
| async def noop_preflight(*args, **kwargs): | ||
| return None | ||
|
|
||
| class TestManageTextureIntegration: |
There was a problem hiding this comment.
suggestion (testing): Integration tests cover create/modify/delete well, but apply_pattern, apply_gradient, and apply_noise paths are untested.
These actions introduce additional parameters (gradient_type, gradient_angle, noise_scale, octaves, palette handling, etc.). Please add at least one integration test for each that checks parameter normalization and the structure of the outgoing params dict to ensure ongoing compatibility with the Unity integration.
Suggested implementation:
async def noop_preflight(*args, **kwargs):
return None
import pytest
import asyncio
from .test_helpers import DummyContext
import services.tools.manage_texture as manage_texture_mod
def run_async(coro):def run_async(coro):
"""Simple wrapper to run a coroutine synchronously."""
loop = asyncio.new_event_loop()
try:
asyncio.set_event_loop(loop)
class TestManageTextureApplyVariantsIntegration:
"""Integration tests for pattern/gradient/noise application parameter handling."""
def _extract_params(self, result):
"""
Helper to normalize access to the params dict from the manage_texture tool result.
The tool may return either:
- an object with a `.params` attribute, or
- a plain dict with a `"params"` key.
This helper keeps the tests resilient to either representation.
"""
if hasattr(result, "params"):
return result.params
if isinstance(result, dict) and "params" in result:
return result["params"]
pytest.fail(f"manage_texture result does not expose 'params': {result!r}")
def test_apply_pattern_params_normalization(self, tmp_path):
"""Ensure apply_pattern parameters are normalized and forwarded correctly."""
ctx = DummyContext(project_root=tmp_path)
request = {
"action": "apply_pattern",
"texture_path": "Assets/Textures/test_pattern.png",
"pattern_name": "checkerboard",
"palette": ["#ff0000", "#00ff00"],
"scale": 2.5,
}
result = run_async(
manage_texture_mod.manage_texture(
context=ctx,
request=request,
preflight=noop_preflight,
)
)
params = self._extract_params(result)
assert params["action"] == "apply_pattern"
assert params["texture_path"].endswith("Assets/Textures/test_pattern.png")
assert params["pattern_name"] == "checkerboard"
# scale should be a float and preserved/normalized
assert isinstance(params["scale"], (int, float))
assert params["scale"] == pytest.approx(2.5)
# palette should be a list of strings (Unity expects color strings)
assert isinstance(params["palette"], list)
assert all(isinstance(color, str) for color in params["palette"])
def test_apply_gradient_params_normalization(self, tmp_path):
"""Ensure apply_gradient parameters (type, angle, palette) are handled correctly."""
ctx = DummyContext(project_root=tmp_path)
request = {
"action": "apply_gradient",
"texture_path": "Assets/Textures/test_gradient.png",
"gradient_type": "linear",
"gradient_angle": 45,
"palette": ["#000000", "#ffffff"],
}
result = run_async(
manage_texture_mod.manage_texture(
context=ctx,
request=request,
preflight=noop_preflight,
)
)
params = self._extract_params(result)
assert params["action"] == "apply_gradient"
assert params["texture_path"].endswith("Assets/Textures/test_gradient.png")
assert params["gradient_type"] == "linear"
# angle should survive any normalization and be numeric
assert isinstance(params["gradient_angle"], (int, float))
assert params["gradient_angle"] == pytest.approx(45)
assert isinstance(params["palette"], list)
assert all(isinstance(color, str) for color in params["palette"])
def test_apply_noise_params_normalization(self, tmp_path):
"""Ensure apply_noise parameters (noise_scale, octaves, seed) are handled correctly."""
ctx = DummyContext(project_root=tmp_path)
request = {
"action": "apply_noise",
"texture_path": "Assets/Textures/test_noise.png",
"noise_scale": 0.75,
"octaves": 4,
"seed": 1234,
}
result = run_async(
manage_texture_mod.manage_texture(
context=ctx,
request=request,
preflight=noop_preflight,
)
)
params = self._extract_params(result)
assert params["action"] == "apply_noise"
assert params["texture_path"].endswith("Assets/Textures/test_noise.png")
# noise_scale should be a float and preserved/normalized
assert isinstance(params["noise_scale"], (int, float))
assert params["noise_scale"] == pytest.approx(0.75)
# octaves should be an integer
assert isinstance(params["octaves"], int)
assert params["octaves"] == 4
# seed should be present and stable
assert params["seed"] == 1234These tests assume the following API, consistent with typical patterns in this codebase:
manage_texture_mod.manage_textureis an async function with signature similar to:
async def manage_texture(context: DummyContext, request: dict, preflight: Callable = noop_preflight) -> ResultLike:- The returned object exposes the outgoing Unity call parameters either via:
- a
.paramsattribute, or - a
"params"key on a dict.
- a
If your actual API differs, adjust:
- The call site inside each test (
manage_texture_mod.manage_texture(...)) to match the real parameter names/order. - The
_extract_paramshelper if the result structure uses a different attribute/key for the Unity parameter dict. - The request dict keys (
"gradient_type","gradient_angle","noise_scale","octaves","palette") to match the exact names expected bymanage_texture.
Finally, if the existing integration tests use a shared fixture or factory for DummyContext or for building requests, you may want to:
- Replace direct
DummyContext(project_root=tmp_path)with that fixture. - Build the
requestdicts via the same helper to keep test style consistent.
| assert resp["success"] is True | ||
| assert captured["params"]["path"] == "Assets/Textures/Old.png" | ||
|
|
||
| def test_invalid_dimensions(self, monkeypatch): |
There was a problem hiding this comment.
nitpick (testing): The fake send_with_unity_instance logic in test_invalid_dimensions is unused; consider simplifying to keep the test focused on local validation.
In test_invalid_dimensions, the fake_send branch for w > 4096 is never reached because normalization fails earlier at width=0, so send_with_unity_instance is never called. This makes the test’s intent unclear. You could either simplify fake_send to just record calls (or skip the patch), or add a separate test for a too‑large positive width that verifies the Unity call and expected warnings.
whatevertogo
merged commit 56a8d23
into
whatevertogo:enhancement/Prefab-management
2 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
This PR improves the prefab stage save functionality for automated/CI workflows, addressing the dialog concern raised in PR CoplayDev#611.
Changes:
forceparameter tosave_open_stage: For automated workflows where Unity'sisDirtyflag may not be correctly set when changes are made programmaticallyPrefabUtility.SaveAsPrefabAsset: Saves prefabs without displaying any dialogs, suitable for unattended automationmanage_gameobject, the prefab stage scene is now properly marked dirtyforce=false(default) and no changes detected, returns early without falsely marking the prefab dirtyUsage
For automated workflows:
Testing
Tested the complete workflow:
manage_gameobject✅force=true✅🤖 Generated with Claude Code
Summary by Sourcery
Add a new cross-platform texture management toolchain, enhance screenshot and prefab save behavior for automation, and improve CLI and docs coverage.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: