[FEATURE] Procedural Texture2D/Sprite Generation #621
Conversation
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.
Reviewer's GuideAdds a new manage_texture MCP tool and corresponding Unity editor implementation for procedural Texture2D generation and sprite creation, exposes it through a new Sequence diagram for texture sprite creation via CLI and manage_texture toolsequenceDiagram
actor Dev as Developer
participant CLI as texture_sprite_CLI
participant RC as run_command
participant Tool as manage_texture_Python_tool
participant Pre as preflight
participant UT as send_with_unity_instance
participant Unity as ManageTexture_CSharp
participant Ops as TextureOps
participant AD as AssetDatabase
Dev->>CLI: unity-mcp texture sprite path --options
CLI->>RC: run_command("manage_texture", params)
RC->>Tool: manage_texture(action=create_sprite, path, ...)
Tool->>Pre: preflight(wait_for_no_compile, refresh_if_dirty)
Pre-->>Tool: preflight result (ok)
Tool->>Tool: normalize colors, palette, pixels, sprite/import settings
Tool->>UT: send_with_unity_instance("manage_texture", params_dict)
UT->>Unity: HandleCommand(JObject params)
Unity->>Unity: validate action and path
Unity->>Unity: CreateTexture(params, asSprite=true)
Unity->>Ops: EncodeTexture(texture, assetPath)
Ops-->>Unity: image bytes (PNG/JPG)
Unity->>AD: File.WriteAllBytes + ImportAsset
Unity->>Unity: ConfigureTextureImporter / ConfigureAsSprite
Unity-->>UT: SuccessResponse(path, width, height, asSprite)
UT-->>Tool: result dict
Tool-->>RC: result with _debug_params
RC-->>CLI: formatted result
CLI-->>Dev: print success "Created sprite: path"
Class diagram for ManageTexture, TextureOps, and manage_texture toolclassDiagram
class TextureOps {
+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 ManageTexture {
<<static>>
-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)
-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)
}
class manage_texture_module {
+manage_texture(Context ctx, string action, string path, int width, int height, list fill_color, string pattern, list palette, int pattern_size, list_or_str pixels, string gradient_type, float gradient_angle, float noise_scale, int octaves, dict set_pixels, dict_or_bool as_sprite, dict import_settings) dict
-list~int~ _normalize_color(object value)
-list~list~ _normalize_palette(object value)
-object _normalize_pixels(object value, int width, int height)
-dict _normalize_sprite_settings(object value)
-dict _normalize_import_settings(object value)
-bool _is_normalized_color(list values)
-tuple _normalize_bool_setting(object value, string name)
}
class texture_cli_module {
+texture()
+create(string path, int width, int height, string color, string pattern, string palette, string import_settings)
+sprite(string path, int width, int height, string color, string pattern, float ppu, string pivot)
+modify(string path, string set_pixels)
+delete(string path)
-object try_parse_json(string value, string context)
-list~int~ _normalize_color(object value, string context)
-list~list~ _normalize_palette(object value, string context)
-list_or_str _normalize_pixels(object value, int width, int height, string context)
-dict _normalize_set_pixels(object value)
-dict _normalize_import_settings(object value)
-list~int~ _parse_hex_color(string value)
-bool _is_normalized_color(list values)
}
class UnityTransportModule {
+send_with_unity_instance(function async_send_command_with_retry, object unity_instance, string tool_name, dict params) dict
}
ManageTexture ..> TextureOps : uses
manage_texture_module ..> UnityTransportModule : uses
manage_texture_module ..> ManageTexture : calls via tool name
texture_cli_module ..> manage_texture_module : invokes tool manage_texture
File-Level Changes
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. 📝 WalkthroughWalkthroughAdds Unity editor texture utilities and a ManageTexture editor tool, a Click-based CLI, a server-side manage_texture service, and tests to support texture create/modify/delete, procedural generation, pixel-region/base64 handling, sprite/import settings, and asset pipeline integration. (37 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI (texture)"
participant Server as "Server Service"
participant Tool as "manage_texture (service)"
participant Unity as "Unity Editor"
participant Ops as "TextureOps"
participant AssetDB as "AssetDatabase"
User->>CLI: texture create --path --width --height ...
CLI->>Server: run_command("manage_texture", params)
Server->>Tool: validate & normalize params
Tool->>Unity: async_send_command_with_retry(manage_texture, params)
rect rgba(0, 100, 200, 0.5)
Unity->>Unity: ManageTexture.HandleCommand(params)
Unity->>Ops: Encode / Fill / ApplyPixelData (region or base64)
Unity->>Unity: configure TextureImporter & Sprite settings
end
Unity->>AssetDB: create/update asset at path
AssetDB-->>Unity: asset created/updated
Unity-->>Tool: { success: true, path: asset_path }
Tool-->>Server: response
Server-->>CLI: response
CLI->>User: print result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
Scriptwonder
changed the title
![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 2 issues, and left some high level feedback:
- The enum mapping and color/pixel normalization logic is duplicated between
services/tools/manage_texture.pyandcli/commands/texture.py; consider centralizing this into shared helpers to keep behavior consistent and reduce maintenance overhead. - The
ManageTextureeditor tool has grown quite large (multiple responsibilities and ~900 lines); consider extracting pattern/gradient/noise generation and importer-configuration helpers into separate classes or static helpers to keep the core command handler more focused and easier to evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The enum mapping and color/pixel normalization logic is duplicated between `services/tools/manage_texture.py` and `cli/commands/texture.py`; consider centralizing this into shared helpers to keep behavior consistent and reduce maintenance overhead.
- The `ManageTexture` editor tool has grown quite large (multiple responsibilities and ~900 lines); consider extracting pattern/gradient/noise generation and importer-configuration helpers into separate classes or static helpers to keep the core command handler more focused and easier to evolve.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/ManageTexture.cs:84-93` </location>
<code_context>
+ if (string.IsNullOrEmpty(path))
+ return new ErrorResponse("'path' is required for apply_gradient.");
+
+ int width = @params["width"]?.ToObject<int>() ?? 64;
+ int height = @params["height"]?.ToObject<int>() ?? 64;
+ string gradientType = @params["gradientType"]?.ToString() ?? "linear";
+ float angle = @params["gradientAngle"]?.ToObject<float>() ?? 0f;
+
+ var palette = TextureOps.ParsePalette(@params["palette"] as JArray);
</code_context>
<issue_to_address>
**issue (bug_risk):** Width/height for gradients and noise are not validated like in `CreateTexture`, which can allow zero or very large textures.
`ApplyGradient` and `ApplyNoise` pass `width`/`height` directly to `new Texture2D(...)`, unlike `CreateTexture` which clamps to 1–4096 and rejects invalid values. This means 0 will throw and very large sizes can cause allocation failures or stalls. Please reuse the same validation (or at least enforce 1 ≤ dimension ≤ reasonable max) before constructing the texture.
</issue_to_address>
### Comment 2
<location> `Server/src/cli/commands/texture.py:209-217` </location>
<code_context>
+ if "texture_shape" in value:
+ result["textureShape"] = _map_enum(value["texture_shape"], _TEXTURE_SHAPES)
+
+ for snake, camel in [
+ ("srgb", "sRGBTexture"),
+ ("alpha_is_transparency", "alphaIsTransparency"),
+ ("readable", "isReadable"),
+ ("generate_mipmaps", "mipmapEnabled"),
+ ("compression_crunched", "crunchedCompression"),
+ ]:
+ if snake in value:
+ result[camel] = bool(value[snake])
+
+ if "alpha_source" in value:
</code_context>
<issue_to_address>
**issue (bug_risk):** Boolean import settings are coerced with `bool()` which misinterprets strings like "false" as True.
In `_normalize_import_settings`, this affects fields like `srgb`, `alpha_is_transparency`, etc., which are currently set via `bool(value[snake])`. To match the server-side behavior and avoid surprising CLI results, consider using the same `coerce_bool` logic (or similar explicit string parsing for "true"/"false"/"1"/"0") and rejecting invalid/ambiguous inputs instead of relying on generic truthiness.
</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: 7
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs`:
- Around line 94-167: The CreateTexture (and similarly ModifyTexture) path
currently constructs a Texture2D into the local variable texture but only calls
UnityEngine.Object.DestroyImmediate(texture) on the normal path, which leaks the
texture when exceptions occur; fix this by wrapping the creation and all uses of
texture in a try/finally where the finally block checks texture != null and
calls UnityEngine.Object.DestroyImmediate(texture), ensuring cleanup even when
ConfigureTextureImporter/EncodeTexture/File.WriteAllBytes or other calls throw.
Locate the texture variable in CreateTexture and the analogous variable in
ModifyTexture and move existing logic into the try block so the finally always
runs.
- Around line 107-114: Guard against zero/negative divisors in
pattern/gradient/noise code: ensure patternSize passed into
ApplyPatternToTexture is clamped to a minimum of 1 (or reject non-positive),
enforce width/height used in gradient/noise calculations are at least 1 (and use
denom = Math.Max(1, width-1) / Math.Max(1, height-1) when normalizing), and
enforce octaves >= 1 before using it to normalize maxValue (or fall back to 1).
Mirror the validation approach used in CreateTexture: validate/clamp
`@params`["patternSize"], texture width/height, and `@params`["octaves"] early, and
replace raw divisions with safe denominators (max(1, ...)) in
ApplyPatternToTexture and the gradient/noise functions to prevent
divide-by-zero/NaN.
In `@Server/src/cli/commands/texture.py`:
- Around line 145-184: The function _normalize_set_pixels currently skips
validating width/height when pixels are not provided, allowing zero/negative
values through; update _normalize_set_pixels so that in the branch where
"pixels" not in value but "color" may be present, if either "width" or "height"
is provided require both to be present, cast them to int, and validate they are
> 0 (raise ValueError otherwise) before assigning result["width"] and
result["height"]; keep existing behavior for the "pixels" branch and continue to
use _normalize_pixels and _normalize_color as before.
In `@Server/src/services/tools/manage_texture.py`:
- Around line 196-200: The validator currently treats only True as valid and
returns an error for False; update the logic in the function handling the
"as_sprite" input (the code using the variable value and the error return
"as_sprite must be a dict or boolean...") so that if isinstance(value, bool) you
return the default sprite dict when value is True, but return (None, None) when
value is False (i.e., treat False as “no sprite settings”); keep the existing
error return for non-bool, non-dict types.
- Around line 55-70: The branch that handles 3-component/color lists calls
_is_normalized_color(value) without guarding non-numeric entries, which can
raise TypeError and bypass your numeric error path; update the list/tuple branch
(the block that currently checks len(value) == 3 and len(value) == 4) to
validate or wrap calls to _is_normalized_color in the same try/except used for
4-component colors so non-numeric inputs return the consistent error (e.g.,
"color values must be numeric, got {value}") instead of raising, and ensure the
subsequent append/conversion logic uses the safely-determined normalized check.
- Around line 535-600: The code is silently defaulting or accepting invalid
numeric inputs (width, height, pattern_size, octaves) via coerce_int(...) or
fallbacks; update the start of the function (where
width/height/pattern_size/octaves are normalized) to explicitly validate after
coercion: use coerce_int(...) to get integers, return {"success": False,
"message": "..."} on invalid values, enforce width and height in 1..4096,
enforce pattern_size > 0, and enforce octaves > 0 (reject non-positive or
non-integer/None values and values >4096 for dimensions); reference the
variables width, height, pattern_size, octaves and the helper coerce_int(...) so
callers get clear error messages instead of silent defaults that lead to Unity
divide-by-zero/NaN issues.
In `@Server/tests/integration/test_manage_texture.py`:
- Around line 18-19: Several mock stub functions (e.g., noop_preflight) have
unused parameters triggering Ruff ARG001; rename those parameters by prefixing
them with an underscore (for example def noop_preflight(_arg1, _arg2=None):) or
change variadic signatures to use *_, **__ so the linter treats them as
intentionally unused. Update every mock/stub in this test file that currently
declares unused parameters (the other stubs corresponding to the reported
ranges) to follow the same convention, leaving function behavior unchanged.
🧹 Nitpick comments (1)
Server/tests/test_cli.py (1)
1333-1341: Make the precedence test assert behavior, not just exit code.
Right now it doesn’t verify that providing--colorsuppresses the default pattern; asserting therun_commandparams would make the test meaningful.♻️ Suggested test enhancement
- def test_texture_sprite_color_and_pattern_precedence(self, runner, mock_unity_response): + def test_texture_sprite_color_and_pattern_precedence(self, runner, mock_unity_response): """Test that color takes precedence over default pattern in sprite command.""" - with patch("cli.commands.texture.run_command", return_value=mock_unity_response): + with patch("cli.commands.texture.run_command", return_value=mock_unity_response) as mock_run: result = runner.invoke(cli, [ "texture", "sprite", "Assets/Sprites/Solid.png", "--color", "[255,0,0,255]" ]) assert result.exit_code == 0 + _, params, _ = mock_run.call_args[0] + assert "pattern" not in params + assert params["fillColor"] == [255, 0, 0, 255]
| def _normalize_set_pixels(value: Any) -> dict[str, Any]: | ||
| if value is None: | ||
| raise ValueError("set-pixels is required") | ||
| if isinstance(value, str): | ||
| value = try_parse_json(value, "set-pixels") | ||
| if not isinstance(value, dict): | ||
| raise ValueError("set-pixels must be a JSON object") | ||
|
|
||
| result: dict[str, Any] = dict(value) | ||
|
|
||
| if "pixels" in value: | ||
| width = value.get("width") | ||
| height = value.get("height") | ||
| if width is None or height is None: | ||
| raise ValueError("set-pixels requires width and height when pixels are provided") | ||
| width = int(width) | ||
| height = int(height) | ||
| if width <= 0 or height <= 0: | ||
| raise ValueError("set-pixels width and height must be positive") | ||
| result["width"] = width | ||
| result["height"] = height | ||
| result["pixels"] = _normalize_pixels(value["pixels"], width, height, "set-pixels pixels") | ||
|
|
||
| if "color" in value: | ||
| result["color"] = _normalize_color(value["color"], "set-pixels color") | ||
|
|
||
| if "pixels" not in value and "color" not in value: | ||
| raise ValueError("set-pixels requires 'color' or 'pixels'") | ||
|
|
||
| if "x" in value: | ||
| result["x"] = int(value["x"]) | ||
| if "y" in value: | ||
| result["y"] = int(value["y"]) | ||
|
|
||
| if "width" in value and "pixels" not in value: | ||
| result["width"] = int(value["width"]) | ||
| if "height" in value and "pixels" not in value: | ||
| result["height"] = int(value["height"]) | ||
|
|
||
| return result |
There was a problem hiding this comment.
Validate width/height for color-only set-pixels.
When pixels isn’t provided, width/height are cast but not validated; zero/negative values will slip through and fail downstream.
🐛 Suggested validation
if "width" in value and "pixels" not in value:
result["width"] = int(value["width"])
+ if result["width"] <= 0:
+ raise ValueError("set-pixels width must be positive")
if "height" in value and "pixels" not in value:
result["height"] = int(value["height"])
+ if result["height"] <= 0:
+ raise ValueError("set-pixels height must be positive")🧰 Tools
🪛 Ruff (0.14.13)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Prefer TypeError exception for invalid type
(TRY004)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
163-163: Avoid specifying long messages outside the exception class
(TRY003)
172-172: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@Server/src/cli/commands/texture.py` around lines 145 - 184, The function
_normalize_set_pixels currently skips validating width/height when pixels are
not provided, allowing zero/negative values through; update
_normalize_set_pixels so that in the branch where "pixels" not in value but
"color" may be present, if either "width" or "height" is provided require both
to be present, cast them to int, and validate they are > 0 (raise ValueError
otherwise) before assigning result["width"] and result["height"]; keep existing
behavior for the "pixels" branch and continue to use _normalize_pixels and
_normalize_color as before.
| if isinstance(value, bool) and value: | ||
| # Just enable sprite mode with defaults | ||
| return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None | ||
|
|
||
| return None, f"as_sprite must be a dict or boolean, got {type(value).__name__}" |
There was a problem hiding this comment.
Treat as_sprite=False as “no sprite settings”.
Line 196 only accepts True, so passing False yields a validation error. Consider returning (None, None) when a false boolean is provided.
🐛 Proposed fix
- if isinstance(value, bool) and value:
- # Just enable sprite mode with defaults
- return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None
+ if isinstance(value, bool):
+ if value:
+ # Just enable sprite mode with defaults
+ return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None
+ return None, None🤖 Prompt for AI Agents
In `@Server/src/services/tools/manage_texture.py` around lines 196 - 200, The
validator currently treats only True as valid and returns an error for False;
update the logic in the function handling the "as_sprite" input (the code using
the variable value and the error return "as_sprite must be a dict or
boolean...") so that if isinstance(value, bool) you return the default sprite
dict when value is True, but return (None, None) when value is False (i.e.,
treat False as “no sprite settings”); keep the existing error return for
non-bool, non-dict types.
| async def noop_preflight(*args, **kwargs): | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the test file to see the current state
wc -l Server/tests/integration/test_manage_texture.pyRepository: CoplayDev/unity-mcp
Length of output: 114
🏁 Script executed:
# Read the relevant sections to see the code
sed -n '18,19p' Server/tests/integration/test_manage_texture.pyRepository: CoplayDev/unity-mcp
Length of output: 121
🏁 Script executed:
# Also check the other mentioned lines
sed -n '28,32p; 52,55p; 74,77p; 101,104p; 131,134p; 158,161p; 193,195p; 221,223p; 241,244p; 259,263p' Server/tests/integration/test_manage_texture.pyRepository: CoplayDev/unity-mcp
Length of output: 1795
🏁 Script executed:
# Check if there's a Ruff configuration in the project
find . -maxdepth 2 -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" \) | head -20Repository: CoplayDev/unity-mcp
Length of output: 98
🏁 Script executed:
# Check Ruff configuration
cat ruff.tomlRepository: CoplayDev/unity-mcp
Length of output: 969
🏁 Script executed:
# Also check Server/pyproject.toml for Ruff config
cat Server/pyproject.toml | grep -A 20 "\[tool.ruff"Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
# Now run Ruff on the test file to see if ARG001 is flagged
cd Server && python -m ruff check tests/integration/test_manage_texture.py --select=ARG001 2>&1 || trueRepository: CoplayDev/unity-mcp
Length of output: 149
🏁 Script executed:
# Read the broader context around the mentioned lines to verify accuracy
sed -n '15,35p' Server/tests/integration/test_manage_texture.pyRepository: CoplayDev/unity-mcp
Length of output: 812
🏁 Script executed:
# Check a couple of the other sections to verify parameter usage
sed -n '25,35p' Server/tests/integration/test_manage_texture.py
sed -n '48,58p' Server/tests/integration/test_manage_texture.py
sed -n '70,80p' Server/tests/integration/test_manage_texture.pyRepository: CoplayDev/unity-mcp
Length of output: 1548
🏁 Script executed:
# Let's check one more section to verify parameter usage patterns
sed -n '155,165p' Server/tests/integration/test_manage_texture.py
sed -n '190,200p' Server/tests/integration/test_manage_texture.pyRepository: CoplayDev/unity-mcp
Length of output: 1026
Fix unused parameters flagged by Ruff ARG001.
Several mock stubs throughout this test file have unused parameters that trigger Ruff's ARG001 rule. Prefix truly unused parameters with _ (or use *_, **__ for variadic parameters) to satisfy the linter.
🔧 Example fix (apply similarly to all stubs)
-async def noop_preflight(*args, **kwargs):
+async def noop_preflight(*_, **__):
return None
- async def fake_send(func, instance, cmd, params, **kwargs):
+ async def fake_send(_func, _instance, cmd, params, **_kwargs):
captured["cmd"] = cmd
captured["params"] = paramsApplies to: Lines 18-19, 28-32, 52-55, 74-77, 101-104, 131-134, 158-161, 193-195, 221-223, 241-244, 259-263
📝 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.
| async def noop_preflight(*args, **kwargs): | |
| return None | |
| async def noop_preflight(*_, **__): | |
| return None |
🧰 Tools
🪛 Ruff (0.14.13)
18-18: Unused function argument: args
(ARG001)
18-18: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In `@Server/tests/integration/test_manage_texture.py` around lines 18 - 19,
Several mock stub functions (e.g., noop_preflight) have unused parameters
triggering Ruff ARG001; rename those parameters by prefixing them with an
underscore (for example def noop_preflight(_arg1, _arg2=None):) or change
variadic signatures to use *_, **__ so the linter treats them as intentionally
unused. Update every mock/stub in this test file that currently declares unused
parameters (the other stubs corresponding to the reported ranges) to follow the
same convention, leaving function behavior unchanged.
There was a problem hiding this comment.
Pull request overview
This PR adds a full end-to-end pipeline for procedural Texture2D generation in Unity (via MCP), including CLI commands, and support for turning generated textures directly into sprites.
Changes:
- Introduces a new
manage_textureMCP tool on the server side for creating, modifying, deleting textures, and for pattern/gradient/noise-based generation plus sprite creation. - Adds a
texturecommand group to the CLI (unity-mcp texture ...) with subcommands for create/sprite/modify/delete, including robust JSON and color normalization. - Implements the Unity Editor-side
ManageTexturetool andTextureOpshelper to perform the actual Texture2D/Sprite operations, and wires in tests (CLI + integration) and documentation updates.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/uv.lock | Bumps the mcpforunityserver locked version to 9.2.0 to stay in sync with the new package version in pyproject.toml. |
| Server/tests/test_cli.py | Adds CLI tests for unity-mcp texture create/sprite/modify/delete including color handling, pattern defaults, and invalid JSON error behavior. |
| Server/tests/integration/test_manage_texture.py | Adds integration tests for the manage_texture tool covering color normalization (0–255 vs 0.0–1.0), import-setting normalization, set-pixels behavior (including error cases), delete parameter pass-through, and dimension error propagation. |
| Server/src/services/tools/manage_texture.py | Implements the new manage_texture MCP tool, including normalization of colors, palettes, pixels, sprite settings, and TextureImporter settings, and dispatches actions to Unity via send_with_unity_instance. |
| Server/src/cli/main.py | Registers the new texture CLI command group as an optional command so it’s available via the main unity-mcp entry point. |
| Server/src/cli/commands/texture.py | Defines the texture CLI group and its create, sprite, modify, and delete subcommands, with helpers for JSON parsing and import-setting/color/pixel normalization before calling manage_texture. |
| README.md | Updates the Available Tools list to include the new manage_texture tool for visibility in the public documentation. |
| MCPForUnity/Editor/Tools/ManageTexture.cs.meta | Unity meta file for the new ManageTexture editor script, enabling it to be tracked and imported properly by Unity. |
| MCPForUnity/Editor/Tools/ManageTexture.cs | Adds the Unity Editor-side implementation of the manage_texture tool, handling actions (create, modify, delete, create_sprite, apply_pattern, apply_gradient, apply_noise) and coordinating Texture2D creation, saving, importer configuration, and sprite creation. |
| MCPForUnity/Editor/Helpers/TextureOps.cs.meta | Unity meta file for the new TextureOps helper script. |
| MCPForUnity/Editor/Helpers/TextureOps.cs | Provides shared helper methods for encoding textures, filling them, parsing colors/palettes, and applying pixel data or base64 payloads to textures, used by ManageTexture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import base64 | ||
| import json |
There was a problem hiding this comment.
base64 and json are imported but never used in this module, which adds noise and may confuse readers about hidden responsibilities. Please remove these unused imports to keep the tool implementation focused and reduce lint warnings.
| import base64 | |
| import json |
| action: Annotated[Literal[ | ||
| "create", | ||
| "modify", | ||
| "delete", | ||
| "create_sprite", | ||
| "apply_pattern", | ||
| "apply_gradient", | ||
| "apply_noise" |
There was a problem hiding this comment.
The new apply_gradient and apply_noise actions are exposed in the tool signature but are not covered by any integration tests, so parameter wiring issues (e.g., gradient_type/noise_scale/octaves or sprite settings) would only be caught at runtime. Consider adding at least one integration test per action that invokes manage_texture with these actions and asserts the constructed params_dict (similar to the existing tests for create/modify/delete).
| foreach (var item in paletteArray) | ||
| { | ||
| if (item is JArray colorArray) | ||
| { | ||
| palette.Add(ParseColor32(colorArray)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| if (success) | ||
| return new SuccessResponse($"Texture deleted: {fullPath}"); | ||
| else | ||
| return new ErrorResponse($"Failed to delete texture: {fullPath}"); |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
| if (success) | |
| return new SuccessResponse($"Texture deleted: {fullPath}"); | |
| else | |
| return new ErrorResponse($"Failed to delete texture: {fullPath}"); | |
| return success | |
| ? new SuccessResponse($"Texture deleted: {fullPath}") | |
| : new ErrorResponse($"Failed to delete texture: {fullPath}"); |
| @@ -0,0 +1,276 @@ | |||
| """Integration tests for manage_texture tool.""" | |||
|
|
|||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
| import pytest |
|
lol, looks great! Let me try it out and give you some feedback. |
|
Hi! I’m really excited to see the Texture2D → Sprite feature—it’s going to be incredibly useful for Game Jam participants! 👍 Here is some feedback based on my testing: What works well: Key areas for improvement: Adding the following APIs would significantly enhance the tool’s practicality: A few details to note: Parameter Inconsistency: There seems to be an inconsistency in parameter passing formats; for example, array parameters like fill_color require batch_execute. Return Value: The return value appears inaccurate (setting as_sprite: true still results in asSprite: false). I just wanted to share my experience. I hope this doesn’t come across the wrong way; I didn’t mean to offend. |
Scriptwonder
left a comment
Scriptwonder
left a comment
There was a problem hiding this comment.
Hi! I’m really excited to see the Texture2D → Sprite feature—it’s going to be incredibly useful for Game Jam participants! 👍
Here is some feedback based on my testing:
What works well:
The basic texture generation (patterns, gradients, noise) and Sprite conversion are very stable. It’s a real boon for anyone working on shaders or textures!Key areas for improvement:
I found that currently no basic drawing APIs available, making it difficult to draw simple shapes like circles, rectangles, or lines. Just wanted to share my experience(Asking an AI to generate something specific, like a picture of a dog, doesn’t seem to work out ideally). This is just my take on a possible solution, which might be off the mark, but I also noticed that we currently only have SetPixel(x, y).Adding the following APIs would significantly enhance the tool’s practicality:
draw_circle, draw_ellipse, draw_rectangle, draw_line, draw_polygonA few details to note:
Parameter Inconsistency: There seems to be an inconsistency in parameter passing formats; for example, array parameters like fill_color require batch_execute.
Return Value: The return value appears inaccurate (setting as_sprite: true still results in asSprite: false).
Missing Region Operations: There is no way to modify only a specific part of the texture (region operations are missing).I just wanted to share my experience. I hope this doesn’t come across the wrong way; I didn’t mean to offend.
Hi Thank you for the feedback. I do think that drawing API or API to create shapes are useful / TODOs in the future, but it should also support more advanxed shapes, so I am thinking of utilizing image generation like Antigravity-Nano Banana to achieve this. This will be a very meaningful next step I suppose.
Set texture size to 1024X1024 to avoid too large texture set
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs`:
- Around line 32-39: The ValidateDimensions function currently only checks total
pixels (MaxTexturePixels) and allows extreme per-dimension values; add an
explicit per-dimension cap (introduce or use a constant like MaxTextureDimension
= 1024) and validate that width and height are each between 1 and
MaxTextureDimension, returning an ErrorResponse if either exceeds the
per-dimension limit (e.g., "Invalid dimensions: {width}x{height}. Each dimension
must be <= {MaxTextureDimension}."); keep the existing total-pixels check
(MaxTexturePixels) as well and update the error messages to clearly indicate
which check failed, referencing the ValidateDimensions method and the
MaxTexturePixels/MaxTextureDimension symbols.
♻️ Duplicate comments (3)
MCPForUnity/Editor/Tools/ManageTexture.cs (1)
115-195: Ensure Texture2D is always destroyed on exceptions.Cleanup only happens on success paths; exceptions before
DestroyImmediatewill leak textures in Create/Modify/Gradient/Noise. Wrap creation/use intry/finally.🔧 Example fix (apply same pattern to the other methods)
- try - { - Texture2D texture = new Texture2D(width, height, TextureFormat.RGBA32, false); + Texture2D texture = null; + try + { + texture = new Texture2D(width, height, TextureFormat.RGBA32, false); ... - UnityEngine.Object.DestroyImmediate(texture); - return new SuccessResponse( $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), new { path = fullPath, width, height, asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite") } ); } catch (Exception e) { return new ErrorResponse($"Failed to create texture: {e.Message}"); } + finally + { + if (texture != null) + UnityEngine.Object.DestroyImmediate(texture); + }Also applies to: 208-285, 341-383, 414-449
Server/src/services/tools/manage_texture.py (1)
222-226: Treat as_sprite=False as “no sprite settings.”Passing
Falsecurrently throws a validation error even though it’s an explicit opt‑out.🔧 Suggested fix
- if isinstance(value, bool) and value: - # Just enable sprite mode with defaults - return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None + if isinstance(value, bool): + if value: + # Just enable sprite mode with defaults + return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None + return None, NoneServer/src/cli/commands/texture.py (1)
196-199: Validate width/height for color-only set-pixels.When
pixelsisn’t provided, width/height are cast but not validated; zero/negative values (or only one dimension) slip through.🔧 Suggested fix
- if "width" in value and "pixels" not in value: - result["width"] = int(value["width"]) - if "height" in value and "pixels" not in value: - result["height"] = int(value["height"]) + if "pixels" not in value: + if ("width" in value) ^ ("height" in value): + raise ValueError("set-pixels requires both width and height when pixels are not provided") + if "width" in value: + result["width"] = int(value["width"]) + if result["width"] <= 0: + raise ValueError("set-pixels width must be positive") + result["height"] = int(value["height"]) + if result["height"] <= 0: + raise ValueError("set-pixels height must be positive")
| private static ErrorResponse ValidateDimensions(int width, int height) | ||
| { | ||
| if (width <= 0 || height <= 0) | ||
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Must be positive."); | ||
| long totalPixels = (long)width * height; | ||
| if (totalPixels > MaxTexturePixels) | ||
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Total pixels must be <= {MaxTexturePixels}."); | ||
| return null; |
There was a problem hiding this comment.
Cap width/height per-dimension to match the 1024×1024 intent.
Right now only total pixels are capped, so sizes like 1×1,048,576 pass but exceed Unity’s max dimension and contradict the “1024×1024” limit. Add an explicit per-dimension cap.
🔧 Suggested fix
private static ErrorResponse ValidateDimensions(int width, int height)
{
if (width <= 0 || height <= 0)
return new ErrorResponse($"Invalid dimensions: {width}x{height}. Must be positive.");
+ int maxDim = (int)Mathf.Sqrt(MaxTexturePixels); // 1024
+ if (width > maxDim || height > maxDim)
+ return new ErrorResponse($"Invalid dimensions: {width}x{height}. Each dimension must be <= {maxDim}.");
long totalPixels = (long)width * height;
if (totalPixels > MaxTexturePixels)
return new ErrorResponse($"Invalid dimensions: {width}x{height}. Total pixels must be <= {MaxTexturePixels}.");
return null;
}📝 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.
| private static ErrorResponse ValidateDimensions(int width, int height) | |
| { | |
| if (width <= 0 || height <= 0) | |
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Must be positive."); | |
| long totalPixels = (long)width * height; | |
| if (totalPixels > MaxTexturePixels) | |
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Total pixels must be <= {MaxTexturePixels}."); | |
| return null; | |
| private static ErrorResponse ValidateDimensions(int width, int height) | |
| { | |
| if (width <= 0 || height <= 0) | |
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Must be positive."); | |
| int maxDim = (int)Mathf.Sqrt(MaxTexturePixels); // 1024 | |
| if (width > maxDim || height > maxDim) | |
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Each dimension must be <= {maxDim}."); | |
| long totalPixels = (long)width * height; | |
| if (totalPixels > MaxTexturePixels) | |
| return new ErrorResponse($"Invalid dimensions: {width}x{height}. Total pixels must be <= {MaxTexturePixels}."); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs` around lines 32 - 39, The
ValidateDimensions function currently only checks total pixels
(MaxTexturePixels) and allows extreme per-dimension values; add an explicit
per-dimension cap (introduce or use a constant like MaxTextureDimension = 1024)
and validate that width and height are each between 1 and MaxTextureDimension,
returning an ErrorResponse if either exceeds the per-dimension limit (e.g.,
"Invalid dimensions: {width}x{height}. Each dimension must be <=
{MaxTextureDimension}."); keep the existing total-pixels check
(MaxTexturePixels) as well and update the error messages to clearly indicate
which check failed, referencing the ValidateDimensions method and the
MaxTexturePixels/MaxTextureDimension symbols.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs`:
- Around line 226-229: The SuccessResponse currently sets asSprite using a
case-sensitive check of importSettingsToken["textureType"], so values like
"sprite" are missed; update the logic in the return statement that constructs
the anonymous payload (the asSprite expression) to compare textureType
case-insensitively (e.g., normalize to lower/upper or use a case-insensitive
comparison) when evaluating importSettingsToken?["textureType"]?.ToString() ==
"Sprite", keeping the existing checks for the asSprite parameter and
spriteSettingsToken unchanged; ensure you only change the boolean expression
used for asSprite in the SuccessResponse constructor.
♻️ Duplicate comments (4)
MCPForUnity/Editor/Tools/ManageTexture.cs (2)
32-38: Add per-dimension cap (current check allows extreme sizes).
The max-pixels check alone still allows a single dimension to exceed Unity’s limits (e.g., 1×1,048,576). Consider adding an explicit per-dimension cap.
121-224: Ensure temporary Texture2D instances are always destroyed on exceptions.
Cleanup currently happens only on the success path; exceptions can leak textures. Apply a try/finally pattern here and in ModifyTexture/ApplyGradient/ApplyNoise.Server/src/services/tools/manage_texture.py (1)
222-226: Treatas_sprite=Falseas “no sprite settings”.
PassingFalsecurrently triggers a validation error instead of disabling sprite config.Server/src/cli/commands/texture.py (1)
196-199: Validate width/height for color-only set-pixels.
Whenpixelsisn’t provided, width/height are cast but not validated; zero/negative values can slip through.
| return new SuccessResponse( | ||
| $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), | ||
| new { path = fullPath, width, height, asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite") } | ||
| ); |
There was a problem hiding this comment.
Fix asSprite response to handle case-insensitive textureType.
The return payload uses a case-sensitive check, so textureType: "sprite" can still report asSprite: false even though the importer is set to Sprite.
🔧 Suggested fix
- return new SuccessResponse(
- $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""),
- new { path = fullPath, width, height, asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite") }
- );
+ bool importSprite = string.Equals(
+ importSettingsToken?["textureType"]?.ToString(),
+ "Sprite",
+ StringComparison.OrdinalIgnoreCase
+ );
+ return new SuccessResponse(
+ $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""),
+ new { path = fullPath, width, height, asSprite = asSprite || spriteSettingsToken != null || importSprite }
+ );📝 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.
| return new SuccessResponse( | |
| $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), | |
| new { path = fullPath, width, height, asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite") } | |
| ); | |
| bool importSprite = string.Equals( | |
| importSettingsToken?["textureType"]?.ToString(), | |
| "Sprite", | |
| StringComparison.OrdinalIgnoreCase | |
| ); | |
| return new SuccessResponse( | |
| $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), | |
| new { path = fullPath, width, height, asSprite = asSprite || spriteSettingsToken != null || importSprite } | |
| ); |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs` around lines 226 - 229, The
SuccessResponse currently sets asSprite using a case-sensitive check of
importSettingsToken["textureType"], so values like "sprite" are missed; update
the logic in the return statement that constructs the anonymous payload (the
asSprite expression) to compare textureType case-insensitively (e.g., normalize
to lower/upper or use a case-insensitive comparison) when evaluating
importSettingsToken?["textureType"]?.ToString() == "Sprite", keeping the
existing checks for the asSprite parameter and spriteSettingsToken unchanged;
ensure you only change the boolean expression used for asSprite in the
SuccessResponse constructor.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs`:
- Around line 364-443: ApplyGradient creates a Texture2D ("texture") but
currently only destroys it on the success path, leaking it if an exception
occurs; wrap the creation and use of the Texture2D in a try/finally (or add a
finally block) that calls UnityEngine.Object.DestroyImmediate(texture) if
texture != null, mirroring the pattern used in CreateTexture, and ensure any
early returns (e.g., on encoding failure) still let the finally run so the
texture is always disposed.
- Around line 445-522: ApplyNoise creates a Texture2D ("texture") and returns
early on exceptions without destroying it; move texture allocation outside the
try, keep the try for operations (ApplyPerlinNoise, EncodeTexture,
File.WriteAllBytes, ImportAsset, ConfigureAsSprite), and add a finally block
that checks if texture != null and calls
UnityEngine.Object.DestroyImmediate(texture) so the texture is always cleaned up
(also remove duplicate DestroyImmediate calls inside the normal path). Ensure
you reference the existing ApplyNoise method and the Texture2D local named
texture when implementing the try/finally.
In `@Server/src/services/tools/manage_texture.py`:
- Around line 622-623: The code currently masks invalid/missing width/height by
using "region_width = coerce_int(set_pixels_normalized.get('width')) or 1" and
similarly for region_height; remove the "or 1" fallback, capture the raw
coerce_int results into region_width and region_height, and explicitly validate
that they are integers > 0 (and return/raise the existing validation
error/response if not), referencing the variables and function names:
set_pixels_normalized, coerce_int, region_width, region_height so the caller
sees an error for invalid inputs instead of silently using 1.
♻️ Duplicate comments (6)
Server/tests/integration/test_manage_texture.py (1)
18-19: Prefix unused mock parameters to satisfy Ruff ARG001.Multiple mock stubs have unused parameters that trigger linter warnings. Prefix them with
_to indicate intentional non-use.♻️ Example fix (apply pattern to all mocks)
-async def noop_preflight(*args, **kwargs): +async def noop_preflight(*_args, **_kwargs): return None - async def fake_send(func, instance, cmd, params, **kwargs): + async def fake_send(_func, _instance, cmd, params, **_kwargs): captured["cmd"] = cmd captured["params"] = params return {"success": True, "message": "Created texture"}Also applies to: 28-32, 52-55, 74-77, 101-104, 131-134, 158-161, 193-195, 221-223, 241-244, 259-263
MCPForUnity/Editor/Tools/ManageTexture.cs (3)
227-237:asSpriteresponse uses case-sensitive comparison.Line 234 checks
importSettingsToken?["textureType"]?.ToString() == "Sprite"but the Unity side accepts case-insensitive input (e.g., "sprite"). This causesasSprite: falsein responses even when the texture is configured as a sprite.🔧 Suggested fix
+ bool importSprite = string.Equals( + importSettingsToken?["textureType"]?.ToString(), + "Sprite", + StringComparison.OrdinalIgnoreCase + ); return new SuccessResponse( $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), new { path = fullPath, width, height, - asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite"), + asSprite = asSprite || spriteSettingsToken != null || importSprite, warnings = warnings.Count > 0 ? warnings : null } );
255-333:ModifyTexturealso leakseditableTextureon exceptions.Similar to
CreateTexture, theeditableTexturecreated at line 264 can leak if exceptions occur before the explicitDestroyImmediateat line 325.🐛 Recommended fix
try { ... - Texture2D editableTexture = new Texture2D(texture.width, texture.height, TextureFormat.RGBA32, false); + Texture2D editableTexture = null; + try + { + editableTexture = new Texture2D(texture.width, texture.height, TextureFormat.RGBA32, false); editableTexture.LoadImage(fileData); // Apply modifications ... - UnityEngine.Object.DestroyImmediate(editableTexture); - return new SuccessResponse($"Texture modified: {fullPath}"); + } + finally + { + if (editableTexture != null) + UnityEngine.Object.DestroyImmediate(editableTexture); + } }
118-242: Texture leak risk on exception paths.The
CreateTexturemethod creates aTexture2Dat lines 145 and 163, but exceptions thrown between creation andDestroyImmediate(line 221) will leak the texture. The existingtry/catchdoes not guarantee cleanup.🐛 Recommended fix using try/finally
try { ... + Texture2D texture = null; + try + { - Texture2D texture; if (hasImage) { ... texture = new Texture2D(2, 2, TextureFormat.RGBA32, false); ... } else { texture = new Texture2D(width, height, TextureFormat.RGBA32, false); ... } ... - // Clean up memory - UnityEngine.Object.DestroyImmediate(texture); ... return new SuccessResponse(...); + } + finally + { + if (texture != null) + UnityEngine.Object.DestroyImmediate(texture); + } } catch (Exception e) { return new ErrorResponse($"Failed to create texture: {e.Message}"); }Server/src/services/tools/manage_texture.py (1)
193-222:as_sprite=Falsestill returns validation error.Line 218 only accepts
Trueboolean values. Passingas_sprite=Falsefalls through to the error at line 222. Consider returning(None, None)forFalseto indicate "no sprite settings wanted."🔧 Suggested fix
- if isinstance(value, bool) and value: - # Just enable sprite mode with defaults - return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None + if isinstance(value, bool): + if value: + # Just enable sprite mode with defaults + return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None + # False means no sprite settings + return None, NoneServer/src/cli/commands/texture.py (1)
201-204: Missing validation forwidth/heightwhen onlycoloris provided inset-pixels.When
"pixels"is not invaluebut"width"or"height"are provided, they're cast to int without validation. Zero/negative values will pass through and fail downstream.🔧 Suggested fix
if "width" in value and "pixels" not in value: result["width"] = int(value["width"]) + if result["width"] <= 0: + raise ValueError("set-pixels width must be positive") if "height" in value and "pixels" not in value: result["height"] = int(value["height"]) + if result["height"] <= 0: + raise ValueError("set-pixels height must be positive")
🧹 Nitpick comments (6)
MCPForUnity/Editor/Tools/ManageTexture.cs (1)
344-356: Simplify conditional return inDeleteTexture.Both branches return, so a ternary expression improves clarity.
♻️ Suggested refactor
try { bool success = AssetDatabase.DeleteAsset(fullPath); - if (success) - return new SuccessResponse($"Texture deleted: {fullPath}"); - else - return new ErrorResponse($"Failed to delete texture: {fullPath}"); + return success + ? new SuccessResponse($"Texture deleted: {fullPath}") + : new ErrorResponse($"Failed to delete texture: {fullPath}"); }Server/src/services/tools/manage_texture.py (2)
4-5: Remove unused imports.
base64andjsonare imported but never used in this module.♻️ Suggested fix
-import base64 -import json from typing import Annotated, Any, Literal
568-592: PotentialTypeErrorwhenimage_pathis set but dimensions areNone.When
image_pathis provided,widthandheightare set toNoneat lines 584-585. However, ifpixelsis also provided (which is already rejected at line 557-558), the_normalize_pixelscall at line 590 would receiveNonefor width/height. The current validation prevents this, but the code path is fragile.Consider adding explicit guards or a comment clarifying that
pixelsvalidation at lines 557-558 prevents this path.Server/src/cli/commands/texture.py (1)
273-278: Missing validation for integer import settings.
aniso_level,max_texture_size, andcompression_qualityare cast to int but not validated for range. The server validates these, but early validation in CLI improves UX.♻️ Suggested validation
if "aniso_level" in value: - result["anisoLevel"] = int(value["aniso_level"]) + level = int(value["aniso_level"]) + if not 0 <= level <= 16: + raise ValueError("aniso_level must be 0-16") + result["anisoLevel"] = level if "max_texture_size" in value: - result["maxTextureSize"] = int(value["max_texture_size"]) + size = int(value["max_texture_size"]) + valid_sizes = [32, 64, 128, 256, 512, 1024, 2048, 4096, 8192, 16384] + if size not in valid_sizes: + raise ValueError(f"max_texture_size must be one of {valid_sizes}") + result["maxTextureSize"] = size if "compression_quality" in value: - result["compressionQuality"] = int(value["compression_quality"]) + quality = int(value["compression_quality"]) + if not 0 <= quality <= 100: + raise ValueError("compression_quality must be 0-100") + result["compressionQuality"] = qualityServer/tests/integration/test_manage_texture.py (2)
3-3: Remove unusedpytestimport.The
pytestmodule is imported but not directly used. Tests usemonkeypatchfixture which is injected automatically.♻️ Suggested fix
"""Integration tests for manage_texture tool.""" -import pytest import asyncio
8-16: Consider usingpytest-asyncioinstead of manual event loop management.The
run_asynchelper manually creates/closes event loops. Usingpytest-asynciowith@pytest.mark.asynciois more idiomatic and handles cleanup automatically.# With pytest-asyncio: `@pytest.mark.asyncio` async def test_create_texture_with_color_array(self, monkeypatch): ... resp = await manage_texture_mod.manage_texture(...)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs`:
- Around line 46-53: The code in HandleCommand currently calls
`@params`["action"]?.ToString().ToLower() which can throw if ToString() returns
null; change the extraction to guard nulls (e.g., obtain the JToken first and
use null-propagation: var token = `@params`["action"]; string action =
token?.ToString()?.ToLowerInvariant(); or explicitly check token/string before
calling ToLower), then keep the existing null/empty check and error return;
apply the same defensive pattern to the corresponding HandleCommand (or
equivalent) methods in ManageShader.cs, ManageEditor.cs, and ManageAsset.cs to
ensure consistent null-safe behavior.
♻️ Duplicate comments (4)
MCPForUnity/Editor/Tools/ManageTexture.cs (3)
94-243: Ensure temporary Texture2D instances are always disposed on exceptions.
Exceptions between allocation and explicitDestroyImmediatecalls will leak textures; wrap allocations in try/finally (like ApplyGradient/ApplyNoise) and avoid double-destroys.🧹 Suggested pattern (apply similarly in ModifyTexture)
- Texture2D texture; + Texture2D texture = null; if (hasImage) { ... - UnityEngine.Object.DestroyImmediate(texture); - return new ErrorResponse($"Failed to load image from '{imagePath}'."); + UnityEngine.Object.DestroyImmediate(texture); + texture = null; + return new ErrorResponse($"Failed to load image from '{imagePath}'."); } ... } ... - UnityEngine.Object.DestroyImmediate(texture); + // cleanup moved to finally ... - catch (Exception e) + catch (Exception e) { return new ErrorResponse($"Failed to create texture: {e.Message}"); } + finally + { + if (texture != null) + UnityEngine.Object.DestroyImmediate(texture); + }#!/bin/bash # Review cleanup paths for CreateTexture/ModifyTexture. rg -n "CreateTexture|ModifyTexture|DestroyImmediate" MCPForUnity/Editor/Tools/ManageTexture.cs -C 3Also applies to: 245-333
227-235: MakeasSpriteresponse tolerant to case forimportSettings.textureType.
Direct MCP calls that pass"sprite"(lowercase) will reportasSprite: falseeven though the importer is set to Sprite.🔧 Suggested fix
- return new SuccessResponse( + bool importSprite = string.Equals( + importSettingsToken?["textureType"]?.ToString(), + "Sprite", + StringComparison.OrdinalIgnoreCase + ); + return new SuccessResponse( $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), new { path = fullPath, width, height, - asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite"), + asSprite = asSprite || spriteSettingsToken != null || importSprite, warnings = warnings.Count > 0 ? warnings : null } );
475-483: Move Texture2D allocation inside the try block.If
new Texture2D(...)throws an exception (e.g., from invalid dimensions), it will not be caught by the catch block since the exception occurs before entering the try statement. Move the allocation inside the try block to ensure proper error handling.Suggested fix
- Texture2D texture = new Texture2D(width, height, TextureFormat.RGBA32, false); - try + Texture2D texture = null; + try { + texture = new Texture2D(width, height, TextureFormat.RGBA32, false); ApplyPerlinNoise(texture, palette, scale, octaves);Server/src/services/tools/manage_texture.py (1)
193-223: Treatas_sprite=Falseas “no sprite settings.”
CurrentlyFalsereturns an error even though it is a valid “disable” value.🔧 Suggested fix
- if isinstance(value, bool) and value: - # Just enable sprite mode with defaults - return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None + if isinstance(value, bool): + if value: + # Just enable sprite mode with defaults + return {"pivot": [0.5, 0.5], "pixelsPerUnit": 100}, None + return None, None
| public static object HandleCommand(JObject @params) | ||
| { | ||
| string action = @params["action"]?.ToString().ToLower(); | ||
| if (string.IsNullOrEmpty(action)) | ||
| { | ||
| return new ErrorResponse("Action parameter is required."); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read the ManageTexture.cs file around lines 46-53
fd ManageTexture.csRepository: CoplayDev/unity-mcp
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Get the file and read the relevant lines
find . -name "ManageTexture.cs" -type f -exec cat -n {} \; | sed -n '40,60p'Repository: CoplayDev/unity-mcp
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Search for HandleCommand call sites
rg -n "HandleCommand\(" -C 3Repository: CoplayDev/unity-mcp
Length of output: 50376
🏁 Script executed:
#!/bin/bash
# Verify null-coalescing operator behavior - search for other similar patterns
rg -n "\.ToString\(\)\.[a-zA-Z]" MCPForUnity/Editor/Tools/ManageTexture.csRepository: CoplayDev/unity-mcp
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check if there are defensive null checks already in place in similar tools
rg -B5 "string action = `@params`" MCPForUnity/Editor/Tools/ CustomTools/Repository: CoplayDev/unity-mcp
Length of output: 5504
Guard against null reference in action extraction.
On line 48, @params["action"]?.ToString().ToLower() invokes .ToLower() on a potentially null string—if the "action" key is missing, .ToString() returns null and .ToLower() throws a NullReferenceException before the error check at line 49 can execute.
🛠️ Suggested fix
+ if (`@params` == null)
+ return new ErrorResponse("Parameters are required.");
- string action = `@params`["action"]?.ToString().ToLower();
+ string action = `@params`["action"]?.ToString()?.ToLowerInvariant();Note: Other tools (ManageShader.cs, ManageEditor.cs, ManageAsset.cs) have the same issue and should be fixed consistently.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs` around lines 46 - 53, The code in
HandleCommand currently calls `@params`["action"]?.ToString().ToLower() which can
throw if ToString() returns null; change the extraction to guard nulls (e.g.,
obtain the JToken first and use null-propagation: var token = `@params`["action"];
string action = token?.ToString()?.ToLowerInvariant(); or explicitly check
token/string before calling ToLower), then keep the existing null/empty check
and error return; apply the same defensive pattern to the corresponding
HandleCommand (or equivalent) methods in ManageShader.cs, ManageEditor.cs, and
ManageAsset.cs to ensure consistent null-safe behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs`:
- Around line 962-968: The code reads settings["spriteExtrude"] and blindly
casts ToObject<int>() to uint which will underflow on negative values; update
the logic in ManageTexture.cs around importerSettings.spriteExtrude so you first
parse the int (via extrudeToken.ToObject<int>()), validate/clamp it (e.g. if
value < 0 set to 0, and optionally cap to a sensible max), then assign the
validated value cast to uint (importerSettings.spriteExtrude =
(uint)validatedValue) and keep settingsChanged = true; also handle non-integer
tokens by checking extrudeToken.Type or wrapping the parse in a try/if to avoid
exceptions.
- Around line 118-242: The CreateTexture method leaks the Texture2D on exception
because texture is declared inside the try and not guaranteed to be destroyed;
move the Texture2D texture declaration to the outer scope (before the try) and
add a finally block that calls UnityEngine.Object.DestroyImmediate(texture) if
texture != null, mirroring the pattern used in ApplyGradient/ApplyNoise, and
ensure any early returns inside the try set texture to null only when it has
already been destroyed so the finally block is safe; update references to
texture, ResolveImagePath, ValidateDimensions, ConfigureTextureImporter,
ConfigureAsSprite, and TextureOps usage accordingly.
- Around line 255-332: ModifyTexture currently creates editableTexture inside
the try block so exceptions after creation leak it; move the declaration
Texture2D editableTexture = null to before the try (or ensure a finally block)
and assign the new Texture2D to that variable, then in a finally (or in the
catch) check if editableTexture != null and call
UnityEngine.Object.DestroyImmediate(editableTexture) to ensure the texture is
always cleaned up; update the catch to re-use the shared editableTexture
variable name so the resource is freed on all exception paths.
♻️ Duplicate comments (3)
MCPForUnity/Editor/Tools/ManageTexture.cs (3)
46-52: Add null check for@paramsparameter.While the null-propagation on line 48 was fixed,
@paramsitself is not validated. If@paramsis null,@params["action"]will throw aNullReferenceException.🛠️ Suggested fix
public static object HandleCommand(JObject `@params`) { + if (`@params` == null) + return new ErrorResponse("Parameters are required."); + string action = `@params`["action"]?.ToString()?.ToLowerInvariant();
227-237: FixasSpriteresponse to handle case-insensitivetextureType.Line 234 uses a case-sensitive comparison (
== "Sprite"), sotextureType: "sprite"will incorrectly reportasSprite: false. This matches the reviewer feedback about return-value mismatch.🔧 Suggested fix
+ bool importSprite = string.Equals( + importSettingsToken?["textureType"]?.ToString(), + "Sprite", + StringComparison.OrdinalIgnoreCase + ); return new SuccessResponse( $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), new { path = fullPath, width, height, - asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite"), + asSprite = asSprite || spriteSettingsToken != null || importSprite, warnings = warnings.Count > 0 ? warnings : null } );
347-350: Consider using ternary for conciseness.Both branches return, so a ternary expression would be more concise.
♻️ Optional refactor
- if (success) - return new SuccessResponse($"Texture deleted: {fullPath}"); - else - return new ErrorResponse($"Failed to delete texture: {fullPath}"); + return success + ? new SuccessResponse($"Texture deleted: {fullPath}") + : new ErrorResponse($"Failed to delete texture: {fullPath}");
🧹 Nitpick comments (3)
MCPForUnity/Editor/Tools/ManageTexture.cs (3)
478-527: Minor inconsistency: texture created outside try block.Unlike
ApplyGradientwhich declarestexture = nulland assigns inside try, here the texture is created at line 478 before the try. Both work correctly, but theApplyGradientpattern is slightly safer (handles constructor exceptions within the catch block). Consider aligning for consistency.
552-600: Consider defensive null check forpatternparameter.
GetPatternColorcallspattern.ToLower()at line 556 without a null check. While callers currently validate this, adding a guard or defaulting would make the helper more robust.♻️ Optional defensive fix
private static Color32 GetPatternColor(int x, int y, string pattern, List<Color32> palette, int size, int width, int height) { int colorIndex = 0; + if (string.IsNullOrEmpty(pattern)) + return palette[0]; switch (pattern.ToLower())
743-976: Consider extracting repetitive importer setting logic.
ConfigureTextureImporterfollows a repetitive pattern for each setting (check token → parse → apply). While readable, extracting helpers could reduce duplication. Low priority given the method's clarity.
| try | ||
| { | ||
| var fillColorToken = @params["fillColor"]; | ||
| var patternToken = @params["pattern"]; | ||
| var pixelsToken = @params["pixels"]; | ||
|
|
||
| if (hasImage && (fillColorToken != null || patternToken != null || pixelsToken != null)) | ||
| { | ||
| return new ErrorResponse("imagePath cannot be combined with fillColor, pattern, or pixels."); | ||
| } | ||
|
|
||
| int patternSize = 8; | ||
| if (!hasImage && patternToken != null) | ||
| { | ||
| patternSize = @params["patternSize"]?.ToObject<int>() ?? 8; | ||
| if (patternSize <= 0) | ||
| return new ErrorResponse("patternSize must be greater than 0."); | ||
| } | ||
|
|
||
| Texture2D texture; | ||
| if (hasImage) | ||
| { | ||
| string resolvedImagePath = ResolveImagePath(imagePath); | ||
| if (!File.Exists(resolvedImagePath)) | ||
| return new ErrorResponse($"Image file not found at '{imagePath}'."); | ||
|
|
||
| byte[] imageBytes = File.ReadAllBytes(resolvedImagePath); | ||
| texture = new Texture2D(2, 2, TextureFormat.RGBA32, false); | ||
| if (!texture.LoadImage(imageBytes)) | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(texture); | ||
| return new ErrorResponse($"Failed to load image from '{imagePath}'."); | ||
| } | ||
|
|
||
| width = texture.width; | ||
| height = texture.height; | ||
| var imageDimensionError = ValidateDimensions(width, height, warnings); | ||
| if (imageDimensionError != null) | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(texture); | ||
| return imageDimensionError; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| texture = new Texture2D(width, height, TextureFormat.RGBA32, false); | ||
|
|
||
| // Check for fill color | ||
| if (fillColorToken != null && fillColorToken.Type == JTokenType.Array) | ||
| { | ||
| Color32 fillColor = TextureOps.ParseColor32(fillColorToken as JArray); | ||
| TextureOps.FillTexture(texture, fillColor); | ||
| } | ||
|
|
||
| // Check for pattern | ||
| if (patternToken != null) | ||
| { | ||
| string pattern = patternToken.ToString(); | ||
| var palette = TextureOps.ParsePalette(@params["palette"] as JArray); | ||
| ApplyPatternToTexture(texture, pattern, palette, patternSize); | ||
| } | ||
|
|
||
| // Check for direct pixel data | ||
| if (pixelsToken != null) | ||
| { | ||
| TextureOps.ApplyPixelData(texture, pixelsToken, width, height); | ||
| } | ||
|
|
||
| // If nothing specified, create transparent texture | ||
| if (fillColorToken == null && patternToken == null && pixelsToken == null) | ||
| { | ||
| TextureOps.FillTexture(texture, new Color32(0, 0, 0, 0)); | ||
| } | ||
| } | ||
|
|
||
| texture.Apply(); | ||
|
|
||
| // Save to disk | ||
| byte[] imageData = TextureOps.EncodeTexture(texture, fullPath); | ||
| if (imageData == null || imageData.Length == 0) | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(texture); | ||
| return new ErrorResponse($"Failed to encode texture for '{fullPath}'"); | ||
| } | ||
| File.WriteAllBytes(GetAbsolutePath(fullPath), imageData); | ||
|
|
||
| AssetDatabase.ImportAsset(fullPath, ImportAssetOptions.ForceUpdate); | ||
|
|
||
| // Configure texture importer settings if provided | ||
| JToken importSettingsToken = @params["importSettings"]; | ||
| JToken spriteSettingsToken = @params["spriteSettings"]; | ||
|
|
||
| if (importSettingsToken != null) | ||
| { | ||
| ConfigureTextureImporter(fullPath, importSettingsToken); | ||
| } | ||
| else if (asSprite || spriteSettingsToken != null) | ||
| { | ||
| // Legacy sprite configuration | ||
| ConfigureAsSprite(fullPath, spriteSettingsToken); | ||
| } | ||
|
|
||
| // Clean up memory | ||
| UnityEngine.Object.DestroyImmediate(texture); | ||
| foreach (var warning in warnings) | ||
| { | ||
| McpLog.Warn($"[ManageTexture] {warning}"); | ||
| } | ||
|
|
||
| return new SuccessResponse( | ||
| $"Texture created at '{fullPath}' ({width}x{height})" + (asSprite ? " as sprite" : ""), | ||
| new | ||
| { | ||
| path = fullPath, | ||
| width, | ||
| height, | ||
| asSprite = asSprite || spriteSettingsToken != null || (importSettingsToken?["textureType"]?.ToString() == "Sprite"), | ||
| warnings = warnings.Count > 0 ? warnings : null | ||
| } | ||
| ); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| return new ErrorResponse($"Failed to create texture: {e.Message}"); | ||
| } |
There was a problem hiding this comment.
Texture leak on exception path in CreateTexture.
The texture variable is declared inside the try block (line 137), making it inaccessible in the catch block. If an exception occurs after texture creation (e.g., during File.WriteAllBytes or ConfigureTextureImporter) but before DestroyImmediate, the texture leaks.
Apply the same try/finally pattern used in ApplyGradient and ApplyNoise.
🐛 Proposed fix
string fullPath = AssetPathUtility.SanitizeAssetPath(path);
EnsureDirectoryExists(fullPath);
+ Texture2D texture = null;
try
{
var fillColorToken = `@params`["fillColor"];
var patternToken = `@params`["pattern"];
var pixelsToken = `@params`["pixels"];
// ... validation code ...
- Texture2D texture;
if (hasImage)
{
// ... image loading ...
texture = new Texture2D(2, 2, TextureFormat.RGBA32, false);
// ...
}
else
{
texture = new Texture2D(width, height, TextureFormat.RGBA32, false);
// ...
}
// ... rest of the method ...
- UnityEngine.Object.DestroyImmediate(texture);
-
return new SuccessResponse(
// ...
);
}
catch (Exception e)
{
return new ErrorResponse($"Failed to create texture: {e.Message}");
}
+ finally
+ {
+ if (texture != null)
+ UnityEngine.Object.DestroyImmediate(texture);
+ }🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs` around lines 118 - 242, The
CreateTexture method leaks the Texture2D on exception because texture is
declared inside the try and not guaranteed to be destroyed; move the Texture2D
texture declaration to the outer scope (before the try) and add a finally block
that calls UnityEngine.Object.DestroyImmediate(texture) if texture != null,
mirroring the pattern used in ApplyGradient/ApplyNoise, and ensure any early
returns inside the try set texture to null only when it has already been
destroyed so the finally block is safe; update references to texture,
ResolveImagePath, ValidateDimensions, ConfigureTextureImporter,
ConfigureAsSprite, and TextureOps usage accordingly.
| try | ||
| { | ||
| Texture2D texture = AssetDatabase.LoadAssetAtPath<Texture2D>(fullPath); | ||
| if (texture == null) | ||
| return new ErrorResponse($"Failed to load texture at path: {fullPath}"); | ||
|
|
||
| // Make the texture readable | ||
| string absolutePath = GetAbsolutePath(fullPath); | ||
| byte[] fileData = File.ReadAllBytes(absolutePath); | ||
| Texture2D editableTexture = new Texture2D(texture.width, texture.height, TextureFormat.RGBA32, false); | ||
| editableTexture.LoadImage(fileData); | ||
|
|
||
| // Apply modifications | ||
| var setPixelsToken = @params["setPixels"] as JObject; | ||
| if (setPixelsToken != null) | ||
| { | ||
| int x = setPixelsToken["x"]?.ToObject<int>() ?? 0; | ||
| int y = setPixelsToken["y"]?.ToObject<int>() ?? 0; | ||
| int w = setPixelsToken["width"]?.ToObject<int>() ?? 1; | ||
| int h = setPixelsToken["height"]?.ToObject<int>() ?? 1; | ||
|
|
||
| if (w <= 0 || h <= 0) | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(editableTexture); | ||
| return new ErrorResponse("setPixels width and height must be positive."); | ||
| } | ||
|
|
||
| var pixelsToken = setPixelsToken["pixels"]; | ||
| var colorToken = setPixelsToken["color"]; | ||
|
|
||
| if (pixelsToken != null) | ||
| { | ||
| TextureOps.ApplyPixelDataToRegion(editableTexture, pixelsToken, x, y, w, h); | ||
| } | ||
| else if (colorToken != null) | ||
| { | ||
| Color32 color = TextureOps.ParseColor32(colorToken as JArray); | ||
| int startX = Mathf.Max(0, x); | ||
| int startY = Mathf.Max(0, y); | ||
| int endX = Mathf.Min(x + w, editableTexture.width); | ||
| int endY = Mathf.Min(y + h, editableTexture.height); | ||
|
|
||
| for (int py = startY; py < endY; py++) | ||
| { | ||
| for (int px = startX; px < endX; px++) | ||
| { | ||
| editableTexture.SetPixel(px, py, color); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(editableTexture); | ||
| return new ErrorResponse("setPixels requires 'color' or 'pixels'."); | ||
| } | ||
| } | ||
|
|
||
| editableTexture.Apply(); | ||
|
|
||
| // Save back to disk | ||
| byte[] imageData = TextureOps.EncodeTexture(editableTexture, fullPath); | ||
| if (imageData == null || imageData.Length == 0) | ||
| { | ||
| UnityEngine.Object.DestroyImmediate(editableTexture); | ||
| return new ErrorResponse($"Failed to encode texture for '{fullPath}'"); | ||
| } | ||
| File.WriteAllBytes(absolutePath, imageData); | ||
|
|
||
| AssetDatabase.ImportAsset(fullPath, ImportAssetOptions.ForceUpdate); | ||
|
|
||
| UnityEngine.Object.DestroyImmediate(editableTexture); | ||
|
|
||
| return new SuccessResponse($"Texture modified: {fullPath}"); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| return new ErrorResponse($"Failed to modify texture: {e.Message}"); | ||
| } |
There was a problem hiding this comment.
Texture leak on exception path in ModifyTexture.
Same issue as CreateTexture: editableTexture is declared inside the try block (line 264), so the catch block cannot access it. Exceptions after texture creation will leak the texture.
🐛 Proposed fix
try
{
Texture2D texture = AssetDatabase.LoadAssetAtPath<Texture2D>(fullPath);
if (texture == null)
return new ErrorResponse($"Failed to load texture at path: {fullPath}");
string absolutePath = GetAbsolutePath(fullPath);
byte[] fileData = File.ReadAllBytes(absolutePath);
- Texture2D editableTexture = new Texture2D(texture.width, texture.height, TextureFormat.RGBA32, false);
+ Texture2D editableTexture = null;
+ try
+ {
+ editableTexture = new Texture2D(texture.width, texture.height, TextureFormat.RGBA32, false);
editableTexture.LoadImage(fileData);
// Apply modifications
// ... existing modification code ...
editableTexture.Apply();
byte[] imageData = TextureOps.EncodeTexture(editableTexture, fullPath);
if (imageData == null || imageData.Length == 0)
{
- UnityEngine.Object.DestroyImmediate(editableTexture);
return new ErrorResponse($"Failed to encode texture for '{fullPath}'");
}
File.WriteAllBytes(absolutePath, imageData);
AssetDatabase.ImportAsset(fullPath, ImportAssetOptions.ForceUpdate);
- UnityEngine.Object.DestroyImmediate(editableTexture);
-
return new SuccessResponse($"Texture modified: {fullPath}");
+ }
+ finally
+ {
+ if (editableTexture != null)
+ UnityEngine.Object.DestroyImmediate(editableTexture);
+ }
}
catch (Exception e)
{
return new ErrorResponse($"Failed to modify texture: {e.Message}");
}🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs` around lines 255 - 332,
ModifyTexture currently creates editableTexture inside the try block so
exceptions after creation leak it; move the declaration Texture2D
editableTexture = null to before the try (or ensure a finally block) and assign
the new Texture2D to that variable, then in a finally (or in the catch) check if
editableTexture != null and call
UnityEngine.Object.DestroyImmediate(editableTexture) to ensure the texture is
always cleaned up; update the catch to re-use the shared editableTexture
variable name so the resource is freed on all exception paths.
| // Sprite Extrude | ||
| var extrudeToken = settings["spriteExtrude"]; | ||
| if (extrudeToken != null) | ||
| { | ||
| importerSettings.spriteExtrude = (uint)extrudeToken.ToObject<int>(); | ||
| settingsChanged = true; | ||
| } |
There was a problem hiding this comment.
Validate spriteExtrude before casting to uint.
Line 966 casts ToObject<int>() to uint. A negative input value would underflow to a large uint. Consider clamping or validating.
🔧 Suggested fix
var extrudeToken = settings["spriteExtrude"];
if (extrudeToken != null)
{
- importerSettings.spriteExtrude = (uint)extrudeToken.ToObject<int>();
+ int extrudeValue = extrudeToken.ToObject<int>();
+ importerSettings.spriteExtrude = (uint)Mathf.Max(0, extrudeValue);
settingsChanged = true;
}📝 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.
| // Sprite Extrude | |
| var extrudeToken = settings["spriteExtrude"]; | |
| if (extrudeToken != null) | |
| { | |
| importerSettings.spriteExtrude = (uint)extrudeToken.ToObject<int>(); | |
| settingsChanged = true; | |
| } | |
| // Sprite Extrude | |
| var extrudeToken = settings["spriteExtrude"]; | |
| if (extrudeToken != null) | |
| { | |
| int extrudeValue = extrudeToken.ToObject<int>(); | |
| importerSettings.spriteExtrude = (uint)Mathf.Max(0, extrudeValue); | |
| settingsChanged = true; | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageTexture.cs` around lines 962 - 968, The code
reads settings["spriteExtrude"] and blindly casts ToObject<int>() to uint which
will underflow on negative values; update the logic in ManageTexture.cs around
importerSettings.spriteExtrude so you first parse the int (via
extrudeToken.ToObject<int>()), validate/clamp it (e.g. if value < 0 set to 0,
and optionally cap to a sensible max), then assign the validated value cast to
uint (importerSettings.spriteExtrude = (uint)validatedValue) and keep
settingsChanged = true; also handle non-integer tokens by checking
extrudeToken.Type or wrapping the parse in a try/if to avoid exceptions.
2 participants
Given the choice to generate Texture2D based on patterns and color, also introduce a pipeline to turn Texture2D directly into a Sprite. Update the CLI command to include this, too.
Description
Hi guys! Happy to share the latest command which is to directly generate Texture2D with MCP commands, and turn that Tex2D into Sprite. I was using Unity MCP for the gamejam and realize this function is not able yet, so I did a workaround to implement this.
Type of Change
Save your change type
Changes Made
Introduce Manage_Texture to the repo
Testing/Screenshots/Recordings
Directly calling the MCP tools or using the CLI are both tested. Basic testing on functions are conducted on my end (Unity6.2, Windows with latest version of MCP)
Documentation Updates
Additional Notes
@msanatan @dsarno I do want this to be shipped for the gamejam user quickly, as this can be a huge benefit for 2D developers! Let me know what you all think.
Summary by Sourcery
Add a new manage_texture tool and CLI commands for procedural texture generation and sprite creation, along with tests and documentation updates.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.