diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f353ca..e1b7d4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,65 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **draw_pixels: a later call reverts previously-transparent pixels to opaque black** + - On an indexed sprite `CreateCanvas` sets `Sprite.transparentColor` to 255 so + palette index 0 can hold a real color, which means index 0 is opaque. When + `draw_pixels` normalizes the target cel it builds a full-canvas image with + `Image(spr.width, spr.height, spr.colorMode)`, and a freshly created indexed + image is filled with index 0 (opaque), not the transparent index + - Compositing the existing content with the default NORMAL blend + (`full:drawImage(cel.image, cel.position)`) skips the source's transparent + (index 255) pixels, so every pixel a previous call had made transparent but + the current call does not rewrite is left showing the opaque index-0 + background, silently turning transparent areas into opaque black (the exact + symptom warned about for locked-palette sprites) + - Fixed by clearing the full-canvas image to the sprite's transparent color + (`full:clear(spr.transparentColor)`) before compositing/drawing on indexed + sprites, in both the existing-cel and new-cel paths; RGB and grayscale + behavior is unchanged +- **create_canvas: a new indexed canvas starts opaque instead of transparent** + - A new indexed sprite's initial cel is filled with palette index 0, but + `CreateCanvas` designates index 255 as the transparent color, so index 0 is + an opaque color (black on DawnBringer 32) and a fresh canvas read back as an + opaque background rather than transparent + - Fixed by clearing the initial cel to the sprite's transparent color right + after creation, so a fresh indexed canvas is genuinely transparent; RGB and + grayscale are unaffected +- **quantize_palette: median cut blends distinct colors under imbalanced pixel frequency** + - `MedianCutQuantization` split each bucket at the pixel-array midpoint + (`mid := len(pixels) / 2`), not at a boundary between distinct colors. + Pixel art typically has wildly imbalanced color frequency (a large + solid fill vastly outnumbering a 1-2px accent detail), so splitting at + the raw pixel-count median lands the cut inside the dominant color's + own run instead of between colors + - The dominant color got needlessly re-subdivided into near-duplicate + buckets while rare accent colors never got isolated into their own + bucket, and were instead averaged into a blended, off-palette hex + value - even when `target_colors` comfortably exceeded the number of + unique colors actually present, which should guarantee lossless + recovery + - Fixed by reducing to a frequency-weighted histogram of unique colors + before bucketing, so a split always falls between distinct colors; + the split point still favors common colors over rare ones by + cumulative pixel count, preserving normal median-cut reduction + behavior when `target_colors` is genuinely smaller than the number of + unique colors +- **Indexed palette: transparentColor collides with a real color after resize** + - `Palette:resize` silently clamps `Sprite.transparentColor` into the new + palette's valid index range, so after `set_palette`, `quantize_palette`, + `add_palette_color`, or `apply_auto_shading` resize an indexed sprite's + palette, the sprite's transparent-color index can land on the same index + as a real, in-use color (typically the palette's last entry) + - Once that collision happens, painting "transparent" (e.g. `fill_area` + with a fully-transparent color) writes that colliding index, so + background fills read back as an opaque real color instead of + transparent, and pixels intentionally drawn in that color can be + auto-trimmed away as if they were transparent + - Fixed by re-asserting `transparentColor = 255` after every palette + resize on an indexed sprite, keeping it safely out of the palette's + real, addressable range + ## [0.5.0] - 2025-10-18 ### Added diff --git a/pkg/aseprite/lua_auto_shading.go b/pkg/aseprite/lua_auto_shading.go index 01ac300..fc88611 100644 --- a/pkg/aseprite/lua_auto_shading.go +++ b/pkg/aseprite/lua_auto_shading.go @@ -105,7 +105,7 @@ if palette then end end end - +%s -- Build final palette for JSON output local paletteHex = {} for i = 0, #palette - 1 do @@ -126,13 +126,14 @@ spr:saveAs(spr.filename) -- Print JSON result print(json)`, - EscapeString(layerName), // layer name for finding - EscapeString(layerName), // layer name for error - tempImagePath, // shaded image path - frameNumber, // frame number for cel lookup - frameNumber, // frame number for error message - frameNumber, // frame number for newCel - colorList, // generated colors - len(generatedColors), // colors_added - regionsShadedCount) // regions_shaded + EscapeString(layerName), // layer name for finding + EscapeString(layerName), // layer name for error + tempImagePath, // shaded image path + frameNumber, // frame number for cel lookup + frameNumber, // frame number for error message + frameNumber, // frame number for newCel + colorList, // generated colors + ReassertIndexedTransparentColor(), // keep transparentColor out of the real palette range + len(generatedColors), // colors_added + regionsShadedCount) // regions_shaded } diff --git a/pkg/aseprite/lua_canvas.go b/pkg/aseprite/lua_canvas.go index 5c89996..f9d2b9b 100644 --- a/pkg/aseprite/lua_canvas.go +++ b/pkg/aseprite/lua_canvas.go @@ -25,6 +25,15 @@ func (g *LuaGenerator) CreateCanvas(width, height int, colorMode ColorMode, file if colorMode == ColorModeIndexed { return fmt.Sprintf(`local spr = Sprite(%d, %d, %s) spr.transparentColor = 255 +-- A new indexed sprite's initial cel is filled with palette index 0, but index +-- 255 is the transparent index (set above), so index 0 is now an opaque color +-- and the canvas would start as an opaque background instead of transparent. +-- Clear the initial cel to the transparent color so a fresh indexed canvas is +-- genuinely transparent. +local initialCel = spr.layers[1]:cel(spr.frames[1]) +if initialCel and initialCel.image then + initialCel.image:clear(spr.transparentColor) +end spr:saveAs("%s") print("%s")`, width, height, colorMode.ToLua(), escapedFilename, escapedFilename) } diff --git a/pkg/aseprite/lua_core.go b/pkg/aseprite/lua_core.go index 0960602..7606a94 100644 --- a/pkg/aseprite/lua_core.go +++ b/pkg/aseprite/lua_core.go @@ -54,29 +54,36 @@ func FormatColor(c Color) string { // FormatColorWithPalette formats a Color with optional palette snapping for img:putPixel. // -// If usePalette is false, returns a direct Color constructor call. -// If usePalette is true, wraps the color in snapToPaletteForPixel() to find the nearest palette color. +// If usePalette is false, wraps the color in resolveExactPaletteColor() to require an exact +// palette match on indexed sprites (errors otherwise) while passing RGB/grayscale colors through +// unchanged. If usePalette is true, wraps the color in snapToPaletteForPixel() to find the +// nearest palette color. // -// The snapToPaletteForPixel function must be defined in the script (use GeneratePaletteSnapperHelper). -// This is useful for palette-constrained pixel art to ensure all colors match the palette. -// Returns palette index in indexed mode, pixel color in other modes. +// The resolveExactPaletteColor function must be defined in the script when usePalette is false +// (use ResolveExactPaletteColorHelper); snapToPaletteForPixel must be defined when usePalette is +// true (use GeneratePaletteSnapperHelper). Returns palette index in indexed mode, pixel color in +// other modes. func FormatColorWithPalette(c Color, usePalette bool) string { if !usePalette { - return FormatColor(c) + return fmt.Sprintf("resolveExactPaletteColor(%d, %d, %d, %d)", c.R, c.G, c.B, c.A) } return fmt.Sprintf("snapToPaletteForPixel(%d, %d, %d, %d)", c.R, c.G, c.B, c.A) } // FormatColorWithPaletteForTool formats a Color with optional palette snapping for app.useTool. // -// If usePalette is false, returns a direct Color constructor call. -// If usePalette is true, wraps the color in snapToPaletteForTool() to find the nearest palette color. +// If usePalette is false, wraps the color in resolveExactPaletteColor() to require an exact +// palette match on indexed sprites (errors otherwise) while passing RGB/grayscale colors through +// unchanged. If usePalette is true, wraps the color in snapToPaletteForTool() to find the nearest +// palette color. // -// The snapToPaletteForTool function must be defined in the script (use GeneratePaletteSnapperHelper). -// Always returns a pixel color value suitable for app.useTool in all color modes. +// The resolveExactPaletteColor function must be defined in the script when usePalette is false +// (use ResolveExactPaletteColorHelper); snapToPaletteForTool must be defined when usePalette is +// true (use GeneratePaletteSnapperHelper). Always returns a value suitable for app.useTool's +// color field in all color modes. func FormatColorWithPaletteForTool(c Color, usePalette bool) string { if !usePalette { - return FormatColor(c) + return fmt.Sprintf("resolveExactPaletteColor(%d, %d, %d, %d)", c.R, c.G, c.B, c.A) } return fmt.Sprintf("snapToPaletteForTool(%d, %d, %d, %d)", c.R, c.G, c.B, c.A) } @@ -159,6 +166,88 @@ end ` } +// ReassertIndexedTransparentColor returns a Lua snippet that pins an indexed +// sprite's transparentColor back to 255 after the palette has been resized. +// +// CreateCanvas sets Sprite.transparentColor to 255 for indexed sprites so that +// palette index 0 can hold a real, opaque color instead of being silently +// treated as transparent (see the palette index 0 fix). However, Aseprite +// automatically clamps Sprite.transparentColor into the current palette's +// valid index range whenever Palette:resize shrinks the palette below 256 +// colors. For a locked N-color palette (N < 256), transparentColor ends up +// equal to N-1, i.e. the last real color the caller just set. +// +// Once that collision happens, any operation that paints "transparent" +// (for example fill_area's paint bucket, or fully-transparent pixels in an +// imported image) writes the sprite's designated transparent index, which is +// now indistinguishable from that last real color: reading it back reports +// the real color's opaque RGBA instead of transparency, and a pixel +// deliberately drawn in that color can be auto-trimmed away as if it were +// transparent. +// +// Call this immediately after any Palette:resize on an indexed sprite (and +// before spr:saveAs) to keep transparentColor safely out of the palette's +// real, addressable range. +func ReassertIndexedTransparentColor() string { + return `if spr.colorMode == ColorMode.INDEXED then + spr.transparentColor = 255 +end +` +} + +// ResolveExactPaletteColorHelper returns Lua code defining resolveExactPaletteColor, +// a color-resolution helper for img:putPixel and app.useTool calls made with +// usePalette=false (exact colors, no nearest-match snapping). +// +// On an indexed sprite, both img:putPixel and app.useTool expect a palette +// index, not an arbitrary RGBA value. Passing a Color object directly works +// when the sprite's palette was set earlier in the SAME Aseprite process, but +// pixel-mcp runs each tool call as its own aseprite --batch invocation: the +// palette is set in one process and the drawing happens in the next. In that +// cross-process scenario, Aseprite's own Color-to-index resolution for +// img:putPixel/app.useTool does not reliably match the sprite's actual, +// correctly-loaded palette (confirmed by reading spr.palettes[1] back at the +// point of failure) and silently resolves to an unrelated index instead, +// corrupting the drawn color with no error. +// +// resolveExactPaletteColor works around this by resolving the color itself: +// on an indexed sprite, a fully-transparent color (alpha 0, e.g. from a +// "paint transparent" fill_area call) always resolves to spr.transparentColor +// rather than a palette search, since transparency is a designated index, not +// a color that is expected to exist in the palette. Any other color looks up +// an EXACT match in the active palette and returns that index, erroring if +// none exists (callers should pass use_palette=true to snap to the nearest +// palette color instead). On a non-indexed sprite there is no palette to +// resolve against, so it returns the raw color unchanged, preserving existing +// RGB/grayscale behavior. +func ResolveExactPaletteColorHelper() string { + return ` +-- Helper: Resolve an exact color for img:putPixel/app.useTool (usePalette=false) +local function resolveExactPaletteColor(r, g, b, a) + local spr = app.activeSprite + if spr.colorMode ~= ColorMode.INDEXED then + return Color(r, g, b, a) + end + + if a == 0 then + return spr.transparentColor + end + + local palette = spr.palettes[1] + for i = 0, #palette - 1 do + local palColor = palette:getColor(i) + if palColor.red == r and palColor.green == g and palColor.blue == b and palColor.alpha == a then + return i + end + end + + error(string.format( + "Color #%02X%02X%02X%02X has no exact match in the sprite's palette; use use_palette=true to snap to the nearest palette color", + r, g, b, a)) +end +` +} + // FormatPoint formats a Point as a Lua Point constructor call. // // Returns a string like "Point(10, 20)" suitable for embedding in Lua scripts. diff --git a/pkg/aseprite/lua_drawing.go b/pkg/aseprite/lua_drawing.go index 8345010..5902e5d 100644 --- a/pkg/aseprite/lua_drawing.go +++ b/pkg/aseprite/lua_drawing.go @@ -30,11 +30,14 @@ func (g *LuaGenerator) DrawPixels(layerName string, frameNumber int, pixels []Pi escapedName := EscapeString(layerName) - // Add palette snapper helper if needed + // Add the color resolution helper: nearest-match snapping when requested, + // otherwise exact-match-or-error (see ResolveExactPaletteColorHelper). if usePalette { sb.WriteString(GeneratePaletteSnapperHelper()) - sb.WriteString("\n") + } else { + sb.WriteString(ResolveExactPaletteColorHelper()) } + sb.WriteString("\n") sb.WriteString(fmt.Sprintf(`local spr = app.activeSprite if not spr then @@ -61,8 +64,35 @@ end app.transaction(function() local cel = layer:cel(frame) - if not cel then - cel = spr:newCel(layer, frame) + if cel and cel.image then + -- Composite existing content into a full-canvas image at (0,0) so that + -- sprite coordinates match image coordinates for putPixel below. Without + -- this, Image:putPixel uses cel-local coordinates and pixels are offset + -- by the cel's position whenever content does not start at (0,0). + local full = Image(spr.width, spr.height, spr.colorMode) + -- A freshly created indexed Image is filled with index 0, but on these + -- sprites the transparent index is 255 (set by CreateCanvas). Clear the + -- new image to the sprite's transparent color first; otherwise the + -- default (NORMAL blend) drawImage below skips the source's transparent + -- pixels and leaves the opaque index-0 background showing through, + -- silently turning already-transparent pixels into opaque black. + if spr.colorMode == ColorMode.INDEXED then + full:clear(spr.transparentColor) + end + full:drawImage(cel.image, cel.position) + cel.image = full + cel.position = Point(0, 0) + else + -- No cel yet (or nil image, e.g. a freshly added layer): create a + -- full-canvas cel WITH an image so putPixel has a valid target. + local full = Image(spr.width, spr.height, spr.colorMode) + -- Same transparent-background fix as above: without clearing to the + -- transparent index, every untouched pixel of a new indexed cel would be + -- opaque index 0 instead of transparent. + if spr.colorMode == ColorMode.INDEXED then + full:clear(spr.transparentColor) + end + cel = spr:newCel(layer, frame, full, Point(0, 0)) end local img = cel.image @@ -106,11 +136,14 @@ print("Pixels drawn successfully")`) func (g *LuaGenerator) DrawLine(layerName string, frameNumber int, x1, y1, x2, y2 int, color Color, thickness int, usePalette bool) string { var sb strings.Builder - // Add palette snapper helper if needed + // Add the color resolution helper: nearest-match snapping when requested, + // otherwise exact-match-or-error (see ResolveExactPaletteColorHelper). if usePalette { sb.WriteString(GeneratePaletteSnapperHelper()) - sb.WriteString("\n") + } else { + sb.WriteString(ResolveExactPaletteColorHelper()) } + sb.WriteString("\n") escapedName := EscapeString(layerName) sb.WriteString(fmt.Sprintf(`local spr = app.activeSprite @@ -190,11 +223,14 @@ print("Line drawn successfully")`, func (g *LuaGenerator) DrawContour(layerName string, frameNumber int, points []Point, color Color, thickness int, closed bool, usePalette bool) string { var sb strings.Builder - // Add palette snapper helper if needed + // Add the color resolution helper: nearest-match snapping when requested, + // otherwise exact-match-or-error (see ResolveExactPaletteColorHelper). if usePalette { sb.WriteString(GeneratePaletteSnapperHelper()) - sb.WriteString("\n") + } else { + sb.WriteString(ResolveExactPaletteColorHelper()) } + sb.WriteString("\n") escapedName := EscapeString(layerName) sb.WriteString(fmt.Sprintf(`local spr = app.activeSprite @@ -295,11 +331,14 @@ print("Contour drawn successfully")`) func (g *LuaGenerator) DrawRectangle(layerName string, frameNumber int, x, y, width, height int, color Color, filled bool, usePalette bool) string { var sb strings.Builder - // Add palette snapper helper if needed + // Add the color resolution helper: nearest-match snapping when requested, + // otherwise exact-match-or-error (see ResolveExactPaletteColorHelper). if usePalette { sb.WriteString(GeneratePaletteSnapperHelper()) - sb.WriteString("\n") + } else { + sb.WriteString(ResolveExactPaletteColorHelper()) } + sb.WriteString("\n") escapedName := EscapeString(layerName) tool := "rectangle" @@ -379,11 +418,14 @@ print("Rectangle drawn successfully")`, func (g *LuaGenerator) DrawCircle(layerName string, frameNumber int, centerX, centerY, radius int, color Color, filled bool, usePalette bool) string { var sb strings.Builder - // Add palette snapper helper if needed + // Add the color resolution helper: nearest-match snapping when requested, + // otherwise exact-match-or-error (see ResolveExactPaletteColorHelper). if usePalette { sb.WriteString(GeneratePaletteSnapperHelper()) - sb.WriteString("\n") + } else { + sb.WriteString(ResolveExactPaletteColorHelper()) } + sb.WriteString("\n") escapedName := EscapeString(layerName) tool := "ellipse" @@ -469,11 +511,14 @@ print("Circle drawn successfully")`, func (g *LuaGenerator) FillArea(layerName string, frameNumber int, x, y int, color Color, tolerance int, usePalette bool) string { var sb strings.Builder - // Add palette snapper helper if needed + // Add the color resolution helper: nearest-match snapping when requested, + // otherwise exact-match-or-error (see ResolveExactPaletteColorHelper). if usePalette { sb.WriteString(GeneratePaletteSnapperHelper()) - sb.WriteString("\n") + } else { + sb.WriteString(ResolveExactPaletteColorHelper()) } + sb.WriteString("\n") escapedName := EscapeString(layerName) sb.WriteString(fmt.Sprintf(`local spr = app.activeSprite diff --git a/pkg/aseprite/lua_palette.go b/pkg/aseprite/lua_palette.go index 0d29f7a..5a7a93c 100644 --- a/pkg/aseprite/lua_palette.go +++ b/pkg/aseprite/lua_palette.go @@ -81,8 +81,9 @@ for i, color in ipairs(colors) do palette:setColor(i - 1, color) -- Palette is 0-indexed end +%s spr:saveAs(spr.filename) -print("Palette set successfully")`, len(colors), colorList) +print("Palette set successfully")`, len(colors), colorList, ReassertIndexedTransparentColor()) } // GetPalette generates a Lua script to retrieve the sprite's current palette. @@ -235,11 +236,12 @@ local newIndex = #palette palette:resize(#palette + 1) palette:setColor(newIndex, Color{r=%d, g=%d, b=%d, a=%d}) +%s spr:saveAs(spr.filename) -- Output JSON with color_index local output = string.format('{"color_index":%%d}', newIndex) -print(output)`, red, green, blue, alpha) +print(output)`, red, green, blue, alpha, ReassertIndexedTransparentColor()) } // SortPalette generates a Lua script to sort the palette by a specified method. diff --git a/pkg/aseprite/lua_palette_test.go b/pkg/aseprite/lua_palette_test.go index e6531a6..7db1576 100644 --- a/pkg/aseprite/lua_palette_test.go +++ b/pkg/aseprite/lua_palette_test.go @@ -8,7 +8,7 @@ import ( func TestFormatColorWithPalette_NoPalette(t *testing.T) { color := Color{R: 255, G: 128, B: 64, A: 255} result := FormatColorWithPalette(color, false) - expected := "Color(255, 128, 64, 255)" + expected := "resolveExactPaletteColor(255, 128, 64, 255)" if result != expected { t.Errorf("FormatColorWithPalette(usePalette=false) = %s, want %s", result, expected) } diff --git a/pkg/aseprite/lua_quantization.go b/pkg/aseprite/lua_quantization.go index b0898e4..53e0874 100644 --- a/pkg/aseprite/lua_quantization.go +++ b/pkg/aseprite/lua_quantization.go @@ -95,6 +95,7 @@ for i, color in ipairs(colors) do palette:setColor(i - 1, color) -- Palette is 0-indexed end %s +%s -- Prepare palette for JSON output local paletteHex = {} for i = 0, #palette - 1 do @@ -121,12 +122,13 @@ spr:saveAs(spr.filename) -- Print JSON result print(json)`, - len(palette), // palette resize - colorList, // color list - conversionCode, // conversion code - originalColors, // original_colors - len(palette), // quantized_colors - EscapeString(algorithm)) // algorithm_used + len(palette), // palette resize + colorList, // color list + conversionCode, // conversion code + ReassertIndexedTransparentColor(), // keep transparentColor out of the real palette range + originalColors, // original_colors + len(palette), // quantized_colors + EscapeString(algorithm)) // algorithm_used } // ReplaceWithImage generates a Lua script to replace sprite content with an external image. diff --git a/pkg/aseprite/lua_test.go b/pkg/aseprite/lua_test.go index 61d0db6..1c9f020 100644 --- a/pkg/aseprite/lua_test.go +++ b/pkg/aseprite/lua_test.go @@ -119,6 +119,31 @@ func TestLuaGenerator_CreateCanvas(t *testing.T) { if !strings.Contains(script, `print("test.aseprite")`) { t.Error("script missing print statement") } + + // RGB canvases have no transparent-index remapping, so no initial-cel clear. + if strings.Contains(script, "initialCel.image:clear") { + t.Error("RGB script should not clear the initial cel") + } +} + +// TestLuaGenerator_CreateCanvas_IndexedClearsInitialCel verifies that an indexed +// canvas clears its initial cel to the transparent color. +// +// A new indexed sprite's initial cel is filled with palette index 0, but index +// 255 is designated transparent, so index 0 is opaque. Without clearing the +// initial cel, a fresh indexed canvas starts with an opaque (black on DB32) +// background instead of a transparent one. Pure string check so it runs in CI +// without a real Aseprite install. +func TestLuaGenerator_CreateCanvas_IndexedClearsInitialCel(t *testing.T) { + gen := NewLuaGenerator() + script := gen.CreateCanvas(32, 32, ColorModeIndexed, "sprite.aseprite") + + if !strings.Contains(script, "spr.transparentColor = 255") { + t.Error("indexed script missing transparentColor assignment") + } + if !strings.Contains(script, "initialCel.image:clear(spr.transparentColor)") { + t.Error("indexed script does not clear the initial cel to the transparent color") + } } func TestLuaGenerator_AddLayer(t *testing.T) { @@ -159,15 +184,70 @@ func TestLuaGenerator_DrawPixels(t *testing.T) { t.Error("script missing layer iteration") } - if !strings.Contains(script, "img:putPixel(0, 0, Color(255, 0, 0, 255))") { + if !strings.Contains(script, "img:putPixel(0, 0, resolveExactPaletteColor(255, 0, 0, 255))") { t.Error("script missing first pixel") } - if !strings.Contains(script, "img:putPixel(1, 1, Color(0, 255, 0, 255))") { + if !strings.Contains(script, "img:putPixel(1, 1, resolveExactPaletteColor(0, 255, 0, 255))") { t.Error("script missing second pixel") } } +// TestLuaGenerator_DrawPixels_NormalizesCel verifies the generated script +// normalizes the target cel to a full-canvas image at (0,0) before writing. +// +// Image:putPixel uses cel-LOCAL coordinates, so the script must guarantee the +// cel covers the whole sprite at origin (0,0); otherwise pixels are offset by +// the cel's position whenever the layer's content does not start at (0,0) +// (e.g. after a shape tool trims the cel). This is a pure string check so it +// runs without Aseprite — guarding the fix in CI, where the integration tests +// (which require a real Aseprite install) do not run. +func TestLuaGenerator_DrawPixels_NormalizesCel(t *testing.T) { + gen := NewLuaGenerator() + pixels := []Pixel{{Point: Point{X: 3, Y: 4}, Color: Color{R: 1, G: 2, B: 3, A: 255}}} + script := gen.DrawPixels("Layer 1", 1, pixels, false) + + wants := []string{ + "Image(spr.width, spr.height, spr.colorMode)", // full-canvas image + "full:drawImage(cel.image, cel.position)", // preserve existing content + "cel.position = Point(0, 0)", // reposition existing cel to origin + "spr:newCel(layer, frame, full, Point(0, 0))", // new cel carries a full-canvas image + } + for _, w := range wants { + if !strings.Contains(script, w) { + t.Errorf("generated script missing %q; cel is not normalized to full-canvas at (0,0)", w) + } + } +} + +// TestLuaGenerator_DrawPixels_ClearsIndexedTransparentBackground verifies the +// generated script clears each freshly-created full-canvas image to the sprite's +// transparent color on indexed sprites. +// +// A new indexed Image is filled with index 0, but CreateCanvas designates index +// 255 as the transparent color, so an uncleared background is opaque. Without the +// clear, the default (NORMAL blend) drawImage skips the source's transparent +// pixels and leaves previously-transparent pixels as opaque index 0 -- silently +// turning transparent areas into opaque black. The clear must apply to both the +// composite branch (existing cel) and the new-cel branch. This is a pure string +// check so it runs without Aseprite, guarding the fix in CI where the integration +// tests do not run. +func TestLuaGenerator_DrawPixels_ClearsIndexedTransparentBackground(t *testing.T) { + gen := NewLuaGenerator() + pixels := []Pixel{{Point: Point{X: 0, Y: 0}, Color: Color{R: 1, G: 2, B: 3, A: 255}}} + script := gen.DrawPixels("Layer 1", 1, pixels, false) + + // Guarded on indexed mode so RGB/grayscale behavior is unchanged. + if !strings.Contains(script, "if spr.colorMode == ColorMode.INDEXED then") { + t.Error("generated script missing indexed-mode guard for the transparent-background clear") + } + // Clears to the sprite's transparent color (index 255), not the default index 0. + // Must apply to BOTH the composite branch and the new-cel branch. + if got := strings.Count(script, "full:clear(spr.transparentColor)"); got != 2 { + t.Errorf("expected the full-canvas image to be cleared to the transparent color in both cel branches, got %d occurrence(s)", got) + } +} + func TestLuaGenerator_ExportSprite(t *testing.T) { gen := NewLuaGenerator() diff --git a/pkg/aseprite/quantization.go b/pkg/aseprite/quantization.go index bba435f..b3b601d 100644 --- a/pkg/aseprite/quantization.go +++ b/pkg/aseprite/quantization.go @@ -110,14 +110,27 @@ func MedianCutQuantization(pixels []color.Color, targetColors int) []color.Color return nil } + // Reduce to a frequency histogram of unique colors before bucketing. A + // raw, duplicate-laden pixel list biases every split toward whatever + // large solid-fill color dominates the pixel count: cutting at the + // pixel-COUNT median of a mostly-one-color bucket lands the split point + // inside that same color's run (not at a boundary between colors), so + // the algorithm keeps re-subdividing the dominant fill while small + // accent regions (a 2px highlight next to a 200px fill, typical of + // pixel art) never get isolated into their own bucket and end up + // averaged into a blended, off-palette color instead. Splitting the + // unique-color histogram instead guarantees the split always falls + // between distinct colors, so targetColors >= the number of unique + // colors recovers every one of them exactly. + entries := buildColorHistogram(pixels) + // Ensure targetColors doesn't exceed number of unique colors - uniqueColors := countUniqueColorsInSlice(pixels) - if targetColors > uniqueColors { - targetColors = uniqueColors + if targetColors > len(entries) { + targetColors = len(entries) } - // Create initial bucket with all pixels - buckets := []colorBucket{{pixels: pixels}} + // Create initial bucket with all unique colors + buckets := []colorBucket{{entries: entries}} // Recursively split buckets until we have targetColors for len(buckets) < targetColors { @@ -132,7 +145,7 @@ func MedianCutQuantization(pixels []color.Color, targetColors int) []color.Color } } - // Stop if no bucket has any range (all colors are identical) + // Stop if no bucket has any range (all remaining buckets are single colors) if maxRange == 0 { break } @@ -154,22 +167,48 @@ func MedianCutQuantization(pixels []color.Color, targetColors int) []color.Color return palette } -// colorBucket represents a bucket of colors for median cut algorithm. +// colorCount pairs a unique color with the number of pixels that had it. +type colorCount struct { + c color.Color + count int +} + +// buildColorHistogram reduces a (possibly duplicate-laden) pixel slice to a +// list of unique colors with their pixel counts, preserving first-seen order. +func buildColorHistogram(pixels []color.Color) []colorCount { + index := make(map[uint32]int, len(pixels)) + entries := make([]colorCount, 0, len(pixels)) + + for _, p := range pixels { + r, g, b, _ := p.RGBA() + key := (r&0xFF00)<<8 | (g & 0xFF00) | (b&0xFF00)>>8 + if i, ok := index[key]; ok { + entries[i].count++ + continue + } + index[key] = len(entries) + entries = append(entries, colorCount{c: p, count: 1}) + } + + return entries +} + +// colorBucket represents a bucket of unique colors (with pixel counts) for the median cut algorithm. type colorBucket struct { - pixels []color.Color + entries []colorCount } // colorRange calculates the range of the bucket in RGB space. func (b *colorBucket) colorRange() float64 { - if len(b.pixels) == 0 { + if len(b.entries) == 0 { return 0 } var minR, minG, minB uint32 = 65535, 65535, 65535 var maxR, maxG, maxB uint32 = 0, 0, 0 - for _, p := range b.pixels { - r, g, bl, _ := p.RGBA() + for _, e := range b.entries { + r, g, bl, _ := e.c.RGBA() if r < minR { minR = r } @@ -197,9 +236,15 @@ func (b *colorBucket) colorRange() float64 { return rRange + gRange + bRange } -// split splits the bucket along the dimension with the largest range. +// split splits the bucket along the dimension with the largest range. The +// split point is chosen where cumulative pixel count crosses half of the +// bucket's total pixel mass (so common colors still get split priority over +// rare ones, matching standard median-cut behavior), but since entries are +// already unique colors, the split always falls BETWEEN distinct colors - +// never through the middle of one - so a bucket of already-few unique colors +// is never blended together by a split. func (b *colorBucket) split() (colorBucket, colorBucket) { - if len(b.pixels) < 2 { + if len(b.entries) < 2 { return *b, colorBucket{} } @@ -207,8 +252,8 @@ func (b *colorBucket) split() (colorBucket, colorBucket) { var minR, minG, minB uint32 = 65535, 65535, 65535 var maxR, maxG, maxB uint32 = 0, 0, 0 - for _, p := range b.pixels { - r, g, bl, _ := p.RGBA() + for _, e := range b.entries { + r, g, bl, _ := e.c.RGBA() if r < minR { minR = r } @@ -233,57 +278,82 @@ func (b *colorBucket) split() (colorBucket, colorBucket) { gRange := maxG - minG bRange := maxB - minB - // Sort pixels by the dimension with largest range - pixels := make([]color.Color, len(b.pixels)) - copy(pixels, b.pixels) + // Sort unique colors by the dimension with largest range + entries := make([]colorCount, len(b.entries)) + copy(entries, b.entries) if rRange >= gRange && rRange >= bRange { // Sort by red - sort.Slice(pixels, func(i, j int) bool { - r1, _, _, _ := pixels[i].RGBA() - r2, _, _, _ := pixels[j].RGBA() + sort.Slice(entries, func(i, j int) bool { + r1, _, _, _ := entries[i].c.RGBA() + r2, _, _, _ := entries[j].c.RGBA() return r1 < r2 }) } else if gRange >= bRange { // Sort by green - sort.Slice(pixels, func(i, j int) bool { - _, g1, _, _ := pixels[i].RGBA() - _, g2, _, _ := pixels[j].RGBA() + sort.Slice(entries, func(i, j int) bool { + _, g1, _, _ := entries[i].c.RGBA() + _, g2, _, _ := entries[j].c.RGBA() return g1 < g2 }) } else { // Sort by blue - sort.Slice(pixels, func(i, j int) bool { - _, _, b1, _ := pixels[i].RGBA() - _, _, b2, _ := pixels[j].RGBA() + sort.Slice(entries, func(i, j int) bool { + _, _, b1, _ := entries[i].c.RGBA() + _, _, b2, _ := entries[j].c.RGBA() return b1 < b2 }) } - // Split at median - mid := len(pixels) / 2 - return colorBucket{pixels: pixels[:mid]}, colorBucket{pixels: pixels[mid:]} + // Split where cumulative pixel count crosses half the bucket's total + // mass, clamped so both sides always get at least one unique color. + total := 0 + for _, e := range entries { + total += e.count + } + half := total / 2 + + cum := 0 + splitIdx := 1 + for i, e := range entries { + cum += e.count + if cum >= half { + splitIdx = i + 1 + break + } + } + if splitIdx < 1 { + splitIdx = 1 + } + if splitIdx > len(entries)-1 { + splitIdx = len(entries) - 1 + } + + return colorBucket{entries: entries[:splitIdx]}, colorBucket{entries: entries[splitIdx:]} } -// average calculates the average color of the bucket. +// average calculates the pixel-count-weighted average color of the bucket. +// A single-entry bucket returns that entry's exact color unchanged. func (b *colorBucket) average() color.Color { - if len(b.pixels) == 0 { + if len(b.entries) == 0 { return color.RGBA{R: 0, G: 0, B: 0, A: 255} } var sumR, sumG, sumB uint64 - for _, p := range b.pixels { - r, g, bl, _ := p.RGBA() - sumR += uint64(r) - sumG += uint64(g) - sumB += uint64(bl) + var totalCount uint64 + for _, e := range b.entries { + r, g, bl, _ := e.c.RGBA() + w := uint64(e.count) + sumR += uint64(r) * w + sumG += uint64(g) * w + sumB += uint64(bl) * w + totalCount += w } - count := uint64(len(b.pixels)) return color.RGBA{ - R: uint8((sumR / count) >> 8), - G: uint8((sumG / count) >> 8), - B: uint8((sumB / count) >> 8), + R: uint8((sumR / totalCount) >> 8), + G: uint8((sumG / totalCount) >> 8), + B: uint8((sumB / totalCount) >> 8), A: 255, } } @@ -597,16 +667,3 @@ func CountUniqueColors(img image.Image, preserveTransparency bool) int { return len(colorSet) } - -// countUniqueColorsInSlice counts unique colors in a slice of colors. -func countUniqueColorsInSlice(pixels []color.Color) int { - colorSet := make(map[uint32]bool) - - for _, p := range pixels { - r, g, b, _ := p.RGBA() - packed := (r&0xFF00)<<8 | (g & 0xFF00) | (b&0xFF00)>>8 - colorSet[packed] = true - } - - return len(colorSet) -} diff --git a/pkg/aseprite/quantization_test.go b/pkg/aseprite/quantization_test.go index c790b75..8ac6696 100644 --- a/pkg/aseprite/quantization_test.go +++ b/pkg/aseprite/quantization_test.go @@ -55,6 +55,66 @@ func TestMedianCutQuantization(t *testing.T) { } } +// TestMedianCutQuantization_ImbalancedFrequencyPreservesExactColors reproduces +// the split-by-pixel-count bug: a bucket containing a large solid-fill color +// alongside a handful of rare accent colors used to get bisected at the +// pixel-count median, landing the split point inside the dominant color's own +// run instead of at a boundary between colors. That blended rare accent +// colors into off-palette hex values and could even drop them from the +// palette entirely, even though targetColors comfortably exceeded the number +// of unique colors present (which should guarantee lossless recovery). +func TestMedianCutQuantization_ImbalancedFrequencyPreservesExactColors(t *testing.T) { + // Mirrors a typical pixel-art color distribution: a couple of large + // fills (many samples) plus several tiny accent regions (1-2 samples). + pixels := make([]color.Color, 0, 320) + appendN := func(c color.Color, n int) { + for i := 0; i < n; i++ { + pixels = append(pixels, c) + } + } + + tunicGreen := color.RGBA{R: 0x6A, G: 0xBE, B: 0x30, A: 255} + skin := color.RGBA{R: 0xD9, G: 0xA0, B: 0x66, A: 255} + outline := color.RGBA{R: 0x22, G: 0x20, B: 0x34, A: 255} + highlight := color.RGBA{R: 0x99, G: 0xE5, B: 0x50, A: 255} + buckle := color.RGBA{R: 0xFB, G: 0xF2, B: 0x36, A: 255} + + appendN(tunicGreen, 200) + appendN(skin, 90) + appendN(outline, 20) + appendN(highlight, 2) + appendN(buckle, 2) + + want := map[color.RGBA]bool{ + tunicGreen: true, skin: true, outline: true, highlight: true, buckle: true, + } + + // Target far exceeds the 5 unique colors present, so every one of them + // must come back exactly - no blending, no duplicates, no drops. + palette := MedianCutQuantization(pixels, 32) + + if len(palette) != len(want) { + t.Fatalf("MedianCutQuantization() returned %d colors, want %d (one bucket per unique input color): %v", + len(palette), len(want), palette) + } + + seen := make(map[color.RGBA]bool, len(palette)) + for _, c := range palette { + rgba, ok := c.(color.RGBA) + if !ok { + r, g, b, a := c.RGBA() + rgba = color.RGBA{R: uint8(r >> 8), G: uint8(g >> 8), B: uint8(b >> 8), A: uint8(a >> 8)} + } + if !want[rgba] { + t.Errorf("palette contains %+v, which is not one of the 5 exact input colors (blended/off-palette result)", rgba) + } + if seen[rgba] { + t.Errorf("palette contains duplicate entry %+v (a unique color's bucket was split into two near-identical averages)", rgba) + } + seen[rgba] = true + } +} + func TestOctreeQuantization(t *testing.T) { tests := []struct { name string diff --git a/pkg/tools/drawing_pixels_after_shape_bug_test.go b/pkg/tools/drawing_pixels_after_shape_bug_test.go new file mode 100644 index 0000000..2ba2c78 --- /dev/null +++ b/pkg/tools/drawing_pixels_after_shape_bug_test.go @@ -0,0 +1,70 @@ +//go:build integration + +package tools + +import ( + "context" + "testing" + "time" + + "github.com/willibrandon/pixel-mcp/internal/testutil" + "github.com/willibrandon/pixel-mcp/pkg/aseprite" +) + +// TestIntegration_DrawPixels_AfterShape_CelOffset reproduces the bug where +// draw_pixels places pixels at the wrong coordinates once a shape tool has +// caused the layer's cel to be trimmed to a non-(0,0) origin. +// +// A filled circle drawn in the middle of the canvas leaves the cel positioned +// at the circle's bounding-box top-left (not the sprite origin). DrawPixels then +// wrote with cel-LOCAL coordinates via Image:putPixel, so a pixel requested at +// sprite (16,16) actually landed at (16 + cel.x, 16 + cel.y). This test asserts +// the pixel ends up exactly where it was requested. +func TestIntegration_DrawPixels_AfterShape_CelOffset(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + spritePath := testutil.TempSpritePath(t, "test-pixels-after-shape.aseprite") + createScript := gen.CreateCanvas(32, 32, aseprite.ColorModeRGB, spritePath) + if _, err := client.ExecuteLua(ctx, createScript, ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + + // Draw a filled circle in the center. After save, Aseprite trims the cel to + // this content, so the cel origin becomes non-zero (≈(2,2) for r14 on 32px). + circleColor := aseprite.Color{R: 0, G: 170, B: 255, A: 255} // #00AAFF + circleScript := gen.DrawCircle("Layer 1", 1, 16, 16, 14, circleColor, true, false) + if _, err := client.ExecuteLua(ctx, circleScript, spritePath); err != nil { + t.Fatalf("Failed to draw circle: %v", err) + } + + // Draw a single distinct pixel at a known sprite coordinate inside the circle. + dotColor := aseprite.Color{R: 255, G: 0, B: 0, A: 255} // #FF0000 + pixels := []aseprite.Pixel{ + {Point: aseprite.Point{X: 16, Y: 16}, Color: dotColor}, + } + drawScript := gen.DrawPixels("Layer 1", 1, pixels, false) + if _, err := client.ExecuteLua(ctx, drawScript, spritePath); err != nil { + t.Fatalf("Failed to draw pixel: %v", err) + } + + // The pixel must be exactly at sprite (16,16), not offset by the cel origin. + getScript := gen.GetPixels("Layer 1", 1, 16, 16, 1, 1) + output, err := client.ExecuteLua(ctx, getScript, spritePath) + if err != nil { + t.Fatalf("Failed to get pixels: %v", err) + } + got, err := testutil.ParsePixelData(output) + if err != nil { + t.Fatalf("Failed to parse pixel data: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 pixel for region (16,16,1,1), got %d", len(got)) + } + if got[0].Color != "#FF0000FF" { + t.Errorf("BUG CONFIRMED: pixel at sprite (16,16) = %s, want #FF0000FF "+ + "(draw_pixels offset by the cel origin)", got[0].Color) + } +} diff --git a/pkg/tools/drawing_pixels_outside_shape_bug_test.go b/pkg/tools/drawing_pixels_outside_shape_bug_test.go new file mode 100644 index 0000000..5b6119b --- /dev/null +++ b/pkg/tools/drawing_pixels_outside_shape_bug_test.go @@ -0,0 +1,73 @@ +//go:build integration + +package tools + +import ( + "context" + "testing" + "time" + + "github.com/willibrandon/pixel-mcp/internal/testutil" + "github.com/willibrandon/pixel-mcp/pkg/aseprite" +) + +// TestIntegration_DrawPixels_AfterShape_OutsideShapeBounds verifies that +// draw_pixels can place a pixel anywhere on the sprite after a shape tool has +// trimmed the cel to a non-(0,0), ASYMMETRIC origin — including a coordinate +// OUTSIDE the shape's bounding box. +// +// This covers two facets the simple offset test does not: +// - asymmetric origin (x != y), proving both axes are handled independently +// - a pixel outside the trimmed cel, which the stock code dropped entirely +// (writing to cel-local coords that map elsewhere, leaving the requested +// coordinate transparent) +func TestIntegration_DrawPixels_AfterShape_OutsideShapeBounds(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + spritePath := testutil.TempSpritePath(t, "test-pixels-outside-shape.aseprite") + createScript := gen.CreateCanvas(32, 32, aseprite.ColorModeRGB, spritePath) + if _, err := client.ExecuteLua(ctx, createScript, ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + + // Asymmetric, inset rectangle -> cel origin becomes ≈(10,6) after trimming, + // so the x and y offsets differ. + rect := aseprite.Color{R: 0, G: 170, B: 255, A: 255} // #00AAFF + rectScript := gen.DrawRectangle("Layer 1", 1, 10, 6, 8, 8, rect, true, false) + if _, err := client.ExecuteLua(ctx, rectScript, spritePath); err != nil { + t.Fatalf("Failed to draw rectangle: %v", err) + } + + // One pixel inside the rect, one OUTSIDE it near the top-left corner. + red := aseprite.Color{R: 255, G: 0, B: 0, A: 255} // #FF0000 inside + green := aseprite.Color{R: 0, G: 255, B: 0, A: 255} // #00FF00 outside + pixels := []aseprite.Pixel{ + {Point: aseprite.Point{X: 12, Y: 8}, Color: red}, + {Point: aseprite.Point{X: 1, Y: 1}, Color: green}, + } + drawScript := gen.DrawPixels("Layer 1", 1, pixels, false) + if _, err := client.ExecuteLua(ctx, drawScript, spritePath); err != nil { + t.Fatalf("Failed to draw pixels: %v", err) + } + + assertPixel := func(x, y int, want string) { + t.Helper() + out, err := client.ExecuteLua(ctx, gen.GetPixels("Layer 1", 1, x, y, 1, 1), spritePath) + if err != nil { + t.Fatalf("get_pixels(%d,%d): %v", x, y, err) + } + got, err := testutil.ParsePixelData(out) + if err != nil { + t.Fatalf("parse (%d,%d): %v", x, y, err) + } + if len(got) != 1 || got[0].Color != want { + t.Errorf("BUG CONFIRMED: pixel (%d,%d) = %v, want %s", x, y, got, want) + } + } + + assertPixel(12, 8, "#FF0000FF") // inside the shape: placed at the right spot + assertPixel(1, 1, "#00FF00FF") // outside the shape: present, not lost +} diff --git a/pkg/tools/drawing_pixels_transparent_background_bug_test.go b/pkg/tools/drawing_pixels_transparent_background_bug_test.go new file mode 100644 index 0000000..67f9443 --- /dev/null +++ b/pkg/tools/drawing_pixels_transparent_background_bug_test.go @@ -0,0 +1,190 @@ +//go:build integration + +package tools + +import ( + "context" + "testing" + "time" + + "github.com/willibrandon/pixel-mcp/internal/testutil" + "github.com/willibrandon/pixel-mcp/pkg/aseprite" +) + +// TestIntegration_DrawPixels_TransparentBackground_Bug reproduces the bug where a +// later draw_pixels call silently corrupts pixels that an earlier call had +// explicitly made transparent, flipping them to opaque palette-index-0 (e.g. +// opaque black on a DawnBringer 32 sprite). +// +// BUG SCENARIO (reported while drawing DB32 sprites across multiple calls): +// 1. On an indexed sprite CreateCanvas sets transparentColor = 255, so the +// transparent index is 255 and index 0 is a real, opaque palette color. +// 2. A draw_pixels call explicitly writes transparent pixels (alpha 0, which +// resolves to index 255) at some coordinates. Those read back as fully +// transparent. +// 3. A SUBSEQUENT draw_pixels call normalizes the cel to a full-canvas image via +// `Image(spr.width, spr.height, spr.colorMode)`, which is filled with index 0 +// (opaque), NOT the transparent index. With the default NORMAL blend, +// `full:drawImage(cel.image, cel.position)` skips the source's transparent +// (index 255) pixels, leaving the opaque index-0 background showing through. +// 4. RESULT: the previously-transparent pixels the second call did not rewrite +// flip from transparent (#00000000) to opaque black (#000000FF). +// +// The fix clears the full-canvas image to the sprite's transparent color before +// compositing, so index-255 pixels survive the round-trip. +// +// Palette index 0 is opaque black (#000000FF) so corruption is unambiguous: a +// truly transparent pixel reads back as #00000000 (index 255 is outside the +// 3-color palette's addressable range), a corrupted pixel reads back as +// #000000FF. +func TestIntegration_DrawPixels_TransparentBackground_Bug(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + spritePath := testutil.TempSpritePath(t, "test-pixels-transparent-bg.aseprite") + createScript := gen.CreateCanvas(8, 8, aseprite.ColorModeIndexed, spritePath) + if _, err := client.ExecuteLua(ctx, createScript, ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + + setPaletteScript := gen.SetPalette([]string{ + "#000000FF", // Index 0: opaque BLACK (what corruption collapses to) + "#FF0000FF", // Index 1: RED + "#00FF00FF", // Index 2: GREEN + }) + if _, err := client.ExecuteLua(ctx, setPaletteScript, spritePath); err != nil { + t.Fatalf("Failed to set palette: %v", err) + } + + const transparent = "#00000000" + + colorMap := func(t *testing.T) map[string]string { + t.Helper() + output, err := client.ExecuteLua(ctx, gen.GetPixels("Layer 1", 1, 0, 0, 8, 8), spritePath) + if err != nil { + t.Fatalf("Failed to get pixels: %v", err) + } + pixels, err := testutil.ParsePixelData(output) + if err != nil { + t.Fatalf("Failed to parse pixel data: %v", err) + } + got := make(map[string]string, len(pixels)) + for _, p := range pixels { + got[testutil.FormatPixelPos(p.X, p.Y)] = p.Color + } + return got + } + + // Pixels this test explicitly makes transparent in the first call. They must + // remain transparent after the second call. + transparentPixels := []aseprite.Point{{X: 2, Y: 2}, {X: 3, Y: 3}, {X: 4, Y: 4}, {X: 6, Y: 6}} + + // First call: a red marker plus an explicitly-transparent set. + firstBatch := []aseprite.Pixel{ + {Point: aseprite.Point{X: 0, Y: 0}, Color: aseprite.Color{R: 255, G: 0, B: 0, A: 255}}, + } + for _, p := range transparentPixels { + firstBatch = append(firstBatch, aseprite.Pixel{Point: p, Color: aseprite.Color{R: 0, G: 0, B: 0, A: 0}}) + } + if _, err := client.ExecuteLua(ctx, gen.DrawPixels("Layer 1", 1, firstBatch, false), spritePath); err != nil { + t.Fatalf("Failed to draw first batch: %v", err) + } + + // Sanity: the explicitly-transparent pixels are transparent after call 1. + got := colorMap(t) + for _, p := range transparentPixels { + pos := testutil.FormatPixelPos(p.X, p.Y) + if got[pos] != transparent { + t.Fatalf("setup failed: pixel (%d,%d) = %s after first draw, want %s", p.X, p.Y, got[pos], transparent) + } + } + + // Second call: an unrelated edit elsewhere (opaque green) plus a transparent + // write (the exact scenario from the report). This must NOT disturb the + // already-transparent pixels from the first call. + secondBatch := []aseprite.Pixel{ + {Point: aseprite.Point{X: 7, Y: 7}, Color: aseprite.Color{R: 0, G: 255, B: 0, A: 255}}, // opaque green + {Point: aseprite.Point{X: 1, Y: 1}, Color: aseprite.Color{R: 0, G: 0, B: 0, A: 0}}, // transparent + } + if _, err := client.ExecuteLua(ctx, gen.DrawPixels("Layer 1", 1, secondBatch, false), spritePath); err != nil { + t.Fatalf("Failed to draw second batch: %v", err) + } + + got = colorMap(t) + + // The reported bug: previously-transparent pixels corrupted to opaque black. + corrupted := 0 + for _, p := range transparentPixels { + pos := testutil.FormatPixelPos(p.X, p.Y) + if got[pos] != transparent { + if got[pos] == "#000000FF" { + corrupted++ + } + t.Errorf("pixel (%d,%d) = %s after second draw, want %s (transparent)", p.X, p.Y, got[pos], transparent) + } + } + if corrupted > 0 { + t.Logf("BUG CONFIRMED: %d previously-transparent pixels were corrupted to opaque black (#000000FF)", corrupted) + } + + // The foreground pixels must be intact. + if got["0,0"] != "#FF0000FF" { + t.Errorf("red marker (0,0) = %s, want #FF0000FF", got["0,0"]) + } + if got["7,7"] != "#00FF00FF" { + t.Errorf("green marker (7,7) = %s, want #00FF00FF", got["7,7"]) + } +} + +// TestIntegration_CreateCanvas_IndexedBackgroundTransparent verifies that a +// freshly created indexed canvas starts fully transparent rather than opaque +// palette-index-0. +// +// A new indexed sprite's initial cel is filled with index 0. Because CreateCanvas +// designates index 255 as transparent, index 0 is an opaque color, so without +// clearing the initial cel the whole canvas reads back as opaque black +// (#000000FF) instead of transparent (#00000000). +func TestIntegration_CreateCanvas_IndexedBackgroundTransparent(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + spritePath := testutil.TempSpritePath(t, "test-fresh-indexed-transparent.aseprite") + if _, err := client.ExecuteLua(ctx, gen.CreateCanvas(4, 4, aseprite.ColorModeIndexed, spritePath), ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + + // Index 0 = opaque black; a corrupted (uncleared) background collapses to it. + setPaletteScript := gen.SetPalette([]string{"#000000FF", "#FF0000FF"}) + if _, err := client.ExecuteLua(ctx, setPaletteScript, spritePath); err != nil { + t.Fatalf("Failed to set palette: %v", err) + } + + output, err := client.ExecuteLua(ctx, gen.GetPixels("Layer 1", 1, 0, 0, 4, 4), spritePath) + if err != nil { + t.Fatalf("Failed to get pixels: %v", err) + } + pixels, err := testutil.ParsePixelData(output) + if err != nil { + t.Fatalf("Failed to parse pixel data: %v", err) + } + if len(pixels) != 16 { + t.Fatalf("Expected 16 pixels for a 4x4 canvas, got %d", len(pixels)) + } + + opaqueBlack := 0 + for _, p := range pixels { + if p.Color != "#00000000" { + if p.Color == "#000000FF" { + opaqueBlack++ + } + t.Errorf("fresh-canvas pixel (%d,%d) = %s, want #00000000 (transparent)", p.X, p.Y, p.Color) + } + } + if opaqueBlack > 0 { + t.Logf("BUG CONFIRMED: %d/16 fresh-canvas pixels are opaque black (#000000FF) instead of transparent", opaqueBlack) + } +} diff --git a/pkg/tools/exact_palette_color_indexed_mode_bug_test.go b/pkg/tools/exact_palette_color_indexed_mode_bug_test.go new file mode 100644 index 0000000..2e965f3 --- /dev/null +++ b/pkg/tools/exact_palette_color_indexed_mode_bug_test.go @@ -0,0 +1,111 @@ +//go:build integration +// +build integration + +package tools + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/willibrandon/pixel-mcp/internal/testutil" + "github.com/willibrandon/pixel-mcp/pkg/aseprite" +) + +// TestIntegration_IndexedMode_ExactColorResolution tests that drawing an exact +// palette color (usePalette=false) on an indexed sprite resolves to the right +// palette index, even though the palette was set in a PREVIOUS aseprite +// process (pixel-mcp runs one process per tool call, so this is the normal +// case, not an edge case). +// +// ROOT CAUSE: on an indexed sprite, img:putPixel and app.useTool both expect +// a palette index. Given a raw Color object instead, Aseprite resolves it to +// an index using an internal color cache that is only correctly populated +// when the palette was set earlier in the SAME process. When the palette was +// set in an earlier process and the sprite is reopened fresh (exactly how +// every pixel-mcp tool call works), that resolution silently returns an +// unrelated index, even though spr.palettes[1] reads back the correct, +// expected palette when inspected directly. The result reads back as the +// wrong color, or as transparent if the resolved index happens to be out of +// the palette's range. +// +// FIX: resolveExactPaletteColor (pkg/aseprite/lua_core.go) looks up the exact +// index itself by reading the sprite's actual active palette at Lua runtime, +// bypassing Aseprite's unreliable internal resolution entirely. +func TestIntegration_IndexedMode_ExactColorResolution(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + // 4-color palette, set in its own process (as create_canvas + set_palette + // always are via the real MCP tools). + newIndexedSprite := func(t *testing.T, name string) string { + t.Helper() + spritePath := testutil.TempSpritePath(t, name) + createScript := gen.CreateCanvas(32, 32, aseprite.ColorModeIndexed, spritePath) + if _, err := client.ExecuteLua(ctx, createScript, ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + setPaletteScript := gen.SetPalette([]string{ + "#FF0000FF", // 0: RED + "#00FF00FF", // 1: GREEN + "#0000FFFF", // 2: BLUE + "#FFFF00FF", // 3: YELLOW + }) + if _, err := client.ExecuteLua(ctx, setPaletteScript, spritePath); err != nil { + t.Fatalf("Failed to set palette: %v", err) + } + return spritePath + } + + assertPixel := func(t *testing.T, spritePath string, x, y int, want string) { + t.Helper() + out, err := client.ExecuteLua(ctx, gen.GetPixels("Layer 1", 1, x, y, 1, 1), spritePath) + if err != nil { + t.Fatalf("get_pixels(%d,%d): %v", x, y, err) + } + got, err := testutil.ParsePixelData(out) + if err != nil { + t.Fatalf("parse (%d,%d): %v", x, y, err) + } + if len(got) != 1 || got[0].Color != want { + t.Errorf("BUG CONFIRMED: pixel (%d,%d) = %v, want %s", x, y, got, want) + } + } + + t.Run("DrawRectangle_ExactMatch", func(t *testing.T) { + spritePath := newIndexedSprite(t, "test-exact-color-rect.aseprite") + green := aseprite.Color{R: 0, G: 255, B: 0, A: 255} + rectScript := gen.DrawRectangle("Layer 1", 1, 0, 0, 4, 4, green, true, false) + if _, err := client.ExecuteLua(ctx, rectScript, spritePath); err != nil { + t.Fatalf("Failed to draw rectangle: %v", err) + } + assertPixel(t, spritePath, 1, 1, "#00FF00FF") + }) + + t.Run("DrawPixels_ExactMatch", func(t *testing.T) { + spritePath := newIndexedSprite(t, "test-exact-color-pixels.aseprite") + blue := aseprite.Color{R: 0, G: 0, B: 255, A: 255} + pixels := []aseprite.Pixel{{Point: aseprite.Point{X: 5, Y: 5}, Color: blue}} + drawScript := gen.DrawPixels("Layer 1", 1, pixels, false) + if _, err := client.ExecuteLua(ctx, drawScript, spritePath); err != nil { + t.Fatalf("Failed to draw pixels: %v", err) + } + assertPixel(t, spritePath, 5, 5, "#0000FFFF") + }) + + t.Run("NoExactMatch_Errors", func(t *testing.T) { + spritePath := newIndexedSprite(t, "test-exact-color-no-match.aseprite") + purple := aseprite.Color{R: 128, G: 0, B: 128, A: 255} // not in the 4-color palette + rectScript := gen.DrawRectangle("Layer 1", 1, 0, 0, 4, 4, purple, true, false) + _, err := client.ExecuteLua(ctx, rectScript, spritePath) + if err == nil { + t.Fatal("expected an error for a color with no exact palette match, got nil") + } + if !strings.Contains(err.Error(), "has no exact match in the sprite's palette") { + t.Errorf("expected a no-exact-match error, got: %v", err) + } + }) +} diff --git a/pkg/tools/palette_integration_test.go b/pkg/tools/palette_integration_test.go index f6c1c52..f7177b6 100644 --- a/pkg/tools/palette_integration_test.go +++ b/pkg/tools/palette_integration_test.go @@ -238,8 +238,9 @@ func TestIntegration_SetPalette_ThenApplyShading(t *testing.T) { t.Fatalf("Failed to set palette: %v", err) } - // Step 2: Draw a circle with mid-tone - circleScript := gen.DrawCircle("Layer 1", 1, 32, 32, 25, aseprite.Color{R: 195, G: 87, B: 54, A: 255}, true, false) + // Step 2: Draw a circle with an approximate mid-tone, snapped to the locked + // palette (this color is not one of the 16 exact palette entries). + circleScript := gen.DrawCircle("Layer 1", 1, 32, 32, 25, aseprite.Color{R: 195, G: 87, B: 54, A: 255}, true, true) _, err = client.ExecuteLua(ctx, circleScript, spritePath) if err != nil { t.Fatalf("Failed to draw circle: %v", err) diff --git a/pkg/tools/palette_transparent_color_collision_bug_test.go b/pkg/tools/palette_transparent_color_collision_bug_test.go new file mode 100644 index 0000000..39c93ae --- /dev/null +++ b/pkg/tools/palette_transparent_color_collision_bug_test.go @@ -0,0 +1,108 @@ +//go:build integration +// +build integration + +package tools + +import ( + "context" + "testing" + "time" + + "github.com/willibrandon/pixel-mcp/internal/testutil" + "github.com/willibrandon/pixel-mcp/pkg/aseprite" +) + +// TestIntegration_SetPalette_TransparentColorCollisionBug tests the bug where +// SetPalette's Palette:resize call causes Aseprite to silently clamp +// Sprite.transparentColor down to the new last valid index (paletteSize-1), +// colliding it with the last real color the caller just set. +// +// CreateCanvas sets Sprite.transparentColor to 255 for indexed sprites so +// that palette index 0 can hold a real color instead of being treated as +// transparent (see the palette index 0 fix). Palette:resize(N) does not +// preserve that: Aseprite clamps transparentColor into [0, N-1], so after +// SetPalette with a locked N-color palette, transparentColor becomes N-1. +// +// EXACT REPRODUCTION: +// 1. Create an indexed sprite and set a 4-color palette: RED, GREEN, BLUE, +// YELLOW. YELLOW is palette index 3, the palette's last color. +// 2. Draw a YELLOW rectangle (using the last palette color). +// 3. Fill an unrelated background region with a fully transparent color via +// fill_area, exactly like drawing a locked-palette sprite's background. +// 4. Read back both regions with get_pixels. +// +// BUG: because transparentColor collided with YELLOW's index, the "transparent" +// fill paints pixels that read back as opaque YELLOW instead of transparent, +// and/or the intentionally-drawn YELLOW rectangle gets treated as transparent. +// Either way, YELLOW (the palette's last real color) becomes indistinguishable +// from "no color here." +func TestIntegration_SetPalette_TransparentColorCollisionBug(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + spritePath := testutil.TempSpritePath(t, "test-transparent-color-collision.aseprite") + createScript := gen.CreateCanvas(32, 32, aseprite.ColorModeIndexed, spritePath) + if _, err := client.ExecuteLua(ctx, createScript, ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + + // 4-color palette; YELLOW (index 3) is the last real color. + setPaletteScript := gen.SetPalette([]string{ + "#FF0000FF", // Index 0: RED + "#00FF00FF", // Index 1: GREEN + "#0000FFFF", // Index 2: BLUE + "#FFFF00FF", // Index 3: YELLOW (last color -> historically collided with transparentColor) + }) + if _, err := client.ExecuteLua(ctx, setPaletteScript, spritePath); err != nil { + t.Fatalf("Failed to set palette: %v", err) + } + + // Draw a YELLOW rectangle at (0,0)-(3,3), using the palette's last color. + drawYellowScript := gen.DrawRectangle("Layer 1", 1, 0, 0, 4, 4, + aseprite.Color{R: 255, G: 255, B: 0, A: 255}, true, true) + if _, err := client.ExecuteLua(ctx, drawYellowScript, spritePath); err != nil { + t.Fatalf("Failed to draw YELLOW rectangle: %v", err) + } + + // Fill an unrelated background pixel with a fully transparent color, + // exactly like a locked-palette sprite establishing a transparent background. + fillScript := gen.FillArea("Layer 1", 1, 20, 20, aseprite.Color{R: 0, G: 0, B: 0, A: 0}, 0, false) + if _, err := client.ExecuteLua(ctx, fillScript, spritePath); err != nil { + t.Fatalf("Failed to fill background: %v", err) + } + + getPixelsScript := gen.GetPixels("Layer 1", 1, 0, 0, 32, 32) + output, err := client.ExecuteLua(ctx, getPixelsScript, spritePath) + if err != nil { + // One observed manifestation of this bug: once transparentColor + // collides with the only real color drawn, Aseprite's cel trimming + // treats the entire cel as fully transparent and deletes it outright, + // so get_pixels has no cel left to read and errors out. + t.Fatalf("BUG CONFIRMED: get_pixels failed after the transparent fill, "+ + "the drawn content was likely trimmed away as transparent: %v", err) + } + + pixels, err := testutil.ParsePixelData(output) + if err != nil { + t.Fatalf("Failed to parse pixel data: %v", err) + } + + pixelAt := make(map[[2]int]string, len(pixels)) + for _, p := range pixels { + pixelAt[[2]int{p.X, p.Y}] = p.Color + } + + // The YELLOW rectangle must still read back as opaque YELLOW. + if c, ok := pixelAt[[2]int{1, 1}]; !ok || c != "#FFFF00FF" { + t.Errorf("BUG CONFIRMED: expected opaque YELLOW (#FFFF00FF) at (1,1), got %q (present=%v) - "+ + "the palette's last color was treated as transparent", c, ok) + } + + // The background fill must read back as transparent, not opaque YELLOW. + if c, ok := pixelAt[[2]int{20, 20}]; ok { + t.Errorf("BUG CONFIRMED: expected background pixel (20,20) to be transparent, got opaque color %q - "+ + "transparentColor collided with a real palette color", c) + } +} diff --git a/pkg/tools/quantize_palette_imbalanced_frequency_bug_test.go b/pkg/tools/quantize_palette_imbalanced_frequency_bug_test.go new file mode 100644 index 0000000..40f1dc7 --- /dev/null +++ b/pkg/tools/quantize_palette_imbalanced_frequency_bug_test.go @@ -0,0 +1,165 @@ +//go:build integration +// +build integration + +package tools + +import ( + "context" + "image/png" + "os" + "path/filepath" + "testing" + "time" + + "github.com/willibrandon/pixel-mcp/internal/testutil" + "github.com/willibrandon/pixel-mcp/pkg/aseprite" +) + +// TestIntegration_QuantizePalette_ImbalancedFrequencyPreservesExactColors +// reproduces the median-cut split-by-pixel-count bug end to end through the +// real quantize_palette pipeline: export sprite -> aseprite.QuantizePalette +// (Go clustering) -> ApplyQuantizedPalette (Lua, converts to indexed). +// +// The bug: MedianCutQuantization's bucket split picked the pixel-array +// midpoint (mid := len(pixels) / 2) as the split point, not a boundary +// between distinct colors. A typical pixel-art sprite has wildly imbalanced +// color frequency - large solid fills (skin, clothing) vastly outnumber +// small accent details (a 1-2px highlight or buckle) - so splitting at the +// raw pixel-count median lands the cut inside the dominant fill color's own +// run instead of between colors. The dominant color gets needlessly +// re-subdivided into near-duplicate buckets while rare accent colors never +// get isolated into their own bucket and are averaged into a blended, +// off-palette hex value instead, even when target_colors comfortably +// exceeds the number of unique colors actually present (which should +// guarantee lossless recovery). +// +// This is the exact shape that surfaced the bug on a real 32x32 hero sprite: +// a ~200px tunic fill, a ~90px skin fill, a thin outline, and 2px highlight/ +// buckle accents. +func TestIntegration_QuantizePalette_ImbalancedFrequencyPreservesExactColors(t *testing.T) { + cfg := testutil.LoadTestConfig(t) + client := aseprite.NewClient(cfg.AsepritePath, cfg.TempDir, 30*time.Second) + gen := aseprite.NewLuaGenerator() + ctx := context.Background() + + spritePath := testutil.TempSpritePath(t, "test-quantize-imbalanced.aseprite") + createScript := gen.CreateCanvas(32, 32, aseprite.ColorModeRGB, spritePath) + if _, err := client.ExecuteLua(ctx, createScript, ""); err != nil { + t.Fatalf("Failed to create canvas: %v", err) + } + + tunicGreen := aseprite.Color{R: 0x6A, G: 0xBE, B: 0x30, A: 255} + skin := aseprite.Color{R: 0xD9, G: 0xA0, B: 0x66, A: 255} + outline := aseprite.Color{R: 0x22, G: 0x20, B: 0x34, A: 255} + highlight := aseprite.Color{R: 0x99, G: 0xE5, B: 0x50, A: 255} + buckle := aseprite.Color{R: 0xFB, G: 0xF2, B: 0x36, A: 255} + + var pixels []aseprite.Pixel + add := func(x, y int, c aseprite.Color) { + pixels = append(pixels, aseprite.Pixel{Point: aseprite.Point{X: x, Y: y}, Color: c}) + } + for y := 11; y < 21; y++ { // tunic: 20 x 10 = 200px + for x := 6; x < 26; x++ { + add(x, y, tunicGreen) + } + } + for y := 4; y < 10; y++ { // skin: 16 x 6 = 96px + for x := 6; x < 22; x++ { + add(x, y, skin) + } + } + for x := 6; x < 26; x++ { // outline: 20px + add(x, 2, outline) + } + add(15, 14, highlight) // 2px accent + add(16, 14, highlight) + add(15, 20, buckle) // 2px accent + add(16, 20, buckle) + + drawScript := gen.DrawPixels("Layer 1", 1, pixels, false) + if _, err := client.ExecuteLua(ctx, drawScript, spritePath); err != nil { + t.Fatalf("Failed to draw pixels: %v", err) + } + + // Real pipeline: export to PNG, quantize in Go, apply via Lua - matches + // pkg/tools/quantization.go's actual quantize_palette tool flow. + tempPNG := filepath.Join(t.TempDir(), "sprite.png") + exportScript := gen.ExportSprite(tempPNG, 0) + if _, err := client.ExecuteLua(ctx, exportScript, spritePath); err != nil { + t.Fatalf("Failed to export sprite: %v", err) + } + imgFile, err := os.Open(tempPNG) + if err != nil { + t.Fatalf("Failed to open exported PNG: %v", err) + } + defer imgFile.Close() + img, err := png.Decode(imgFile) + if err != nil { + t.Fatalf("Failed to decode PNG: %v", err) + } + + // target_colors (32) comfortably exceeds the 5 unique colors present, so + // every one of them must survive quantization exactly. + palette, originalColors, err := aseprite.QuantizePalette(img, 32, "median_cut", true) + if err != nil { + t.Fatalf("QuantizePalette failed: %v", err) + } + + want := map[string]bool{ + "#00000000": true, "#6ABE30": true, "#D9A066": true, "#222034": true, + "#99E550": true, "#FBF236": true, + } + if len(palette) != len(want) { + t.Fatalf("BUG CONFIRMED: QuantizePalette(target=32) returned %d colors from %d unique input colors, want %d (lossless recovery): %v", + len(palette), originalColors, len(want), palette) + } + for _, c := range palette { + if !want[c] { + t.Errorf("BUG CONFIRMED: palette contains %s, not one of the 6 exact input colors (blended/off-palette result)", c) + } + } + + applyScript := gen.ApplyQuantizedPalette(palette, originalColors, "median_cut", true, false) + if _, err := client.ExecuteLua(ctx, applyScript, spritePath); err != nil { + t.Fatalf("Failed to apply quantized palette: %v", err) + } + + getPixelsScript := gen.GetPixels("Layer 1", 1, 0, 0, 32, 32) + output, err := client.ExecuteLua(ctx, getPixelsScript, spritePath) + if err != nil { + t.Fatalf("Failed to get pixels after conversion: %v", err) + } + got, err := testutil.ParsePixelData(output) + if err != nil { + t.Fatalf("Failed to parse pixel data: %v", err) + } + pixelAt := make(map[[2]int]string, len(got)) + for _, p := range got { + pixelAt[[2]int{p.X, p.Y}] = p.Color + } + + wantPixels := map[[2]int]string{ + {0, 0}: "#00000000", + {10, 15}: "#6ABE30FF", + {10, 6}: "#D9A066FF", + {10, 2}: "#222034FF", + {15, 14}: "#99E550FF", + {15, 20}: "#FBF236FF", + } + for pos, want := range wantPixels { + got, ok := pixelAt[pos] + if want == "#00000000" { + if ok && got != "#00000000" { + t.Errorf("BUG CONFIRMED: pixel %v should be transparent, got opaque %q", pos, got) + } + continue + } + if !ok { + t.Errorf("BUG CONFIRMED: pixel %v (want %s) missing from converted sprite", pos, want) + continue + } + if got != want { + t.Errorf("BUG CONFIRMED: pixel %v = %q, want %q", pos, got, want) + } + } +}