fix(indexed): prevent transparentColor from colliding with a real palette color#21
Open
Fuitad wants to merge 6 commits into
Open
fix(indexed): prevent transparentColor from colliding with a real palette color#21Fuitad wants to merge 6 commits into
Fuitad wants to merge 6 commits into
Conversation
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.
…ette 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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an indexed-color-mode transparency corruption bug I hit while using the
pixel-pluginClaude Code plugin (which bundles this server) for a game project. Opening the fix here directly since this is where the root cause actually lives.Palette:resizesilently clampsSprite.transparentColorinto the palette's new valid index range.CreateCanvassetstransparentColor = 255for indexed sprites specifically so palette index 0 can hold a real color (see b5175b8). But every tool that later resizes the palette -set_palette,quantize_palette,add_palette_color,apply_auto_shading- loses that protection: after resizing to an N-color palette,transparentColorbecomesN-1, colliding with whatever real color the caller just set at the last index.Once that collision exists, anything that paints "transparent" (most commonly
fill_areawith a fully-transparent color, which is how a locked-palette sprite typically establishes its background) writes that colliding index. Reading it back reports the real color's opaque RGBA instead of transparency, and a pixel intentionally drawn in that color can be auto-trimmed away as if it were transparent. In the reproduction I hit, this manifested as background regions rendering as an unrelated opaque palette color, and in a smaller repro, an entire cel being deleted because Aseprite's auto-trim decided all of its content (foreground included) was "transparent."This is the same class of bug already fixed for index 0 in b5175b8, just reintroduced at index
N-1by the newer palette-resizing tools.Fix
Adds
ReassertIndexedTransparentColor()inpkg/aseprite/lua_core.go, a small Lua snippet that re-pinstransparentColorto 255 for indexed sprites. Spliced in immediately after everyPalette:resizecall in:SetPaletteAddPaletteColorApplyQuantizedPalette(after the optional RGB->indexed conversion, so the check sees the final color mode)ApplyAutoShadingNo changes needed in
get_pixelsor export: its existing "index out of palette bounds -> transparent" branch already does the right thing oncetransparentColorstays safely out of the real, addressable palette range.Root cause verification
I traced this by hand with the actual
asepriteCLI outside the Go test suite (create indexed sprite ->transparentColor=255->set_palettewith a 32-color array ->transparentColorreads back as31), then confirmed the downstream effect (a "transparent"fill_areawrites index 31, which is a real, opaque color, so it reads back as that color instead of transparent). Full before/after logs available if useful.Testing
go build ./...,go vet ./...: cleango test ./...: 627 passed (no regressions)go test -tags=integration ./...: 796 passed (twoSetFrameDuration/ApplyShading_HardStylefailures on the full run were 30s timeouts from process contention running the whole suite at once; both pass individually)TestIntegration_SetPalette_TransparentColorCollisionBug(pkg/tools/palette_transparent_color_collision_bug_test.go), which reproduces the exact collision (4-color palette, draw with the last color,fill_areaa separate region transparent,get_pixelsboth). Verified RED without the fix (get_pixels errors with "No cel found in frame 1" because the whole cel got trimmed away) and GREEN with it.PaletteIndex0/CelPosition/NewLayer/indexed-export bug regression tests still pass.Changelog
Added an
[Unreleased] / Fixedentry inCHANGELOG.md.