From 8303d63ec4609afec6c2e5fc233f60e9a23d4f34 Mon Sep 17 00:00:00 2001 From: Christopher Hanson Date: Fri, 26 Jun 2026 18:51:27 -0400 Subject: [PATCH 1/5] fix: correct draw_pixels placement when cel is not at origin (0,0) draw_pixels wrote to cel.image via Image:putPixel using sprite coordinates, but putPixel expects cel-LOCAL coordinates. Once a layer's cel is trimmed to a non-(0,0) origin (e.g. after a shape tool runs), every pixel was offset by the cel's position, and pixels falling outside the cel's bounds were dropped entirely. The shape tools are unaffected because they use app.useTool, which operates in sprite space. Normalize the target cel to a full-canvas image at (0,0) before writing (compositing any existing content first), so sprite coordinates equal image coordinates. This also gives a freshly created cel a valid image, fixing draw_pixels on newly added layers. Aseprite re-trims the cel on save, so canvas dimensions and file size are unchanged. Tests: - pkg/aseprite/lua_test.go: unit assertion that the generated script normalizes the cel to full-canvas at (0,0). Runs without Aseprite, so it guards the fix in CI, where the integration tests do not run. - pkg/tools: integration tests for the offset-after-shape case and for an asymmetric cel origin with a pixel outside the shape's bounds (the lost-pixel case). Both fail on stock and pass with this change. --- pkg/aseprite/lua_drawing.go | 16 +++- pkg/aseprite/lua_test.go | 27 +++++++ .../drawing_pixels_after_shape_bug_test.go | 70 ++++++++++++++++++ .../drawing_pixels_outside_shape_bug_test.go | 73 +++++++++++++++++++ 4 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 pkg/tools/drawing_pixels_after_shape_bug_test.go create mode 100644 pkg/tools/drawing_pixels_outside_shape_bug_test.go diff --git a/pkg/aseprite/lua_drawing.go b/pkg/aseprite/lua_drawing.go index 8345010..9b4d34f 100644 --- a/pkg/aseprite/lua_drawing.go +++ b/pkg/aseprite/lua_drawing.go @@ -61,8 +61,20 @@ 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) + 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) + cel = spr:newCel(layer, frame, full, Point(0, 0)) end local img = cel.image diff --git a/pkg/aseprite/lua_test.go b/pkg/aseprite/lua_test.go index 61d0db6..8bb013b 100644 --- a/pkg/aseprite/lua_test.go +++ b/pkg/aseprite/lua_test.go @@ -168,6 +168,33 @@ func TestLuaGenerator_DrawPixels(t *testing.T) { } } +// 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) + } + } +} + func TestLuaGenerator_ExportSprite(t *testing.T) { gen := NewLuaGenerator() 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 +} From 854aca8a70fc874ab461647f6d47d3fc6c2baa69 Mon Sep 17 00:00:00 2001 From: Pierre-Luc Brunet Date: Thu, 2 Jul 2026 08:30:14 -0400 Subject: [PATCH 2/5] fix(indexed): prevent transparentColor from colliding with a real palette color Palette:resize silently clamps Sprite.transparentColor into the new palette's valid index range. After set_palette, quantize_palette, add_palette_color, or apply_auto_shading resize an indexed sprite's palette, transparentColor can land on the same index as a real, in-use color (typically the palette's last entry). Once that happens, painting "transparent" (fill_area with a fully transparent color) writes the 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 - the same class of bug fixed for index 0 in b5175b8, reintroduced at index N-1. Fix: re-assert transparentColor = 255 after every palette resize on an indexed sprite via a shared ReassertIndexedTransparentColor() helper, keeping it out of the palette's real, addressable range. Adds a regression test reproducing the exact collision and verifying both the drawn color and the transparent fill read back correctly. --- CHANGELOG.md | 16 +++ pkg/aseprite/lua_auto_shading.go | 21 ++-- pkg/aseprite/lua_core.go | 29 +++++ pkg/aseprite/lua_palette.go | 6 +- pkg/aseprite/lua_quantization.go | 14 ++- ...te_transparent_color_collision_bug_test.go | 108 ++++++++++++++++++ 6 files changed, 176 insertions(+), 18 deletions(-) create mode 100644 pkg/tools/palette_transparent_color_collision_bug_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f353ca..3df1568 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **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_core.go b/pkg/aseprite/lua_core.go index 0960602..e463492 100644 --- a/pkg/aseprite/lua_core.go +++ b/pkg/aseprite/lua_core.go @@ -159,6 +159,35 @@ 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 +` +} + // 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_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_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/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) + } +} From 7bc0829353653de0074f3886064636b1c9770530 Mon Sep 17 00:00:00 2001 From: Pierre-Luc Brunet Date: Thu, 2 Jul 2026 09:21:34 -0400 Subject: [PATCH 3/5] fix(indexed): resolve exact colors against the sprite's own palette Root cause: on an indexed sprite, img:putPixel and app.useTool both expect a palette index, not an arbitrary RGBA value. Passing a Color object directly resolves correctly 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 drawing happens in the next. In that (normal, for every real tool call) 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 (verified 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. Affects draw_pixels, draw_line, draw_contour, draw_rectangle, draw_circle, and fill_area whenever use_palette is false (the default) on an indexed sprite. Fix: resolveExactPaletteColor (pkg/aseprite/lua_core.go) resolves the color itself by reading the sprite's actual active palette at Lua runtime, bypassing Aseprite's unreliable resolution entirely. - Fully-transparent colors (alpha 0, e.g. a "paint transparent" fill_area call) resolve directly to spr.transparentColor, since transparency is a designated index, not a color expected to exist in the palette. - Any other color requires an exact match in the active palette and errors if none exists, so a caller drawing an approximate/non-locked color silently gets the wrong pixel instead of an error telling them to pass use_palette=true to snap to the nearest palette color. Wired into DrawPixels (FormatColorWithPalette) and DrawLine/ DrawContour/DrawRectangle/DrawCircle/FillArea (FormatColorWithPaletteForTool). Testing: - go test ./...: 628 passed - go test -tags=integration ./...: 805 passed - Added TestIntegration_IndexedMode_ExactColorResolution (pkg/tools/exact_palette_color_indexed_mode_bug_test.go): exact match via draw_rectangle, exact match via draw_pixels, and error on no match. Verified RED without the fix (all three fail/misbehave exactly as described above) and GREEN with it. - Updated two pre-existing tests that asserted the old, buggy behavior: TestFormatColorWithPalette_NoPalette and TestLuaGenerator_DrawPixels now expect resolveExactPaletteColor(...) instead of a raw Color(...) literal. - Fixed TestIntegration_SetPalette_ThenApplyShading, which drew an approximate "mid-tone" color with use_palette=false even though that color was never one of the locked palette's 16 exact entries; it now correctly passes use_palette=true to snap it. --- pkg/aseprite/lua_core.go | 82 +++++++++++-- pkg/aseprite/lua_drawing.go | 42 +++++-- pkg/aseprite/lua_palette_test.go | 2 +- pkg/aseprite/lua_test.go | 4 +- ...act_palette_color_indexed_mode_bug_test.go | 111 ++++++++++++++++++ pkg/tools/palette_integration_test.go | 5 +- 6 files changed, 218 insertions(+), 28 deletions(-) create mode 100644 pkg/tools/exact_palette_color_indexed_mode_bug_test.go diff --git a/pkg/aseprite/lua_core.go b/pkg/aseprite/lua_core.go index e463492..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) } @@ -188,6 +195,59 @@ 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 9b4d34f..235c4bd 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 @@ -118,11 +121,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 @@ -202,11 +208,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 @@ -307,11 +316,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" @@ -391,11 +403,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" @@ -481,11 +496,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_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_test.go b/pkg/aseprite/lua_test.go index 0497c79..517346b 100644 --- a/pkg/aseprite/lua_test.go +++ b/pkg/aseprite/lua_test.go @@ -159,11 +159,11 @@ 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") } } 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) From e8887b3b6bc0fe0a0abd83188db56ea021207008 Mon Sep 17 00:00:00 2001 From: Pierre-Luc Brunet Date: Thu, 2 Jul 2026 10:21:55 -0400 Subject: [PATCH 4/5] fix: 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. On a 32x32 sprite with a ~200px tunic fill, ~90px skin fill, a thin outline, and 2px highlight/buckle accents, quantizing to target_colors=32 (13 unique colors present) returned colors like #373130 that did not exist anywhere in the source image. Fix: reduce 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. I initially suspected a second bug in ApplyQuantizedPalette/ ChangePixelFormat (pixels resolving to the wrong palette index, transparency lost on conversion to indexed mode). Verified via aseprite --batch scripts and full end-to-end integration tests (Go clustering -> Lua apply -> get_pixels) that ChangePixelFormat correctly honors a palette set immediately before it runs, in both the lossless (target_colors >= unique colors) and genuine lossy reduction cases. The apparent scrambling in my initial hand-written repro script was caused by omitting the existing ReassertIndexedTransparentColor() call, not a new bug - so no change was needed there. Testing: - go test ./...: 633 passed - go test -tags=integration ./...: unchanged pre-existing suite passed, plus the new regression test - Added TestMedianCutQuantization_ImbalancedFrequencyPreservesExactColors (pkg/aseprite/quantization_test.go): pure-Go unit test, no Aseprite needed, guards the fix in CI - Added TestIntegration_QuantizePalette_ImbalancedFrequencyPreservesExactColors (pkg/tools/quantize_palette_imbalanced_frequency_bug_test.go): reproduces the exact hero-sprite shape end to end through the real quantize_palette pipeline (export -> Go quantize -> Lua apply -> get_pixels), checking both exact colors and transparency survive conversion to indexed mode. Verified RED without the fix (blended colors and wrong pixels, matching the originally observed corruption) and GREEN with it. --- CHANGELOG.md | 19 ++ pkg/aseprite/quantization.go | 165 ++++++++++++------ pkg/aseprite/quantization_test.go | 60 +++++++ ...e_palette_imbalanced_frequency_bug_test.go | 165 ++++++++++++++++++ 4 files changed, 355 insertions(+), 54 deletions(-) create mode 100644 pkg/tools/quantize_palette_imbalanced_frequency_bug_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3df1568..46a4c2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed +- **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`, 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/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) + } + } +} From 3719159bada4bc05b1742bab3a7f0b6b7112658d Mon Sep 17 00:00:00 2001 From: Pierre-Luc Brunet Date: Fri, 3 Jul 2026 12:26:42 -0400 Subject: [PATCH 5/5] fix(indexed): stop transparent pixels reverting to opaque black On indexed sprites CreateCanvas sets transparentColor = 255 so palette index 0 can hold a real color, which makes index 0 an opaque color. Two paths left transparent areas showing that opaque index-0 background: - draw_pixels normalizes the target cel into a full-canvas Image() (filled with index 0) and composites the existing content with the default NORMAL blend, which skips the source's transparent (index 255) pixels. Any pixel a previous call had made transparent but the current call does not rewrite flipped to opaque black. Fixed by clearing the full-canvas image to spr.transparentColor before compositing/drawing, in both the existing-cel and new-cel paths. - create_canvas left the initial cel filled with index 0, so a freshly created indexed canvas started with an opaque background. Fixed by clearing the initial cel to spr.transparentColor right after creation. RGB and grayscale behavior is unchanged (both guarded to indexed mode). Adds behavioral integration tests that reproduce both cases and CI-safe string checks. --- CHANGELOG.md | 24 +++ pkg/aseprite/lua_canvas.go | 9 + pkg/aseprite/lua_drawing.go | 15 ++ pkg/aseprite/lua_test.go | 53 +++++ ..._pixels_transparent_background_bug_test.go | 190 ++++++++++++++++++ 5 files changed, 291 insertions(+) create mode 100644 pkg/tools/drawing_pixels_transparent_background_bug_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 46a4c2e..e1b7d4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,30 @@ 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. 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_drawing.go b/pkg/aseprite/lua_drawing.go index 235c4bd..5902e5d 100644 --- a/pkg/aseprite/lua_drawing.go +++ b/pkg/aseprite/lua_drawing.go @@ -70,6 +70,15 @@ app.transaction(function() -- 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) @@ -77,6 +86,12 @@ app.transaction(function() -- 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 diff --git a/pkg/aseprite/lua_test.go b/pkg/aseprite/lua_test.go index 517346b..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) { @@ -195,6 +220,34 @@ func TestLuaGenerator_DrawPixels_NormalizesCel(t *testing.T) { } } +// 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/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) + } +}