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..1d48d13c3 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteClipBuilder.cs @@ -0,0 +1,254 @@ +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) + /// overwrite - bool (default false); an existing clip is kept unless this is true + /// + 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); + if (path == null) + return new ErrorResponse("'path' must stay under Assets/ and cannot contain '..'."); + + 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); + + bool overwrite = @params["overwrite"]?.ToObject() ?? false; + + var createdClips = new List(); + + 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; } + + // 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; + } + + // 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 + // 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; + } + // 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 + // 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); + 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) + { + 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]); + + // 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 + { + 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); + + 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 ────────────────────────────────────────────────── + + 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..9babf0992 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteControllerBuilder.cs @@ -0,0 +1,231 @@ +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; + + var entries = new List<(SpriteAnimEntry entry, AnimationClip clip)>(); + 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() ?? ""; + 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)); + } + + if (entries.Count == 0) + // 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. + 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 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..002f2cc4f --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteFullSetup.cs @@ -0,0 +1,235 @@ +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 (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 + // 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(); + + // ── 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, + }); + } + + 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) + 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"; + + // 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(); + 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 + { + ["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() }; + // 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(); + + // 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) + { + var controller = AssetDatabase.LoadAssetAtPath( + controllerPath); + if (controller != null) + { + // `??` 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.AddError("SCENE_TARGET_NOT_FOUND", + $"GameObject '{sceneTarget}' not found in scene.", + null, + new[] { "Check GameObject name or open the correct scene first." }); + } + } + + 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 = 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); + 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..64c035930 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteImportSetup.cs @@ -0,0 +1,385 @@ +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); + // 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?"); + + var texture = AssetDatabase.LoadAssetAtPath(path); + int w = texture != null ? texture.width : 0; + int h = texture != null ? texture.height : 0; + + // 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 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 (!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 (!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 + // 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, + y = (int)s.rect.y, + width = (int)s.rect.width, + 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 + // 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. + // 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; + if (cursor > 0) + { + // 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 + { + // 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)) + { + // 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 + { + 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 {}`. + // 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."; + } + } + + 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 = totalSlices, + slices = existingSlices, + next_cursor = nextCursor, + image_base64 = imageBase64, + image_omitted_reason = imageOmittedReason, + }; + + 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) + { + string path = @params["path"]?.ToString(); + if (string.IsNullOrEmpty(path)) + 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}'."); + + // 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. + // 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."); + + // `?? 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 + // 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; + EditorUtility.SetDirty(importer); + importer.SaveAndReimport(); + } + + 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; + + if (frameW <= 0) frameW = texW / cols; + if (frameH <= 0) frameH = texH / rows; + 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. + // 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", + "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() }; + } + + // 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 + // 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( + "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" } + ); + RestoreTextureType(importer, previousType); + return new { success = false, diagnostics = diagnostics.Build() }; + } + + string baseName = @params["base_name"]?.ToString() + ?? Path.GetFileNameWithoutExtension(path); + + var metas = new SpriteMetaData[(int)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..3347f163a --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteNamingDetector.cs @@ -0,0 +1,144 @@ +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) + { + var entry = new SpriteAnimEntry { ClipName = clipName }; + // 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; + } + + 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 name, SpriteAnimEntry entry) + { + var words = Words(name); + + 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(name.ToLowerInvariant()); + } + + /// + /// 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])) + // 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])); + + 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/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs new file mode 100644 index 000000000..a5a4991c6 --- /dev/null +++ b/MCPForUnity/Editor/Tools/Sprite2D/SpriteParams.cs @@ -0,0 +1,166 @@ +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. `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) + { + 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/Server/src/services/tools/manage_sprite.py b/Server/src/services/tools/manage_sprite.py new file mode 100644 index 000000000..fb8ea173f --- /dev/null +++ b/Server/src/services/tools/manage_sprite.py @@ -0,0 +1,194 @@ +""" +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; " + "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." + ), + 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, + "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, + "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). " + "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.""" + + 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."} + + # 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 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."} + + 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 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: + 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/Server/tests/test_manage_sprite.py b/Server/tests/test_manage_sprite.py new file mode 100644 index 000000000..866045331 --- /dev/null +++ b/Server/tests/test_manage_sprite.py @@ -0,0 +1,214 @@ +"""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_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_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 + 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} + + 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". + + 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 + + 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 + + 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. + + 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 + + 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}" + # 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}" 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..02ec2ad4f --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSpriteTests.cs @@ -0,0 +1,1818 @@ +using System.Collections.Generic; +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); + + // 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; + } + + + /// + /// 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, bool assertOverCeiling = true) + { + 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); + + // 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; + } + + /// 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, 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"; + 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; + } + + 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) + { + 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) => + 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")); + Assert.That(ErrorText(result), Does.Contain("'path' is required"), + "success alone would also be false on the importer-not-found branch"); + } + + [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.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] + 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_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] + 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 }); + + // 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. + // 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"); + } + + [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.Value("next_cursor")); + } + + [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.Value("next_cursor"); + } + + 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")); + } + + [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"); + } + + [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")); + } + + [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")] + [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) + { + 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")); + // 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] + 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)] + 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 + // ===================================================================== + + [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")); + // 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] + 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_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")); + // 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] + 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() + { + 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")); + // 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")); + } + + [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_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() + { + 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")); + // 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] + 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")); + // 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] + 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() + { + 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")); + + // 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"); + } + + // ===================================================================== + // 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 + // ===================================================================== + + [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")); + // 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] + 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")); + } + + // ===================================================================== + // 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")); + } + + // ===================================================================== + // 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")); + } + + [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 before = ((TextureImporter)AssetImporter.GetAtPath(path)).textureType; + + 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"); + // 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"); + } + + // ===================================================================== + // 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); + } + + // ===================================================================== + // 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")); + } + + [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"); + } + + [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"); + } + } +} 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 diff --git a/website/docs/reference/tools/animation/index.md b/website/docs/reference/tools/animation/index.md index f2a57be64..79bdbc270 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; 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 new file mode 100644 index 000000000..bcef2e1c2 --- /dev/null +++ b/website/docs/reference/tools/animation/manage_sprite.md @@ -0,0 +1,111 @@ +--- +title: manage_sprite +sidebar_label: manage_sprite +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` + +> **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; 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 + +| 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` | — | 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 + +A `dict` containing the Unity response. The exact shape depends on the action. + +## Examples + + +### 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" } +``` + +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. + +```json +{ "action": "get_info", "path": "Assets/Sprites/atlas.png", "cursor": 512 } +``` + +### 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 } +``` + + diff --git a/website/docs/reference/tools/index.md b/website/docs/reference/tools/index.md index a7466a134..9f37f38b2 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; 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)