From 41cb82446a9eaab8acfddbf64c51a515c5868391 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 17:54:16 +0300 Subject: [PATCH 01/29] feat(sprite): add manage_sprite for 2D sprite sheet animation Nothing in the package covers 2D sprite animation today: slicing a sheet, turning the frames into AnimationClips, and wiring those into a controller are all manual editor work. manage_sprite adds five actions - get_info, slice_sheet, setup_clips, setup_controller and full_setup - built on the existing helpers and grouped with the other animation tools. Clip names drive the controller's shape: locomotion names collapse into a Speed-driven 1D blend tree, combat and object names each get a trigger, and an idle clip becomes the default state. Several details below are not obvious from the API, and each of them was settled by running the code on 6000.4.4f1 rather than by reading it: - The grid is measured only after the texture is imported as a Sprite. A Default-type import rescales a non-power-of-two sheet (96px becomes 128px), and a grid computed against that size puts the trailing frames outside the real texture, where Unity drops them without complaint. A 96x16 sheet asked for six columns yielded four sprites of 21px and still reported success. - The importer is marked dirty before the reimport. Assigning spritesheet on an importer that is already Multiple does not dirty it, so the reimport restores the previously serialised grid and slicing a sheet a second time leaves the first grid in place. - rows is rejected when it is zero or less. Reading it as `?? 1` only covers an absent key, so an explicit zero reached the texH / rows division and threw instead of answering. - Composed asset paths go through SanitizeAssetPath and honour its refusal. output_dir previously fell back to the raw value when the helper refused it, clip names were joined into a path unchecked, and a refused controller_path was dereferenced straight away. - Clip names are matched by word rather than by substring. The letters of 'hit' sit inside 'white' and those of 'run' inside 'grunt', which filed both clips under categories they do not belong to. Triggers are now named after the matched action, so hero_attack arms Attack rather than Hero. - A non-positive fps is refused. Keyframe times are i / fps, so it wrote a clip whose keys sat at infinity: accepted by Unity, impossible to play. --- MCPForUnity/Editor/Tools/Sprite2D.meta | 8 + .../Editor/Tools/Sprite2D/ManageSprite.cs | 47 ++++ .../Tools/Sprite2D/ManageSprite.cs.meta | 2 + .../Tools/Sprite2D/SpriteClipBuilder.cs | 197 ++++++++++++++++ .../Tools/Sprite2D/SpriteClipBuilder.cs.meta | 2 + .../Tools/Sprite2D/SpriteControllerBuilder.cs | 214 ++++++++++++++++++ .../Sprite2D/SpriteControllerBuilder.cs.meta | 2 + .../Tools/Sprite2D/SpriteDiagnostics.cs | 31 +++ .../Tools/Sprite2D/SpriteDiagnostics.cs.meta | 2 + .../Editor/Tools/Sprite2D/SpriteFullSetup.cs | 170 ++++++++++++++ .../Tools/Sprite2D/SpriteFullSetup.cs.meta | 2 + .../Tools/Sprite2D/SpriteImportSetup.cs | 182 +++++++++++++++ .../Tools/Sprite2D/SpriteImportSetup.cs.meta | 2 + .../Tools/Sprite2D/SpriteNamingDetector.cs | 135 +++++++++++ .../Sprite2D/SpriteNamingDetector.cs.meta | 2 + Server/src/services/tools/manage_sprite.py | 140 ++++++++++++ .../docs/reference/tools/animation/index.md | 1 + .../tools/animation/manage_sprite.md | 45 ++++ website/docs/reference/tools/index.md | 3 +- 19 files changed, 1186 insertions(+), 1 deletion(-) create mode 100644 MCPForUnity/Editor/Tools/Sprite2D.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs.meta create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs.meta create mode 100644 Server/src/services/tools/manage_sprite.py create mode 100644 website/docs/reference/tools/animation/manage_sprite.md diff --git a/MCPForUnity/Editor/Tools/Sprite2D.meta b/MCPForUnity/Editor/Tools/Sprite2D.meta new file mode 100644 index 000000000..574f9695a --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D.meta @@ -0,0 +1,8 @@ +fileFormatVersion: 2 +guid: e7ba99f77eb524525964129bb1fcd94c +folderAsset: yes +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs b/MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs new file mode 100644 index 000000000..79074be0c --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs @@ -0,0 +1,47 @@ +using Newtonsoft.Json.Linq; +using MCPForUnity.Editor.Helpers; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + [McpForUnityTool("manage_sprite", AutoRegister = false, Group = "animation")] + public static class ManageSprite + { + private static readonly string[] ValidActions = + { + "get_info", "slice_sheet", "setup_clips", + "setup_controller", "full_setup" + }; + + public static object HandleCommand(JObject @params) + { + string action = @params["action"]?.ToString()?.ToLowerInvariant(); + if (string.IsNullOrEmpty(action)) + return new ErrorResponse( + "'action' is required. Valid: " + string.Join(", ", ValidActions)); + + var diagnostics = new SpriteDiagnosticBuilder(); + + switch (action) + { + case "get_info": + return SpriteImportSetup.GetInfo(@params); + + case "slice_sheet": + return SpriteImportSetup.SliceSheet(@params, diagnostics); + + case "setup_clips": + return SpriteClipBuilder.SetupClips(@params, diagnostics); + + case "setup_controller": + return SpriteControllerBuilder.Build(@params, diagnostics); + + case "full_setup": + return SpriteFullSetup.Run(@params); + + default: + return new ErrorResponse( + $"Unknown action '{action}'. Valid: " + string.Join(", ", ValidActions)); + } + } + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs.meta new file mode 100644 index 000000000..9445f93fa --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/ManageSprite.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 636047e62387a46a39a2cdfd31172f8a \ No newline at end of file diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs new file mode 100644 index 000000000..22a40d48d --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -0,0 +1,197 @@ +using System.Collections.Generic; +using System.IO; +using System.Linq; +using Newtonsoft.Json.Linq; +using UnityEditor; +using UnityEngine; +using MCPForUnity.Editor.Helpers; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + internal static class SpriteClipBuilder + { + /// + /// Builds AnimationClips out of sliced sprites and saves them as .anim assets. + /// params: + /// path - sprite texture asset path + /// clips - [{name, start_frame, end_frame, fps (opt, def=12), loop (opt)}] + /// output_dir - where the clips are written (default: the sprite's own folder) + /// + public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnostics) + { + string path = @params["path"]?.ToString(); + if (string.IsNullOrEmpty(path)) + return new ErrorResponse("'path' is required."); + + path = AssetPathUtility.SanitizeAssetPath(path); + + var allSprites = AssetDatabase.LoadAllAssetsAtPath(path) + .OfType() + .OrderBy(s => NaturalSortKey(s.name)) + .ToArray(); + + if (allSprites.Length == 0) + return new ErrorResponse($"No sprites found at '{path}'. Run slice_sheet first."); + + var clipsToken = @params["clips"] as JArray; + if (clipsToken == null || clipsToken.Count == 0) + return new ErrorResponse("'clips' array is required."); + + string outputDir = @params["output_dir"]?.ToString() + ?? Path.GetDirectoryName(path)?.Replace('\\', '/') ?? "Assets"; + + // SanitizeAssetPath returns null when it refuses a path, so falling back to the + // raw value would hand traversal sequences straight through. + outputDir = AssetPathUtility.SanitizeAssetPath(outputDir); + if (outputDir == null) + return new ErrorResponse("'output_dir' must stay under Assets/ and cannot contain '..'."); + if (!AssetDatabase.IsValidFolder(outputDir)) + CreateFolders(outputDir); + + var createdClips = new List(); + + foreach (JObject clipDef in clipsToken) + { + string clipName = clipDef["name"]?.ToString(); + if (string.IsNullOrEmpty(clipName)) + { diagnostics.AddWarning("CLIP_NO_NAME", "Clip name is missing — skipped.", null, new[] { "Add a 'name' field to each clip definition." }); continue; } + + int startFrame = clipDef["start_frame"]?.ToObject() ?? 0; + int endFrame = clipDef["end_frame"]?.ToObject() ?? allSprites.Length - 1; + float fps = clipDef["fps"]?.ToObject() ?? 12f; + if (fps <= 0f) + { + // Keyframe times are i / fps, so a non-positive rate writes a clip whose + // keys sit at infinity - accepted by Unity, useless to play. + diagnostics.AddWarning("CLIP_BAD_FPS", $"Clip '{clipName}': fps must be greater than 0, got {fps} - skipped.", null, new[] { "Leave fps out to use the default of 12." }); + continue; + } + + var entry = SpriteNamingDetector.Detect(clipName); + bool loop = clipDef["loop"]?.ToObject() ?? entry.Loop; + + var frameSprites = allSprites.Skip(startFrame).Take(endFrame - startFrame + 1).ToArray(); + if (frameSprites.Length == 0) + { + diagnostics.AddWarning("CLIP_EMPTY", $"Clip '{clipName}': no frames in range [{startFrame},{endFrame}].", null, new[] { "Check start_frame/end_frame against total sprite count." }); + continue; + } + + if (frameSprites.Length <= 2) + diagnostics.AddWarning("LOW_FRAME_COUNT", $"Clip '{clipName}' has only {frameSprites.Length} frame(s) — animation may not be visible.", null, new string[0]); + + var clip = new AnimationClip { frameRate = fps }; + + var binding = new EditorCurveBinding + { + type = typeof(SpriteRenderer), + path = "", + propertyName = "m_Sprite", + }; + + var keyframes = new ObjectReferenceKeyframe[frameSprites.Length]; + for (int i = 0; i < frameSprites.Length; i++) + { + keyframes[i] = new ObjectReferenceKeyframe + { + time = i / fps, + value = frameSprites[i], + }; + } + + AnimationUtility.SetObjectReferenceCurve(clip, binding, keyframes); + + var settings = AnimationUtility.GetAnimationClipSettings(clip); + settings.loopTime = loop; + AnimationUtility.SetAnimationClipSettings(clip, settings); + + string clipPath = AssetPathUtility.SanitizeAssetPath($"{outputDir}/{clipName}.anim"); + if (clipPath == null) + { + diagnostics.AddWarning("CLIP_BAD_NAME", $"Clip '{clipName}': the name cannot be used as a file name - skipped.", null, new[] { "Remove '..' and path separators from the clip name." }); + continue; + } + var existing = AssetDatabase.LoadAssetAtPath(clipPath); + if (existing != null) AssetDatabase.DeleteAsset(clipPath); + + AssetDatabase.CreateAsset(clip, clipPath); + + createdClips.Add(new + { + name = clipName, + path = clipPath, + frame_count = frameSprites.Length, + fps, + loop, + duration = frameSprites.Length / fps, + }); + } + + AssetDatabase.SaveAssets(); + + return new + { + success = true, + sprite_path = path, + clip_count = createdClips.Count, + clips = createdClips, + diagnostics = diagnostics.Build(), + }; + } + + // ── Internal helper ────────────────────────────────────────────────── + + /// + /// For full_setup: returns the clip name to asset path mapping. + /// + internal static List<(string name, string path)> GetClipPaths(JArray clipsToken, string outputDir) + { + var result = new List<(string, string)>(); + foreach (JObject cd in clipsToken) + { + string name = cd["name"]?.ToString(); + if (string.IsNullOrEmpty(name)) continue; + // Must match the path SetupClips wrote, refusals included. + string clipPath = AssetPathUtility.SanitizeAssetPath($"{outputDir}/{name}.anim"); + if (clipPath != null) + result.Add((name, clipPath)); + } + return result; + } + + internal static AnimationClip LoadClip(string clipPath) => + AssetDatabase.LoadAssetAtPath(clipPath); + + // Plain string sort puts hero_10 before hero_2, which reorders the animation. + private static string NaturalSortKey(string name) + { + var sb = new System.Text.StringBuilder(); + int i = 0; + while (i < name.Length) + { + if (char.IsDigit(name[i])) + { + int start = i; + while (i < name.Length && char.IsDigit(name[i])) i++; + // Left-pad the run of digits so a lexicographic sort compares them numerically. + sb.Append(name.Substring(start, i - start).PadLeft(10, '0')); + } + else + { + sb.Append(name[i++]); + } + } + return sb.ToString(); + } + + private static void CreateFolders(string path) + { + string parent = Path.GetDirectoryName(path)?.Replace('\\', '/') ?? "Assets"; + if (!AssetDatabase.IsValidFolder(parent)) + CreateFolders(parent); + string folderName = Path.GetFileName(path); + if (!string.IsNullOrEmpty(folderName)) + AssetDatabase.CreateFolder(parent, folderName); + } + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs.meta new file mode 100644 index 000000000..35d61a4a3 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 8e9b1196056fa41d5ba138f1dba9d544 \ No newline at end of file diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs new file mode 100644 index 000000000..6ef686b73 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs @@ -0,0 +1,214 @@ +using System.Collections.Generic; +using System.IO; +using System.Linq; +using Newtonsoft.Json.Linq; +using UnityEditor; +using UnityEditor.Animations; +using UnityEngine; +using MCPForUnity.Editor.Helpers; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + internal static class SpriteControllerBuilder + { + /// + /// params: + /// clips - [{name, path}] where path is an .anim asset path + /// controller_path - output .controller path (required) + /// overwrite - bool (default false) + /// + public static object Build(JObject @params, SpriteDiagnosticBuilder diagnostics) + { + var clipsToken = @params["clips"] as JArray; + if (clipsToken == null || clipsToken.Count == 0) + return new ErrorResponse("'clips' array is required."); + + string controllerPath = @params["controller_path"]?.ToString(); + if (string.IsNullOrEmpty(controllerPath)) + return new ErrorResponse("'controller_path' is required."); + + controllerPath = AssetPathUtility.SanitizeAssetPath(controllerPath); + if (controllerPath == null) + return new ErrorResponse("'controller_path' must stay under Assets/ and cannot contain '..'."); + if (!controllerPath.EndsWith(".controller")) + controllerPath += ".controller"; + + bool overwrite = @params["overwrite"]?.ToObject() ?? false; + + if (AssetDatabase.LoadAssetAtPath(controllerPath) != null) + { + if (!overwrite) + { + diagnostics.AddError( + "CONTROLLER_EXISTS", + $"Controller already exists at '{controllerPath}'.", + new { path = controllerPath }, + new[] { "Set overwrite=true to replace it." } + ); + return new { success = false, diagnostics = diagnostics.Build() }; + } + AssetDatabase.DeleteAsset(controllerPath); + } + + string dir = Path.GetDirectoryName(controllerPath)?.Replace('\\', '/'); + if (!string.IsNullOrEmpty(dir) && !AssetDatabase.IsValidFolder(dir)) + CreateFolders(dir); + + var entries = new List<(SpriteAnimEntry entry, AnimationClip clip)>(); + foreach (JObject cd in clipsToken) + { + string clipName = cd["name"]?.ToString() ?? ""; + string clipPath = cd["path"]?.ToString() ?? ""; + var clip = AssetDatabase.LoadAssetAtPath( + AssetPathUtility.SanitizeAssetPath(clipPath)); + if (clip == null) + { diagnostics.AddWarning("CLIP_NOT_FOUND", $"Clip '{clipName}' not found at '{clipPath}' — skipped.", null, new string[0]); continue; } + entries.Add((SpriteNamingDetector.Detect(clipName), clip)); + } + + if (entries.Count == 0) + return new ErrorResponse("No valid clips loaded."); + + var complexity = SpriteNamingDetector.DecideComplexity(entries.Select(e => e.entry)); + var controller = AnimatorController.CreateAnimatorControllerAtPath(controllerPath); + var rootSM = controller.layers[0].stateMachine; + + // ── Parameters ────────────────────────────────────────────────── + + if (complexity == ControllerComplexity.BlendTree1D || complexity == ControllerComplexity.Full) + controller.AddParameter("Speed", AnimatorControllerParameterType.Float); + + var triggerNames = entries + .Where(e => !string.IsNullOrEmpty(e.entry.TriggerName) && + (e.entry.Category == SpriteAnimCategory.Combat || + e.entry.Category == SpriteAnimCategory.Jump || + e.entry.Category == SpriteAnimCategory.Object)) + .Select(e => e.entry.TriggerName) + .Distinct(); + foreach (var t in triggerNames) + controller.AddParameter(t, AnimatorControllerParameterType.Trigger); + + // ── Idle state ──────────────────────────────────────────────────── + + var idlePair = entries.FirstOrDefault(e => e.entry.Category == SpriteAnimCategory.Idle); + AnimatorState idleState = null; + if (idlePair.clip != null) + { + idleState = rootSM.AddState("Idle"); + idleState.motion = idlePair.clip; + rootSM.defaultState = idleState; + } + + // ── Locomotion ──────────────────────────────────────────────────── + + var locomotionPairs = entries.Where(e => e.entry.Category == SpriteAnimCategory.Locomotion).ToList(); + if (locomotionPairs.Count > 0) + { + if (locomotionPairs.Count == 1) + { + // A single locomotion clip: one plain state. + var locoState = rootSM.AddState(locomotionPairs[0].entry.ClipName); + locoState.motion = locomotionPairs[0].clip; + if (rootSM.defaultState == null) rootSM.defaultState = locoState; + if (idleState != null) + { + var t1 = idleState.AddTransition(locoState); + t1.AddCondition(AnimatorConditionMode.Greater, 0.1f, "Speed"); + t1.hasExitTime = false; + var t2 = locoState.AddTransition(idleState); + t2.AddCondition(AnimatorConditionMode.Less, 0.1f, "Speed"); + t2.hasExitTime = false; + } + } + else + { + // More than one locomotion clip: a 1D blend tree. + var blendState = rootSM.AddState("Locomotion"); + var blendTree = new BlendTree { name = "LocomotionTree", blendType = BlendTreeType.Simple1D, blendParameter = "Speed" }; + AssetDatabase.AddObjectToAsset(blendTree, controllerPath); + + foreach (var pair in locomotionPairs.OrderBy(p => p.entry.BlendValue)) + blendTree.AddChild(pair.clip, pair.entry.BlendValue); + + blendState.motion = blendTree; + if (rootSM.defaultState == null) rootSM.defaultState = blendState; + + if (idleState != null) + { + var t1 = idleState.AddTransition(blendState); + t1.AddCondition(AnimatorConditionMode.Greater, 0.1f, "Speed"); + t1.hasExitTime = false; + var t2 = blendState.AddTransition(idleState); + t2.AddCondition(AnimatorConditionMode.Less, 0.1f, "Speed"); + t2.hasExitTime = false; + } + } + } + + // ── Trigger states (combat, jump, object) ───────────────────────── + + var triggerPairs = entries.Where(e => + e.entry.Category == SpriteAnimCategory.Combat || + e.entry.Category == SpriteAnimCategory.Jump || + e.entry.Category == SpriteAnimCategory.Object).ToList(); + + foreach (var pair in triggerPairs) + { + var state = rootSM.AddState(pair.entry.ClipName); + state.motion = pair.clip; + + string trigger = pair.entry.TriggerName ?? pair.entry.ClipName; + + foreach (var existingState in rootSM.states.Select(s => s.state)) + { + if (existingState == state) continue; + var tr = existingState.AddTransition(state); + tr.AddCondition(AnimatorConditionMode.If, 0, trigger); + tr.hasExitTime = false; + } + + // A one-shot state has to hand control back, so it exits to idle on its own. + if (idleState != null && !pair.entry.Loop) + { + var exitTr = state.AddTransition(idleState); + exitTr.hasExitTime = true; + exitTr.exitTime = 1f; + exitTr.hasFixedDuration = false; + } + } + + // ── Generic / single animation ─────────────────────────────────────── + + foreach (var pair in entries.Where(e => e.entry.Category == SpriteAnimCategory.Generic)) + { + var state = rootSM.AddState(pair.entry.ClipName); + state.motion = pair.clip; + if (rootSM.defaultState == null) + rootSM.defaultState = state; + } + + AssetDatabase.SaveAssets(); + EditorUtility.SetDirty(controller); + AssetDatabase.SaveAssets(); + + return new + { + success = true, + controller_path = controllerPath, + complexity = complexity.ToString(), + state_count = rootSM.states.Length, + diagnostics = diagnostics.Build(), + }; + } + + private static void CreateFolders(string path) + { + string parent = Path.GetDirectoryName(path)?.Replace('\\', '/') ?? "Assets"; + if (!AssetDatabase.IsValidFolder(parent)) + CreateFolders(parent); + string folderName = Path.GetFileName(path); + if (!string.IsNullOrEmpty(folderName)) + AssetDatabase.CreateFolder(parent, folderName); + } + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs.meta new file mode 100644 index 000000000..56de6ad80 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 41822fd4062094cf38edf541771a53d2 \ No newline at end of file diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs new file mode 100644 index 000000000..704840b63 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs @@ -0,0 +1,31 @@ +using System.Collections.Generic; +using System.Linq; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + internal class SpriteDiagnostic + { + public string code; + public string severity; + public string message; + public object detail; + public string[] fix_options; + } + + internal class SpriteDiagnosticBuilder + { + private readonly List _list = new List(); + public bool HasErrors => _list.Any(d => d.severity == "error"); + + public void AddError(string code, string message, object detail, string[] fixes) => + _list.Add(new SpriteDiagnostic { code = code, severity = "error", message = message, detail = detail, fix_options = fixes }); + + public void AddWarning(string code, string message, object detail, string[] fixes) => + _list.Add(new SpriteDiagnostic { code = code, severity = "warning", message = message, detail = detail, fix_options = fixes }); + + public void AddInfo(string code, string message, object detail) => + _list.Add(new SpriteDiagnostic { code = code, severity = "info", message = message, detail = detail, fix_options = new string[0] }); + + public List Build() => new List(_list); + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs.meta new file mode 100644 index 000000000..d2720d644 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteDiagnostics.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 54884f7a243d94239b8ad49db451e83d \ No newline at end of file diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs new file mode 100644 index 000000000..d0ee321e0 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs @@ -0,0 +1,170 @@ +using System.Collections.Generic; +using System.IO; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using UnityEditor; +using MCPForUnity.Editor.Helpers; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + internal static class SpriteFullSetup + { + /// + /// params: + /// path - sprite texture path (required) + /// cols - grid columns (required) + /// rows - grid rows (default 1) + /// frame_width - alternative to cols: explicit frame size + /// frame_height - alternative to rows: explicit frame size + /// clips - [{name, start_frame, end_frame, fps, loop}]; + /// omitted means every frame becomes one clip named animation_name + /// animation_name - used when clips is omitted (default: the file name) + /// controller_path - default: the sprite's own folder + /// overwrite - bool (default false) + /// add_to_scene - add an Animator to a target GameObject + /// scene_target - GameObject name + /// + public static object Run(JObject @params) + { + string path = @params["path"]?.ToString(); + if (string.IsNullOrEmpty(path)) + return new ErrorResponse("'path' is required."); + + path = AssetPathUtility.SanitizeAssetPath(path); + if (!AssetDatabase.AssetPathExists(path)) + return new ErrorResponse($"Sprite not found: '{path}'"); + + var diagnostics = new SpriteDiagnosticBuilder(); + + // ── Step 1: Slice ────────────────────────────────────────────────── + + var sliceResult = SpriteImportSetup.SliceSheet(@params, diagnostics); + if (sliceResult is ErrorResponse) + return new { success = false, step = "slice_sheet", error = ((ErrorResponse)sliceResult).Error, diagnostics = diagnostics.Build() }; + if (diagnostics.HasErrors) + return new { success = false, step = "slice_sheet", diagnostics = diagnostics.Build() }; + + // ── Step 2: Clips ────────────────────────────────────────────────── + + string outputDir = @params["output_dir"]?.ToString() + ?? Path.GetDirectoryName(path)?.Replace('\\', '/') ?? "Assets"; + + var clipsToken = @params["clips"] as JArray; + if (clipsToken == null || clipsToken.Count == 0) + { + // No clips given: one clip spanning every frame. + string animName = @params["animation_name"]?.ToString() + ?? Path.GetFileNameWithoutExtension(path); + int totalFrames = GetSliceCount(path); + clipsToken = new JArray(new JObject + { + ["name"] = animName, + ["start_frame"] = 0, + ["end_frame"] = totalFrames - 1, + ["fps"] = 12, + }); + } + + var clipsParams = new JObject + { + ["path"] = path, + ["clips"] = clipsToken, + ["output_dir"] = outputDir, + }; + var clipResult = SpriteClipBuilder.SetupClips(clipsParams, diagnostics); + if (clipResult is ErrorResponse) + return new { success = false, step = "setup_clips", error = ((ErrorResponse)clipResult).Error, diagnostics = diagnostics.Build() }; + if (diagnostics.HasErrors) + return new { success = false, step = "setup_clips", diagnostics = diagnostics.Build() }; + + // ── Step 3: Controller ───────────────────────────────────────────── + + string controllerPath = @params["controller_path"]?.ToString() + ?? $"{outputDir}/{Path.GetFileNameWithoutExtension(path)}_Controller.controller"; + bool overwrite = @params["overwrite"]?.ToObject() ?? false; + + // Derive the paths from clipsToken directly - the builder's return value is an + // anonymous object, and parsing it back would be the long way round. + var clipPaths = SpriteClipBuilder.GetClipPaths(clipsToken, outputDir); + var createdClips = new JArray(); + foreach (var (cname, cpath) in clipPaths) + createdClips.Add(new JObject { ["name"] = cname, ["path"] = cpath }); + + var ctrlParams = new JObject + { + ["clips"] = createdClips, + ["controller_path"] = controllerPath, + ["overwrite"] = overwrite, + }; + var ctrlResult = SpriteControllerBuilder.Build(ctrlParams, diagnostics); + + // The builder returns ErrorResponse (not a throw) for cases like "No valid clips + // loaded", so the failure has to be checked for explicitly. + if (ctrlResult is ErrorResponse) + return new { success = false, step = "setup_controller", + error = ((ErrorResponse)ctrlResult).Error, diagnostics = diagnostics.Build() }; + + // ── Step 4: Add to scene ─────────────────────────────────────────── + + bool addToScene = @params["add_to_scene"]?.ToObject() ?? false; + string sceneTarget = @params["scene_target"]?.ToString(); + + if (addToScene && !string.IsNullOrEmpty(sceneTarget)) + { + var go = UnityEngine.GameObject.Find(sceneTarget); + if (go != null) + { + var controller = AssetDatabase.LoadAssetAtPath( + AssetPathUtility.SanitizeAssetPath(controllerPath)); + if (controller != null) + { + var animator = go.GetComponent() + ?? go.AddComponent(); + animator.runtimeAnimatorController = controller; + diagnostics.AddInfo("SCENE_ANIMATOR_SET", + $"Animator set on '{sceneTarget}'.", new { target = sceneTarget }); + } + } + else + { + diagnostics.AddWarning("SCENE_TARGET_NOT_FOUND", + $"GameObject '{sceneTarget}' not found in scene.", + null, + new[] { "Check GameObject name or open the correct scene first." }); + } + } + + // ctrlResult is an anonymous object, so round-trip it through JSON to read two fields. + string complexity = null; + int stateCount = 0; + try + { + var ctrlJson = JsonConvert.SerializeObject(ctrlResult); + var ctrlObj = JObject.Parse(ctrlJson); + complexity = ctrlObj["complexity"]?.ToString(); + stateCount = ctrlObj["state_count"]?.ToObject() ?? 0; + } + catch { /* non-critical */ } + + return new + { + success = !diagnostics.HasErrors, + sprite_path = path, + controller_path = controllerPath, + controller_complexity = complexity, + state_count = stateCount, + clip_count = clipPaths.Count, + diagnostics = diagnostics.Build(), + }; + } + + private static int GetSliceCount(string path) + { + var sprites = AssetDatabase.LoadAllAssetsAtPath(path); + int count = 0; + foreach (var a in sprites) + if (a is UnityEngine.Sprite) count++; + return count > 0 ? count : 1; + } + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs.meta new file mode 100644 index 000000000..d72bd8bf3 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 2a02bdfbdc3b049aa81ab404b6e48590 \ No newline at end of file diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs new file mode 100644 index 000000000..660c011f4 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -0,0 +1,182 @@ +using System; +using System.IO; +using System.Linq; +using Newtonsoft.Json.Linq; +using UnityEditor; +using UnityEngine; +using MCPForUnity.Editor.Helpers; +// TextureImporter.spritesheet is obsolete as of Unity 6, but the replacement +// (ISpriteEditorDataProvider) needs the 2D Sprite package and a good deal more setup for the +// same result. Revisit if the property is actually removed. +#pragma warning disable CS0618 + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + internal static class SpriteImportSetup + { + // ── GetInfo ────────────────────────────────────────────────────────── + + public static object GetInfo(JObject @params) + { + string path = @params["path"]?.ToString(); + if (string.IsNullOrEmpty(path)) + return new ErrorResponse("'path' is required."); + + path = AssetPathUtility.SanitizeAssetPath(path); + var importer = AssetImporter.GetAtPath(path) as TextureImporter; + if (importer == null) + return new ErrorResponse($"No TextureImporter found at '{path}'. Is it a texture/sprite?"); + + var texture = AssetDatabase.LoadAssetAtPath(path); + int w = texture != null ? texture.width : 0; + int h = texture != null ? texture.height : 0; + + var existingSlices = importer.spritesheet.Select(s => new + { + name = s.name, + x = (int)s.rect.x, + y = (int)s.rect.y, + width = (int)s.rect.width, + height = (int)s.rect.height, + }).ToArray(); + + // Base64 payload so a vision-capable caller can read the grid off the image. + string imageBase64 = null; + try + { + string fullPath = Path.Combine( + Application.dataPath.Replace("/Assets", ""), + path + ); + if (File.Exists(fullPath)) + { + byte[] bytes = File.ReadAllBytes(fullPath); + string ext = Path.GetExtension(path).ToLowerInvariant(); + string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; + imageBase64 = $"data:{mime};base64," + Convert.ToBase64String(bytes); + } + } + catch { /* The base64 payload is optional; leaving it null is a valid answer. */ } + + var result = new + { + success = true, + path, + width = w, + height = h, + sprite_mode = importer.spriteImportMode.ToString(), + pixels_per_unit = importer.spritePixelsPerUnit, + filter_mode = importer.filterMode.ToString(), + slice_count = existingSlices.Length, + slices = existingSlices, + image_base64 = imageBase64, + }; + + return result; + } + + // ── SliceSheet ─────────────────────────────────────────────────────── + + public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnostics) + { + string path = @params["path"]?.ToString(); + if (string.IsNullOrEmpty(path)) + return new ErrorResponse("'path' is required."); + + path = AssetPathUtility.SanitizeAssetPath(path); + var importer = AssetImporter.GetAtPath(path) as TextureImporter; + if (importer == null) + return new ErrorResponse($"No TextureImporter found at '{path}'."); + + // Measure the texture only once it is imported the way a sprite sheet is. + // A Default-type import rescales a non-power-of-two sheet (96px becomes 128px), + // and a grid computed against that size puts the trailing frames outside the real + // texture, where Unity drops them without an error. Measured on 6000.4.4f1: a + // 96x16 sheet asked for 6 columns produced 4 sprites of 21px. + if (importer.textureType != TextureImporterType.Sprite) + { + importer.textureType = TextureImporterType.Sprite; + EditorUtility.SetDirty(importer); + importer.SaveAndReimport(); + } + + var texture = AssetDatabase.LoadAssetAtPath(path); + if (texture == null) + return new ErrorResponse($"Could not load texture at '{path}'."); + + int texW = texture.width; + int texH = texture.height; + + int cols = @params["cols"]?.ToObject() ?? 0; + int rows = @params["rows"]?.ToObject() ?? 1; + int frameW = @params["frame_width"]?.ToObject() ?? 0; + int frameH = @params["frame_height"]?.ToObject() ?? 0; + + if (cols <= 0 && frameW <= 0) + return new ErrorResponse("Either 'cols' or 'frame_width' is required."); + + // `?? 1` above only covers an absent key, so an explicit rows=0 reaches the + // texH / rows division below and throws instead of answering. + if (rows <= 0 && frameH <= 0) + return new ErrorResponse("'rows' must be 1 or more; pass 'frame_height' instead if the row count is unknown."); + + if (frameW <= 0) frameW = texW / cols; + if (frameH <= 0) frameH = texH / rows; + if (cols <= 0) cols = texW / frameW; + if (rows <= 0) rows = texH / frameH; + + int totalFrames = cols * rows; + if (totalFrames == 0) + { + diagnostics.AddError( + "SLICE_EMPTY", + "The grid works out to 0 frames - cols/rows or the frame size is wrong.", + new { cols, rows, frame_width = frameW, frame_height = frameH, texture_width = texW, texture_height = texH }, + new[] { "Check the cols and rows values", "Confirm the texture dimensions with get_info" } + ); + return new { success = false, diagnostics = diagnostics.Build() }; + } + + string baseName = @params["base_name"]?.ToString() + ?? Path.GetFileNameWithoutExtension(path); + + var metas = new SpriteMetaData[totalFrames]; + for (int r = 0; r < rows; r++) + { + for (int c = 0; c < cols; c++) + { + int i = r * cols + c; + metas[i] = new SpriteMetaData + { + name = $"{baseName}_{i}", + rect = new Rect(c * frameW, texH - (r + 1) * frameH, frameW, frameH), + pivot = new Vector2(0.5f, 0.5f), + alignment = 0, + }; + } + } + + importer.spriteImportMode = SpriteImportMode.Multiple; + importer.spritesheet = metas; + importer.filterMode = FilterMode.Point; // pixel-perfect default + // Assigning spritesheet on an importer that is already Multiple does not mark it + // dirty, so SaveAndReimport would re-import the previously serialised grid and the + // new one would be silently dropped. Measured on 6000.4.4f1: without this, slicing + // a second time leaves the first grid in place. + EditorUtility.SetDirty(importer); + importer.SaveAndReimport(); + + return new + { + success = true, + path, + cols, + rows, + frame_width = frameW, + frame_height = frameH, + total_frames = totalFrames, + diagnostics = diagnostics.Build(), + }; + } + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs.meta new file mode 100644 index 000000000..88b6df14b --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 9f82d8c42bec5436fb4bdef16db29f93 \ No newline at end of file diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs new file mode 100644 index 000000000..9bf6b870f --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs @@ -0,0 +1,135 @@ +using System.Collections.Generic; +using System.Linq; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + internal enum SpriteAnimCategory + { + Idle, + Locomotion, // walk or run: a candidate for a 1D blend tree. + Jump, + Combat, // attack, slash, combo and the like: a trigger state. + Object, // open, close, activate: a single state. + Generic, + } + + internal enum ControllerComplexity + { + Single, // a lone animation, or an object/generic name: one plain state. + BlendTree1D, // locomotion: a 1D blend tree driven by a Speed float. + StateMachine, // combat present: trigger states. + Full, // locomotion + combat: a blend tree plus trigger states. + } + + internal class SpriteAnimEntry + { + public string ClipName; + public SpriteAnimCategory Category; + public bool Loop; + public string TriggerName; + public float BlendValue; // Position on the 1D blend tree: walk=1, run=2. + } + + internal static class SpriteNamingDetector + { + public static SpriteAnimEntry Detect(string clipName) + { + string lower = clipName.ToLowerInvariant(); + var entry = new SpriteAnimEntry { ClipName = clipName }; + Categorize(lower, entry); + entry.Loop = AutoDetectLoop(entry.Category); + return entry; + } + + public static ControllerComplexity DecideComplexity(IEnumerable entries) + { + bool hasLocomotion = entries.Any(e => e.Category == SpriteAnimCategory.Locomotion); + bool hasCombat = entries.Any(e => e.Category == SpriteAnimCategory.Combat); + + if (hasLocomotion && hasCombat) return ControllerComplexity.Full; + if (hasLocomotion) return ControllerComplexity.BlendTree1D; + if (hasCombat) return ControllerComplexity.StateMachine; + return ControllerComplexity.Single; + } + + // ── Private ────────────────────────────────────────────────────────── + + private static void Categorize(string lower, SpriteAnimEntry entry) + { + var words = Words(lower); + + if (Has(words, "idle", "stand")) + { entry.Category = SpriteAnimCategory.Idle; return; } + + if (words.Contains("walk")) + { entry.Category = SpriteAnimCategory.Locomotion; entry.BlendValue = 1f; return; } + + if (Has(words, "run", "sprint")) + { entry.Category = SpriteAnimCategory.Locomotion; entry.BlendValue = 2f; return; } + + string hit = Match(words, "jump", "fall", "land"); + if (hit != null) + { entry.Category = SpriteAnimCategory.Jump; entry.TriggerName = Capitalize(hit); return; } + + hit = Match(words, "attack", "slash", "punch", "combo", "cast", "shoot"); + if (hit != null) + { entry.Category = SpriteAnimCategory.Combat; entry.TriggerName = Capitalize(hit); return; } + + hit = Match(words, "open", "close", "activate", "die", "death", "hurt", "hit"); + if (hit != null) + { entry.Category = SpriteAnimCategory.Object; entry.TriggerName = Capitalize(hit); return; } + + entry.Category = SpriteAnimCategory.Generic; + entry.TriggerName = Capitalize(lower); + } + + /// + /// The words in a clip name, split on separators, camelCase humps and letter/digit + /// boundaries. Matching on raw substrings instead files 'white_flash' under 'hit' + /// and 'drunk_walk' under 'run', which then shapes the controller around a category + /// the clip never belonged to. + /// + private static HashSet Words(string name) + { + var words = new HashSet(); + var word = new System.Text.StringBuilder(); + + for (int i = 0; i < name.Length; i++) + { + char c = name[i]; + bool breaks = !char.IsLetterOrDigit(c) + || (i > 0 && char.IsUpper(c) && char.IsLower(name[i - 1])) + || (i > 0 && char.IsDigit(c) && char.IsLetter(name[i - 1])) + || (i > 0 && char.IsLetter(c) && char.IsDigit(name[i - 1])); + + if (breaks && word.Length > 0) + { + words.Add(word.ToString().ToLowerInvariant()); + word.Clear(); + } + if (char.IsLetterOrDigit(c)) word.Append(c); + } + if (word.Length > 0) words.Add(word.ToString().ToLowerInvariant()); + + return words; + } + + private static bool Has(HashSet words, params string[] keys) => + Match(words, keys) != null; + + /// The first key the name actually contains, so a trigger is named after the + /// action rather than after whatever happened to come first in the clip name. + private static string Match(HashSet words, params string[] keys) + { + foreach (string k in keys) + if (words.Contains(k)) return k; + return null; + } + + private static bool AutoDetectLoop(SpriteAnimCategory cat) => + cat == SpriteAnimCategory.Idle || cat == SpriteAnimCategory.Locomotion; + + private static string Capitalize(string s) => + string.IsNullOrEmpty(s) ? s : char.ToUpperInvariant(s[0]) + s.Substring(1); + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs.meta new file mode 100644 index 000000000..53d29cdac --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 59e42167a60fa4590b40c0c6edaca5f9 \ No newline at end of file diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py new file mode 100644 index 000000000..23318cf01 --- /dev/null +++ b/Server/src/services/tools/manage_sprite.py @@ -0,0 +1,140 @@ +""" +2D sprite animation tool. +Automates: sprite sheet slicing, AnimationClip creation from sliced frames, +and AnimatorController generation. +""" +from typing import Annotated, Any, Literal + +from fastmcp import Context +from mcp.types import ToolAnnotations + +from services.registry import mcp_for_unity_tool +from services.tools import get_unity_instance_from_context +from transport.unity_transport import send_with_unity_instance +from transport.legacy.unity_connection import async_send_command_with_retry + +VALID_ACTIONS = [ + "get_info", + "slice_sheet", + "setup_clips", + "setup_controller", + "full_setup", +] + + +@mcp_for_unity_tool( + group="animation", + description=( + "2D sprite animation tool. " + "get_info: read sprite import settings + return image for vision analysis. " + "slice_sheet: apply grid slicing to a sprite sheet. " + "setup_clips: create AnimationClips from sliced sprites. " + "setup_controller: build AnimatorController with smart complexity (1D blend tree for locomotion, " + "trigger states for combat, simple state for single animations). " + "full_setup: one command — slice → clips → controller." + ), + annotations=ToolAnnotations( + title="Manage Sprite", + destructiveHint=True, + ), +) +async def manage_sprite( + ctx: Context, + action: Annotated[ + Literal["get_info", "slice_sheet", "setup_clips", "setup_controller", "full_setup"], + "Action to perform.", + ], + path: Annotated[ + str | None, + "Sprite texture asset path (e.g. 'Assets/Sprites/hero_walk.png'). Required for get_info, slice_sheet, setup_clips, full_setup.", + ] = None, + cols: Annotated[ + int | None, + "Number of columns in the sprite sheet grid. Used by slice_sheet and full_setup.", + ] = None, + rows: Annotated[ + int | None, + "Number of rows in the sprite sheet grid. Default: 1.", + ] = None, + frame_width: Annotated[ + int | None, + "Frame width in pixels. Alternative to cols.", + ] = None, + frame_height: Annotated[ + int | None, + "Frame height in pixels. Alternative to rows.", + ] = None, + base_name: Annotated[ + str | None, + "Base name for sliced sprite frames (default: texture filename).", + ] = None, + clips: Annotated[ + list[dict[str, Any]] | None, + "Clip definitions: [{name, start_frame, end_frame, fps (default 12), loop (auto-detect if omitted)}]. " + "For setup_controller: [{name, path}] where path is the .anim asset path.", + ] = None, + animation_name: Annotated[ + str | None, + "Animation name for full_setup when clips are not specified (all frames = one clip).", + ] = None, + output_dir: Annotated[ + str | None, + "Output directory for .anim and .controller assets (default: same folder as sprite).", + ] = None, + controller_path: Annotated[ + str | None, + "Path for the .controller asset (e.g. 'Assets/Animators/Hero.controller').", + ] = None, + overwrite: Annotated[bool, "Overwrite existing controller if it already exists."] = False, + add_to_scene: Annotated[bool, "Attach Animator + controller to a scene GameObject."] = False, + scene_target: Annotated[ + str | None, + "Existing GameObject name to attach Animator to.", + ] = None, +) -> dict[str, Any]: + """2D sprite animation tool.""" + + action_lower = action.lower() if action else "" + + if action_lower not in VALID_ACTIONS: + return { + "success": False, + "message": f"Unknown action '{action}'. Valid: {', '.join(VALID_ACTIONS)}", + } + + # Python-side validation + if action_lower in ("get_info", "slice_sheet", "setup_clips", "full_setup") and not path: + return {"success": False, "message": f"'path' is required for action '{action}'."} + + if action_lower in ("slice_sheet", "full_setup") and not cols and not frame_width: + return {"success": False, "message": f"'cols' or 'frame_width' is required for '{action}'. " + "Use get_info first to retrieve image_base64, analyze the grid visually, then call full_setup with cols/rows."} + + if action_lower == "setup_controller" and not controller_path: + return {"success": False, "message": "'controller_path' is required for setup_controller (e.g. 'Assets/Animators/Hero.controller')."} + + unity_instance = await get_unity_instance_from_context(ctx) + + params: dict[str, Any] = {"action": action_lower} + + if path is not None: params["path"] = path + if cols is not None: params["cols"] = cols + if rows is not None: params["rows"] = rows + if frame_width is not None: params["frame_width"] = frame_width + if frame_height is not None: params["frame_height"] = frame_height + if base_name is not None: params["base_name"] = base_name + if clips is not None: params["clips"] = clips + if animation_name is not None: params["animation_name"] = animation_name + if output_dir is not None: params["output_dir"] = output_dir + if controller_path is not None: params["controller_path"]= controller_path + if overwrite: params["overwrite"] = True + if add_to_scene: params["add_to_scene"] = True + if scene_target is not None: params["scene_target"] = scene_target + + result = await send_with_unity_instance( + async_send_command_with_retry, + unity_instance, + "manage_sprite", + params, + ) + return result if isinstance(result, dict) else {"success": False, "message": str(result)} diff --git a/website/docs/reference/tools/animation/index.md b/website/docs/reference/tools/animation/index.md index f2a57be64..50835b746 100644 --- a/website/docs/reference/tools/animation/index.md +++ b/website/docs/reference/tools/animation/index.md @@ -9,3 +9,4 @@ description: "MCP for Unity tools in the animation group." Animator control & AnimationClip creation - **[`manage_animation`](./manage_animation.md)** — Manage Unity animation: Animator control and AnimationClip creation. +- **[`manage_sprite`](./manage_sprite.md)** — 2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build Animat… diff --git a/website/docs/reference/tools/animation/manage_sprite.md b/website/docs/reference/tools/animation/manage_sprite.md new file mode 100644 index 000000000..66d2135b0 --- /dev/null +++ b/website/docs/reference/tools/animation/manage_sprite.md @@ -0,0 +1,45 @@ +--- +title: manage_sprite +sidebar_label: manage_sprite +description: "2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build Animat…" +--- + +# `manage_sprite` + +> **Auto-generated** from the Python tool registry. Do not hand-edit outside `` blocks — the generator (`tools/generate_docs_reference.py`) will overwrite them. + +**Group:** `animation`  ·  **Module:** `services.tools.manage_sprite` + +## Description + +2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build AnimatorController with smart complexity (1D blend tree for locomotion, trigger states for combat, simple state for single animations). full_setup: one command — slice → clips → controller. + +## Parameters + +| Name | Type | Required | Description | +|------|------|----------|-------------| +| `action` | `Literal['get_info', 'slice_sheet', 'setup_clips', 'setup_controller', 'full_setup']` | yes | Action to perform. | +| `path` | `str \| None` | — | Sprite texture asset path (e.g. 'Assets/Sprites/hero_walk.png'). Required for get_info, slice_sheet, setup_clips, full_setup. | +| `cols` | `int \| None` | — | Number of columns in the sprite sheet grid. Used by slice_sheet and full_setup. | +| `rows` | `int \| None` | — | Number of rows in the sprite sheet grid. Default: 1. | +| `frame_width` | `int \| None` | — | Frame width in pixels. Alternative to cols. | +| `frame_height` | `int \| None` | — | Frame height in pixels. Alternative to rows. | +| `base_name` | `str \| None` | — | Base name for sliced sprite frames (default: texture filename). | +| `clips` | `list[dict[str, Any]] \| None` | — | Clip definitions: [{name, start_frame, end_frame, fps (default 12), loop (auto-detect if omitted)}]. For setup_controller: [{name, path}] where path is the .anim asset path. | +| `animation_name` | `str \| None` | — | Animation name for full_setup when clips are not specified (all frames = one clip). | +| `output_dir` | `str \| None` | — | Output directory for .anim and .controller assets (default: same folder as sprite). | +| `controller_path` | `str \| None` | — | Path for the .controller asset (e.g. 'Assets/Animators/Hero.controller'). | +| `overwrite` | `bool` | — | Overwrite existing controller if it already exists. | +| `add_to_scene` | `bool` | — | Attach Animator + controller to a scene GameObject. | +| `scene_target` | `str \| None` | — | Existing GameObject name to attach Animator to. | + +## Returns + +A `dict` containing the Unity response. The exact shape depends on the action. + +## Examples + + +*No examples yet. Add usage examples here — they will be preserved across regenerations.* + + diff --git a/website/docs/reference/tools/index.md b/website/docs/reference/tools/index.md index a7466a134..f5a8d39c1 100644 --- a/website/docs/reference/tools/index.md +++ b/website/docs/reference/tools/index.md @@ -12,9 +12,10 @@ description: Auto-generated catalog of every MCP for Unity tool, grouped by doma Every tool MCP for Unity exposes, generated directly from the Python `@mcp_for_unity_tool` registry under `Server/src/services/tools/`. -## `animation`   (1 tool) +## `animation`   (2 tools) Animator control & AnimationClip creation - **[`manage_animation`](./animation/manage_animation.md)** — Manage Unity animation: Animator control and AnimationClip creation. +- **[`manage_sprite`](./animation/manage_sprite.md)** — 2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build Animat… ## `asset_gen`   (5 tools) AI asset generation – 3D model gen/import, 2D image gen & audio gen (bring-your-own-key) From 49c74e0d5b6c163161b54a926e3bc032838b6df9 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 17:54:30 +0300 Subject: [PATCH 02/29] test(sprite): cover manage_sprite against a real AssetDatabase The EditMode tests write an actual PNG into the project and import it, then assert on the sub-assets and the .anim/.controller files that end up on disk, rather than on the success flag the tool returns. That distinction is the whole point: both import bugs these tests found reported success while losing frames. Discrimination was measured by reverting each guard and re-running. Nine reverts, nine results: the sprite conversion breaks 22 cases; the dirty flag, the rows guard, the output_dir check, the clip-name check, the controller_path check, the fps check and the trigger naming each break exactly the one case written for them; and word matching breaks two. Removing the clip-name check also leaves an evil.anim outside the requested directory, which is what that check is for. Two cases survive every revert and are not offered as evidence: the already-converted-texture case, which pins the branch the conversion skips, and the natural-ordering case. The Python tests cover the argument checks that run before Unity is contacted. Asserting on success alone was not enough there: with a check removed the call reaches an absent Unity and fails for the wrong reason, so each case asserts on the message too. --- Server/tests/test_manage_sprite.py | 119 +++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 777 ++++++++++++++++++ .../EditMode/Tools/ManageSpriteTests.cs.meta | 2 + 3 files changed, 898 insertions(+) create mode 100644 Server/tests/test_manage_sprite.py create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs create mode 100644 TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs.meta diff --git a/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py new file mode 100644 index 000000000..68da272df --- /dev/null +++ b/Server/tests/test_manage_sprite.py @@ -0,0 +1,119 @@ +"""Tests for the manage_sprite tool. + +These cover the Python side only: the action list and the argument checks that run +before anything is sent to Unity. The behaviour of the slicing, clip and controller +builders is covered by the EditMode tests in TestProjects, because it only means +anything against a real AssetDatabase. +""" +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +from services.tools.manage_sprite import VALID_ACTIONS + + +class TestActionList: + def test_actions_are_the_documented_five(self): + assert set(VALID_ACTIONS) == { + "get_info", "slice_sheet", "setup_clips", + "setup_controller", "full_setup", + } + + def test_no_duplicate_actions(self): + assert len(VALID_ACTIONS) == len(set(VALID_ACTIONS)) + + +class TestManageSpriteValidation: + """Every case here must fail before a Unity round-trip is attempted.""" + + def _run(self, coro): + return asyncio.run(coro) + + def _ctx(self): + ctx = MagicMock() + ctx.get_state = AsyncMock(return_value=None) + return ctx + + def _call(self, **kwargs): + from services.tools.manage_sprite import manage_sprite + return self._run(manage_sprite(self._ctx(), **kwargs)) + + def test_unknown_action_returns_error(self): + result = self._call(action="nonexistent") + assert result["success"] is False + # The message has to name the alternatives, or the caller has nowhere to go. + assert "get_info" in result["message"] + + def test_get_info_requires_path(self): + result = self._call(action="get_info", path=None) + assert result["success"] is False + assert "path" in result["message"] + + def test_slice_sheet_requires_path(self): + result = self._call(action="slice_sheet", path=None) + assert result["success"] is False + assert "path" in result["message"] + + def test_slice_sheet_requires_cols_or_frame_width(self): + result = self._call(action="slice_sheet", path="Assets/hero.png") + assert result["success"] is False + assert "cols" in result["message"] + + def test_slice_sheet_accepts_frame_width_instead_of_cols(self): + # frame_width is the documented alternative to cols; rejecting it would make + # the error message above a lie. + with patch("services.tools.manage_sprite.get_unity_instance_from_context", + new=AsyncMock(return_value=None)), \ + patch("services.tools.manage_sprite.send_with_unity_instance", + new=AsyncMock(return_value={"success": True})) as sent: + result = self._call(action="slice_sheet", path="Assets/hero.png", frame_width=32) + + assert result["success"] is True + assert sent.await_count == 1 + + def test_setup_clips_requires_path(self): + result = self._call(action="setup_clips", path=None) + assert result["success"] is False + assert "path" in result["message"] + + def test_setup_controller_requires_controller_path(self): + result = self._call(action="setup_controller", clips=[{"name": "walk", "path": "a.anim"}]) + assert result["success"] is False + assert "controller_path" in result["message"] + + def test_full_setup_requires_path(self): + result = self._call(action="full_setup", path=None) + assert result["success"] is False + assert "path" in result["message"] + + def test_full_setup_requires_cols_or_frame_width(self): + result = self._call(action="full_setup", path="Assets/hero.png") + assert result["success"] is False + # Asserting on success alone would pass for the wrong reason: with the check + # removed the call reaches an absent Unity and fails there instead. + assert "cols" in result["message"] + + +class TestParameterForwarding: + def _ctx(self): + ctx = MagicMock() + ctx.get_state = AsyncMock(return_value=None) + return ctx + + def test_only_supplied_parameters_are_forwarded(self): + """Unset optional arguments must not reach Unity as nulls. + + The C# side reads `@params["rows"]?.ToObject() ?? 1`, so an explicitly + forwarded null and a missing key behave the same - but forwarding every + argument would still bury the real ones in noise on the wire. + """ + from services.tools.manage_sprite import manage_sprite + + with patch("services.tools.manage_sprite.get_unity_instance_from_context", + new=AsyncMock(return_value=None)), \ + patch("services.tools.manage_sprite.send_with_unity_instance", + new=AsyncMock(return_value={"success": True})) as sent: + asyncio.run(manage_sprite(self._ctx(), action="slice_sheet", + path="Assets/hero.png", cols=4)) + + params = sent.await_args.args[3] + assert params == {"action": "slice_sheet", "path": "Assets/hero.png", "cols": 4} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs new file mode 100644 index 000000000..0bff2fac2 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -0,0 +1,777 @@ +using System.IO; +using System.Linq; +using Newtonsoft.Json.Linq; +using NUnit.Framework; +using UnityEditor; +using UnityEditor.Animations; +using UnityEngine; +using MCPForUnity.Editor.Tools.Sprite2D; +using static MCPForUnityTests.Editor.TestUtilities; + +namespace MCPForUnityTests.Editor.Tools +{ + public class ManageSpriteTests + { + private const string TempRoot = "Assets/Temp/ManageSpriteTests"; + + // Each cell is 16x16, so a 4x2 sheet is 64x32. Small enough to import fast, + // big enough that a wrong row/column order is visible in the rects. + private const int Cell = 16; + + [SetUp] + public void SetUp() => EnsureFolder(TempRoot); + + [TearDown] + public void TearDown() + { + if (AssetDatabase.IsValidFolder(TempRoot)) + AssetDatabase.DeleteAsset(TempRoot); + CleanupEmptyParentFolders(TempRoot); + } + + // ===================================================================== + // Helpers + // ===================================================================== + + /// + /// Writes a real PNG into the project and imports it, so the tools run against + /// an actual TextureImporter rather than a stand-in. + /// + private static string CreateSheet(string name, int cols, int rows) + { + var tex = new Texture2D(cols * Cell, rows * Cell, TextureFormat.RGBA32, false); + var pixels = new Color32[tex.width * tex.height]; + for (int i = 0; i < pixels.Length; i++) + pixels[i] = new Color32(255, 0, 0, 255); + tex.SetPixels32(pixels); + tex.Apply(); + + string assetPath = $"{TempRoot}/{name}.png"; + string sysPath = Path.Combine( + Directory.GetParent(Application.dataPath).FullName, assetPath); + File.WriteAllBytes(sysPath, tex.EncodeToPNG()); + Object.DestroyImmediate(tex); + + AssetDatabase.ImportAsset(assetPath, ImportAssetOptions.ForceSynchronousImport); + return assetPath; + } + + private static JObject Run(JObject p) => ToJObject(ManageSprite.HandleCommand(p)); + + /// + /// The failure text, whichever key it arrived under. ErrorResponse serialises it as + /// "error", while anonymous failures elsewhere in the codebase use "message", and + /// Server/src/services/tools/__init__.py reads both. Pinning one key here would test + /// the response shape rather than the behaviour. + /// + private static string ErrorText(JObject result) => + result.Value("error") ?? result.Value("message") ?? ""; + + private static JObject Slice(string path, int cols, int rows) => Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = path, + ["cols"] = cols, + ["rows"] = rows, + }); + + /// The sliced frames, in the natural order their names imply. + private static Sprite[] SpritesOf(string path) => + AssetDatabase.LoadAllAssetsAtPath(path) + .OfType() + .OrderBy(s => int.Parse(s.name.Split('_').Last())) + .ToArray(); + + // ===================================================================== + // Dispatch + // ===================================================================== + + [Test] + public void HandleCommand_MissingAction_ReturnsError() + { + var result = Run(new JObject()); + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("'action' is required")); + } + + [Test] + public void HandleCommand_UnknownAction_NamesTheValidOnes() + { + var result = Run(new JObject { ["action"] = "not_an_action" }); + Assert.IsFalse(result.Value("success")); + // Listing the alternatives is the difference between a dead end and a retry. + Assert.That(ErrorText(result), Does.Contain("slice_sheet")); + Assert.That(ErrorText(result), Does.Contain("full_setup")); + } + + // ===================================================================== + // get_info + // ===================================================================== + + [Test] + public void GetInfo_MissingPath_ReturnsError() + { + var result = Run(new JObject { ["action"] = "get_info" }); + Assert.IsFalse(result.Value("success")); + } + + [Test] + public void GetInfo_PathIsNotATexture_ReturnsError() + { + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = $"{TempRoot}/nothing_here.png", + }); + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("TextureImporter")); + } + + [Test] + public void GetInfo_ReportsTheTextureDimensions() + { + string path = CreateSheet("info", 4, 2); + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(4 * Cell, result.Value("width")); + Assert.AreEqual(2 * Cell, result.Value("height")); + } + + [Test] + public void GetInfo_OnAnUnslicedSheet_ReportsNoSlices() + { + string path = CreateSheet("unsliced", 4, 2); + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + + Assert.AreEqual(0, result.Value("slice_count")); + } + + [Test] + public void GetInfo_AfterSlicing_ReportsEverySlice() + { + string path = CreateSheet("sliced", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + Assert.AreEqual(8, result.Value("slice_count")); + } + + // ===================================================================== + // slice_sheet + // ===================================================================== + + [Test] + public void SliceSheet_WithoutColsOrFrameWidth_ReturnsError() + { + string path = CreateSheet("nogrid", 4, 2); + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path }); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("frame_width")); + } + + [Test] + public void SliceSheet_ProducesOneSpritePerGridCell() + { + string path = CreateSheet("grid", 4, 2); + var result = Slice(path, 4, 2); + + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(8, result.Value("total_frames")); + // The reported count is a claim; the sub-assets on disk are the fact. + Assert.AreEqual(8, SpritesOf(path).Length); + } + + [Test] + public void SliceSheet_FrameZeroIsTheTopLeftCell() + { + // Sprite sheets are read left-to-right, top-to-bottom, but Unity's texture + // origin is bottom-left. Getting this backwards silently plays the animation + // in the wrong order, which no success flag would reveal. + string path = CreateSheet("order", 4, 2); + Slice(path, 4, 2); + + var first = SpritesOf(path).First(); + Assert.AreEqual(0, (int)first.rect.x, "frame 0 should sit at the left edge"); + Assert.AreEqual(Cell, (int)first.rect.y, "frame 0 should sit on the top row"); + } + + [Test] + public void SliceSheet_LastFrameIsTheBottomRightCell() + { + string path = CreateSheet("order2", 4, 2); + Slice(path, 4, 2); + + var last = SpritesOf(path).Last(); + Assert.AreEqual(3 * Cell, (int)last.rect.x); + Assert.AreEqual(0, (int)last.rect.y); + } + + [Test] + public void SliceSheet_EveryFrameHasTheCellSize() + { + string path = CreateSheet("size", 4, 2); + Slice(path, 4, 2); + + foreach (var s in SpritesOf(path)) + { + Assert.AreEqual(Cell, (int)s.rect.width, $"{s.name} width"); + Assert.AreEqual(Cell, (int)s.rect.height, $"{s.name} height"); + } + } + + [Test] + public void SliceSheet_NonPowerOfTwoSheet_KeepsEveryFrame() + { + // 6 cells of 16px is 96px wide, which is not a power of two. A Default-type + // import rescales it to 128, and a grid measured there is 21px per cell - the + // last two frames then fall outside the real texture and Unity discards them, + // reporting success all the same. + string path = CreateSheet("npot", 6, 1); + var result = Slice(path, 6, 1); + + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(Cell, result.Value("frame_width"), + "the grid must be measured against the sheet's real width"); + Assert.AreEqual(6, SpritesOf(path).Length, "no frame may be dropped"); + } + + [Test] + public void SliceSheet_TextureAlreadyConvertedToSprite_KeepsEveryFrame() + { + // The conversion above is skipped when the texture is already a Sprite, so this + // pins the other branch. It survives every mutation of the slicing code, because + // Unity ignores npotScale on sprite textures - it is a boundary guard, not + // evidence for the fix. + string path = CreateSheet("npot_preset", 6, 1); + var importer = (TextureImporter)AssetImporter.GetAtPath(path); + importer.textureType = TextureImporterType.Sprite; + importer.npotScale = TextureImporterNPOTScale.ToNearest; + EditorUtility.SetDirty(importer); + importer.SaveAndReimport(); + + var result = Slice(path, 6, 1); + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(6, SpritesOf(path).Length, "no frame may be dropped"); + } + + [Test] + public void SliceSheet_FrameWidthAloneDerivesTheColumnCount() + { + string path = CreateSheet("derive", 4, 1); + var result = Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = path, + ["frame_width"] = Cell, + ["frame_height"] = Cell, + }); + + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(4, result.Value("cols")); + Assert.AreEqual(4, SpritesOf(path).Length); + } + + [Test] + public void SliceSheet_BaseNameOverridesTheFileName() + { + string path = CreateSheet("filename", 2, 1); + Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = path, + ["cols"] = 2, + ["base_name"] = "hero", + }); + + Assert.That(SpritesOf(path).Select(s => s.name), Is.EquivalentTo(new[] { "hero_0", "hero_1" })); + } + + [Test] + public void SliceSheet_FrameWiderThanTheTexture_ReportsSliceEmpty() + { + string path = CreateSheet("toobig", 2, 1); + var result = Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = path, + ["frame_width"] = 4096, + }); + + Assert.IsFalse(result.Value("success")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_EMPTY")); + } + + [Test] + public void SliceSheet_ZeroRows_FailsWithAMessageInsteadOfThrowing() + { + // rows is read as `?? 1`, which only covers a missing key - an explicit 0 + // survives and reaches the `texH / rows` division. + string path = CreateSheet("zerorows", 4, 1); + + JObject result = null; + Assert.DoesNotThrow(() => result = Slice(path, 4, 0), + "a bad grid value must come back as an error, not an exception"); + Assert.IsFalse(result.Value("success")); + } + + [Test] + public void SliceSheet_ReslicingWithADifferentGrid_ReplacesTheOldFrames() + { + string path = CreateSheet("reslice", 4, 2); + Slice(path, 4, 2); + Assert.AreEqual(8, SpritesOf(path).Length); + + Slice(path, 2, 1); + var after = SpritesOf(path).Select(s => s.name).ToArray(); +#pragma warning disable CS0618 // same API the tool writes through + int configured = ((TextureImporter)AssetImporter.GetAtPath(path)).spritesheet.Length; +#pragma warning restore CS0618 + Assert.AreEqual(2, after.Length, + $"stale frames must not survive a reslice; importer holds {configured}, " + + "project holds: " + string.Join(", ", after)); + } + + // ===================================================================== + // setup_clips + // ===================================================================== + + private static JObject SetupClips(string path, JArray clips) => Run(new JObject + { + ["action"] = "setup_clips", + ["path"] = path, + ["clips"] = clips, + ["output_dir"] = TempRoot, + }); + + private static JArray OneClip(string name, int start, int end, float? fps = null, bool? loop = null) + { + var clip = new JObject { ["name"] = name, ["start_frame"] = start, ["end_frame"] = end }; + if (fps.HasValue) clip["fps"] = fps.Value; + if (loop.HasValue) clip["loop"] = loop.Value; + return new JArray { clip }; + } + + [Test] + public void SetupClips_OnAnUnslicedSheet_TellsYouToSliceFirst() + { + string path = CreateSheet("noslice", 4, 1); + var result = SetupClips(path, OneClip("walk", 0, 3)); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("slice_sheet")); + } + + [Test] + public void SetupClips_WritesAClipAssetWithOneKeyPerFrame() + { + string path = CreateSheet("clips", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, OneClip("walk", 0, 3)); + Assert.IsTrue(result.Value("success")); + + var clip = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + Assert.IsNotNull(clip, "the .anim asset should exist on disk"); + + var binding = AnimationUtility.GetObjectReferenceCurveBindings(clip).Single(); + Assert.AreEqual(typeof(SpriteRenderer), binding.type); + Assert.AreEqual("m_Sprite", binding.propertyName, + "anything else animates the wrong property and shows nothing"); + Assert.AreEqual(4, AnimationUtility.GetObjectReferenceCurve(clip, binding).Length); + } + + [Test] + public void SetupClips_FpsDrivesTheFrameRateAndTheKeyTimes() + { + string path = CreateSheet("fps", 4, 1); + Slice(path, 4, 1); + SetupClips(path, OneClip("walk", 0, 3, fps: 8f)); + + var clip = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + Assert.AreEqual(8f, clip.frameRate); + + var binding = AnimationUtility.GetObjectReferenceCurveBindings(clip).Single(); + var keys = AnimationUtility.GetObjectReferenceCurve(clip, binding); + Assert.AreEqual(0f, keys[0].time, 0.0001f); + Assert.AreEqual(1f / 8f, keys[1].time, 0.0001f); + } + + [Test] + public void SetupClips_KeyframesFollowTheSlicedOrder() + { + string path = CreateSheet("seq", 4, 1); + Slice(path, 4, 1); + SetupClips(path, OneClip("walk", 0, 3)); + + var clip = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + var binding = AnimationUtility.GetObjectReferenceCurveBindings(clip).Single(); + var keys = AnimationUtility.GetObjectReferenceCurve(clip, binding); + + var expected = SpritesOf(path).Select(s => s.name).ToArray(); + var actual = keys.Select(k => k.value.name).ToArray(); + Assert.AreEqual(expected, actual, "frames must play in sheet order"); + } + + [Test] + public void SetupClips_TenthFrameSortsAfterTheSecond() + { + // A plain string sort puts hero_10 between hero_1 and hero_2, which reorders + // the animation without failing anything. + string path = CreateSheet("natural", 11, 1); + Slice(path, 11, 1); + SetupClips(path, OneClip("walk", 0, 10)); + + var clip = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + var binding = AnimationUtility.GetObjectReferenceCurveBindings(clip).Single(); + var keys = AnimationUtility.GetObjectReferenceCurve(clip, binding); + + Assert.AreEqual("natural_2", keys[2].value.name); + Assert.AreEqual("natural_10", keys[10].value.name); + } + + [Test] + public void SetupClips_LoopIsInferredFromTheClipName() + { + string path = CreateSheet("loopname", 4, 1); + Slice(path, 4, 1); + SetupClips(path, new JArray + { + new JObject { ["name"] = "walk", ["start_frame"] = 0, ["end_frame"] = 1 }, + new JObject { ["name"] = "attack", ["start_frame"] = 2, ["end_frame"] = 3 }, + }); + + var walk = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + var attack = AssetDatabase.LoadAssetAtPath($"{TempRoot}/attack.anim"); + + Assert.IsTrue(AnimationUtility.GetAnimationClipSettings(walk).loopTime, + "locomotion should loop"); + Assert.IsFalse(AnimationUtility.GetAnimationClipSettings(attack).loopTime, + "a one-shot attack should not loop"); + } + + [Test] + public void SetupClips_ExplicitLoopBeatsTheNameGuess() + { + string path = CreateSheet("loopflag", 4, 1); + Slice(path, 4, 1); + SetupClips(path, OneClip("walk", 0, 3, loop: false)); + + var clip = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + Assert.IsFalse(AnimationUtility.GetAnimationClipSettings(clip).loopTime); + } + + [Test] + public void SetupClips_RangeBeyondTheSheet_WarnsAndWritesNothing() + { + string path = CreateSheet("range", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, OneClip("walk", 90, 99)); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_EMPTY")); + Assert.IsNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim")); + } + + [Test] + public void SetupClips_UnnamedClip_IsSkippedWithAWarning() + { + string path = CreateSheet("noname", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, new JArray { new JObject { ["start_frame"] = 0, ["end_frame"] = 3 } }); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_NO_NAME")); + } + + [Test] + public void SetupClips_OutputDirEscapingAssets_IsRefused() + { + string path = CreateSheet("escape", 4, 1); + Slice(path, 4, 1); + + var result = Run(new JObject + { + ["action"] = "setup_clips", + ["path"] = path, + ["clips"] = OneClip("walk", 0, 3), + ["output_dir"] = $"{TempRoot}/../../../outside", + }); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("output_dir")); + } + + [Test] + public void SetupClips_ClipNameEscapingTheOutputDir_IsSkipped() + { + // The clip name is joined into a file path, so a name carrying separators would + // otherwise write outside the directory the caller asked for. + string path = CreateSheet("escapename", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, OneClip("../../evil", 0, 3)); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_NAME")); + } + + [Test] + public void SetupClips_ZeroFps_IsSkippedInsteadOfWritingInfiniteKeyTimes() + { + string path = CreateSheet("zerofps", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, OneClip("walk", 0, 3, fps: 0f)); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_FPS")); + Assert.IsNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim")); + } + + [Test] + public void SetupClips_NameThatMerelyContainsAKeyword_IsNotTreatedAsLocomotion() + { + // 'grunt' contains the letters of 'run'. Matching on substrings makes it loop + // like a walk cycle. + string path = CreateSheet("substr", 4, 1); + Slice(path, 4, 1); + SetupClips(path, OneClip("grunt", 0, 3)); + + var clip = AssetDatabase.LoadAssetAtPath($"{TempRoot}/grunt.anim"); + Assert.IsFalse(AnimationUtility.GetAnimationClipSettings(clip).loopTime); + } + + // ===================================================================== + // setup_controller + // ===================================================================== + + private static JObject SetupController(JArray clips, bool overwrite = false) => Run(new JObject + { + ["action"] = "setup_controller", + ["clips"] = clips, + ["controller_path"] = $"{TempRoot}/Hero.controller", + ["overwrite"] = overwrite, + }); + + /// Slices a sheet and builds the named clips, returning [{name, path}] for the controller. + private static JArray BuildClips(string sheet, params string[] names) + { + string path = CreateSheet(sheet, names.Length * 2, 1); + Slice(path, names.Length * 2, 1); + + var defs = new JArray(); + for (int i = 0; i < names.Length; i++) + defs.Add(new JObject { ["name"] = names[i], ["start_frame"] = i * 2, ["end_frame"] = i * 2 + 1 }); + var clipResult = SetupClips(path, defs); + + var refs = new JArray(); + foreach (string n in names) + { + string clipPath = $"{TempRoot}/{n}.anim"; + Assert.IsNotNull(AssetDatabase.LoadAssetAtPath(clipPath), + $"fixture: clip '{n}' was not written to {clipPath}; setup_clips said " + + clipResult.ToString(Newtonsoft.Json.Formatting.None)); + refs.Add(new JObject { ["name"] = n, ["path"] = clipPath }); + } + return refs; + } + + [Test] + public void SetupController_WithoutClips_ReturnsError() + { + var result = Run(new JObject + { + ["action"] = "setup_controller", + ["controller_path"] = $"{TempRoot}/Hero.controller", + }); + Assert.IsFalse(result.Value("success")); + } + + [Test] + public void SetupController_WithoutControllerPath_ReturnsError() + { + var result = Run(new JObject + { + ["action"] = "setup_controller", + ["clips"] = new JArray { new JObject { ["name"] = "walk", ["path"] = "x.anim" } }, + }); + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("controller_path")); + } + + [Test] + public void SetupController_ClipsThatDoNotExist_ReturnsError() + { + var result = SetupController(new JArray + { + new JObject { ["name"] = "walk", ["path"] = $"{TempRoot}/missing.anim" }, + }); + Assert.IsFalse(result.Value("success")); + } + + [Test] + public void SetupController_IdleAndWalk_WritesAControllerWithBothStates() + { + var result = SetupController(BuildClips("ctrl", "idle", "walk")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + Assert.IsNotNull(controller); + + var states = controller.layers[0].stateMachine.states.Select(s => s.state.name).ToArray(); + Assert.That(states, Contains.Item("Idle")); + Assert.That(states, Contains.Item("walk")); + Assert.AreEqual("Idle", controller.layers[0].stateMachine.defaultState.name, + "idle is the state a character rests in, so it should be the entry point"); + } + + [Test] + public void SetupController_WalkAndRun_BuildsASpeedDrivenBlendTree() + { + var result = SetupController(BuildClips("blend", "idle", "walk", "run")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + Assert.That(controller.parameters.Select(p => p.name), Contains.Item("Speed")); + + var loco = controller.layers[0].stateMachine.states + .Select(s => s.state) + .SingleOrDefault(s => s.name == "Locomotion"); + Assert.IsNotNull(loco, + "two locomotion clips should collapse into one blend tree state; states were: " + + string.Join(", ", controller.layers[0].stateMachine.states.Select(s => s.state.name))); + + var tree = loco.motion as BlendTree; + Assert.IsNotNull(tree); + Assert.AreEqual("Speed", tree.blendParameter); + // walk sits below run on the axis, otherwise the character sprints while strolling. + Assert.AreEqual(new[] { "walk", "run" }, + tree.children.Select(c => c.motion.name).ToArray()); + } + + [Test] + public void SetupController_CombatClip_GetsATrigger() + { + var result = SetupController(BuildClips("combat", "idle", "attack")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + var attack = controller.parameters.SingleOrDefault(p => p.name == "Attack"); + Assert.IsNotNull(attack, "a combat clip needs a trigger to be reachable"); + Assert.AreEqual(AnimatorControllerParameterType.Trigger, attack.type); + } + + [Test] + public void SetupController_ControllerPathEscapingAssets_FailsWithAMessage() + { + var clips = BuildClips("escapectrl", "idle", "walk"); + JObject result = null; + Assert.DoesNotThrow(() => result = Run(new JObject + { + ["action"] = "setup_controller", + ["clips"] = clips, + ["controller_path"] = $"{TempRoot}/../../../Hero.controller", + }), "a refused path must not surface as an exception"); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("controller_path")); + } + + [Test] + public void SetupController_TriggerIsNamedAfterTheAction() + { + // 'hero_attack' should arm an Attack trigger. Naming it after the first segment + // of the clip name gives 'Hero', which tells the caller nothing. + var result = SetupController(BuildClips("trig", "idle", "hero_attack")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + var names = controller.parameters.Select(p => p.name).ToArray(); + Assert.That(names, Contains.Item("Attack")); + Assert.That(names, Has.No.Member("Hero")); + } + + [Test] + public void SetupController_NameThatMerelyContainsAKeyword_GetsNoTrigger() + { + // The letters of 'hit' sit inside 'white'. Under substring matching the clip is + // filed as an object animation and picks up a trigger it never asked for. + var result = SetupController(BuildClips("wf", "idle", "white_flash")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + var names = controller.parameters.Select(p => p.name).ToArray(); + Assert.That(names, Has.No.Member("Hit")); + Assert.That(names, Has.No.Member("White")); + } + + [Test] + public void SetupController_ExistingControllerWithoutOverwrite_RefusesInsteadOfReplacing() + { + var clips = BuildClips("exists", "idle", "walk"); + Assert.IsTrue(SetupController(clips).Value("success")); + + var second = SetupController(clips); + Assert.IsFalse(second.Value("success")); + Assert.That(second["diagnostics"].ToString(), Does.Contain("CONTROLLER_EXISTS")); + } + + [Test] + public void SetupController_ExistingControllerWithOverwrite_Replaces() + { + var clips = BuildClips("overwrite", "idle", "walk"); + Assert.IsTrue(SetupController(clips).Value("success")); + Assert.IsTrue(SetupController(clips, overwrite: true).Value("success")); + } + + // ===================================================================== + // full_setup + // ===================================================================== + + [Test] + public void FullSetup_WithoutColsOrFrameWidth_ReturnsError() + { + string path = CreateSheet("fullnogrid", 4, 1); + var result = Run(new JObject { ["action"] = "full_setup", ["path"] = path }); + Assert.IsFalse(result.Value("success")); + } + + [Test] + public void FullSetup_SlicesBuildsClipsAndWritesAController() + { + string path = CreateSheet("full", 4, 1); + var result = Run(new JObject + { + ["action"] = "full_setup", + ["path"] = path, + ["cols"] = 4, + ["animation_name"] = "walk", + ["output_dir"] = TempRoot, + ["controller_path"] = $"{TempRoot}/Full.controller", + }); + + Assert.IsTrue(result.Value("success"), result.ToString()); + Assert.AreEqual(4, SpritesOf(path).Length, "the sheet should end up sliced"); + Assert.IsNotNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"), + "the clip should end up on disk"); + Assert.IsNotNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/Full.controller"), + "the controller should end up on disk"); + } + + [Test] + public void FullSetup_DefaultsTheClipNameToTheFileName() + { + string path = CreateSheet("hero_idle", 4, 1); + Run(new JObject + { + ["action"] = "full_setup", + ["path"] = path, + ["cols"] = 4, + ["output_dir"] = TempRoot, + ["controller_path"] = $"{TempRoot}/Named.controller", + }); + + Assert.IsNotNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/hero_idle.anim")); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs.meta new file mode 100644 index 000000000..c5b02b23c --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: 14ddca8684ebb412985c717045378811 \ No newline at end of file From 6f10270948faf003c04e9398e82d96f120aa5fb7 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 18:26:27 +0300 Subject: [PATCH 03/29] fix(sprite): close the defects an outside audit found in manage_sprite An independent red-team pass over the new tool produced fourteen candidate findings across three lenses. Ten reproduced against the live tree and are fixed here; each one has an EditMode test asserting the behaviour it was missing, and every fix was verified by that test turning green. Destructive work no longer runs before the replacement is known to be good: - setup_controller loaded and validated the replacement clips only AFTER deleting the existing controller, so a rebuild that could not succeed left the caller with no controller at all. Measured: the asset was gone and the response never mentioned it. - setup_clips deleted whatever AnimationClip sat at the composed path. An unrelated clip that merely shared a name was destroyed by a request that asked for nothing of the sort. It now honours the `overwrite` flag that already existed on the tool surface, the same way setup_controller does. - slice_sheet converted the texture to a Sprite before validating the caller's grid arguments, so a refused request still left the texture converted. The arguments need no texture, so they are checked first. full_setup was not the first-failure sequence it documents: - An existing-controller refusal arrives as an error diagnostic rather than an ErrorResponse, and only the latter was checked - so a failed controller step fell through and attached the OLD controller to the scene object. - An extensionless controller_path was suffixed inside the builder only, so the scene step looked up a path nothing had been written to. - A requested scene attachment that did not happen was a warning, and warnings do not affect success - the call reported success having attached nothing. - clip_count was rebuilt from the requested clip definitions, so refused clips were counted and their paths were still handed to the controller, which could then pick up a stale asset from an earlier run. And the scene attachment had never worked at all: `GetComponent() ?? AddComponent()` compares references, which bypasses Unity's overloaded ==, so AddComponent was never reached and the next line threw MissingComponentException. It now checks with ==, records the change through Undo and marks the object dirty, matching controller_assign. A clip name may no longer contain a path separator. `..` was refused but a plain `/` was not, so a name selected a directory: with the folder absent CreateAsset threw, and where it existed the clip was written outside output_dir. Unity refuses the name and the Python surface refuses it earlier, where it can answer without a round-trip. Smaller things the same pass surfaced: a non-positive fps wrote keys at infinity; a refused clip leaked an AnimationClip that never became an asset; `catch { /* non-critical */ }` turned an unreadable controller result into a silent state_count of 0 and now reports it; and GetClipPaths is gone with its last caller. --- .../Tools/Sprite2D/SpriteClipBuilder.cs | 59 +++--- .../Tools/Sprite2D/SpriteControllerBuilder.cs | 32 +-- .../Editor/Tools/Sprite2D/SpriteFullSetup.cs | 108 +++++++--- .../Tools/Sprite2D/SpriteImportSetup.cs | 28 +-- Server/src/services/tools/manage_sprite.py | 10 + Server/tests/test_manage_sprite.py | 6 + .../Tests/EditMode/Tools/ManageSpriteTests.cs | 187 ++++++++++++++++++ 7 files changed, 351 insertions(+), 79 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs index 22a40d48d..8f4821ab0 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -16,6 +16,7 @@ internal static class SpriteClipBuilder /// path - sprite texture asset path /// clips - [{name, start_frame, end_frame, fps (opt, def=12), loop (opt)}] /// output_dir - where the clips are written (default: the sprite's own folder) + /// overwrite - bool (default false); an existing clip is kept unless this is true /// public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnostics) { @@ -48,6 +49,8 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos if (!AssetDatabase.IsValidFolder(outputDir)) CreateFolders(outputDir); + bool overwrite = @params["overwrite"]?.ToObject() ?? false; + var createdClips = new List(); foreach (JObject clipDef in clipsToken) @@ -56,6 +59,15 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos if (string.IsNullOrEmpty(clipName)) { diagnostics.AddWarning("CLIP_NO_NAME", "Clip name is missing — skipped.", null, new[] { "Add a 'name' field to each clip definition." }); continue; } + // Measured: a name like "nested/walk" composes into a path under a folder that + // does not exist and AssetDatabase.CreateAsset throws an uncaught UnityException; + // where the folder happens to exist the clip is written outside output_dir instead. + if (clipName.Contains("/") || clipName.Contains("\\")) + { + diagnostics.AddWarning("CLIP_BAD_NAME", $"Clip '{clipName}': the name cannot contain a path separator - skipped.", null, new[] { "Remove '..' and path separators from the clip name." }); + continue; + } + int startFrame = clipDef["start_frame"]?.ToObject() ?? 0; int endFrame = clipDef["end_frame"]?.ToObject() ?? allSprites.Length - 1; float fps = clipDef["fps"]?.ToObject() ?? 12f; @@ -80,6 +92,27 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos if (frameSprites.Length <= 2) diagnostics.AddWarning("LOW_FRAME_COUNT", $"Clip '{clipName}' has only {frameSprites.Length} frame(s) — animation may not be visible.", null, new string[0]); + // Both refusals below come before the clip is allocated: a `new AnimationClip` + // that never becomes an asset is a leaked UnityEngine.Object, not a collected one. + // The delete stays down next to CreateAsset, so nothing is destroyed until the + // replacement has actually been built. + string clipPath = AssetPathUtility.SanitizeAssetPath($"{outputDir}/{clipName}.anim"); + if (clipPath == null) + { + diagnostics.AddWarning("CLIP_BAD_NAME", $"Clip '{clipName}': the name cannot be used as a file name - skipped.", null, new[] { "Remove '..' and path separators from the clip name." }); + continue; + } + + var existing = AssetDatabase.LoadAssetAtPath(clipPath); + if (existing != null && !overwrite) + { + // Measured: an unrelated clip already at this path was deleted and replaced by a + // request that carried no overwrite field. The sibling controller builder refuses + // instead, so clips follow the same policy: destruction needs authorisation. + diagnostics.AddWarning("CLIP_EXISTS", $"Clip '{clipName}': an animation clip already exists at '{clipPath}' - skipped.", new { path = clipPath }, new[] { "Set overwrite=true to replace it.", "Choose a different clip name or output_dir." }); + continue; + } + var clip = new AnimationClip { frameRate = fps }; var binding = new EditorCurveBinding @@ -105,15 +138,7 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos settings.loopTime = loop; AnimationUtility.SetAnimationClipSettings(clip, settings); - string clipPath = AssetPathUtility.SanitizeAssetPath($"{outputDir}/{clipName}.anim"); - if (clipPath == null) - { - diagnostics.AddWarning("CLIP_BAD_NAME", $"Clip '{clipName}': the name cannot be used as a file name - skipped.", null, new[] { "Remove '..' and path separators from the clip name." }); - continue; - } - var existing = AssetDatabase.LoadAssetAtPath(clipPath); if (existing != null) AssetDatabase.DeleteAsset(clipPath); - AssetDatabase.CreateAsset(clip, clipPath); createdClips.Add(new @@ -141,24 +166,6 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos // ── Internal helper ────────────────────────────────────────────────── - /// - /// For full_setup: returns the clip name to asset path mapping. - /// - internal static List<(string name, string path)> GetClipPaths(JArray clipsToken, string outputDir) - { - var result = new List<(string, string)>(); - foreach (JObject cd in clipsToken) - { - string name = cd["name"]?.ToString(); - if (string.IsNullOrEmpty(name)) continue; - // Must match the path SetupClips wrote, refusals included. - string clipPath = AssetPathUtility.SanitizeAssetPath($"{outputDir}/{name}.anim"); - if (clipPath != null) - result.Add((name, clipPath)); - } - return result; - } - internal static AnimationClip LoadClip(string clipPath) => AssetDatabase.LoadAssetAtPath(clipPath); diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs index 6ef686b73..799532218 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs @@ -35,6 +35,23 @@ public static object Build(JObject @params, SpriteDiagnosticBuilder diagnostics) bool overwrite = @params["overwrite"]?.ToObject() ?? false; + var entries = new List<(SpriteAnimEntry entry, AnimationClip clip)>(); + foreach (JObject cd in clipsToken) + { + string clipName = cd["name"]?.ToString() ?? ""; + string clipPath = cd["path"]?.ToString() ?? ""; + var clip = AssetDatabase.LoadAssetAtPath( + AssetPathUtility.SanitizeAssetPath(clipPath)); + if (clip == null) + { diagnostics.AddWarning("CLIP_NOT_FOUND", $"Clip '{clipName}' not found at '{clipPath}' — skipped.", null, new string[0]); continue; } + entries.Add((SpriteNamingDetector.Detect(clipName), clip)); + } + + if (entries.Count == 0) + return new ErrorResponse("No valid clips loaded."); + + // The existing controller is only removed once the replacement is known to be + // buildable: deleting first left a failed rebuild with no controller at all. if (AssetDatabase.LoadAssetAtPath(controllerPath) != null) { if (!overwrite) @@ -54,21 +71,6 @@ public static object Build(JObject @params, SpriteDiagnosticBuilder diagnostics) if (!string.IsNullOrEmpty(dir) && !AssetDatabase.IsValidFolder(dir)) CreateFolders(dir); - var entries = new List<(SpriteAnimEntry entry, AnimationClip clip)>(); - foreach (JObject cd in clipsToken) - { - string clipName = cd["name"]?.ToString() ?? ""; - string clipPath = cd["path"]?.ToString() ?? ""; - var clip = AssetDatabase.LoadAssetAtPath( - AssetPathUtility.SanitizeAssetPath(clipPath)); - if (clip == null) - { diagnostics.AddWarning("CLIP_NOT_FOUND", $"Clip '{clipName}' not found at '{clipPath}' — skipped.", null, new string[0]); continue; } - entries.Add((SpriteNamingDetector.Detect(clipName), clip)); - } - - if (entries.Count == 0) - return new ErrorResponse("No valid clips loaded."); - var complexity = SpriteNamingDetector.DecideComplexity(entries.Select(e => e.entry)); var controller = AnimatorController.CreateAnimatorControllerAtPath(controllerPath); var rootSM = controller.layers[0].stateMachine; diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs index d0ee321e0..6e3d66bec 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using System.IO; using Newtonsoft.Json; using Newtonsoft.Json.Linq; @@ -65,11 +64,14 @@ public static object Run(JObject @params) }); } + bool overwrite = @params["overwrite"]?.ToObject() ?? false; + var clipsParams = new JObject { ["path"] = path, ["clips"] = clipsToken, ["output_dir"] = outputDir, + ["overwrite"] = overwrite, }; var clipResult = SpriteClipBuilder.SetupClips(clipsParams, diagnostics); if (clipResult is ErrorResponse) @@ -81,14 +83,36 @@ public static object Run(JObject @params) string controllerPath = @params["controller_path"]?.ToString() ?? $"{outputDir}/{Path.GetFileNameWithoutExtension(path)}_Controller.controller"; - bool overwrite = @params["overwrite"]?.ToObject() ?? false; - // Derive the paths from clipsToken directly - the builder's return value is an - // anonymous object, and parsing it back would be the long way round. - var clipPaths = SpriteClipBuilder.GetClipPaths(clipsToken, outputDir); + // The builder suffixes its own local copy, so keeping the raw string here made the + // scene step load '/S7' instead of '/S7.controller' and attach nothing. + controllerPath = AssetPathUtility.SanitizeAssetPath(controllerPath); + if (controllerPath == null) + return new { success = false, step = "setup_controller", + error = "'controller_path' must stay under Assets/ and cannot contain '..'.", + diagnostics = diagnostics.Build() }; + if (!controllerPath.EndsWith(".controller")) + controllerPath += ".controller"; + + // Only the clips SetupClips really wrote may reach the controller: rebuilding the + // list from the request counted refused clips and fed the controller stale assets. var createdClips = new JArray(); - foreach (var (cname, cpath) in clipPaths) - createdClips.Add(new JObject { ["name"] = cname, ["path"] = cpath }); + var clipObj = AsJObject(clipResult); + if (clipObj == null) + { + diagnostics.AddError("CLIP_RESULT_UNREADABLE", + "The clip step result could not be read back, so the created clips are unknown.", + null, new[] { "Run setup_clips on its own to see which clips were created." }); + } + else + { + foreach (var c in clipObj["clips"] as JArray ?? new JArray()) + { + string cpath = c["path"]?.ToString(); + if (!string.IsNullOrEmpty(cpath)) + createdClips.Add(new JObject { ["name"] = c["name"]?.ToString(), ["path"] = cpath }); + } + } var ctrlParams = new JObject { @@ -103,13 +127,26 @@ public static object Run(JObject @params) if (ctrlResult is ErrorResponse) return new { success = false, step = "setup_controller", error = ((ErrorResponse)ctrlResult).Error, diagnostics = diagnostics.Build() }; + // An existing-controller refusal arrives as an error diagnostic, not an ErrorResponse; + // without this the scene step went on to attach the OLD controller. + if (diagnostics.HasErrors) + return new { success = false, step = "setup_controller", diagnostics = diagnostics.Build() }; // ── Step 4: Add to scene ─────────────────────────────────────────── bool addToScene = @params["add_to_scene"]?.ToObject() ?? false; string sceneTarget = @params["scene_target"]?.ToString(); - if (addToScene && !string.IsNullOrEmpty(sceneTarget)) + // An attachment that was asked for but did not happen is not a success, so both + // misses below are errors rather than a warning or nothing at all. + if (addToScene && string.IsNullOrEmpty(sceneTarget)) + { + diagnostics.AddError("SCENE_TARGET_MISSING", + "'add_to_scene' is true but 'scene_target' is empty.", + null, + new[] { "Pass 'scene_target' with the GameObject name.", "Set add_to_scene=false." }); + } + else if (addToScene) { var go = UnityEngine.GameObject.Find(sceneTarget); if (go != null) @@ -118,46 +155,67 @@ public static object Run(JObject @params) AssetPathUtility.SanitizeAssetPath(controllerPath)); if (controller != null) { - var animator = go.GetComponent() - ?? go.AddComponent(); + // `??` compares references and so never sees Unity's overloaded ==: a + // GameObject without an Animator yields an object that equals null but is + // not a null reference, so AddComponent was never called and the next line + // threw MissingComponentException. Measured: this path never once worked. + var animator = go.GetComponent(); + if (animator == null) + { + UnityEditor.Undo.RecordObject(go, "Add Animator Component"); + animator = UnityEditor.Undo.AddComponent(go); + } + // Recorded and dirtied like the sibling controller_assign path, so the + // change is undoable and survives a scene save. + UnityEditor.Undo.RecordObject(animator, "Assign AnimatorController"); animator.runtimeAnimatorController = controller; + EditorUtility.SetDirty(go); + diagnostics.AddInfo("SCENE_ANIMATOR_SET", $"Animator set on '{sceneTarget}'.", new { target = sceneTarget }); } + else + { + diagnostics.AddError("SCENE_CONTROLLER_NOT_LOADED", + $"The controller at '{controllerPath}' could not be loaded, so '{sceneTarget}' was left unchanged.", + new { path = controllerPath }, + new[] { "Check the controller_path in the response." }); + } } else { - diagnostics.AddWarning("SCENE_TARGET_NOT_FOUND", + diagnostics.AddError("SCENE_TARGET_NOT_FOUND", $"GameObject '{sceneTarget}' not found in scene.", null, new[] { "Check GameObject name or open the correct scene first." }); } } - // ctrlResult is an anonymous object, so round-trip it through JSON to read two fields. - string complexity = null; - int stateCount = 0; - try - { - var ctrlJson = JsonConvert.SerializeObject(ctrlResult); - var ctrlObj = JObject.Parse(ctrlJson); - complexity = ctrlObj["complexity"]?.ToString(); - stateCount = ctrlObj["state_count"]?.ToObject() ?? 0; - } - catch { /* non-critical */ } + var ctrlObj = AsJObject(ctrlResult); + if (ctrlObj == null) + diagnostics.AddWarning("CONTROLLER_RESULT_UNREADABLE", + "The controller step result could not be read back; complexity and state_count are unknown.", + null, new string[0]); return new { success = !diagnostics.HasErrors, sprite_path = path, controller_path = controllerPath, - controller_complexity = complexity, - state_count = stateCount, - clip_count = clipPaths.Count, + controller_complexity = ctrlObj?["complexity"]?.ToString(), + state_count = ctrlObj?["state_count"]?.ToObject() ?? 0, + clip_count = createdClips.Count, diagnostics = diagnostics.Build(), }; } + /// Reads a builder's anonymous result back as JSON; null when it cannot be parsed. + private static JObject AsJObject(object result) + { + try { return JObject.Parse(JsonConvert.SerializeObject(result)); } + catch { return null; } + } + private static int GetSliceCount(string path) { var sprites = AssetDatabase.LoadAllAssetsAtPath(path); diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 660c011f4..8c62709d3 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -88,6 +88,21 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos if (importer == null) return new ErrorResponse($"No TextureImporter found at '{path}'."); + // These arguments need no texture, so they are checked before the conversion below: + // a refused request used to return an error with the texture already turned into a Sprite. + int cols = @params["cols"]?.ToObject() ?? 0; + int rows = @params["rows"]?.ToObject() ?? 1; + int frameW = @params["frame_width"]?.ToObject() ?? 0; + int frameH = @params["frame_height"]?.ToObject() ?? 0; + + if (cols <= 0 && frameW <= 0) + return new ErrorResponse("Either 'cols' or 'frame_width' is required."); + + // `?? 1` above only covers an absent key, so an explicit rows=0 reaches the + // texH / rows division below and throws instead of answering. + if (rows <= 0 && frameH <= 0) + return new ErrorResponse("'rows' must be 1 or more; pass 'frame_height' instead if the row count is unknown."); + // Measure the texture only once it is imported the way a sprite sheet is. // A Default-type import rescales a non-power-of-two sheet (96px becomes 128px), // and a grid computed against that size puts the trailing frames outside the real @@ -107,19 +122,6 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos int texW = texture.width; int texH = texture.height; - int cols = @params["cols"]?.ToObject() ?? 0; - int rows = @params["rows"]?.ToObject() ?? 1; - int frameW = @params["frame_width"]?.ToObject() ?? 0; - int frameH = @params["frame_height"]?.ToObject() ?? 0; - - if (cols <= 0 && frameW <= 0) - return new ErrorResponse("Either 'cols' or 'frame_width' is required."); - - // `?? 1` above only covers an absent key, so an explicit rows=0 reaches the - // texH / rows division below and throws instead of answering. - if (rows <= 0 && frameH <= 0) - return new ErrorResponse("'rows' must be 1 or more; pass 'frame_height' instead if the row count is unknown."); - if (frameW <= 0) frameW = texW / cols; if (frameH <= 0) frameH = texH / rows; if (cols <= 0) cols = texW / frameW; diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py index 23318cf01..1e4a26229 100644 --- a/Server/src/services/tools/manage_sprite.py +++ b/Server/src/services/tools/manage_sprite.py @@ -110,6 +110,16 @@ async def manage_sprite( return {"success": False, "message": f"'cols' or 'frame_width' is required for '{action}'. " "Use get_info first to retrieve image_base64, analyze the grid visually, then call full_setup with cols/rows."} + # The Unity side is the authority here - it composes the asset path and refuses the + # name again. Checking it up front turns a round-trip into an immediate answer, and a + # separator in a clip name is wrong under every configuration. + for clip in clips or []: + name = clip.get("name") if isinstance(clip, dict) else None + if name and ("/" in name or "\\" in name): + return {"success": False, + "message": f"Clip name '{name}' cannot contain a path separator; " + "use 'output_dir' to choose where clips are written."} + if action_lower == "setup_controller" and not controller_path: return {"success": False, "message": "'controller_path' is required for setup_controller (e.g. 'Assets/Animators/Hero.controller')."} diff --git a/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py index 68da272df..ccecbddce 100644 --- a/Server/tests/test_manage_sprite.py +++ b/Server/tests/test_manage_sprite.py @@ -75,6 +75,12 @@ def test_setup_clips_requires_path(self): assert result["success"] is False assert "path" in result["message"] + def test_clip_name_with_a_separator_is_refused(self): + result = self._call(action="setup_clips", path="Assets/hero.png", + clips=[{"name": "nested/walk"}]) + assert result["success"] is False + assert "separator" in result["message"] + def test_setup_controller_requires_controller_path(self): result = self._call(action="setup_controller", clips=[{"name": "walk", "path": "a.anim"}]) assert result["success"] is False diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 0bff2fac2..b0512698f 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -516,6 +516,68 @@ public void SetupClips_ClipNameEscapingTheOutputDir_IsSkipped() Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_NAME")); } + [Test] + public void SetupClips_ClipNameWithASeparator_IsSkipped() + { + // A separator is not traversal, so the '..' check lets it through - and the name + // then selects a path in a descendant directory instead of a leaf in output_dir. + // The tool's own CLIP_BAD_NAME hint already tells callers to remove separators. + string path = CreateSheet("sepname", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, OneClip("nested/walk", 0, 3)); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.IsNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/nested/walk.anim"), + "a clip name must not choose the directory it lands in"); + } + + [Test] + public void SetupClips_ExistingClipWithoutOverwrite_IsLeftAlone() + { + // setup_controller refuses an existing controller unless overwrite is set. Clips + // took the opposite policy and deleted whatever sat at the composed path, so an + // unrelated clip that merely shared a name was destroyed by a request that never + // asked for a replacement. + string path = CreateSheet("existing", 4, 1); + Slice(path, 4, 1); + + var sentinel = new AnimationClip { frameRate = 99f }; + AssetDatabase.CreateAsset(sentinel, $"{TempRoot}/walk.anim"); + AssetDatabase.SaveAssets(); + + var result = SetupClips(path, OneClip("walk", 0, 3)); + + var after = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + Assert.IsNotNull(after, "the existing clip must survive"); + Assert.AreEqual(99f, after.frameRate, "the existing clip must not be replaced"); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_EXISTS")); + } + + [Test] + public void SetupClips_ExistingClipWithOverwrite_IsReplaced() + { + string path = CreateSheet("existing2", 4, 1); + Slice(path, 4, 1); + + var sentinel = new AnimationClip { frameRate = 99f }; + AssetDatabase.CreateAsset(sentinel, $"{TempRoot}/walk.anim"); + AssetDatabase.SaveAssets(); + + var result = Run(new JObject + { + ["action"] = "setup_clips", + ["path"] = path, + ["clips"] = OneClip("walk", 0, 3), + ["output_dir"] = TempRoot, + ["overwrite"] = true, + }); + + Assert.AreEqual(1, result.Value("clip_count")); + var after = AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim"); + Assert.AreEqual(12f, after.frameRate, "an authorised overwrite must actually replace it"); + } + [Test] public void SetupClips_ZeroFps_IsSkippedInsteadOfWritingInfiniteKeyTimes() { @@ -724,6 +786,131 @@ public void SetupController_ExistingControllerWithOverwrite_Replaces() Assert.IsTrue(SetupController(clips, overwrite: true).Value("success")); } + // ===================================================================== + // Audit verification - each of these asserts the behaviour a finding says + // is missing. Red here means the finding reproduces. + // ===================================================================== + + [Test] + public void AuditS2_OverwriteThatCannotBuildAReplacement_KeepsTheOldController() + { + var clips = BuildClips("s2", "idle", "walk"); + Assert.IsTrue(SetupController(clips).Value("success")); + string ctrl = $"{TempRoot}/Hero.controller"; + var before = AssetDatabase.LoadAssetAtPath(ctrl); + Assert.IsNotNull(before); + + // Every replacement clip is unloadable, so the rebuild cannot succeed. + var doomed = new JArray { + new JObject { ["name"] = "idle", ["path"] = $"{TempRoot}/does_not_exist.anim" }, + }; + Run(new JObject + { + ["action"] = "setup_controller", + ["clips"] = doomed, + ["controller_path"] = ctrl, + ["overwrite"] = true, + }); + + Assert.IsNotNull(AssetDatabase.LoadAssetAtPath(ctrl), + "a failed rebuild must not leave the caller without the controller they had"); + } + + [Test] + public void AuditS5_ControllerRefusal_StopsBeforeTouchingTheScene() + { + string path = CreateSheet("s5", 4, 1); + var go = new GameObject("SpriteTest_S5"); + try + { + string ctrl = $"{TempRoot}/S5.controller"; + Run(new JObject { ["action"] = "full_setup", ["path"] = path, ["cols"] = 4, + ["output_dir"] = TempRoot, ["controller_path"] = ctrl }); + + // Second run: the controller exists and overwrite is not set, so the + // controller step fails - and a failed step must not fall through. + var result = Run(new JObject { ["action"] = "full_setup", ["path"] = path, ["cols"] = 4, + ["output_dir"] = TempRoot, ["controller_path"] = ctrl, + ["add_to_scene"] = true, ["scene_target"] = "SpriteTest_S5" }); + + Assert.IsFalse(result.Value("success")); + Assert.AreEqual("setup_controller", result.Value("step"), + "the response must name the step that failed"); + Assert.IsNull(go.GetComponent(), + "a refused controller step must not go on to modify the scene"); + } + finally { Object.DestroyImmediate(go); } + } + + [Test] + public void AuditS6_RequestedSceneTargetMissing_IsNotReportedAsSuccess() + { + string path = CreateSheet("s6", 4, 1); + var result = Run(new JObject { ["action"] = "full_setup", ["path"] = path, ["cols"] = 4, + ["output_dir"] = TempRoot, ["controller_path"] = $"{TempRoot}/S6.controller", + ["add_to_scene"] = true, ["scene_target"] = "NoSuchObject" }); + + Assert.IsFalse(result.Value("success"), + "an attachment that was asked for and did not happen is not a success"); + } + + [Test] + public void AuditS7_ControllerPathWithoutExtension_StillReachesTheSceneObject() + { + string path = CreateSheet("s7", 4, 1); + var go = new GameObject("SpriteTest_S7"); + try + { + var result = Run(new JObject { ["action"] = "full_setup", ["path"] = path, ["cols"] = 4, + ["output_dir"] = TempRoot, + ["controller_path"] = $"{TempRoot}/S7", // no .controller suffix + ["add_to_scene"] = true, ["scene_target"] = "SpriteTest_S7" }); + + // Count the components rather than null-checking the result of GetComponent: + // a missing component compares equal to null but is not a null reference, so + // Assert.IsNotNull passes and the next member access throws instead of failing. + Assert.AreEqual(1, go.GetComponents().Length, + "the object should have received an Animator; result was " + result.ToString(Newtonsoft.Json.Formatting.None)); + Assert.IsTrue(go.GetComponents()[0].runtimeAnimatorController != null, + "the suffix the builder added must not lose the controller on the way to the scene"); + } + finally { Object.DestroyImmediate(go); } + } + + [Test] + public void AuditS4_RefusedClip_IsNotCountedAsCreated() + { + string path = CreateSheet("s4", 6, 1); + var result = Run(new JObject + { + ["action"] = "full_setup", ["path"] = path, ["cols"] = 6, + ["output_dir"] = TempRoot, ["controller_path"] = $"{TempRoot}/S4.controller", + ["clips"] = new JArray { + new JObject { ["name"] = "idle", ["start_frame"] = 0, ["end_frame"] = 1 }, + new JObject { ["name"] = "attack", ["start_frame"] = 2, ["end_frame"] = 3, ["fps"] = 0 }, + new JObject { ["name"] = "walk", ["start_frame"] = 4, ["end_frame"] = 5 }, + }, + }); + + int onDisk = AssetDatabase.FindAssets("t:AnimationClip", new[] { TempRoot }).Length; + Assert.AreEqual(onDisk, result.Value("clip_count"), + "clip_count must count the clips that exist, not the ones that were asked for"); + } + + [Test] + public void AuditS1_RowsRejected_LeavesTheTextureTypeAlone() + { + string path = CreateSheet("s1", 4, 1); + var before = ((TextureImporter)AssetImporter.GetAtPath(path)).textureType; + + var result = Slice(path, 4, 0); // refused: rows must be >= 1 + Assert.IsFalse(result.Value("success")); + + var after = ((TextureImporter)AssetImporter.GetAtPath(path)).textureType; + Assert.AreEqual(before, after, + "a refused request must not leave the texture converted behind it"); + } + // ===================================================================== // full_setup // ===================================================================== From f9ddbf291b218dd73e0d1b9ea775a2cd19ad0ee1 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 18:31:06 +0300 Subject: [PATCH 04/29] test(sprite): make four tests fail for the reason they are named after The audit's test-validity lens found cases that would stay green in a world where the behaviour they name was never implemented. Each is now pinned to that behaviour, and each pinning was checked by removing the behaviour and watching the right tests turn red: - GetInfo_MissingPath asserted only that the call failed. With the required-path branch removed the same request fails on the importer-not-found branch instead. It now asserts the message; removing the branch breaks it, which it did not do before. - The two slice_count tests read the field with Value, and an absent field reads back as the zero one of them expects. They now assert the field is present; deleting it from the response breaks both. - The controller overwrite test asserted two successes, which is equally true of a run that reused the existing asset. It now marks the first controller and asserts the mark is gone. - CreateSheet and Slice claimed fixtures they never checked. Slice now asserts the frame count it asked for. CreateSheet asserts only that the asset imported: its dimensions cannot be asserted there, because the texture is still Default-type and that import rescales a non-power-of-two sheet - 96px reads back as 128px, which is the behaviour slice_sheet exists to work around. --- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 46 ++++++++++++++++--- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index b0512698f..dded5d9c3 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -53,6 +53,15 @@ private static string CreateSheet(string name, int cols, int rows) Object.DestroyImmediate(tex); AssetDatabase.ImportAsset(assetPath, ImportAssetOptions.ForceSynchronousImport); + + // A fixture that quietly produces less than it claims weakens every test built on + // it, so it states what it can: the asset exists. Its dimensions cannot be asserted + // here - the texture is still Default-type at this point, and that import rescales a + // non-power-of-two sheet (96px is read back as 128px), which is the very behaviour + // slice_sheet works around. The frame count after slicing is the real postcondition + // and Slice() below asserts it. + Assert.IsNotNull(AssetDatabase.LoadAssetAtPath(assetPath), + $"fixture: {assetPath} did not import"); return assetPath; } @@ -67,13 +76,22 @@ private static string CreateSheet(string name, int cols, int rows) private static string ErrorText(JObject result) => result.Value("error") ?? result.Value("message") ?? ""; - private static JObject Slice(string path, int cols, int rows) => Run(new JObject + private static JObject Slice(string path, int cols, int rows) { - ["action"] = "slice_sheet", - ["path"] = path, - ["cols"] = cols, - ["rows"] = rows, - }); + var result = Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = path, + ["cols"] = cols, + ["rows"] = rows, + }); + // Only assert the postcondition for a call that was meant to succeed; the refusal + // tests call this helper too and check the failure themselves. + if (result.Value("success")) + Assert.AreEqual(cols * rows, SpritesOf(path).Length, + "fixture: slice_sheet produced fewer frames than the grid asked for"); + return result; + } /// The sliced frames, in the natural order their names imply. private static Sprite[] SpritesOf(string path) => @@ -113,6 +131,8 @@ public void GetInfo_MissingPath_ReturnsError() { var result = Run(new JObject { ["action"] = "get_info" }); Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("'path' is required"), + "success alone would also be false on the importer-not-found branch"); } [Test] @@ -144,6 +164,8 @@ public void GetInfo_OnAnUnslicedSheet_ReportsNoSlices() string path = CreateSheet("unsliced", 4, 2); var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + Assert.IsNotNull(result["slice_count"], + "an absent field also reads as 0, so the field itself has to be there"); Assert.AreEqual(0, result.Value("slice_count")); } @@ -783,7 +805,19 @@ public void SetupController_ExistingControllerWithOverwrite_Replaces() { var clips = BuildClips("overwrite", "idle", "walk"); Assert.IsTrue(SetupController(clips).Value("success")); + + // Mark the first controller so a run that merely reused it is distinguishable + // from one that replaced it: two successes alone are true of both. + string ctrlPath = $"{TempRoot}/Hero.controller"; + var first = AssetDatabase.LoadAssetAtPath(ctrlPath); + first.AddParameter("SentinelFromFirstBuild", AnimatorControllerParameterType.Bool); + AssetDatabase.SaveAssets(); + Assert.IsTrue(SetupController(clips, overwrite: true).Value("success")); + + var second = AssetDatabase.LoadAssetAtPath(ctrlPath); + Assert.That(second.parameters.Select(p => p.name), Has.No.Member("SentinelFromFirstBuild"), + "an authorised overwrite must build a new controller, not reuse the old one"); } // ===================================================================== From e55c553be6ea2429237c53f310c4bac72551675e Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 18:47:07 +0300 Subject: [PATCH 05/29] fix(sprite): close what the verification round found in the fixes themselves A second audit pass took the fix diff as its target rather than the tool, on the principle that a fix is fresh code nobody has read. It found three things, all of them introduced or left behind by the previous commit. - The new clip-name check tested `"/" in name` on a value the tool's own signature types as Any, so a JSON number raised TypeError before the call could answer at all. A non-string name is now refused with a message. - Moving slice_sheet's argument checks above the texture conversion closed only the arguments that can be judged without a texture. A frame larger than the sheet can be judged only after measuring it, so that refusal still left the texture converted behind it. The previous type is now restored on every refusal that happens after the conversion - the oversized frame, the oversized row height, and a texture that fails to load. - Writing the test for that second case surfaced a third defect: an oversized frame_height was not refused at all. The grid still works out to a non-zero frame count, so the empty-grid check never sees it, and the rects simply land outside the texture where Unity discards them - with the call reporting success. The grid is now required to fit inside the texture. Discrimination measured, as before: removing the type check breaks the one Python case; neutering the restore breaks both importer cases; removing the bounds check breaks the height case. --- .../Tools/Sprite2D/SpriteImportSetup.cs | 33 +++++++++++++++++++ Server/src/services/tools/manage_sprite.py | 9 ++++- Server/tests/test_manage_sprite.py | 6 ++++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 30 +++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 8c62709d3..9cf4cadaf 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -75,6 +75,15 @@ public static object GetInfo(JObject @params) return result; } + /// Undoes the conversion above when the request is refused after it. + private static void RestoreTextureType(TextureImporter importer, TextureImporterType previous) + { + if (importer.textureType == previous) return; + importer.textureType = previous; + EditorUtility.SetDirty(importer); + importer.SaveAndReimport(); + } + // ── SliceSheet ─────────────────────────────────────────────────────── public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnostics) @@ -108,6 +117,10 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos // and a grid computed against that size puts the trailing frames outside the real // texture, where Unity drops them without an error. Measured on 6000.4.4f1: a // 96x16 sheet asked for 6 columns produced 4 sprites of 21px. + // Some refusals can only be reached after the texture has been measured - a frame + // size larger than the sheet is one - so the previous type is kept and restored on + // the way out. A request that was refused must not leave a converted texture behind. + var previousType = importer.textureType; if (importer.textureType != TextureImporterType.Sprite) { importer.textureType = TextureImporterType.Sprite; @@ -117,7 +130,10 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos var texture = AssetDatabase.LoadAssetAtPath(path); if (texture == null) + { + RestoreTextureType(importer, previousType); return new ErrorResponse($"Could not load texture at '{path}'."); + } int texW = texture.width; int texH = texture.height; @@ -127,6 +143,22 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos if (cols <= 0) cols = texW / frameW; if (rows <= 0) rows = texH / frameH; + // A frame larger than the sheet still yields a non-zero grid, so the empty-grid + // check below never sees it: the rects simply land outside the texture and Unity + // drops them while the call reports success. Measured on 6000.4.4f1 with + // frame_height=4096 on a 16px-tall sheet. + if (cols * frameW > texW || rows * frameH > texH) + { + diagnostics.AddError( + "SLICE_OUT_OF_BOUNDS", + "The grid does not fit inside the texture, so some frames would fall outside it.", + new { cols, rows, frame_width = frameW, frame_height = frameH, texture_width = texW, texture_height = texH }, + new[] { "Reduce frame_width/frame_height, or cols/rows", "Confirm the texture dimensions with get_info" } + ); + RestoreTextureType(importer, previousType); + return new { success = false, diagnostics = diagnostics.Build() }; + } + int totalFrames = cols * rows; if (totalFrames == 0) { @@ -136,6 +168,7 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos new { cols, rows, frame_width = frameW, frame_height = frameH, texture_width = texW, texture_height = texH }, new[] { "Check the cols and rows values", "Confirm the texture dimensions with get_info" } ); + RestoreTextureType(importer, previousType); return new { success = false, diagnostics = diagnostics.Build() }; } diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py index 1e4a26229..000a34f83 100644 --- a/Server/src/services/tools/manage_sprite.py +++ b/Server/src/services/tools/manage_sprite.py @@ -115,7 +115,14 @@ async def manage_sprite( # separator in a clip name is wrong under every configuration. for clip in clips or []: name = clip.get("name") if isinstance(clip, dict) else None - if name and ("/" in name or "\\" in name): + if name is None: + continue + # `clips` is typed as list[dict[str, Any]], so a JSON number reaches this check. + # Testing membership on one raises TypeError before the tool can answer at all. + if not isinstance(name, str): + return {"success": False, + "message": f"Clip name must be a string, got {type(name).__name__}."} + if "/" in name or "\\" in name: return {"success": False, "message": f"Clip name '{name}' cannot contain a path separator; " "use 'output_dir' to choose where clips are written."} diff --git a/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py index ccecbddce..22a778e9d 100644 --- a/Server/tests/test_manage_sprite.py +++ b/Server/tests/test_manage_sprite.py @@ -81,6 +81,12 @@ def test_clip_name_with_a_separator_is_refused(self): assert result["success"] is False assert "separator" in result["message"] + def test_non_string_clip_name_is_refused_not_raised(self): + # clips is typed list[dict[str, Any]], so a JSON number reaches the name check. + result = self._call(action="setup_clips", path="Assets/hero.png", clips=[{"name": 7}]) + assert result["success"] is False + assert "must be a string" in result["message"] + def test_setup_controller_requires_controller_path(self): result = self._call(action="setup_controller", clips=[{"name": "walk", "path": "a.anim"}]) assert result["success"] is False diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index dded5d9c3..7b184f3f3 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -338,6 +338,36 @@ public void SliceSheet_ZeroRows_FailsWithAMessageInsteadOfThrowing() Assert.IsFalse(result.Value("success")); } + [Test] + public void SliceSheet_FrameWiderThanTheTexture_LeavesTheTextureTypeAlone() + { + // This refusal is only reachable after the texture has been measured, so it is the + // form of the class that moving the argument checks earlier could not close. + string path = CreateSheet("restore_w", 2, 1); + var before = ((TextureImporter)AssetImporter.GetAtPath(path)).textureType; + + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, ["frame_width"] = 4096 }); + Assert.IsFalse(result.Value("success")); + + Assert.AreEqual(before, ((TextureImporter)AssetImporter.GetAtPath(path)).textureType, + "a refused request must not leave the texture converted behind it"); + } + + [Test] + public void SliceSheet_FrameTallerThanTheTexture_LeavesTheTextureTypeAlone() + { + // The second form of the same class: the height axis reaches the same refusal. + string path = CreateSheet("restore_h", 2, 1); + var before = ((TextureImporter)AssetImporter.GetAtPath(path)).textureType; + + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["frame_width"] = Cell, ["frame_height"] = 4096 }); + Assert.IsFalse(result.Value("success")); + + Assert.AreEqual(before, ((TextureImporter)AssetImporter.GetAtPath(path)).textureType, + "a refused request must not leave the texture converted behind it"); + } + [Test] public void SliceSheet_ReslicingWithADifferentGrid_ReplacesTheOldFrames() { From 37fadcc5005c00eaf201b87291870f6c177e1398 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 19:06:12 +0300 Subject: [PATCH 06/29] fix(sprite): refuse the degenerate grids the bounds check let through A second verification round took the previous fix as its target. It confirmed two of that commit's three claims and broke the third, and the shape of the break is worth stating: a bounds check that asks "does the grid fit" answers yes when the grid has collapsed. - A derived frame size can be zero. Sixty-four columns across thirty-two pixels gives zero-wide frames, the product is then zero, and zero fits inside anything - so sixty-four degenerate rects were written and the call reported success. The same holds on the row axis. - The product was computed in int. cols=65536 with frame_width=65536 wraps to zero in unchecked arithmetic and slips under the comparison, so it is computed in long. The same round noted that a clips entry which is not an object would throw on the typed foreach cast, and left its reachability unestablished. It is reachable: the Python surface forwards `clips=["not_a_dict", 7]` unchanged - measured - so both builders now skip such an entry with a diagnostic instead of raising InvalidCastException. One thing the round reported is deliberately unchanged: a frame wider than the texture is classified SLICE_EMPTY rather than SLICE_OUT_OF_BOUNDS. The count really is zero there and the message already names the frame size as a cause, so the existing test keeps its expectation. --- .../Tools/Sprite2D/SpriteClipBuilder.cs | 10 +++- .../Tools/Sprite2D/SpriteControllerBuilder.cs | 10 +++- .../Tools/Sprite2D/SpriteImportSetup.cs | 9 ++- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 58 +++++++++++++++++++ 4 files changed, 84 insertions(+), 3 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs index 8f4821ab0..630397642 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -53,8 +53,16 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos var createdClips = new List(); - foreach (JObject clipDef in clipsToken) + foreach (JToken clipToken in clipsToken) { + // Measured: the Python surface forwards a clips entry that is not an + // object, and the typed foreach cast threw InvalidCastException on it. + if (!(clipToken is JObject clipDef)) + { + diagnostics.AddWarning("CLIP_NOT_AN_OBJECT", "A clips entry is not an object - skipped.", null, new[] { "Each clip must be an object with a 'name'." }); + continue; + } + string clipName = clipDef["name"]?.ToString(); if (string.IsNullOrEmpty(clipName)) { diagnostics.AddWarning("CLIP_NO_NAME", "Clip name is missing — skipped.", null, new[] { "Add a 'name' field to each clip definition." }); continue; } diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs index 799532218..9476ee6c8 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs @@ -36,8 +36,16 @@ public static object Build(JObject @params, SpriteDiagnosticBuilder diagnostics) bool overwrite = @params["overwrite"]?.ToObject() ?? false; var entries = new List<(SpriteAnimEntry entry, AnimationClip clip)>(); - foreach (JObject cd in clipsToken) + foreach (JToken clipToken in clipsToken) { + // Measured: the Python surface forwards a clips entry that is not an + // object, and the typed foreach cast threw InvalidCastException on it. + if (!(clipToken is JObject cd)) + { + diagnostics.AddWarning("CLIP_NOT_AN_OBJECT", "A clips entry is not an object - skipped.", null, new[] { "Each clip must be an object with a 'name'." }); + continue; + } + string clipName = cd["name"]?.ToString() ?? ""; string clipPath = cd["path"]?.ToString() ?? ""; var clip = AssetDatabase.LoadAssetAtPath( diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 9cf4cadaf..ff6042f88 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -147,7 +147,14 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos // check below never sees it: the rects simply land outside the texture and Unity // drops them while the call reports success. Measured on 6000.4.4f1 with // frame_height=4096 on a 16px-tall sheet. - if (cols * frameW > texW || rows * frameH > texH) + // Three ways a grid fails to fit, and only the first is obvious. Integer division + // can drive a DERIVED frame size to zero - 64 columns across 32 pixels gives 0-wide + // frames - and the product is then 0, which passes any bounds test while the + // metadata is degenerate: measured, 64 zero-width sprites reported as success. And + // the product itself is computed in long, because two large caller-supplied values + // wrap in 32-bit arithmetic and slip under the comparison. + if (frameW <= 0 || frameH <= 0 + || (long)cols * frameW > texW || (long)rows * frameH > texH) { diagnostics.AddError( "SLICE_OUT_OF_BOUNDS", diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 7b184f3f3..40c1873a1 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -368,6 +368,64 @@ public void SliceSheet_FrameTallerThanTheTexture_LeavesTheTextureTypeAlone() "a refused request must not leave the texture converted behind it"); } + [Test] + public void SliceSheet_MoreColumnsThanPixels_IsRefused() + { + // 64 columns across 32 pixels derives a 0-wide frame. The bounds product is then + // 0, which passes any "does it fit" test, and 64 degenerate rects were written + // with the call reporting success. + string path = CreateSheet("degenerate_w", 2, 1); // 32x16 + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, ["cols"] = 64 }); + + Assert.IsFalse(result.Value("success")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_OUT_OF_BOUNDS")); + Assert.AreEqual(0, SpritesOf(path).Length, "no degenerate frame may be written"); + } + + [Test] + public void SliceSheet_MoreRowsThanPixels_IsRefused() + { + string path = CreateSheet("degenerate_h", 2, 1); // 32x16 + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["cols"] = 2, ["rows"] = 32 }); + + Assert.IsFalse(result.Value("success")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_OUT_OF_BOUNDS")); + } + + [Test] + public void SliceSheet_GridProductThatOverflowsInt_IsStillRefused() + { + // 65536 * 65536 wraps to 0 in unchecked 32-bit arithmetic and slipped under the + // comparison; the product is computed in long for that reason. + string path = CreateSheet("overflow", 2, 1); + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["cols"] = 65536, ["frame_width"] = 65536 }); + + Assert.IsFalse(result.Value("success")); + } + + [Test] + public void SetupClips_ClipEntryThatIsNotAnObject_IsSkipped() + { + // The Python surface forwards these unchanged - measured - and the typed foreach + // cast threw InvalidCastException on them. + string path = CreateSheet("nonobj", 4, 1); + Slice(path, 4, 1); + + JObject result = null; + Assert.DoesNotThrow(() => result = Run(new JObject + { + ["action"] = "setup_clips", + ["path"] = path, + ["clips"] = new JArray { "not_an_object", 7 }, + ["output_dir"] = TempRoot, + }), "a malformed clips entry must come back as a diagnostic, not an exception"); + + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_NOT_AN_OBJECT")); + } + [Test] public void SliceSheet_ReslicingWithADifferentGrid_ReplacesTheOldFrames() { From 973853648205959970d13167518b2d01a0d79f45 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 19:21:23 +0300 Subject: [PATCH 07/29] fix(sprite): return the reason when every clip entry is skipped The third verification round confirmed both of the previous commit's claims and qualified one response shape. setup_controller records why it skipped each clips entry, but when every entry is skipped it returns the generic "No valid clips loaded." - and ErrorResponse has no diagnostics field, so a caller who sent a malformed array was told the clips did not load without being told that none of them were objects. The diagnostics now travel in ErrorResponse's existing data field. That placement is the point: returning a diagnostics-carrying anonymous object instead would have been shorter and wrong, because SpriteFullSetup stops on `is ErrorResponse` and CLIP_NOT_AN_OBJECT is a warning, so HasErrors would not have caught it - the controller step would have fallen through to the scene step again, which is the defect two commits ago closed. --- .../Tools/Sprite2D/SpriteControllerBuilder.cs | 7 ++++++- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs index 9476ee6c8..07c6e76d8 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs @@ -56,7 +56,12 @@ public static object Build(JObject @params, SpriteDiagnosticBuilder diagnostics) } if (entries.Count == 0) - return new ErrorResponse("No valid clips loaded."); + // The diagnostics travel in ErrorResponse's data field rather than in a + // diagnostics-carrying anonymous object: SpriteFullSetup stops on + // `is ErrorResponse`, and CLIP_NOT_AN_OBJECT is a warning, so HasErrors would + // not catch it - changing the type here would let a failed controller step + // fall through to the scene step again. + return new ErrorResponse("No valid clips loaded.", new { diagnostics = diagnostics.Build() }); // The existing controller is only removed once the replacement is known to be // buildable: deleting first left a failed rebuild with no controller at all. diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 40c1873a1..8610e2915 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -781,6 +781,25 @@ public void SetupController_ClipsThatDoNotExist_ReturnsError() Assert.IsFalse(result.Value("success")); } + [Test] + public void SetupController_EveryEntrySkipped_StillReportsWhy() + { + // The builder records why it skipped each entry, but the all-skipped path returns + // a generic error - so the caller was told the clips did not load without being + // told that none of them were objects. + JObject result = null; + Assert.DoesNotThrow(() => result = Run(new JObject + { + ["action"] = "setup_controller", + ["clips"] = new JArray { 7 }, + ["controller_path"] = $"{TempRoot}/Skipped.controller", + })); + + Assert.IsFalse(result.Value("success")); + Assert.That(result.ToString(), Does.Contain("CLIP_NOT_AN_OBJECT"), + "the response must carry the reason the builder recorded"); + } + [Test] public void SetupController_IdleAndWalk_WritesAControllerWithBothStates() { From 6653cc07f1908a9c82feb59bb8c9b08e412ac7c3 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 19:30:25 +0300 Subject: [PATCH 08/29] docs(sprite): document what overwrite now covers, and add usage examples The `overwrite` description still said "controller", but the flag governs clips as well since they stopped replacing an existing asset without being asked to. It now says what it actually does, including the default. The examples block covers the part of this tool that is not evident from the parameter table: the grid is the one thing it cannot infer, so `get_info` comes first and returns the sheet as an image for a caller that can look at it; and clip names, not extra parameters, are what decide the controller's shape and each clip's loop flag. --- Server/src/services/tools/manage_sprite.py | 6 +- .../tools/animation/manage_sprite.md | 57 ++++++++++++++++++- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py index 000a34f83..bec9e01ea 100644 --- a/Server/src/services/tools/manage_sprite.py +++ b/Server/src/services/tools/manage_sprite.py @@ -85,7 +85,11 @@ async def manage_sprite( str | None, "Path for the .controller asset (e.g. 'Assets/Animators/Hero.controller').", ] = None, - overwrite: Annotated[bool, "Overwrite existing controller if it already exists."] = False, + overwrite: Annotated[ + bool, + "Replace an existing .anim or .controller at the target path. Off by default: " + "without it an existing asset is kept and reported back, not silently replaced.", + ] = False, add_to_scene: Annotated[bool, "Attach Animator + controller to a scene GameObject."] = False, scene_target: Annotated[ str | None, diff --git a/website/docs/reference/tools/animation/manage_sprite.md b/website/docs/reference/tools/animation/manage_sprite.md index 66d2135b0..89fec3680 100644 --- a/website/docs/reference/tools/animation/manage_sprite.md +++ b/website/docs/reference/tools/animation/manage_sprite.md @@ -29,7 +29,7 @@ description: "2D sprite animation tool. get_info: read sprite import settings + | `animation_name` | `str \| None` | — | Animation name for full_setup when clips are not specified (all frames = one clip). | | `output_dir` | `str \| None` | — | Output directory for .anim and .controller assets (default: same folder as sprite). | | `controller_path` | `str \| None` | — | Path for the .controller asset (e.g. 'Assets/Animators/Hero.controller'). | -| `overwrite` | `bool` | — | Overwrite existing controller if it already exists. | +| `overwrite` | `bool` | — | Replace an existing .anim or .controller at the target path. Off by default: without it an existing asset is kept and reported back, not silently replaced. | | `add_to_scene` | `bool` | — | Attach Animator + controller to a scene GameObject. | | `scene_target` | `str \| None` | — | Existing GameObject name to attach Animator to. | @@ -40,6 +40,59 @@ A `dict` containing the Unity response. The exact shape depends on the action. ## Examples -*No examples yet. Add usage examples here — they will be preserved across regenerations.* +### Read the sheet before slicing it + +The grid is the one thing the tool cannot infer. `get_info` returns the texture's +dimensions and the sheet itself as `image_base64`, so a vision-capable caller can count the +frames before committing to a grid. + +```json +{ "action": "get_info", "path": "Assets/Sprites/hero_walk.png" } +``` + +### One command from sheet to controller + +```json +{ + "action": "full_setup", + "path": "Assets/Sprites/hero.png", + "cols": 6, + "rows": 4, + "clips": [ + { "name": "idle", "start_frame": 0, "end_frame": 5 }, + { "name": "walk", "start_frame": 6, "end_frame": 11 }, + { "name": "run", "start_frame": 12, "end_frame": 17 }, + { "name": "attack", "start_frame": 18, "end_frame": 23, "fps": 18 } + ], + "controller_path": "Assets/Animators/Hero.controller", + "add_to_scene": true, + "scene_target": "Hero" +} +``` + +Clip names decide the controller's shape: `idle` becomes the default state, `walk` and +`run` collapse into a `Speed`-driven 1D blend tree, and `attack` gets an `Attack` trigger. +Looping follows from the same names — locomotion and idle loop, a one-shot does not — and +an explicit `"loop"` on a clip overrides that. + +### Slicing on its own + +```json +{ "action": "slice_sheet", "path": "Assets/Sprites/hero.png", "frame_width": 32, "frame_height": 32 } +``` + +`frame_width`/`frame_height` are the alternative to `cols`/`rows`; supply either pair. A +grid that does not fit inside the texture is refused rather than silently dropping the +frames that fall outside it. + +### Replacing what is already there + +Existing `.anim` and `.controller` assets are kept unless `overwrite` is set, so a repeated +`full_setup` reports what it found instead of overwriting work: + +```json +{ "action": "setup_clips", "path": "Assets/Sprites/hero.png", + "clips": [{ "name": "walk", "start_frame": 0, "end_frame": 5 }], "overwrite": true } +``` From 33766be3b9e125bbd52cd6b2167b57600cb96be2 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:16:18 +0300 Subject: [PATCH 09/29] fix(sprite): use an existence check that compiles on Unity 2021.3 --- MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs index 6e3d66bec..393d83afc 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs @@ -30,7 +30,12 @@ public static object Run(JObject @params) return new ErrorResponse("'path' is required."); path = AssetPathUtility.SanitizeAssetPath(path); - if (!AssetDatabase.AssetPathExists(path)) + // Not AssetDatabase.AssetPathExists: that landed in Unity 2023.1 and package.json + // declares 2021.3, so it does not compile on the lower half of the support range - + // TestProjects/UnityMCPTests is pinned to 2021.3.45f2, which is where CI would have + // caught it. GetMainAssetTypeAtPath answers the same question on every version, + // which is why ManageAsset.cs uses it; no shim is needed when one API spans the range. + if (AssetDatabase.GetMainAssetTypeAtPath(path) == null) return new ErrorResponse($"Sprite not found: '{path}'"); var diagnostics = new SpriteDiagnosticBuilder(); From ed73c24f7465dc540d461b0b3c52d7aa81bbdf5c Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:16:18 +0300 Subject: [PATCH 10/29] fix(sprite): refuse a rejected path instead of forwarding null to Unity --- .../Tools/Sprite2D/SpriteClipBuilder.cs | 2 + .../Tools/Sprite2D/SpriteControllerBuilder.cs | 6 +- .../Editor/Tools/Sprite2D/SpriteFullSetup.cs | 4 +- .../Tools/Sprite2D/SpriteImportSetup.cs | 6 ++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 84 +++++++++++++++++++ 5 files changed, 99 insertions(+), 3 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs index 630397642..cb3d5e987 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -25,6 +25,8 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos return new ErrorResponse("'path' is required."); path = AssetPathUtility.SanitizeAssetPath(path); + if (path == null) + return new ErrorResponse("'path' must stay under Assets/ and cannot contain '..'."); var allSprites = AssetDatabase.LoadAllAssetsAtPath(path) .OfType() diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs index 07c6e76d8..9babf0992 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs @@ -48,8 +48,10 @@ public static object Build(JObject @params, SpriteDiagnosticBuilder diagnostics) string clipName = cd["name"]?.ToString() ?? ""; string clipPath = cd["path"]?.ToString() ?? ""; - var clip = AssetDatabase.LoadAssetAtPath( - AssetPathUtility.SanitizeAssetPath(clipPath)); + string safeClipPath = AssetPathUtility.SanitizeAssetPath(clipPath); + if (safeClipPath == null) + { diagnostics.AddWarning("CLIP_BAD_PATH", $"Clip '{clipName}': path '{clipPath}' must stay under Assets/ and cannot contain '..' - skipped.", null, new string[0]); continue; } + var clip = AssetDatabase.LoadAssetAtPath(safeClipPath); if (clip == null) { diagnostics.AddWarning("CLIP_NOT_FOUND", $"Clip '{clipName}' not found at '{clipPath}' — skipped.", null, new string[0]); continue; } entries.Add((SpriteNamingDetector.Detect(clipName), clip)); diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs index 393d83afc..002f2cc4f 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs @@ -30,6 +30,8 @@ public static object Run(JObject @params) return new ErrorResponse("'path' is required."); path = AssetPathUtility.SanitizeAssetPath(path); + if (path == null) + return new ErrorResponse("'path' must stay under Assets/ and cannot contain '..'."); // Not AssetDatabase.AssetPathExists: that landed in Unity 2023.1 and package.json // declares 2021.3, so it does not compile on the lower half of the support range - // TestProjects/UnityMCPTests is pinned to 2021.3.45f2, which is where CI would have @@ -157,7 +159,7 @@ public static object Run(JObject @params) if (go != null) { var controller = AssetDatabase.LoadAssetAtPath( - AssetPathUtility.SanitizeAssetPath(controllerPath)); + controllerPath); if (controller != null) { // `??` compares references and so never sees Unity's overloaded ==: a diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index ff6042f88..746e9bb5c 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -23,6 +23,10 @@ public static object GetInfo(JObject @params) return new ErrorResponse("'path' is required."); path = AssetPathUtility.SanitizeAssetPath(path); + // A refused path comes back null, and reporting that as "no TextureImporter here" + // names the wrong problem: the path was never looked up. + if (path == null) + return new ErrorResponse("'path' must stay under Assets/ and cannot contain '..'."); var importer = AssetImporter.GetAtPath(path) as TextureImporter; if (importer == null) return new ErrorResponse($"No TextureImporter found at '{path}'. Is it a texture/sprite?"); @@ -93,6 +97,8 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos return new ErrorResponse("'path' is required."); path = AssetPathUtility.SanitizeAssetPath(path); + if (path == null) + return new ErrorResponse("'path' must stay under Assets/ and cannot contain '..'."); var importer = AssetImporter.GetAtPath(path) as TextureImporter; if (importer == null) return new ErrorResponse($"No TextureImporter found at '{path}'."); diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 8610e2915..18be353c5 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1101,5 +1101,89 @@ public void FullSetup_DefaultsTheClipNameToTheFileName() Assert.IsNotNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/hero_idle.anim")); } + + // ===================================================================== + // Refused paths + // + // SanitizeAssetPath answers a traversal path with null, and null is a value every + // AssetDatabase entry point accepts. Each action below used to hand that null on and + // then describe the result - "no TextureImporter here", "no sprites found" - which + // names a lookup that never happened. These pin the refusal itself. + // ===================================================================== + + [Test] + public void GetInfo_PathEscapingAssets_RefusesInsteadOfLookingItUp() + { + JObject result = null; + Assert.DoesNotThrow(() => result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = $"{TempRoot}/../../../outside.png", + })); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("..")); + } + + [Test] + public void SliceSheet_PathEscapingAssets_RefusesInsteadOfLookingItUp() + { + JObject result = null; + Assert.DoesNotThrow(() => result = Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = $"{TempRoot}/../../../outside.png", + ["cols"] = 4, + })); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("..")); + } + + [Test] + public void SetupClips_PathEscapingAssets_RefusesInsteadOfLookingItUp() + { + JObject result = null; + Assert.DoesNotThrow(() => result = SetupClips( + $"{TempRoot}/../../../outside.png", OneClip("walk", 0, 3))); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("..")); + } + + [Test] + public void FullSetup_PathEscapingAssets_RefusesInsteadOfLookingItUp() + { + JObject result = null; + Assert.DoesNotThrow(() => result = Run(new JObject + { + ["action"] = "full_setup", + ["path"] = $"{TempRoot}/../../../outside.png", + ["cols"] = 4, + ["output_dir"] = TempRoot, + })); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("..")); + } + + [Test] + public void SetupController_ClipPathEscapingAssets_SkipsThatClipAndSaysWhy() + { + var clips = BuildClips("badclippath", "idle", "walk"); + clips.Add(new JObject + { + ["name"] = "attack", + ["path"] = $"{TempRoot}/../../../outside.anim", + }); + + JObject result = null; + Assert.DoesNotThrow(() => result = SetupController(clips)); + + // The other two clips are fine, so the controller is still built - but the refused + // entry must be reported as refused, not as merely missing. + Assert.IsTrue(result.Value("success")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_PATH")); + } } } From a548ab519ad163416b1bb95b2f58b8338d8cb505 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:16:18 +0300 Subject: [PATCH 11/29] fix(sprite): reject a negative start_frame instead of shifting the range --- .../Editor/Tools/Sprite2D/SpriteClipBuilder.cs | 9 +++++++++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs index cb3d5e987..01fda7e21 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -80,6 +80,15 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos int startFrame = clipDef["start_frame"]?.ToObject() ?? 0; int endFrame = clipDef["end_frame"]?.ToObject() ?? allSprites.Length - 1; + if (startFrame < 0 || endFrame < startFrame) + { + // Enumerable.Skip yields everything for a negative count, so start_frame=-2 + // with end_frame=3 wrote frames 0..5 and called it a success. A reversed + // range already lands on CLIP_EMPTY, but naming it here says which input + // was wrong instead of which result was empty. + diagnostics.AddWarning("CLIP_BAD_RANGE", $"Clip '{clipName}': frame range [{startFrame},{endFrame}] is invalid - skipped.", null, new[] { "start_frame must be 0 or more, and end_frame must not be below start_frame." }); + continue; + } float fps = clipDef["fps"]?.ToObject() ?? 12f; if (fps <= 0f) { diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 18be353c5..1573bf3e2 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1185,5 +1185,23 @@ public void SetupController_ClipPathEscapingAssets_SkipsThatClipAndSaysWhy() Assert.IsTrue(result.Value("success")); Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_PATH")); } + + // ===================================================================== + // Bounds + // ===================================================================== + + [Test] + public void SetupClips_NegativeStartFrame_IsRefusedRatherThanShiftedToZero() + { + // Enumerable.Skip ignores a negative count, so [-2,3] used to select frames 0..5 + // and report success with a clip the caller never asked for. + string path = CreateSheet("negrange", 4, 1); + Slice(path, 4, 1); + + var result = SetupClips(path, OneClip("walk", -2, 3)); + Assert.AreEqual(0, result.Value("clip_count")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_RANGE")); + Assert.IsNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim")); + } } } From eb2d6bfe1626ef30ae0a94965727c7c310e3dea5 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:18:00 +0300 Subject: [PATCH 12/29] fix(sprite): cap the frame count before allocating slice metadata --- .../Tools/Sprite2D/SpriteImportSetup.cs | 23 +++++++++++++++++-- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 19 +++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 746e9bb5c..b9af1df58 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -172,7 +172,26 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos return new { success = false, diagnostics = diagnostics.Build() }; } - int totalFrames = cols * rows; + // A 4096x4096 sheet cut into 1px frames is 16,777,216 entries, and this method + // allocates and reimports every one of them in one call. That size was not run + // here - the ceiling is a precaution, not a reproduction - but it sits far above + // any real sheet (Unity's own Sprite Editor works in the hundreds), so what it + // actually catches is a typo in cols/rows. The count is long because it is + // compared before it is trusted. + const int MaxFrames = 4096; + long totalFrames = (long)cols * rows; + if (totalFrames > MaxFrames) + { + diagnostics.AddError( + "SLICE_TOO_MANY_FRAMES", + $"The grid works out to {totalFrames} frames, above the {MaxFrames}-frame limit.", + new { cols, rows, frame_width = frameW, frame_height = frameH, total_frames = totalFrames, max_frames = MaxFrames }, + new[] { "Increase frame_width/frame_height", "Slice the sheet in smaller pieces" } + ); + RestoreTextureType(importer, previousType); + return new { success = false, diagnostics = diagnostics.Build() }; + } + if (totalFrames == 0) { diagnostics.AddError( @@ -188,7 +207,7 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos string baseName = @params["base_name"]?.ToString() ?? Path.GetFileNameWithoutExtension(path); - var metas = new SpriteMetaData[totalFrames]; + var metas = new SpriteMetaData[(int)totalFrames]; for (int r = 0; r < rows; r++) { for (int c = 0; c < cols; c++) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 1573bf3e2..7c6536839 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1203,5 +1203,24 @@ public void SetupClips_NegativeStartFrame_IsRefusedRatherThanShiftedToZero() Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_RANGE")); Assert.IsNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim")); } + + [Test] + public void SliceSheet_GridFarBeyondAnyRealSheet_IsRefusedBeforeAllocating() + { + // 128x128 cut into 1px frames is 16,384 entries. It fits inside the texture, so + // every bounds check above passes; what stops it is the frame ceiling. + string path = CreateSheet("huge", 8, 8); // 128x128 + var result = Run(new JObject + { + ["action"] = "slice_sheet", + ["path"] = path, + ["cols"] = 128, + ["rows"] = 128, + }); + + Assert.IsFalse(result.Value("success")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_TOO_MANY_FRAMES")); + Assert.AreEqual(0, SpritesOf(path).Length, "nothing may be written past the limit"); + } } } From 1de11d24fee2b37be1ae2d47eae897652e1d4042 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:18:00 +0300 Subject: [PATCH 13/29] fix(sprite): categorize camelCase clip names --- .../Tools/Sprite2D/SpriteNamingDetector.cs | 14 ++++++++----- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs index 9bf6b870f..e3edb9e1f 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs @@ -34,9 +34,13 @@ internal static class SpriteNamingDetector { public static SpriteAnimEntry Detect(string clipName) { - string lower = clipName.ToLowerInvariant(); var entry = new SpriteAnimEntry { ClipName = clipName }; - Categorize(lower, entry); + // The raw name, not a lowercased one: Words splits on camelCase humps by testing + // char.IsUpper, which is never true once the string has been lowered. 'heroAttack' + // collapsed into the single word 'heroattack', matched nothing, and was filed + // Generic with no Attack trigger. Words lowercases each word it emits, so every + // comparison downstream is still case-insensitive. + Categorize(clipName, entry); entry.Loop = AutoDetectLoop(entry.Category); return entry; } @@ -54,9 +58,9 @@ public static ControllerComplexity DecideComplexity(IEnumerable // ── Private ────────────────────────────────────────────────────────── - private static void Categorize(string lower, SpriteAnimEntry entry) + private static void Categorize(string name, SpriteAnimEntry entry) { - var words = Words(lower); + var words = Words(name); if (Has(words, "idle", "stand")) { entry.Category = SpriteAnimCategory.Idle; return; } @@ -80,7 +84,7 @@ private static void Categorize(string lower, SpriteAnimEntry entry) { entry.Category = SpriteAnimCategory.Object; entry.TriggerName = Capitalize(hit); return; } entry.Category = SpriteAnimCategory.Generic; - entry.TriggerName = Capitalize(lower); + entry.TriggerName = Capitalize(name.ToLowerInvariant()); } /// diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 7c6536839..4bf88bec1 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1222,5 +1222,26 @@ public void SliceSheet_GridFarBeyondAnyRealSheet_IsRefusedBeforeAllocating() Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_TOO_MANY_FRAMES")); Assert.AreEqual(0, SpritesOf(path).Length, "nothing may be written past the limit"); } + + // ===================================================================== + // Clip-name shapes + // ===================================================================== + + [Test] + public void SetupController_CamelCaseClipName_StillGetsItsTrigger() + { + // Detect used to lowercase the name before the tokenizer saw it, and the tokenizer + // splits camelCase by testing char.IsUpper - never true on a lowered string. So + // 'heroAttack' became one word, matched no keyword, and was filed Generic. + var result = SetupController(BuildClips("camel", "idle", "heroAttack")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + var attack = controller.parameters.SingleOrDefault(p => p.name == "Attack"); + Assert.IsNotNull(attack, + "a camelCase combat clip needs the same trigger a snake_case one gets; " + + "parameters present: " + string.Join(", ", controller.parameters.Select(p => p.name))); + Assert.AreEqual(AnimatorControllerParameterType.Trigger, attack.type); + } } } From 3d8604b2d4e2d6bf19b52ea74b22d1406b01d619 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:18:00 +0300 Subject: [PATCH 14/29] style(sprite): assemble params the way every sibling tool does --- Server/src/services/tools/manage_sprite.py | 39 ++++++++++++++-------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py index bec9e01ea..cc654b3e8 100644 --- a/Server/src/services/tools/manage_sprite.py +++ b/Server/src/services/tools/manage_sprite.py @@ -138,19 +138,32 @@ async def manage_sprite( params: dict[str, Any] = {"action": action_lower} - if path is not None: params["path"] = path - if cols is not None: params["cols"] = cols - if rows is not None: params["rows"] = rows - if frame_width is not None: params["frame_width"] = frame_width - if frame_height is not None: params["frame_height"] = frame_height - if base_name is not None: params["base_name"] = base_name - if clips is not None: params["clips"] = clips - if animation_name is not None: params["animation_name"] = animation_name - if output_dir is not None: params["output_dir"] = output_dir - if controller_path is not None: params["controller_path"]= controller_path - if overwrite: params["overwrite"] = True - if add_to_scene: params["add_to_scene"] = True - if scene_target is not None: params["scene_target"] = scene_target + if path is not None: + params["path"] = path + if cols is not None: + params["cols"] = cols + if rows is not None: + params["rows"] = rows + if frame_width is not None: + params["frame_width"] = frame_width + if frame_height is not None: + params["frame_height"] = frame_height + if base_name is not None: + params["base_name"] = base_name + if clips is not None: + params["clips"] = clips + if animation_name is not None: + params["animation_name"] = animation_name + if output_dir is not None: + params["output_dir"] = output_dir + if controller_path is not None: + params["controller_path"] = controller_path + if overwrite: + params["overwrite"] = True + if add_to_scene: + params["add_to_scene"] = True + if scene_target is not None: + params["scene_target"] = scene_target result = await send_with_unity_instance( async_send_command_with_retry, From 88d7d2e0ada18b0931f20f73995eed56ad1120ab Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 20:26:05 +0300 Subject: [PATCH 15/29] fix(sprite): bound the inline image in get_info by size --- .../Tools/Sprite2D/SpriteImportSetup.cs | 26 ++++++-- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 63 +++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index b9af1df58..da38b35db 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -45,7 +45,14 @@ public static object GetInfo(JObject @params) }).ToArray(); // Base64 payload so a vision-capable caller can read the grid off the image. + // It is bounded by size rather than paged: the point of the payload is that one + // response carries one whole image a vision model can look at, and an image split + // across cursors is not an image any client can reassemble. Over the ceiling the + // payload is dropped and the reason is named - width, height and the slice list + // still answer everything the caller needs to compute a grid. + const int MaxImageBytes = 4 * 1024 * 1024; string imageBase64 = null; + string imageOmittedReason = null; try { string fullPath = Path.Combine( @@ -54,10 +61,20 @@ public static object GetInfo(JObject @params) ); if (File.Exists(fullPath)) { - byte[] bytes = File.ReadAllBytes(fullPath); - string ext = Path.GetExtension(path).ToLowerInvariant(); - string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; - imageBase64 = $"data:{mime};base64," + Convert.ToBase64String(bytes); + long size = new FileInfo(fullPath).Length; + if (size > MaxImageBytes) + { + imageOmittedReason = + $"The source file is {size} bytes, above the {MaxImageBytes}-byte inline limit. " + + "Read the file directly if the image itself is needed."; + } + else + { + byte[] bytes = File.ReadAllBytes(fullPath); + string ext = Path.GetExtension(path).ToLowerInvariant(); + string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; + imageBase64 = $"data:{mime};base64," + Convert.ToBase64String(bytes); + } } } catch { /* The base64 payload is optional; leaving it null is a valid answer. */ } @@ -74,6 +91,7 @@ public static object GetInfo(JObject @params) slice_count = existingSlices.Length, slices = existingSlices, image_base64 = imageBase64, + image_omitted_reason = imageOmittedReason, }; return result; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 4bf88bec1..63f20885d 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -65,6 +65,36 @@ private static string CreateSheet(string name, int cols, int rows) return assetPath; } + + /// + /// A square PNG of incompressible noise, used to exceed the inline-image ceiling. + /// Written through the same import path as CreateSheet. + /// + private static string CreateNoiseSheet(string name, int side) + { + var tex = new Texture2D(side, side, TextureFormat.RGBA32, false); + var pixels = new Color32[side * side]; + uint state = 0x13579BDFu; // fixed seed: the file size must not vary between runs + for (int i = 0; i < pixels.Length; i++) + { + state = state * 1664525u + 1013904223u; + pixels[i] = new Color32((byte)(state >> 24), (byte)(state >> 16), (byte)(state >> 8), 255); + } + tex.SetPixels32(pixels); + tex.Apply(); + + string assetPath = $"{TempRoot}/{name}.png"; + string sysPath = Path.Combine( + Directory.GetParent(Application.dataPath).FullName, assetPath); + File.WriteAllBytes(sysPath, tex.EncodeToPNG()); + Object.DestroyImmediate(tex); + + Assert.Greater(new FileInfo(sysPath).Length, 4 * 1024 * 1024, + "fixture: the noise sheet must exceed the inline-image ceiling"); + AssetDatabase.ImportAsset(assetPath, ImportAssetOptions.ForceSynchronousImport); + return assetPath; + } + private static JObject Run(JObject p) => ToJObject(ManageSprite.HandleCommand(p)); /// @@ -1243,5 +1273,38 @@ public void SetupController_CamelCaseClipName_StillGetsItsTrigger() "parameters present: " + string.Join(", ", controller.parameters.Select(p => p.name))); Assert.AreEqual(AnimatorControllerParameterType.Trigger, attack.type); } + + // ===================================================================== + // Inline image bound + // ===================================================================== + + [Test] + public void GetInfo_SmallSheet_CarriesTheImageInline() + { + string path = CreateSheet("inline", 4, 2); + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + + Assert.That(result.Value("image_base64"), Does.StartWith("data:image/png;base64,")); + Assert.IsNull(result.Value("image_omitted_reason")); + } + + [Test] + public void GetInfo_OversizeSheet_DropsTheImageAndSaysWhy() + { + // Two things the fixture has to get right. Noise, not a flat colour: a solid + // sheet compresses to a few kilobytes and would never reach the ceiling. And a + // power-of-two side: a Default-type import rescales anything else, so a 1200px + // sheet reads back as 1024 - measured here first - and the dimensions below + // would then be pinning the rescale rather than the file. + string path = CreateNoiseSheet("oversize", 2048); + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + + Assert.IsTrue(result.Value("success"), "the call still answers"); + Assert.IsNull(result.Value("image_base64")); + Assert.That(result.Value("image_omitted_reason"), Does.Contain("limit")); + // Everything a caller needs to work out a grid is still here. + Assert.AreEqual(2048, result.Value("width")); + Assert.AreEqual(2048, result.Value("height")); + } } } From 869873740392a233b067d1137eb0f52bfd989664 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 21:00:40 +0300 Subject: [PATCH 16/29] fix(sprite): give get_info one honest account of a missing image --- .../Tools/Sprite2D/SpriteImportSetup.cs | 52 ++++++++++++++----- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 35 +++++++++++-- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index da38b35db..ea3b2f71a 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -50,34 +50,58 @@ public static object GetInfo(JObject @params) // across cursors is not an image any client can reassemble. Over the ceiling the // payload is dropped and the reason is named - width, height and the slice list // still answer everything the caller needs to compute a grid. - const int MaxImageBytes = 4 * 1024 * 1024; + // 4 MB because that is a payload a single tool response can carry without the + // transport or the model's context becoming the limiting factor; it is a budget, + // not a measured protocol boundary, and moving it breaks the two fixture + // assertions in ManageSpriteTests on purpose. + // The ceiling is applied to the ENCODED length, not the file size. base64 emits + // 4 characters per 3 bytes, so bounding the source let a 3.67 MB sheet through as + // a 4.89 MB payload - measured, and the reason this arithmetic is written out. + const int MaxInlinePayloadBytes = 4 * 1024 * 1024; string imageBase64 = null; string imageOmittedReason = null; try { - string fullPath = Path.Combine( - Application.dataPath.Replace("/Assets", ""), - path - ); - if (File.Exists(fullPath)) + // Not Application.dataPath.Replace("/Assets", ""): Replace removes EVERY + // occurrence, so a project under a directory like /work/AssetsLab lost the + // wrong segment and the lookup silently missed a file that was really there. + string projectRoot = Directory.GetParent(Application.dataPath)?.FullName; + string fullPath = projectRoot != null ? Path.Combine(projectRoot, path) : null; + if (fullPath == null) + { + imageOmittedReason = "The project root could not be resolved from Application.dataPath."; + } + else if (!File.Exists(fullPath)) { + imageOmittedReason = $"No file on disk at '{fullPath}'."; + } + else + { + string ext = Path.GetExtension(path).ToLowerInvariant(); + string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; + string prefix = $"data:{mime};base64,"; long size = new FileInfo(fullPath).Length; - if (size > MaxImageBytes) + long encoded = 4L * ((size + 2) / 3) + prefix.Length; + if (encoded > MaxInlinePayloadBytes) { imageOmittedReason = - $"The source file is {size} bytes, above the {MaxImageBytes}-byte inline limit. " + - "Read the file directly if the image itself is needed."; + $"The {size}-byte source encodes to {encoded} base64 bytes, above the " + + $"{MaxInlinePayloadBytes}-byte inline limit. Read the file directly if the " + + "image itself is needed."; } else { - byte[] bytes = File.ReadAllBytes(fullPath); - string ext = Path.GetExtension(path).ToLowerInvariant(); - string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; - imageBase64 = $"data:{mime};base64," + Convert.ToBase64String(bytes); + imageBase64 = prefix + Convert.ToBase64String(File.ReadAllBytes(fullPath)); } } } - catch { /* The base64 payload is optional; leaving it null is a valid answer. */ } + catch (Exception ex) + { + // The payload is optional, but a swallowed failure and a deliberate omission + // are different answers and the response now has a field that can tell them + // apart. Leaving it null was the whole complaint about `catch {}`. + imageOmittedReason = $"The image could not be read: {ex.GetType().Name}: {ex.Message}"; + } var result = new { diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 63f20885d..0a52a7c6c 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -70,7 +70,7 @@ private static string CreateSheet(string name, int cols, int rows) /// A square PNG of incompressible noise, used to exceed the inline-image ceiling. /// Written through the same import path as CreateSheet. /// - private static string CreateNoiseSheet(string name, int side) + private static string CreateNoiseSheet(string name, int side, bool assertOverCeiling = true) { var tex = new Texture2D(side, side, TextureFormat.RGBA32, false); var pixels = new Color32[side * side]; @@ -89,8 +89,13 @@ private static string CreateNoiseSheet(string name, int side) File.WriteAllBytes(sysPath, tex.EncodeToPNG()); Object.DestroyImmediate(tex); - Assert.Greater(new FileInfo(sysPath).Length, 4 * 1024 * 1024, - "fixture: the noise sheet must exceed the inline-image ceiling"); + // Both callers depend on where this lands relative to the 4 MB ceiling in + // SpriteImportSetup, so the fixture asserts the side it was asked for rather + // than trusting the compressor. Changing that ceiling breaks these two lines + // loudly, which is the intent. + if (assertOverCeiling) + Assert.Greater(new FileInfo(sysPath).Length, 4 * 1024 * 1024, + "fixture: the noise sheet must exceed the inline-image ceiling"); AssetDatabase.ImportAsset(assetPath, ImportAssetOptions.ForceSynchronousImport); return assetPath; } @@ -1306,5 +1311,29 @@ public void GetInfo_OversizeSheet_DropsTheImageAndSaysWhy() Assert.AreEqual(2048, result.Value("width")); Assert.AreEqual(2048, result.Value("height")); } + [Test] + public void GetInfo_ImageJustUnderTheSourceLimit_StillDoesNotBlowThePayloadLimit() + { + // The ceiling is checked against the file on disk, but what travels in the + // response is base64 - 4 bytes out for every 3 in. A source comfortably under + // the limit therefore still produces a payload above it. + string path = CreateNoiseSheet("midsize", 1024, assertOverCeiling: false); + long sourceBytes = new FileInfo(Path.Combine( + Directory.GetParent(Application.dataPath).FullName, path)).Length; + Assert.Less(sourceBytes, 4 * 1024 * 1024, + "fixture: this sheet must pass the source-size check to test what happens after it"); + + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + + string b64 = result.Value("image_base64"); + if (b64 != null) + Assert.LessOrEqual(System.Text.Encoding.UTF8.GetByteCount(b64), 4 * 1024 * 1024, + $"inline payload is {System.Text.Encoding.UTF8.GetByteCount(b64)} bytes " + + $"from a {sourceBytes}-byte source; the bound must cover what is sent, not what was read"); + else + Assert.IsNotEmpty(result.Value("image_omitted_reason") ?? "", + "an omitted image must say why"); + } + } } From 783deceaedb7cc22168d3a68b8c14a44bff0d0ef Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 21:01:16 +0300 Subject: [PATCH 17/29] fix(sprite): split an acronym from the keyword that follows it --- .../Tools/Sprite2D/SpriteNamingDetector.cs | 5 +++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 36 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs index e3edb9e1f..3347f163a 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs @@ -103,6 +103,11 @@ private static HashSet Words(string name) char c = name[i]; bool breaks = !char.IsLetterOrDigit(c) || (i > 0 && char.IsUpper(c) && char.IsLower(name[i - 1])) + // The end of an acronym: 'heroXMLAttack' has no lower-to-upper boundary + // at the 'A', so without this the tail read as one word 'xmlattack' and + // lost the keyword its snake_case twin matches on. + || (i > 0 && i + 1 < name.Length + && char.IsUpper(c) && char.IsUpper(name[i - 1]) && char.IsLower(name[i + 1])) || (i > 0 && char.IsDigit(c) && char.IsLetter(name[i - 1])) || (i > 0 && char.IsLetter(c) && char.IsDigit(name[i - 1])); diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 0a52a7c6c..15707c35a 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1311,6 +1311,7 @@ public void GetInfo_OversizeSheet_DropsTheImageAndSaysWhy() Assert.AreEqual(2048, result.Value("width")); Assert.AreEqual(2048, result.Value("height")); } + [Test] public void GetInfo_ImageJustUnderTheSourceLimit_StillDoesNotBlowThePayloadLimit() { @@ -1335,5 +1336,40 @@ public void GetInfo_ImageJustUnderTheSourceLimit_StillDoesNotBlowThePayloadLimit "an omitted image must say why"); } + [Test] + public void SetupController_AcronymInACamelCaseClipName_StillGetsItsTrigger() + { + // 'heroAttack' splits because a capital follows a lowercase. 'heroXMLAttack' has + // no such boundary at the acronym's end, so it used to tokenize as one word + // 'xmlattack', match nothing, and lose the trigger its snake_case twin gets. + var result = SetupController(BuildClips("acronym", "idle", "heroXMLAttack")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + var attack = controller.parameters.SingleOrDefault(p => p.name == "Attack"); + Assert.IsNotNull(attack, + "an acronym must not swallow the keyword after it; parameters present: " + + string.Join(", ", controller.parameters.Select(p => p.name))); + } + + [Test] + public void SetupController_TwoOtherAcronymShapes_AlsoKeepTheirTriggers() + { + // Closing the class rather than the one spelling that was reported, with the two + // variants carrying different verdicts - which is the point of naming them. + // 'heroATTACK': a trailing all-caps keyword. Measured to hold ALREADY - the break + // comes from the lowercase 'o' before the run, so the new rule is not what saves + // it. Kept as a parity tripwire, not offered as evidence for the fix. + // 'XMLSlash': an acronym followed directly by the keyword, with no lowercase in + // between. Nothing in the original rule set sees that boundary, so this one does + // depend on the fix - reverting the rule turns this test red. + var result = SetupController(BuildClips("acroshapes", "idle", "heroATTACK", "XMLSlash")); + Assert.IsTrue(result.Value("success")); + + var controller = AssetDatabase.LoadAssetAtPath($"{TempRoot}/Hero.controller"); + var names = controller.parameters.Select(p => p.name).ToArray(); + Assert.Contains("Attack", names, "a trailing all-caps keyword still names its trigger"); + Assert.Contains("Slash", names, "an acronym running straight into the keyword must still split"); + } } } From cd416d3eb9a3d6eae048145be7bb556b9d4d5b4b Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 21:01:16 +0300 Subject: [PATCH 18/29] test(sprite): pin the importer rollback on the frame-ceiling refusal --- .../Assets/Tests/EditMode/Tools/ManageSpriteTests.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 15707c35a..93ad50a18 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1245,6 +1245,8 @@ public void SliceSheet_GridFarBeyondAnyRealSheet_IsRefusedBeforeAllocating() // 128x128 cut into 1px frames is 16,384 entries. It fits inside the texture, so // every bounds check above passes; what stops it is the frame ceiling. string path = CreateSheet("huge", 8, 8); // 128x128 + var before = ((TextureImporter)AssetImporter.GetAtPath(path)).textureType; + var result = Run(new JObject { ["action"] = "slice_sheet", @@ -1256,6 +1258,11 @@ public void SliceSheet_GridFarBeyondAnyRealSheet_IsRefusedBeforeAllocating() Assert.IsFalse(result.Value("success")); Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_TOO_MANY_FRAMES")); Assert.AreEqual(0, SpritesOf(path).Length, "nothing may be written past the limit"); + // Every refusal after the Sprite conversion owes a RestoreTextureType call, and + // that obligation is carried by whoever adds the next early return rather than by + // the code. This assertion is what makes a forgotten one fail loudly. + Assert.AreEqual(before, ((TextureImporter)AssetImporter.GetAtPath(path)).textureType, + "a refused request must not leave the texture converted behind it"); } // ===================================================================== From 79ecf1b0c2fa729a06b4700e80eca3922699b5f0 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 21:47:54 +0300 Subject: [PATCH 19/29] feat(sprite): page the slice list get_info returns slice_sheet caps what it generates at 4096 frames, but get_info reads what is already on the asset: a sheet sliced by hand in the Sprite Editor carries as many entries as someone drew, and every one of them was mapped into the response. The ceiling on the writing end never bounded the reading end. slice_count keeps reporting the total, slices carries one page, and next_cursor appears only while entries remain. The default page of 512 clears any grid a caller would slice through this tool, so an ordinary call gets one page and no cursor; page_size is bounded so it cannot be used to ask for the unbounded result again. The image stays on the first page rather than repeating with each one - paging exists to bound the response, and resending a 4 MB payload per page would multiply exactly what the page size is there to cap. image_omitted_reason says so, which is the field's purpose. Both guards refuse rather than clamp. A negative cursor especially: Skip yields the whole list for a negative count, so without the check the call would answer with every slice and report success - the same trap that let start_frame=-2 write frames 0..5. Measured: ManageSpriteTests 85/85 in EditMode, and each of the seven guards was reverted in turn to confirm it takes exactly the intended tests down with it. Python 1390 passed / 3 skipped; docs --check clean. --- .../Tools/Sprite2D/SpriteImportSetup.cs | 118 +++++++++---- Server/src/services/tools/manage_sprite.py | 18 +- Server/tests/test_manage_sprite.py | 26 +++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 162 ++++++++++++++++++ .../docs/reference/tools/animation/index.md | 2 +- .../tools/animation/manage_sprite.md | 16 +- website/docs/reference/tools/index.md | 2 +- 7 files changed, 304 insertions(+), 40 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index ea3b2f71a..7de87af57 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -35,7 +35,38 @@ public static object GetInfo(JObject @params) int w = texture != null ? texture.width : 0; int h = texture != null ? texture.height : 0; - var existingSlices = importer.spritesheet.Select(s => new + // The slice list is paged, unlike the image below. slice_sheet caps what it + // GENERATES at 4096 frames, but this reads what is already on the asset, and a + // sheet sliced by hand in the Sprite Editor carries as many entries as someone + // drew - the ceiling on the writing end never bounded the reading end. + // 512 is the default page because it clears any grid a caller would slice here + // (a 32x16 sheet fits whole), so the common case gets one page, no cursor, and + // never learns paging exists. The maximum is what stops page_size from being + // used to ask for the unbounded result again. + const int DefaultSlicePageSize = 512; + const int MaxSlicePageSize = 4096; + + // ToObject rather than the ToObject the slice_sheet parameters below + // use: an explicit JSON null reaches here as a JValue, so `?.` does not + // short-circuit and the non-nullable form would read it as 0 and refuse the + // call. Nulling a parameter out means "unset", which is the default. + int pageSize = @params["page_size"]?.ToObject() ?? DefaultSlicePageSize; + if (pageSize < 1 || pageSize > MaxSlicePageSize) + return new ErrorResponse($"'page_size' must be between 1 and {MaxSlicePageSize}; got {pageSize}."); + + int totalSlices = importer.spritesheet.Length; + int cursor = @params["cursor"]?.ToObject() ?? 0; + // Skip yields every element for a negative count rather than throwing, so a + // negative cursor would silently return page one and call it a success - the + // same trap that let start_frame=-2 write frames 0..5 in SpriteClipBuilder. + // Landing exactly on totalSlices returns an empty page rather than an error. + // next_cursor never points there, so this is for a caller walking the list by + // adding page_size itself - and it is what makes cursor 0 legal on a sheet + // with no slices at all, where 0 IS the end. + if (cursor < 0 || cursor > totalSlices) + return new ErrorResponse($"'cursor' must be between 0 and {totalSlices}; got {cursor}."); + + var existingSlices = importer.spritesheet.Skip(cursor).Take(pageSize).Select(s => new { name = s.name, x = (int)s.rect.x, @@ -44,6 +75,9 @@ public static object GetInfo(JObject @params) height = (int)s.rect.height, }).ToArray(); + int nextIndex = cursor + existingSlices.Length; + int? nextCursor = nextIndex < totalSlices ? nextIndex : (int?)null; + // Base64 payload so a vision-capable caller can read the grid off the image. // It is bounded by size rather than paged: the point of the payload is that one // response carries one whole image a vision model can look at, and an image split @@ -60,47 +94,60 @@ public static object GetInfo(JObject @params) const int MaxInlinePayloadBytes = 4 * 1024 * 1024; string imageBase64 = null; string imageOmittedReason = null; - try + if (cursor > 0) { - // Not Application.dataPath.Replace("/Assets", ""): Replace removes EVERY - // occurrence, so a project under a directory like /work/AssetsLab lost the - // wrong segment and the lookup silently missed a file that was really there. - string projectRoot = Directory.GetParent(Application.dataPath)?.FullName; - string fullPath = projectRoot != null ? Path.Combine(projectRoot, path) : null; - if (fullPath == null) - { - imageOmittedReason = "The project root could not be resolved from Application.dataPath."; - } - else if (!File.Exists(fullPath)) - { - imageOmittedReason = $"No file on disk at '{fullPath}'."; - } - else + // Only the first page carries the image. The picture does not change + // between pages, and paging exists to bound the response - sending the + // whole payload again with every page would multiply by the page count + // the very thing the page size is there to cap. + imageOmittedReason = + "The image is returned only on the first page. Request this path with " + + "cursor 0 (or omit cursor) if the image itself is needed."; + } + else + { + try { - string ext = Path.GetExtension(path).ToLowerInvariant(); - string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; - string prefix = $"data:{mime};base64,"; - long size = new FileInfo(fullPath).Length; - long encoded = 4L * ((size + 2) / 3) + prefix.Length; - if (encoded > MaxInlinePayloadBytes) + // Not Application.dataPath.Replace("/Assets", ""): Replace removes EVERY + // occurrence, so a project under a directory like /work/AssetsLab lost the + // wrong segment and the lookup silently missed a file that was really there. + string projectRoot = Directory.GetParent(Application.dataPath)?.FullName; + string fullPath = projectRoot != null ? Path.Combine(projectRoot, path) : null; + if (fullPath == null) + { + imageOmittedReason = "The project root could not be resolved from Application.dataPath."; + } + else if (!File.Exists(fullPath)) { - imageOmittedReason = - $"The {size}-byte source encodes to {encoded} base64 bytes, above the " + - $"{MaxInlinePayloadBytes}-byte inline limit. Read the file directly if the " + - "image itself is needed."; + imageOmittedReason = $"No file on disk at '{fullPath}'."; } else { - imageBase64 = prefix + Convert.ToBase64String(File.ReadAllBytes(fullPath)); + string ext = Path.GetExtension(path).ToLowerInvariant(); + string mime = (ext == ".jpg" || ext == ".jpeg") ? "image/jpeg" : "image/png"; + string prefix = $"data:{mime};base64,"; + long size = new FileInfo(fullPath).Length; + long encoded = 4L * ((size + 2) / 3) + prefix.Length; + if (encoded > MaxInlinePayloadBytes) + { + imageOmittedReason = + $"The {size}-byte source encodes to {encoded} base64 bytes, above the " + + $"{MaxInlinePayloadBytes}-byte inline limit. Read the file directly if the " + + "image itself is needed."; + } + else + { + imageBase64 = prefix + Convert.ToBase64String(File.ReadAllBytes(fullPath)); + } } } - } - catch (Exception ex) - { - // The payload is optional, but a swallowed failure and a deliberate omission - // are different answers and the response now has a field that can tell them - // apart. Leaving it null was the whole complaint about `catch {}`. - imageOmittedReason = $"The image could not be read: {ex.GetType().Name}: {ex.Message}"; + catch (Exception ex) + { + // The payload is optional, but a swallowed failure and a deliberate omission + // are different answers and the response now has a field that can tell them + // apart. Leaving it null was the whole complaint about `catch {}`. + imageOmittedReason = $"The image could not be read: {ex.GetType().Name}: {ex.Message}"; + } } var result = new @@ -112,8 +159,9 @@ public static object GetInfo(JObject @params) sprite_mode = importer.spriteImportMode.ToString(), pixels_per_unit = importer.spritePixelsPerUnit, filter_mode = importer.filterMode.ToString(), - slice_count = existingSlices.Length, + slice_count = totalSlices, slices = existingSlices, + next_cursor = nextCursor, image_base64 = imageBase64, image_omitted_reason = imageOmittedReason, }; diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py index cc654b3e8..3379716d3 100644 --- a/Server/src/services/tools/manage_sprite.py +++ b/Server/src/services/tools/manage_sprite.py @@ -26,7 +26,8 @@ group="animation", description=( "2D sprite animation tool. " - "get_info: read sprite import settings + return image for vision analysis. " + "get_info: read sprite import settings + return image for vision analysis; " + "the slice list is paged (page_size / cursor). " "slice_sheet: apply grid slicing to a sprite sheet. " "setup_clips: create AnimationClips from sliced sprites. " "setup_controller: build AnimatorController with smart complexity (1D blend tree for locomotion, " @@ -95,6 +96,17 @@ async def manage_sprite( str | None, "Existing GameObject name to attach Animator to.", ] = None, + page_size: Annotated[ + int | None, + "get_info: how many entries of the 'slices' list to return (1-4096, default 512). " + "A sheet sliced by hand can hold more slices than one response should carry.", + ] = None, + cursor: Annotated[ + int | None, + "get_info: index to start the 'slices' page at. Pass back the 'next_cursor' from " + "the previous response; absent next_cursor means the list is finished. The image " + "is returned only on the first page.", + ] = None, ) -> dict[str, Any]: """2D sprite animation tool.""" @@ -158,6 +170,10 @@ async def manage_sprite( params["output_dir"] = output_dir if controller_path is not None: params["controller_path"] = controller_path + if page_size is not None: + params["page_size"] = page_size + if cursor is not None: + params["cursor"] = cursor if overwrite: params["overwrite"] = True if add_to_scene: diff --git a/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py index 22a778e9d..d1489f72b 100644 --- a/Server/tests/test_manage_sprite.py +++ b/Server/tests/test_manage_sprite.py @@ -129,3 +129,29 @@ def test_only_supplied_parameters_are_forwarded(self): params = sent.await_args.args[3] assert params == {"action": "slice_sheet", "path": "Assets/hero.png", "cols": 4} + + def test_paging_arguments_reach_unity_only_when_asked_for(self): + """page_size and cursor are get_info's, and absent means "use the default". + + Forwarding cursor=0 unasked would be harmless, but forwarding page_size=0 + would not: the C# side refuses anything below 1, so a null that turned into + a zero on the wire would break every plain get_info call. + """ + from services.tools.manage_sprite import manage_sprite + + with patch("services.tools.manage_sprite.get_unity_instance_from_context", + new=AsyncMock(return_value=None)), \ + patch("services.tools.manage_sprite.send_with_unity_instance", + new=AsyncMock(return_value={"success": True})) as sent: + asyncio.run(manage_sprite(self._ctx(), action="get_info", + path="Assets/atlas.png")) + plain = sent.await_args.args[3] + + asyncio.run(manage_sprite(self._ctx(), action="get_info", + path="Assets/atlas.png", + page_size=100, cursor=200)) + paged = sent.await_args.args[3] + + assert plain == {"action": "get_info", "path": "Assets/atlas.png"} + assert paged["page_size"] == 100 + assert paged["cursor"] == 200 diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 93ad50a18..efcbca296 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using System.IO; using System.Linq; using Newtonsoft.Json.Linq; @@ -214,6 +215,167 @@ public void GetInfo_AfterSlicing_ReportsEverySlice() Assert.AreEqual(8, result.Value("slice_count")); } + [Test] + public void GetInfo_ModestSheet_ComesBackInOnePageWithNoCursor() + { + string path = CreateSheet("onepage", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + + // The point of the default page size is that a sheet anyone would slice through + // this tool never meets paging at all. If this turns red the default shrank. + Assert.AreEqual(8, ((JArray)result["slices"]).Count); + Assert.IsNull(result["next_cursor"].Value(), + "a finished list has no next cursor"); + } + + [Test] + public void GetInfo_MoreSlicesThanThePage_ReturnsOnePageAndPointsAtTheRest() + { + string path = CreateSheet("paged", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["page_size"] = 3, + }); + + Assert.AreEqual(3, ((JArray)result["slices"]).Count, "the page is bounded"); + Assert.AreEqual(8, result.Value("slice_count"), + "slice_count stays the total, not the size of the page"); + Assert.AreEqual(3, result["next_cursor"].Value()); + } + + [Test] + public void GetInfo_WalkingTheCursor_VisitsEverySliceOnceAndThenStops() + { + string path = CreateSheet("walk", 4, 2); + Slice(path, 4, 2); + + var seen = new List(); + int? cursor = 0; + // Bounded so a cursor that never advances fails as a wrong count rather than + // hanging the whole EditMode run. + for (int page = 0; page < 10 && cursor != null; page++) + { + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["page_size"] = 3, + ["cursor"] = cursor.Value, + }); + seen.AddRange(((JArray)result["slices"]).Select(t => t.Value("name"))); + cursor = result["next_cursor"].Value(); + } + + Assert.IsNull(cursor, "the walk has to terminate on its own"); + Assert.AreEqual(8, seen.Count, "no slice returned twice and none skipped"); + CollectionAssert.AllItemsAreUnique(seen); + } + + [Test] + public void GetInfo_CursorAtTheEnd_ReturnsAnEmptyPageRatherThanAnError() + { + string path = CreateSheet("tail", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["cursor"] = 8, + }); + + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(0, ((JArray)result["slices"]).Count); + } + + [Test] + public void GetInfo_NegativeCursor_IsRefusedRatherThanReadAsPageOne() + { + string path = CreateSheet("negcursor", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["cursor"] = -3, + }); + + // Skip(-3) yields the whole list, so without the guard this call answers with + // every slice and reports success - the failure mode is a right-looking answer, + // which is why the assertion is on the refusal and not on the count. + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("cursor")); + } + + [Test] + public void GetInfo_CursorPastTheEnd_IsRefused() + { + string path = CreateSheet("farcursor", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["cursor"] = 9, + }); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("cursor")); + } + + [TestCase(0)] + [TestCase(-1)] + [TestCase(4097)] + public void GetInfo_PageSizeOutsideItsRange_IsRefused(int pageSize) + { + string path = CreateSheet($"pagesize{pageSize}", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["page_size"] = pageSize, + }); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("page_size")); + } + + [Test] + public void GetInfo_PagesAfterTheFirst_DropTheImageAndSayWhy() + { + string path = CreateSheet("imageonce", 4, 2); + Slice(path, 4, 2); + + var first = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["page_size"] = 3, + }); + var second = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["page_size"] = 3, + ["cursor"] = 3, + }); + + Assert.IsNotNull(first.Value("image_base64"), + "fixture: the first page is supposed to carry the image"); + Assert.IsNull(second.Value("image_base64")); + Assert.That(second.Value("image_omitted_reason"), Does.Contain("first page")); + } + // ===================================================================== // slice_sheet // ===================================================================== diff --git a/website/docs/reference/tools/animation/index.md b/website/docs/reference/tools/animation/index.md index 50835b746..79bdbc270 100644 --- a/website/docs/reference/tools/animation/index.md +++ b/website/docs/reference/tools/animation/index.md @@ -9,4 +9,4 @@ description: "MCP for Unity tools in the animation group." Animator control & AnimationClip creation - **[`manage_animation`](./manage_animation.md)** — Manage Unity animation: Animator control and AnimationClip creation. -- **[`manage_sprite`](./manage_sprite.md)** — 2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build Animat… +- **[`manage_sprite`](./manage_sprite.md)** — 2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis; the slice list is paged (page_size / cursor). slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from… diff --git a/website/docs/reference/tools/animation/manage_sprite.md b/website/docs/reference/tools/animation/manage_sprite.md index 89fec3680..ecdf75cd2 100644 --- a/website/docs/reference/tools/animation/manage_sprite.md +++ b/website/docs/reference/tools/animation/manage_sprite.md @@ -1,7 +1,7 @@ --- title: manage_sprite sidebar_label: manage_sprite -description: "2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build Animat…" +description: "2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis; the slice list is paged (page_size / cursor). slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from…" --- # `manage_sprite` @@ -12,7 +12,7 @@ description: "2D sprite animation tool. get_info: read sprite import settings + ## Description -2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build AnimatorController with smart complexity (1D blend tree for locomotion, trigger states for combat, simple state for single animations). full_setup: one command — slice → clips → controller. +2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis; the slice list is paged (page_size / cursor). slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build AnimatorController with smart complexity (1D blend tree for locomotion, trigger states for combat, simple state for single animations). full_setup: one command — slice → clips → controller. ## Parameters @@ -32,6 +32,8 @@ description: "2D sprite animation tool. get_info: read sprite import settings + | `overwrite` | `bool` | — | Replace an existing .anim or .controller at the target path. Off by default: without it an existing asset is kept and reported back, not silently replaced. | | `add_to_scene` | `bool` | — | Attach Animator + controller to a scene GameObject. | | `scene_target` | `str \| None` | — | Existing GameObject name to attach Animator to. | +| `page_size` | `int \| None` | — | get_info: how many entries of the 'slices' list to return (1-4096, default 512). A sheet sliced by hand can hold more slices than one response should carry. | +| `cursor` | `int \| None` | — | get_info: index to start the 'slices' page at. Pass back the 'next_cursor' from the previous response; absent next_cursor means the list is finished. The image is returned only on the first page. | ## Returns @@ -50,6 +52,16 @@ frames before committing to a grid. { "action": "get_info", "path": "Assets/Sprites/hero_walk.png" } ``` +The `slices` list is paged. A grid sliced through this tool comes back whole, but a sheet +sliced by hand in the Sprite Editor can hold more entries than one response should carry, +so `slice_count` reports the total and `next_cursor` appears only while entries remain. +Walk it by passing the previous `next_cursor` back; the image comes with the first page +only, since it is the same picture on every one. + +```json +{ "action": "get_info", "path": "Assets/Sprites/atlas.png", "cursor": 512 } +``` + ### One command from sheet to controller ```json diff --git a/website/docs/reference/tools/index.md b/website/docs/reference/tools/index.md index f5a8d39c1..9f37f38b2 100644 --- a/website/docs/reference/tools/index.md +++ b/website/docs/reference/tools/index.md @@ -15,7 +15,7 @@ Every tool MCP for Unity exposes, generated directly from the Python `@mcp_for_u ## `animation`   (2 tools) Animator control & AnimationClip creation - **[`manage_animation`](./animation/manage_animation.md)** — Manage Unity animation: Animator control and AnimationClip creation. -- **[`manage_sprite`](./animation/manage_sprite.md)** — 2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis. slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from sliced sprites. setup_controller: build Animat… +- **[`manage_sprite`](./animation/manage_sprite.md)** — 2D sprite animation tool. get_info: read sprite import settings + return image for vision analysis; the slice list is paged (page_size / cursor). slice_sheet: apply grid slicing to a sprite sheet. setup_clips: create AnimationClips from… ## `asset_gen`   (5 tools) AI asset generation – 3D model gen/import, 2D image gen & audio gen (bring-your-own-key) From fa1ea62a0378fef0ef24f2387d6747b44a1441e5 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 22:16:47 +0300 Subject: [PATCH 20/29] fix(sprite): refuse pagination values an int cannot hold ToObject both throws and rounds, and get_info was relying on it for page_size and cursor. Measured on 2026-08-21 by sending the values through ManageSprite.HandleCommand: page_size: 2147483648 -> OverflowException, uncaught cursor: 2147483648 -> OverflowException, uncaught page_size: 2.7 -> no error, silently rounded to 3, three slices returned Nothing between GetInfo and the bridge catches the overflow, so a value one past int.MaxValue failed the tool at the transport rather than answering with a named refusal. The rounding is the worse of the two: the caller asked for something this tool cannot do and got a success it has no way to question. TryReadWholeNumber reads the token instead of converting it - absent or JSON null means unset, a non-Integer token is refused by name, and the long read is guarded because an integer too large for long parses as a BigInteger that is still typed Integer. Both new guards were confirmed to discriminate: reverting the type check takes down only the fractional test, and reverting either parameter to ToObject takes down only that parameter's overflow test. --- .../Tools/Sprite2D/SpriteImportSetup.cs | 59 +++++++++++++++++-- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 40 +++++++++++++ 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 7de87af57..00fd67c57 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -46,16 +46,14 @@ public static object GetInfo(JObject @params) const int DefaultSlicePageSize = 512; const int MaxSlicePageSize = 4096; - // ToObject rather than the ToObject the slice_sheet parameters below - // use: an explicit JSON null reaches here as a JValue, so `?.` does not - // short-circuit and the non-nullable form would read it as 0 and refuse the - // call. Nulling a parameter out means "unset", which is the default. - int pageSize = @params["page_size"]?.ToObject() ?? DefaultSlicePageSize; + if (!TryReadWholeNumber(@params, "page_size", DefaultSlicePageSize, out int pageSize, out string paramError)) + return new ErrorResponse(paramError); if (pageSize < 1 || pageSize > MaxSlicePageSize) return new ErrorResponse($"'page_size' must be between 1 and {MaxSlicePageSize}; got {pageSize}."); int totalSlices = importer.spritesheet.Length; - int cursor = @params["cursor"]?.ToObject() ?? 0; + if (!TryReadWholeNumber(@params, "cursor", 0, out int cursor, out paramError)) + return new ErrorResponse(paramError); // Skip yields every element for a negative count rather than throwing, so a // negative cursor would silently return page one and call it a success - the // same trap that let start_frame=-2 write frames 0..5 in SpriteClipBuilder. @@ -169,6 +167,55 @@ public static object GetInfo(JObject @params) return result; } + /// + /// Reads an optional whole-number parameter without throwing and without rounding. + /// ToObject<int> does both, measured 2026-08-21: `page_size: 2147483648` raised + /// an OverflowException that nothing in ManageSprite catches, so it left the tool as + /// a transport failure rather than a named refusal; and `page_size: 2.7` was silently + /// rounded to 3 and answered with three slices, which is worse - the caller asked for + /// something this tool cannot do and got a success. + /// + private static bool TryReadWholeNumber(JObject @params, string key, int fallback, + out int value, out string error) + { + value = fallback; + error = null; + + JToken token = @params[key]; + // An explicit JSON null arrives as a JValue, not a C# null, so it has to be + // named here: it means "unset", which is the default. + if (token == null || token.Type == JTokenType.Null) + return true; + + if (token.Type != JTokenType.Integer) + { + error = $"'{key}' must be a whole number; got {token.Type.ToString().ToLowerInvariant()}."; + return false; + } + + long raw; + try + { + raw = token.Value(); + } + catch (Exception) + { + // An integer too large for long parses as a BigInteger, still typed Integer, + // and the read throws. That is out of range by definition. + error = $"'{key}' is out of range for a 32-bit integer."; + return false; + } + + if (raw < int.MinValue || raw > int.MaxValue) + { + error = $"'{key}' must be between {int.MinValue} and {int.MaxValue}; got {raw}."; + return false; + } + + value = (int)raw; + return true; + } + /// Undoes the conversion above when the request is refused after it. private static void RestoreTextureType(TextureImporter importer, TextureImporterType previous) { diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index efcbca296..757cdce7f 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -331,6 +331,46 @@ public void GetInfo_CursorPastTheEnd_IsRefused() Assert.That(ErrorText(result), Does.Contain("cursor")); } + [TestCase("page_size")] + [TestCase("cursor")] + public void GetInfo_PagingValueTooLargeForAnInt_IsRefusedNotThrown(string key) + { + string path = CreateSheet($"overflow{key}", 4, 2); + Slice(path, 4, 2); + + var request = new JObject { ["action"] = "get_info", ["path"] = path }; + request[key] = 2147483648L; + + // Measured before the guard: ToObject raised OverflowException here, and + // nothing between this and the bridge catches it - the tool failed at the + // transport instead of answering. Run() would propagate it, so reaching the + // assertions at all is half of what this test checks. + var result = Run(request); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain(key)); + } + + [Test] + public void GetInfo_FractionalPageSize_IsRefusedRatherThanRounded() + { + string path = CreateSheet("fractional", 4, 2); + Slice(path, 4, 2); + + var result = Run(new JObject + { + ["action"] = "get_info", + ["path"] = path, + ["page_size"] = 2.7, + }); + + // Measured before the guard: this returned three slices and reported success. + // Answering a request the tool cannot honour is worse than refusing it, because + // the caller has no way to notice. + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("page_size")); + } + [TestCase(0)] [TestCase(-1)] [TestCase(4097)] From e1907dd3858a844dd066b8fb57545566356b23c8 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 22:16:59 +0300 Subject: [PATCH 21/29] fix(sprite): keep absolute paths out of the get_info response Two fields could carry the machine's directory layout across the bridge: the missing-file reason interpolated the absolute path, and the read-failure reason carried ex.Message, which routinely contains the path that threw. Neither told the caller anything it could act on. It asked with the asset path and already has it; what it needs to know is that the image was dropped and roughly why. The absolute path and the full exception are what a human debugging this needs, so they go to the Editor log instead. Worth noting the response keeps the exception TYPE. A swallowed failure and a deliberate omission are still different answers, which was the point of adding image_omitted_reason in the first place. Unlike ManageAsset, whose error text carries a fullPath that is really the sanitised Assets/ path, this one was genuinely absolute - checked before assuming the house pattern covered it. Pinned by a test that deletes the file without refreshing the AssetDatabase, so the importer still resolves and the File.Exists branch is the one that answers; restoring the old interpolation turns exactly that test red. --- .../Tools/Sprite2D/SpriteImportSetup.cs | 15 +++++++++++-- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 21 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 00fd67c57..807039e65 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -117,7 +117,13 @@ public static object GetInfo(JObject @params) } else if (!File.Exists(fullPath)) { - imageOmittedReason = $"No file on disk at '{fullPath}'."; + // The asset path, not fullPath: the response crosses the bridge to + // the caller, and the absolute form discloses the machine's directory + // layout while telling the caller nothing it can act on - it already + // knows the asset path, it asked with it. The absolute path is what a + // human debugging this needs, so it goes to the Editor log instead. + McpLog.Warn($"[Sprite2D] get_info found no file on disk at '{fullPath}'."); + imageOmittedReason = $"No file on disk for '{path}'."; } else { @@ -144,7 +150,12 @@ public static object GetInfo(JObject @params) // The payload is optional, but a swallowed failure and a deliberate omission // are different answers and the response now has a field that can tell them // apart. Leaving it null was the whole complaint about `catch {}`. - imageOmittedReason = $"The image could not be read: {ex.GetType().Name}: {ex.Message}"; + // The exception TYPE crosses the bridge, the message does not: the type + // says which kind of failure this was, while the message routinely + // carries the absolute path that threw. + McpLog.Warn($"[Sprite2D] get_info could not read '{path}': {ex}"); + imageOmittedReason = + $"The image could not be read ({ex.GetType().Name}); the Unity console has the detail."; } } diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 757cdce7f..8d25b0474 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -331,6 +331,27 @@ public void GetInfo_CursorPastTheEnd_IsRefused() Assert.That(ErrorText(result), Does.Contain("cursor")); } + [Test] + public void GetInfo_MissingFileOnDisk_DoesNotPutTheAbsolutePathInTheResponse() + { + string path = CreateSheet("nofile", 4, 2); + // Deleted WITHOUT AssetDatabase.Refresh, so the importer still resolves and the + // File.Exists branch is the one that answers. This is the only branch that used + // to interpolate the absolute path into a field the caller receives. + string full = Path.Combine( + Directory.GetParent(Application.dataPath).FullName, path); + File.Delete(full); + + var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); + string reason = result.Value("image_omitted_reason"); + + Assert.IsNotNull(reason, "fixture: the image was supposed to be dropped here"); + Assert.That(reason, Does.Not.Contain(Application.dataPath), + "the response must not disclose where the project lives on disk"); + Assert.That(reason, Does.Contain(path), + "it still has to say which asset it could not read"); + } + [TestCase("page_size")] [TestCase("cursor")] public void GetInfo_PagingValueTooLargeForAnInt_IsRefusedNotThrown(string key) From 73604d67b75f95e5df6b4b1e212d0720bae72709 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 22:17:10 +0300 Subject: [PATCH 22/29] test(sprite): read next_cursor by value so an omitted field still terminates The paging tests indexed the property and then read it, which only works while the serialiser emits nulls. It does today - that is why they passed - but an omitted next_cursor indexes to a C# null and the tests would throw instead of asserting, and the Python side already treats an absent next_cursor as completion. This is the rule the ErrorText helper a few lines up was written to enforce: assert the behaviour, not the response shape. The tests broke it. --- .../Assets/Tests/EditMode/Tools/ManageSpriteTests.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index 8d25b0474..dadf5ae62 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -226,7 +226,11 @@ public void GetInfo_ModestSheet_ComesBackInOnePageWithNoCursor() // The point of the default page size is that a sheet anyone would slice through // this tool never meets paging at all. If this turns red the default shrank. Assert.AreEqual(8, ((JArray)result["slices"]).Count); - Assert.IsNull(result["next_cursor"].Value(), + // Value("next_cursor") rather than indexing then reading: an omitted + // property indexes to a C# null and would throw here instead of asserting, + // and the Python side treats an absent next_cursor as completion. Same rule + // as ErrorText above - do not pin the response shape. + Assert.IsNull(result.Value("next_cursor"), "a finished list has no next cursor"); } @@ -269,7 +273,7 @@ public void GetInfo_WalkingTheCursor_VisitsEverySliceOnceAndThenStops() ["cursor"] = cursor.Value, }); seen.AddRange(((JArray)result["slices"]).Select(t => t.Value("name"))); - cursor = result["next_cursor"].Value(); + cursor = result.Value("next_cursor"); } Assert.IsNull(cursor, "the walk has to terminate on its own"); From 69ffd5af071b290e969644e67bd8844fe8373458 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 22:17:10 +0300 Subject: [PATCH 23/29] docs(sprite): a sheet sliced by this tool can exceed one page too The example said a grid sliced through slice_sheet "comes back whole". It does not: slice_sheet allows up to 4096 frames and get_info pages at 512, so a thousand-frame sheet needs next_cursor like any other. The sentence drew the line between hand-sliced and tool-sliced sheets, and the line is not there. --- website/docs/reference/tools/animation/manage_sprite.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/website/docs/reference/tools/animation/manage_sprite.md b/website/docs/reference/tools/animation/manage_sprite.md index ecdf75cd2..bcef2e1c2 100644 --- a/website/docs/reference/tools/animation/manage_sprite.md +++ b/website/docs/reference/tools/animation/manage_sprite.md @@ -52,9 +52,10 @@ frames before committing to a grid. { "action": "get_info", "path": "Assets/Sprites/hero_walk.png" } ``` -The `slices` list is paged. A grid sliced through this tool comes back whole, but a sheet -sliced by hand in the Sprite Editor can hold more entries than one response should carry, -so `slice_count` reports the total and `next_cursor` appears only while entries remain. +The `slices` list is paged. A sheet can hold more entries than one response should carry — +`slice_sheet` alone allows up to 4096 — so `slice_count` reports the total and +`next_cursor` appears only while entries remain. Follow it whenever it is present rather +than assuming a sheet arrives whole. Walk it by passing the previous `next_cursor` back; the image comes with the first page only, since it is the same picture on every one. From 028692d1c0fe49b9fd4b8910c35ddc770ccde503 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 23:00:19 +0300 Subject: [PATCH 24/29] fix(sprite): close the numeric-coercion class, not just the paged path An audit found the class still open through every parameter except the two CodeRabbit had pointed at. Measured 2026-08-21 by sending values through the live tool: cols / rows / frame_width / frame_height = 2147483648 -> uncaught OverflowException start_frame = 2147483648 -> uncaught OverflowException start_frame = 2.7 -> rounded to 3, clip written, success fps = NaN -> clip written with NaN keyframe times loop = "maybe" -> uncaught FormatException loop = 2 -> accepted silently Nothing between ManageSprite.HandleCommand and the bridge catches, so each overflow left the tool as a transport failure rather than a named refusal. The silent ones are worse: the caller asked for something the tool cannot do and got an asset back. The reader moved out of SpriteImportSetup into SpriteParams, because keeping it private to one file is exactly how the first version closed one path and left the class open. It reads three kinds of value - whole number, finite float, boolean - and every call site in the tool now goes through it. Two variants were named and checked rather than assumed. `fps` needed the finite check because NaN satisfies neither `> 0` nor `<= 0`, so the existing rate guard let it through. The top-level flags did NOT need one: `overwrite` and `add_to_scene` are typed `bool` at the Python surface, so FastMCP refuses a non-boolean before C# sees it. `loop` is the exception because it hides inside the untyped `clips` array, where nothing above C# looks at it. `end_frame` past the last sprite is refused too. Skip/Take clamps silently, so a hundred-frame request against an eight-frame sheet produced an eight-frame clip and reported success. The pre-existing range test moves from CLIP_EMPTY to CLIP_BAD_RANGE for that reason: same behaviour, a diagnostic that names the wrong input instead of the empty result. Measured: ManageSpriteTests 100/100. Reverting the int-range check turns exactly the six overflow tests red; reverting the boolean type check turns exactly the loop test red. --- .../Tools/Sprite2D/SpriteClipBuilder.cs | 39 ++++- .../Tools/Sprite2D/SpriteImportSetup.cs | 85 +++------- .../Editor/Tools/Sprite2D/SpriteParams.cs | 157 ++++++++++++++++++ .../Tools/Sprite2D/SpriteParams.cs.meta | 2 + .../Tests/EditMode/Tools/ManageSpriteTests.cs | 130 ++++++++++++++- 5 files changed, 348 insertions(+), 65 deletions(-) create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs create mode 100644 MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs.meta diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs index 01fda7e21..1d48d13c3 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -78,8 +78,28 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos continue; } - int startFrame = clipDef["start_frame"]?.ToObject() ?? 0; - int endFrame = clipDef["end_frame"]?.ToObject() ?? allSprites.Length - 1; + // Through SpriteParams, not ToObject: measured 2026-08-21, start_frame at + // 2147483648 raised an OverflowException that left the tool entirely, and + // start_frame 2.7 was silently rounded to 3 and written into a clip. The + // range check below only ever saw values that survived the conversion. + // Sequential, not chained with ||: a short-circuited call leaves its out + // parameter unassigned and the second value is used below. + int endFrame = allSprites.Length - 1; + bool rangeOk = SpriteParams.TryReadWholeNumber(clipDef, "start_frame", 0, out int startFrame, out string frameError); + if (rangeOk) rangeOk = SpriteParams.TryReadWholeNumber(clipDef, "end_frame", allSprites.Length - 1, out endFrame, out frameError); + if (!rangeOk) + { + diagnostics.AddWarning("CLIP_BAD_RANGE", $"Clip '{clipName}': {frameError} - skipped.", null, new[] { "start_frame and end_frame must be whole numbers within a sprite index." }); + continue; + } + if (endFrame > allSprites.Length - 1) + { + // Skip/Take clamps silently, so an end_frame past the last sprite produced + // a shorter clip and reported success - the caller asked for frames that + // do not exist and had no way to notice they were missing. + diagnostics.AddWarning("CLIP_BAD_RANGE", $"Clip '{clipName}': end_frame {endFrame} is past the last sprite index {allSprites.Length - 1} - skipped.", null, new[] { $"This sheet has {allSprites.Length} sprites, so end_frame must be at most {allSprites.Length - 1}." }); + continue; + } if (startFrame < 0 || endFrame < startFrame) { // Enumerable.Skip yields everything for a negative count, so start_frame=-2 @@ -89,7 +109,14 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos diagnostics.AddWarning("CLIP_BAD_RANGE", $"Clip '{clipName}': frame range [{startFrame},{endFrame}] is invalid - skipped.", null, new[] { "start_frame must be 0 or more, and end_frame must not be below start_frame." }); continue; } - float fps = clipDef["fps"]?.ToObject() ?? 12f; + // NaN passes every comparison, so `fps <= 0f` was false for it and a clip + // was written whose keyframe times were all NaN - measured, and reported as + // a success. TryReadFiniteFloat refuses NaN and both infinities by name. + if (!SpriteParams.TryReadFiniteFloat(clipDef, "fps", 12f, out float fps, out string fpsError)) + { + diagnostics.AddWarning("CLIP_BAD_FPS", $"Clip '{clipName}': {fpsError} - skipped.", null, new[] { "Leave fps out to use the default of 12." }); + continue; + } if (fps <= 0f) { // Keyframe times are i / fps, so a non-positive rate writes a clip whose @@ -99,7 +126,11 @@ public static object SetupClips(JObject @params, SpriteDiagnosticBuilder diagnos } var entry = SpriteNamingDetector.Detect(clipName); - bool loop = clipDef["loop"]?.ToObject() ?? entry.Loop; + if (!SpriteParams.TryReadBool(clipDef, "loop", entry.Loop, out bool loop, out string loopError)) + { + diagnostics.AddWarning("CLIP_BAD_LOOP", $"Clip '{clipName}': {loopError} - skipped.", null, new[] { "Leave loop out to let the clip name decide." }); + continue; + } var frameSprites = allSprites.Skip(startFrame).Take(endFrame - startFrame + 1).ToArray(); if (frameSprites.Length == 0) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 807039e65..9f7ad21bd 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -39,20 +39,24 @@ public static object GetInfo(JObject @params) // GENERATES at 4096 frames, but this reads what is already on the asset, and a // sheet sliced by hand in the Sprite Editor carries as many entries as someone // drew - the ceiling on the writing end never bounded the reading end. - // 512 is the default page because it clears any grid a caller would slice here - // (a 32x16 sheet fits whole), so the common case gets one page, no cursor, and - // never learns paging exists. The maximum is what stops page_size from being - // used to ask for the unbounded result again. + // 512 is the default page because it clears the grids callers actually slice - + // a 32x16 sheet fits whole - not because it clears every grid this tool can + // produce: slice_sheet allows up to MaxFrames, so a sheet between 513 and 4096 + // frames pages like any other and next_cursor is not optional for it. The + // maximum stops page_size being used to ask for the unbounded result again. + // Changing either number means changing the page_size description in + // Server/src/services/tools/manage_sprite.py, which is the copy the generated + // reference publishes. These two are the enforcement; that one is the promise. const int DefaultSlicePageSize = 512; const int MaxSlicePageSize = 4096; - if (!TryReadWholeNumber(@params, "page_size", DefaultSlicePageSize, out int pageSize, out string paramError)) + if (!SpriteParams.TryReadWholeNumber(@params, "page_size", DefaultSlicePageSize, out int pageSize, out string paramError)) return new ErrorResponse(paramError); if (pageSize < 1 || pageSize > MaxSlicePageSize) return new ErrorResponse($"'page_size' must be between 1 and {MaxSlicePageSize}; got {pageSize}."); int totalSlices = importer.spritesheet.Length; - if (!TryReadWholeNumber(@params, "cursor", 0, out int cursor, out paramError)) + if (!SpriteParams.TryReadWholeNumber(@params, "cursor", 0, out int cursor, out paramError)) return new ErrorResponse(paramError); // Skip yields every element for a negative count rather than throwing, so a // negative cursor would silently return page one and call it a success - the @@ -178,55 +182,6 @@ public static object GetInfo(JObject @params) return result; } - /// - /// Reads an optional whole-number parameter without throwing and without rounding. - /// ToObject<int> does both, measured 2026-08-21: `page_size: 2147483648` raised - /// an OverflowException that nothing in ManageSprite catches, so it left the tool as - /// a transport failure rather than a named refusal; and `page_size: 2.7` was silently - /// rounded to 3 and answered with three slices, which is worse - the caller asked for - /// something this tool cannot do and got a success. - /// - private static bool TryReadWholeNumber(JObject @params, string key, int fallback, - out int value, out string error) - { - value = fallback; - error = null; - - JToken token = @params[key]; - // An explicit JSON null arrives as a JValue, not a C# null, so it has to be - // named here: it means "unset", which is the default. - if (token == null || token.Type == JTokenType.Null) - return true; - - if (token.Type != JTokenType.Integer) - { - error = $"'{key}' must be a whole number; got {token.Type.ToString().ToLowerInvariant()}."; - return false; - } - - long raw; - try - { - raw = token.Value(); - } - catch (Exception) - { - // An integer too large for long parses as a BigInteger, still typed Integer, - // and the read throws. That is out of range by definition. - error = $"'{key}' is out of range for a 32-bit integer."; - return false; - } - - if (raw < int.MinValue || raw > int.MaxValue) - { - error = $"'{key}' must be between {int.MinValue} and {int.MaxValue}; got {raw}."; - return false; - } - - value = (int)raw; - return true; - } - /// Undoes the conversion above when the request is refused after it. private static void RestoreTextureType(TextureImporter importer, TextureImporterType previous) { @@ -253,10 +208,22 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos // These arguments need no texture, so they are checked before the conversion below: // a refused request used to return an error with the texture already turned into a Sprite. - int cols = @params["cols"]?.ToObject() ?? 0; - int rows = @params["rows"]?.ToObject() ?? 1; - int frameW = @params["frame_width"]?.ToObject() ?? 0; - int frameH = @params["frame_height"]?.ToObject() ?? 0; + // Through SpriteParams, not ToObject: measured 2026-08-21, each of these four + // raised an uncaught OverflowException at 2147483648 and each silently rounded + // a fractional value. The same class was closed for page_size first and left + // open here, which is why the reader is now shared rather than local. + // Sequential rather than chained with ||: a short-circuited call leaves its out + // parameter unassigned, so the chain would not compile once the values are used. + int rows = 1, frameW = 0, frameH = 0; + bool gridOk = SpriteParams.TryReadWholeNumber(@params, "cols", 0, out int cols, out string gridError); + if (gridOk) gridOk = SpriteParams.TryReadWholeNumber(@params, "rows", 1, out rows, out gridError); + if (gridOk) gridOk = SpriteParams.TryReadWholeNumber(@params, "frame_width", 0, out frameW, out gridError); + if (gridOk) gridOk = SpriteParams.TryReadWholeNumber(@params, "frame_height", 0, out frameH, out gridError); + if (!gridOk) + { + diagnostics.AddError("SLICE_BAD_PARAM", gridError, null, new string[0]); + return new { success = false, message = gridError, diagnostics = diagnostics.Build() }; + } if (cols <= 0 && frameW <= 0) return new ErrorResponse("Either 'cols' or 'frame_width' is required."); diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs new file mode 100644 index 000000000..6d5850966 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs @@ -0,0 +1,157 @@ +using System; +using Newtonsoft.Json.Linq; + +namespace MCPForUnity.Editor.Tools.Sprite2D +{ + /// + /// Reads the sprite tool's numeric parameters off the request without throwing and + /// without rounding. + /// + /// It exists because ToObject<T> does both, and the difference was measured on + /// 2026-08-21 by sending values through the live tool: + /// + /// cols / rows / frame_width / frame_height = 2147483648 -> OverflowException + /// start_frame = 2147483648 -> OverflowException + /// start_frame = 2.7 -> silently became 3 + /// fps = NaN -> clip written with NaN times + /// + /// Nothing between ManageSprite.HandleCommand and the bridge catches, so each of those + /// overflows left the tool as a transport failure rather than a named refusal. The + /// rounding is the worse half: the caller asked for something the tool cannot do and + /// got a success it has no way to question. + /// + /// This is one file rather than a private helper because the same parameters are read + /// in three places - the grid in SpriteImportSetup, the frame range and rate in + /// SpriteClipBuilder - and a guard that lives in one of them is how the first version + /// of this fix closed one path and left the class open. + /// + internal static class SpriteParams + { + /// + /// Reads an optional whole number. Returns false with a caller-facing reason when + /// the value is present but is not a whole number an int can hold. + /// + internal static bool TryReadWholeNumber(JObject @params, string key, int fallback, + out int value, out string error) + { + value = fallback; + error = null; + + JToken token = @params?[key]; + // An explicit JSON null arrives as a JValue, not a C# null, so it has to be + // named here: it means "unset", which is the default. + if (token == null || token.Type == JTokenType.Null) + return true; + + if (token.Type != JTokenType.Integer) + { + error = $"'{key}' must be a whole number; got {token.Type.ToString().ToLowerInvariant()}."; + return false; + } + + long raw; + try + { + raw = token.Value(); + } + catch (Exception) + { + // An integer too large for long parses as a BigInteger, still typed Integer, + // and the read throws. That is out of range by definition. + error = $"'{key}' must fit in a 32-bit integer."; + return false; + } + + if (raw < int.MinValue || raw > int.MaxValue) + { + // "must fit in a 32-bit integer", not "must be between X and Y": the range + // guards elsewhere phrase themselves the same way, so a test asserting the + // generic wording passes when this conversion wraps and a LATER guard does + // the refusing. Measured on the page_size and cols tests, which both did. + error = $"'{key}' must fit in a 32-bit integer; got {raw}."; + return false; + } + + value = (int)raw; + return true; + } + + /// + /// Reads an optional flag. Measured 2026-08-21: `loop: "maybe"` raised an uncaught + /// FormatException and `loop: 2` was accepted silently, because ToObject<bool?> + /// converts rather than validates. The top-level flags do not need this - the Python + /// surface types `overwrite` and `add_to_scene` as bool, so FastMCP refuses a + /// non-boolean before it reaches here - but `loop` hides inside the untyped `clips` + /// array, where nothing above C# looks at it. + /// + internal static bool TryReadBool(JObject @params, string key, bool fallback, + out bool value, out string error) + { + value = fallback; + error = null; + + JToken token = @params?[key]; + if (token == null || token.Type == JTokenType.Null) + return true; + + if (token.Type != JTokenType.Boolean) + { + error = $"'{key}' must be true or false; got {token.Type.ToString().ToLowerInvariant()}."; + return false; + } + + value = token.Value(); + return true; + } + + /// + /// Reads an optional rate. Rejects NaN and both infinities, which pass every + /// comparison-based guard: `fps <= 0f` is false for NaN, so a NaN rate reached + /// the keyframe arithmetic and produced a clip whose frame times were all NaN. + /// + internal static bool TryReadFiniteFloat(JObject @params, string key, float fallback, + out float value, out string error) + { + value = fallback; + error = null; + + JToken token = @params?[key]; + if (token == null || token.Type == JTokenType.Null) + return true; + + if (token.Type != JTokenType.Integer && token.Type != JTokenType.Float) + { + error = $"'{key}' must be a number; got {token.Type.ToString().ToLowerInvariant()}."; + return false; + } + + double raw; + try + { + raw = token.Value(); + } + catch (Exception) + { + error = $"'{key}' is out of range for a number."; + return false; + } + + if (double.IsNaN(raw) || double.IsInfinity(raw)) + { + error = $"'{key}' must be a finite number."; + return false; + } + + // Read as double first so a value outside float's range is refused by name + // rather than silently becoming an infinity on the cast. + if (raw > float.MaxValue || raw < -float.MaxValue) + { + error = $"'{key}' is out of range for a 32-bit float."; + return false; + } + + value = (float)raw; + return true; + } + } +} diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs.meta b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs.meta new file mode 100644 index 000000000..a6302d408 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: e8816110e93149fcb381bf3b9a01ba40 diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index dadf5ae62..a9fff61a0 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -356,6 +356,121 @@ public void GetInfo_MissingFileOnDisk_DoesNotPutTheAbsolutePathInTheResponse() "it still has to say which asset it could not read"); } + [Test] + public void SetupClips_NonBooleanLoop_IsRefusedRatherThanConverted() + { + string path = CreateSheet("cliploop", 4, 2); + Slice(path, 4, 2); + var clips = OneClip("walk", 0, 3); + ((JObject)clips[0])["loop"] = "maybe"; + + // Measured 2026-08-21: this raised an uncaught FormatException out of the tool, + // and `loop: 2` was accepted silently. loop is the one flag with no type above + // C# - it lives inside the untyped `clips` array. + var result = SetupClips(path, clips); + + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_LOOP")); + Assert.AreEqual(0, result.Value("clip_count")); + } + + [TestCase("cols")] + [TestCase("rows")] + [TestCase("frame_width")] + [TestCase("frame_height")] + public void SliceSheet_GridValueTooLargeForAnInt_IsRefusedNotThrown(string key) + { + string path = CreateSheet($"gridovf{key}", 4, 2); + var request = new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["cols"] = 4, ["rows"] = 2 }; + request[key] = 2147483648L; + + // Measured 2026-08-21, before the guard: all four raised an uncaught + // OverflowException. Nothing between here and the bridge catches, so the tool + // failed at the transport instead of answering - reaching the assertions at + // all is half of what this test checks. + var result = Run(request); + + Assert.IsFalse(result.Value("success")); + // "32-bit", not just the key name: if the conversion wrapped instead of + // refusing, cols would land on the "either cols or frame_width" message, which + // also contains the key - measured, the weaker assertion passed the mutation. + Assert.That(ErrorText(result), Does.Contain(key).And.Contain("32-bit")); + } + + [Test] + public void SliceSheet_FractionalGridValue_IsRefusedRatherThanRounded() + { + string path = CreateSheet("gridfrac", 4, 2); + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["cols"] = 2.7, ["rows"] = 2 }); + + Assert.IsFalse(result.Value("success")); + Assert.That(ErrorText(result), Does.Contain("cols")); + } + + [TestCase("start_frame")] + [TestCase("end_frame")] + public void SetupClips_FrameIndexTooLargeForAnInt_IsRefusedNotThrown(string key) + { + string path = CreateSheet($"clipovf{key}", 4, 2); + Slice(path, 4, 2); + var clip = new JObject { ["name"] = "walk", ["start_frame"] = 0, ["end_frame"] = 3 }; + clip[key] = 2147483648L; + + var result = SetupClips(path, new JArray { clip }); + + // The clip is skipped with a named diagnostic rather than taking the whole + // call down; before the guard this threw out of the tool entirely. + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_RANGE")); + } + + [Test] + public void SetupClips_FractionalStartFrame_IsRefusedRatherThanRounded() + { + string path = CreateSheet("clipfrac", 4, 2); + Slice(path, 4, 2); + var clips = OneClip("walk", 0, 5); + ((JObject)clips[0])["start_frame"] = 2.7; + + var result = SetupClips(path, clips); + + // Measured before the guard: this rounded to 3 and wrote a clip, reporting + // success. A caller asking for a frame index that does not exist got an asset. + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_RANGE")); + Assert.AreEqual(0, result.Value("clip_count")); + } + + [Test] + public void SetupClips_NaNFps_IsRefusedRatherThanWrittenIntoTheClip() + { + string path = CreateSheet("clipnan", 4, 2); + Slice(path, 4, 2); + var clips = OneClip("walk", 0, 5); + ((JObject)clips[0])["fps"] = double.NaN; + + var result = SetupClips(path, clips); + + // NaN is not greater than 0 and not less than or equal to 0, so the existing + // `fps <= 0f` guard let it through and the clip was written with NaN keyframe + // times - measured, and reported as a success. + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_FPS")); + Assert.AreEqual(0, result.Value("clip_count")); + } + + [Test] + public void SetupClips_EndFramePastTheLastSprite_IsRefusedRatherThanTruncated() + { + string path = CreateSheet("clippast", 4, 2); + Slice(path, 4, 2); + + var result = SetupClips(path, OneClip("walk", 0, 99)); + + // Skip/Take clamps silently, so this used to produce an eight-frame clip for a + // hundred-frame request and call it a success. + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_RANGE")); + Assert.AreEqual(0, result.Value("clip_count")); + } + [TestCase("page_size")] [TestCase("cursor")] public void GetInfo_PagingValueTooLargeForAnInt_IsRefusedNotThrown(string key) @@ -373,7 +488,10 @@ public void GetInfo_PagingValueTooLargeForAnInt_IsRefusedNotThrown(string key) var result = Run(request); Assert.IsFalse(result.Value("success")); - Assert.That(ErrorText(result), Does.Contain(key)); + // Same reason as the grid case: a wrapped value is still refused, but by the + // 1..4096 range guard, whose message also names the key. Only this phrase + // distinguishes the conversion refusing from something downstream refusing. + Assert.That(ErrorText(result), Does.Contain(key).And.Contain("32-bit")); } [Test] @@ -842,7 +960,11 @@ public void SetupClips_RangeBeyondTheSheet_WarnsAndWritesNothing() var result = SetupClips(path, OneClip("walk", 90, 99)); Assert.AreEqual(0, result.Value("clip_count")); - Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_EMPTY")); + // CLIP_BAD_RANGE, not CLIP_EMPTY: the range is now refused for naming frames + // that do not exist, before it can produce an empty selection. The behaviour + // this test is named for - warn, write nothing - is unchanged; only the + // diagnostic moved from describing the result to naming the wrong input. + Assert.That(result["diagnostics"].ToString(), Does.Contain("CLIP_BAD_RANGE")); Assert.IsNull(AssetDatabase.LoadAssetAtPath($"{TempRoot}/walk.anim")); } @@ -1041,6 +1163,10 @@ public void SetupController_ClipsThatDoNotExist_ReturnsError() new JObject { ["name"] = "walk", ["path"] = $"{TempRoot}/missing.anim" }, }); Assert.IsFalse(result.Value("success")); + // Not just success=false: Newtonsoft reads an absent "success" as false too, + // so a response of `{ }` would satisfy that alone and this test is named for + // the explanation, not the failure. + Assert.That(ErrorText(result), Is.Not.Empty); } [Test] From 2354610f5dea407934b09165d3e45c02b08f39d5 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 23:00:36 +0300 Subject: [PATCH 25/29] test(sprite): make five tests fail for the reason they are named for The audit mutation-tested tests that had never been mutation-tested, and five of them passed against code that did not do what their name promises. Each was confirmed in Unity by applying the exact mutation before changing the test: - SliceSheet_GridProductThatOverflowsInt_IsStillRefused asserted only that the call failed. With the (long) cast removed the wrapped product slipped past the bounds check and the request was refused by the independent frame ceiling instead - a different guard, same green. It now asserts SLICE_OUT_OF_BOUNDS. - GetInfo_AfterSlicing_ReportsEverySlice asserted only slice_count, which comes from importer.spritesheet.Length and is independent of the projected list. Emptying `slices` entirely left it green. It now reads the slices. - Four tests named for returning an error asserted only that success was false. Newtonsoft reads an absent "success" as false, so a response of `{ }` would have satisfied them. They now assert the explanation. - GetInfo_OnAnUnslicedSheet_ReportsNoSlices checked the count but not the list. - One more next_cursor read still indexed the response object before reading it, which throws rather than asserts if the property is ever omitted. That was supposedly fixed last commit; it was fixed in two of three places. Two comments were wrong rather than weak. One said a sheet sliced through this tool never meets paging - slice_sheet allows 4096 frames and the page is 512, so 513 and up do. The same claim was corrected in the docs one commit earlier and left standing here, which is the same per-path habit the commit above is about. The other credited the Python bridge with treating an absent next_cursor as completion; the bridge forwards the response untouched and never reads it. --- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 40 ++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index a9fff61a0..ce1efaef1 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -203,6 +203,9 @@ public void GetInfo_OnAnUnslicedSheet_ReportsNoSlices() Assert.IsNotNull(result["slice_count"], "an absent field also reads as 0, so the field itself has to be there"); Assert.AreEqual(0, result.Value("slice_count")); + // The count alone would pass against a response that reported slices it never + // counted; the observable behaviour is that the list is empty too. + Assert.AreEqual(0, ((JArray)result["slices"]).Count); } [Test] @@ -213,6 +216,12 @@ public void GetInfo_AfterSlicing_ReportsEverySlice() var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); Assert.AreEqual(8, result.Value("slice_count")); + // slice_count comes from importer.spritesheet.Length, which is independent of + // the projected list - measured 2026-08-21: emptying `slices` entirely left + // this test green. A test named for reporting every slice has to read them. + var names = ((JArray)result["slices"]).Select(t => t.Value("name")).ToArray(); + Assert.AreEqual(8, names.Length, "every slice is reported, not just counted"); + CollectionAssert.AllItemsAreUnique(names); } [Test] @@ -223,13 +232,15 @@ public void GetInfo_ModestSheet_ComesBackInOnePageWithNoCursor() var result = Run(new JObject { ["action"] = "get_info", ["path"] = path }); - // The point of the default page size is that a sheet anyone would slice through - // this tool never meets paging at all. If this turns red the default shrank. + // Eight slices is under the default page, so this sheet arrives whole. That is + // NOT true of every sheet slice_sheet can produce - it allows up to 4096 frames + // and the default page is 512 - so this asserts the default, not a guarantee. Assert.AreEqual(8, ((JArray)result["slices"]).Count); // Value("next_cursor") rather than indexing then reading: an omitted - // property indexes to a C# null and would throw here instead of asserting, - // and the Python side treats an absent next_cursor as completion. Same rule - // as ErrorText above - do not pin the response shape. + // property indexes to a C# null and would throw here instead of asserting. + // Absent and null both mean "finished" to the caller - that contract is the + // documented one, not something the Python bridge implements; it forwards the + // response untouched. Same rule as ErrorText above: do not pin the shape. Assert.IsNull(result.Value("next_cursor"), "a finished list has no next cursor"); } @@ -250,7 +261,7 @@ public void GetInfo_MoreSlicesThanThePage_ReturnsOnePageAndPointsAtTheRest() Assert.AreEqual(3, ((JArray)result["slices"]).Count, "the page is bounded"); Assert.AreEqual(8, result.Value("slice_count"), "slice_count stays the total, not the size of the page"); - Assert.AreEqual(3, result["next_cursor"].Value()); + Assert.AreEqual(3, result.Value("next_cursor")); } [Test] @@ -716,6 +727,10 @@ public void SliceSheet_ZeroRows_FailsWithAMessageInsteadOfThrowing() Assert.DoesNotThrow(() => result = Slice(path, 4, 0), "a bad grid value must come back as an error, not an exception"); Assert.IsFalse(result.Value("success")); + // Not just success=false: Newtonsoft reads an absent "success" as false too, + // so a response of `{ }` would satisfy that alone and this test is named for + // the explanation, not the failure. + Assert.That(ErrorText(result), Is.Not.Empty); } [Test] @@ -783,6 +798,11 @@ public void SliceSheet_GridProductThatOverflowsInt_IsStillRefused() ["cols"] = 65536, ["frame_width"] = 65536 }); Assert.IsFalse(result.Value("success")); + // The code, not just the failure. Measured 2026-08-21: with the (long) cast + // removed this test stayed GREEN, because the wrapped product slipped past the + // bounds check and the request was then refused by the independent frame + // ceiling instead. Asserting only "it failed" cannot tell the two apart. + Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_OUT_OF_BOUNDS")); } [Test] @@ -1141,6 +1161,10 @@ public void SetupController_WithoutClips_ReturnsError() ["controller_path"] = $"{TempRoot}/Hero.controller", }); Assert.IsFalse(result.Value("success")); + // Not just success=false: Newtonsoft reads an absent "success" as false too, + // so a response of `{ }` would satisfy that alone and this test is named for + // the explanation, not the failure. + Assert.That(ErrorText(result), Does.Contain("clips")); } [Test] @@ -1450,6 +1474,10 @@ public void FullSetup_WithoutColsOrFrameWidth_ReturnsError() string path = CreateSheet("fullnogrid", 4, 1); var result = Run(new JObject { ["action"] = "full_setup", ["path"] = path }); Assert.IsFalse(result.Value("success")); + // Not just success=false: Newtonsoft reads an absent "success" as false too, + // so a response of `{ }` would satisfy that alone and this test is named for + // the explanation, not the failure. + Assert.That(ErrorText(result), Does.Contain("frame_width")); } [Test] From c1a6cd5b3f971ba0ae87deffab25598c675c98e9 Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 23:00:49 +0300 Subject: [PATCH 26/29] test(server): guard the hand-written forwarding block Fifteen optional arguments are copied into the request one `if` at a time, so adding a parameter to the signature and forgetting its branch produces a tool that accepts the value and drops it - no error anywhere, at either end. Review was the only thing preventing that, which is a habit rather than a check. The new test calls the tool with every optional argument set and asserts each one reached the bridge. It reads the signature rather than a hand-kept list, so a parameter added without a sample value fails loudly instead of being skipped silently. Confirmed to discriminate: deleting the page_size branch fails it, and the message names page_size. Also corrects a docstring the audit caught inventing a failure mode: it claimed forwarding an unset page_size as null would become a zero and break plain get_info. SpriteParams treats a JSON null as unset and falls back to the default, so the wire contract is about staying readable, not about avoiding a broken call. And records which copy of the paging bounds is authoritative. The Python annotation publishes 1-4096/512 into the generated reference while the C# side enforces it independently; both now say so, so a change to either is a change to both. --- Server/src/services/tools/manage_sprite.py | 4 ++ Server/tests/test_manage_sprite.py | 53 ++++++++++++++++++++-- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py index 3379716d3..fb8ea173f 100644 --- a/Server/src/services/tools/manage_sprite.py +++ b/Server/src/services/tools/manage_sprite.py @@ -96,6 +96,10 @@ async def manage_sprite( str | None, "Existing GameObject name to attach Animator to.", ] = None, + # The numbers below are documentation, not enforcement: SpriteParams and + # SpriteImportSetup.GetInfo are what actually refuse an out-of-range page_size, and + # this text is what the generated reference publishes to callers. Two copies, so + # changing the C# bounds means changing this line in the same commit. page_size: Annotated[ int | None, "get_info: how many entries of the 'slices' list to return (1-4096, default 512). " diff --git a/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py index d1489f72b..87afbe459 100644 --- a/Server/tests/test_manage_sprite.py +++ b/Server/tests/test_manage_sprite.py @@ -133,9 +133,11 @@ def test_only_supplied_parameters_are_forwarded(self): def test_paging_arguments_reach_unity_only_when_asked_for(self): """page_size and cursor are get_info's, and absent means "use the default". - Forwarding cursor=0 unasked would be harmless, but forwarding page_size=0 - would not: the C# side refuses anything below 1, so a null that turned into - a zero on the wire would break every plain get_info call. + The receiver reads an absent key and an explicit JSON null the same way - + SpriteParams.TryReadWholeNumber names the null case and falls back - so this + is about the wire staying readable, not about avoiding a broken call. An + earlier version of this docstring claimed a null would become a zero and + break plain get_info; that was wrong, and an audit caught it. """ from services.tools.manage_sprite import manage_sprite @@ -155,3 +157,48 @@ def test_paging_arguments_reach_unity_only_when_asked_for(self): assert plain == {"action": "get_info", "path": "Assets/atlas.png"} assert paged["page_size"] == 100 assert paged["cursor"] == 200 + + def test_every_optional_argument_has_a_forwarding_branch(self): + """A parameter accepted at the surface but dropped before the bridge is silent. + + The forwarder rebuilds the request by hand, one `if` per parameter, so adding + an argument to the signature and forgetting its branch produces a tool that + accepts the value and ignores it - no error anywhere. Fifteen branches are + fifteen chances for that, and review is the only thing preventing it, which + is a habit rather than a check. This is the check. + """ + import inspect + + from services.tools.manage_sprite import manage_sprite + + fn = getattr(manage_sprite, "fn", manage_sprite) + # A value each parameter's annotation accepts. Booleans must be True: the + # forwarder deliberately omits a False flag, so False would look like a + # dropped branch and this guard would cry wolf. + sample = { + "path": "Assets/a.png", "cols": 1, "rows": 1, "frame_width": 1, + "frame_height": 1, "base_name": "b", "clips": [{"name": "walk"}], + "animation_name": "walk", "output_dir": "Assets/out", + "controller_path": "Assets/a.controller", "overwrite": True, + "add_to_scene": True, "scene_target": "Hero", "page_size": 1, "cursor": 1, + } + optional = [ + name for name, prm in inspect.signature(fn).parameters.items() + if name not in ("ctx", "action") and prm.default is not inspect.Parameter.empty + ] + missing_sample = [n for n in optional if n not in sample] + assert not missing_sample, ( + f"this test has no sample value for {missing_sample}; add one rather than " + "narrowing the guard" + ) + + with patch("services.tools.manage_sprite.get_unity_instance_from_context", + new=AsyncMock(return_value=None)), \ + patch("services.tools.manage_sprite.send_with_unity_instance", + new=AsyncMock(return_value={"success": True})) as sent: + asyncio.run(manage_sprite(self._ctx(), action="full_setup", + **{k: sample[k] for k in optional})) + + forwarded = sent.await_args.args[3] + dropped = [n for n in optional if n not in forwarded] + assert not dropped, f"accepted at the surface but never sent to Unity: {dropped}" From b5ca3c01516e9c306700291c2875ec66196a468f Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 23:15:49 +0300 Subject: [PATCH 27/29] fix(sprite): correct why the top-level parameters need no guard The verification round caught the rationale, not the code. The previous commit said FastMCP "refuses a non-boolean before C# sees it". It does not refuse - it coerces. Measured through server.call_tool on 2026-08-21: overwrite='yes' -> bridge receives True cols='4' -> bridge receives 4 overwrite=1 -> bridge receives True cols=2.7 -> refused by Pydantic overwrite=2 -> refused by Pydantic rows=3.0 -> bridge receives 3 The conclusion survives: what reaches C# is already the right type either way, so the top-level parameters need no C# guard for their type. What changes is the reachability claim underneath it - of the classes the new guards cover, only an out-of-int-range integer arrives from a real caller, because Python integers have no ceiling while a fractional one is refused upstream. The nested clip scalars are the exception and the reason the guards exist: they sit inside an untyped `clips` array that nothing above C# inspects. The guards stay for every class regardless. This layer owns the conversion, and a caller-facing refusal is not something to leave to the layer above. Also strengthens the forwarding guard the same round found incomplete: it compared key membership only, so a branch that kept the key and replaced the caller's value passed it. It now compares values, and fails naming the parameter and both values. Its one assumption - that the forwarder stays action-agnostic - is now written into the docstring rather than left implicit, because scoping a branch to the action that owns it would make this test fail on correct code. --- .../Editor/Tools/Sprite2D/SpriteParams.cs | 17 +++++++++++++---- Server/tests/test_manage_sprite.py | 10 ++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs index 6d5850966..a5a4991c6 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs @@ -79,10 +79,19 @@ internal static bool TryReadWholeNumber(JObject @params, string key, int fallbac /// /// Reads an optional flag. Measured 2026-08-21: `loop: "maybe"` raised an uncaught /// FormatException and `loop: 2` was accepted silently, because ToObject<bool?> - /// converts rather than validates. The top-level flags do not need this - the Python - /// surface types `overwrite` and `add_to_scene` as bool, so FastMCP refuses a - /// non-boolean before it reaches here - but `loop` hides inside the untyped `clips` - /// array, where nothing above C# looks at it. + /// converts rather than validates. `loop` needs this because it hides inside the + /// untyped `clips` array, where nothing above C# looks at it. + /// + /// The top-level flags do not, but not for the reason it first looked like. FastMCP + /// does not REFUSE a non-boolean `overwrite`; it coerces one - measured 2026-08-21 + /// through server.call_tool: 'yes' and 1 both arrive here as a real bool, 'off' + /// arrives as false, and only a value Pydantic cannot read as a boolean (2) is + /// refused. Either way what reaches C# is already the right type. The same holds + /// for the top-level integers: '4' arrives as 4 and 2.7 is refused outright, so of + /// the classes below only an out-of-int-range integer reaches C# from a real + /// caller - Python ints have no ceiling. The guards still cover the rest, because + /// this layer owns the conversion and a caller-facing refusal is not something to + /// leave to the layer above. /// internal static bool TryReadBool(JObject @params, string key, bool fallback, out bool value, out string error) diff --git a/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py index 87afbe459..866045331 100644 --- a/Server/tests/test_manage_sprite.py +++ b/Server/tests/test_manage_sprite.py @@ -166,6 +166,12 @@ def test_every_optional_argument_has_a_forwarding_branch(self): accepts the value and ignores it - no error anywhere. Fifteen branches are fifteen chances for that, and review is the only thing preventing it, which is a habit rather than a check. This is the check. + + Known limitation, named rather than fixed: this drives every parameter through + one action, so it assumes the forwarder stays action-agnostic - which it is + today, every branch being a plain `is not None`. If a branch is ever scoped to + the actions that own it (page_size and cursor belong to get_info), this test + will fail on correct code and must be scoped with it. """ import inspect @@ -202,3 +208,7 @@ def test_every_optional_argument_has_a_forwarding_branch(self): forwarded = sent.await_args.args[3] dropped = [n for n in optional if n not in forwarded] assert not dropped, f"accepted at the surface but never sent to Unity: {dropped}" + # The value too, not only the key. An audit reproduced a branch that kept the key + # and replaced the caller's value; membership alone stayed green for it. + changed = {n: (sample[n], forwarded[n]) for n in optional if forwarded[n] != sample[n]} + assert not changed, f"forwarded under a different value than the caller sent: {changed}" From ae9f537b58c16c58a50d2e8243254cff5296ac2f Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 23:49:48 +0300 Subject: [PATCH 28/29] fix(sprite): say when a grid leaves part of the sheet unused The bounds guard only refuses a grid that is too big. One that is too small passed it and the leftover pixels were dropped without a word. Measured on 6000.4.4f1: a 100x16 sheet asked for 6 columns produced six 16px sprites covering 96 of 100 pixels, `success: true`, and an empty diagnostics list. The caller had no way to learn that a strip of the sheet had been ignored. This warns instead of refusing, which is a deliberate difference from the suggested fix. A remainder is not always a mistake: sheets with a trailing margin or a separator column are ordinary, and a caller passing `frame_width` explicitly may want a sub-region on purpose. Requiring exact coverage would refuse all of those. The defect here is the silence, not the behaviour, so `SLICE_GRID_REMAINDER` names the covered area, the texture size and the leftover on each axis, and the call still succeeds. Both directions are pinned, because a warning that fires on every slice looks exactly like one that fires on the right slices: reverting the condition turns the remainder test red, and forcing it always-true turns the exact-fit test red. Measured: ManageSpriteTests 102/102, Python 1391 passed / 3 skipped, docs --check clean. --- .../Tools/Sprite2D/SpriteImportSetup.cs | 21 ++++++++ .../Tests/EditMode/Tools/ManageSpriteTests.cs | 50 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs index 9f7ad21bd..64c035930 100644 --- a/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -287,6 +287,27 @@ public static object SliceSheet(JObject @params, SpriteDiagnosticBuilder diagnos return new { success = false, diagnostics = diagnostics.Build() }; } + // Fitting is not the same as covering. The guard above only refuses a grid that + // is too BIG; one that is too small passes and the leftover pixels are dropped + // without a word - measured on 6000.4.4f1: a 100x16 sheet asked for 6 columns + // produced six 16px sprites covering 96 of 100 pixels, success, no diagnostic. + // This warns rather than refusing, because a remainder is not always a mistake: + // sheets with a trailing margin or a separator column are ordinary, and a caller + // passing frame_width explicitly may want a sub-region on purpose. Refusing would + // break those; staying silent is what hid the mistaken ones. + int uncoveredW = texW - cols * frameW; + int uncoveredH = texH - rows * frameH; + if (uncoveredW > 0 || uncoveredH > 0) + { + diagnostics.AddWarning( + "SLICE_GRID_REMAINDER", + $"The grid covers {cols * frameW}x{rows * frameH} of a {texW}x{texH} texture, leaving {uncoveredW}px on the right and {uncoveredH}px at the bottom unused.", + new { cols, rows, frame_width = frameW, frame_height = frameH, texture_width = texW, texture_height = texH, uncovered_width = uncoveredW, uncovered_height = uncoveredH }, + new[] { "Deliberate if the sheet has a margin or a separator", "Otherwise check cols/rows against the texture size with get_info" } + ); + } + + // A 4096x4096 sheet cut into 1px frames is 16,777,216 entries, and this method // allocates and reimports every one of them in one call. That size was not run // here - the ceiling is a precaution, not a reproduction - but it sits far above diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index ce1efaef1..b5e084673 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -101,6 +101,23 @@ private static string CreateNoiseSheet(string name, int side, bool assertOverCei return assetPath; } + /// A flat sheet of an exact pixel size, for grids that do not divide evenly. + private static string CreateSheetOfSize(string name, int width, int height) + { + var tex = new Texture2D(width, height); + var px = new Color32[width * height]; + for (int i = 0; i < px.Length; i++) px[i] = new Color32(255, 0, 0, 255); + tex.SetPixels32(px); + tex.Apply(); + + string assetPath = $"{TempRoot}/{name}.png"; + System.IO.File.WriteAllBytes( + System.IO.Path.Combine(Directory.GetParent(Application.dataPath).FullName, assetPath), + tex.EncodeToPNG()); + AssetDatabase.ImportAsset(assetPath, ImportAssetOptions.ForceSynchronousImport); + return assetPath; + } + private static JObject Run(JObject p) => ToJObject(ManageSprite.HandleCommand(p)); /// @@ -384,6 +401,39 @@ public void SetupClips_NonBooleanLoop_IsRefusedRatherThanConverted() Assert.AreEqual(0, result.Value("clip_count")); } + [Test] + public void SliceSheet_GridThatDoesNotCoverTheTexture_SucceedsButSaysSo() + { + // 100 / 6 = 16, so the grid covers 96px and drops four. Measured before the + // warning existed: success, six sprites, and an empty diagnostics list - the + // caller had no way to learn that a strip of the sheet was ignored. + string path = CreateSheetOfSize("remainder", 100, 16); + + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["cols"] = 6, ["rows"] = 1 }); + + // Still a success: a trailing margin is ordinary and refusing would break it. + Assert.IsTrue(result.Value("success")); + Assert.AreEqual(6, SpritesOf(path).Length); + Assert.That(result["diagnostics"].ToString(), Does.Contain("SLICE_GRID_REMAINDER")); + Assert.That(result["diagnostics"].ToString(), Does.Contain("100"), + "the warning has to name the texture size, or it cannot be acted on"); + } + + [Test] + public void SliceSheet_GridThatCoversTheTextureExactly_WarnsAboutNothing() + { + // The other direction. Without this, a warning that fired on every slice would + // look exactly like a warning that fires on the right ones. + string path = CreateSheetOfSize("exact", 96, 16); + + var result = Run(new JObject { ["action"] = "slice_sheet", ["path"] = path, + ["cols"] = 6, ["rows"] = 1 }); + + Assert.IsTrue(result.Value("success")); + Assert.That(result["diagnostics"].ToString(), Does.Not.Contain("SLICE_GRID_REMAINDER")); + } + [TestCase("cols")] [TestCase("rows")] [TestCase("frame_width")] From 3dd76dff85f6121d9a5a5342c5cd622b1ffbf95c Mon Sep 17 00:00:00 2001 From: Burak Erdemci Date: Fri, 21 Aug 2026 23:58:03 +0300 Subject: [PATCH 29/29] test(sprite): destroy the working texture in the new sheet helper CreateSheetOfSize built a Texture2D and never released it, while both sibling helpers a few lines up destroy theirs after encoding. A Texture2D created in an EditMode test is not collected on its own, so the helper leaked one per call for as long as the run lasted. The helper now matches the siblings exactly - same format arguments, same Path.Combine style, same DestroyImmediate placement - which is the point: an inconsistency between three functions doing the same job is what let this one be written without the line. --- .../Tests/EditMode/Tools/ManageSpriteTests.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs index b5e084673..02ec2ad4f 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -104,16 +104,21 @@ private static string CreateNoiseSheet(string name, int side, bool assertOverCei /// A flat sheet of an exact pixel size, for grids that do not divide evenly. private static string CreateSheetOfSize(string name, int width, int height) { - var tex = new Texture2D(width, height); - var px = new Color32[width * height]; - for (int i = 0; i < px.Length; i++) px[i] = new Color32(255, 0, 0, 255); - tex.SetPixels32(px); + var tex = new Texture2D(width, height, TextureFormat.RGBA32, false); + var pixels = new Color32[width * height]; + for (int i = 0; i < pixels.Length; i++) + pixels[i] = new Color32(255, 0, 0, 255); + tex.SetPixels32(pixels); tex.Apply(); string assetPath = $"{TempRoot}/{name}.png"; - System.IO.File.WriteAllBytes( - System.IO.Path.Combine(Directory.GetParent(Application.dataPath).FullName, assetPath), - tex.EncodeToPNG()); + string sysPath = Path.Combine( + Directory.GetParent(Application.dataPath).FullName, assetPath); + File.WriteAllBytes(sysPath, tex.EncodeToPNG()); + // Both sibling helpers destroy their working texture here; this one did not, and + // a Texture2D built in an EditMode test is not collected on its own. + Object.DestroyImmediate(tex); + AssetDatabase.ImportAsset(assetPath, ImportAssetOptions.ForceSynchronousImport); return assetPath; }