From bbdf6bc2a384d39728ca6fc664d58d9d6129eb05 Mon Sep 17 00:00:00 2001 From: Rangi42 Date: Fri, 19 Dec 2025 12:23:49 -0500 Subject: [PATCH 1/2] Improve some RGBGFX error messages --- man/rgbgfx.1 | 6 +++-- src/gfx/process.cpp | 48 +++++++++++++++++++++++------------ test/gfx/ambiguous.err | 2 ++ test/gfx/ambiguous.png | Bin 0 -> 124 bytes test/gfx/bad_slice.err | 2 +- test/gfx/bg_fuse.err | 2 +- test/gfx/gray_alpha.err | 2 ++ test/gfx/gray_alpha.flags | 1 + test/gfx/gray_alpha.png | Bin 0 -> 111 bytes test/gfx/gray_conflict.err | 2 ++ test/gfx/gray_conflict.flags | 1 + test/gfx/gray_conflict.png | Bin 0 -> 116 bytes test/gfx/gray_nongray.err | 2 ++ test/gfx/gray_nongray.flags | 1 + test/gfx/gray_nongray.png | Bin 0 -> 127 bytes test/gfx/gray_too_many.err | 2 ++ test/gfx/gray_too_many.flags | 1 + test/gfx/gray_too_many.png | Bin 0 -> 147 bytes 18 files changed, 52 insertions(+), 20 deletions(-) create mode 100644 test/gfx/ambiguous.err create mode 100644 test/gfx/ambiguous.png create mode 100644 test/gfx/gray_alpha.err create mode 100644 test/gfx/gray_alpha.flags create mode 100644 test/gfx/gray_alpha.png create mode 100644 test/gfx/gray_conflict.err create mode 100644 test/gfx/gray_conflict.flags create mode 100644 test/gfx/gray_conflict.png create mode 100644 test/gfx/gray_nongray.err create mode 100644 test/gfx/gray_nongray.flags create mode 100644 test/gfx/gray_nongray.png create mode 100644 test/gfx/gray_too_many.err create mode 100644 test/gfx/gray_too_many.flags create mode 100644 test/gfx/gray_too_many.png diff --git a/man/rgbgfx.1 b/man/rgbgfx.1 index e129813d7..12adba338 100644 --- a/man/rgbgfx.1 +++ b/man/rgbgfx.1 @@ -313,8 +313,10 @@ This is useful for example if the input image is a sheet of some sort, and you w The default is to process the whole image as-is. .Pp .Ar slice -must be two number pairs, separated by a colon. -The numbers must be separated by commas; space is allowed around all punctuation. +must be formatted as +.Ql Ar X , Ns Ar Y : Ns Ar W , Ns Ar H : +two comma-separated number pairs, separated by a colon. +Whitespace is allowed around all punctuation. The first number pair specifies the X and Y coordinates of the top-left pixel that will be processed (anything above it or to its left will be ignored). The second number pair specifies how many tiles to process horizontally and vertically, respectively. .Pp diff --git a/src/gfx/process.cpp b/src/gfx/process.cpp index 280f49240..136e628f2 100644 --- a/src/gfx/process.cpp +++ b/src/gfx/process.cpp @@ -84,7 +84,13 @@ struct Image { Rgba &pixel(uint32_t x, uint32_t y) { return png.pixels[y * png.width + x]; } Rgba const &pixel(uint32_t x, uint32_t y) const { return png.pixels[y * png.width + x]; } - bool isSuitableForGrayscale() const { + enum GrayscaleResult { + GRAY_OK, + GRAY_TOO_MANY, + GRAY_NONGRAY, + GRAY_CONFLICT, + }; + std::pair> isSuitableForGrayscale() const { // Check that all of the grays don't fall into the same "bin" if (colors.size() > options.maxOpaqueColors()) { // Apply the Pigeonhole Principle verbosePrint( @@ -93,7 +99,7 @@ struct Image { colors.size(), options.maxOpaqueColors() ); - return false; + return {GrayscaleResult::GRAY_TOO_MANY, std::nullopt}; } uint8_t bins = 0; for (std::optional const &color : colors) { @@ -106,7 +112,7 @@ struct Image { "Found non-gray color #%08x, not using grayscale sorting\n", color->toCSS() ); - return false; + return {GrayscaleResult::GRAY_NONGRAY, color}; } uint8_t mask = 1 << color->grayIndex(); if (bins & mask) { // Two in the same bin! @@ -115,11 +121,11 @@ struct Image { "Color #%08x conflicts with another one, not using grayscale sorting\n", color->toCSS() ); - return false; + return {GrayscaleResult::GRAY_CONFLICT, color}; } bins |= mask; } - return true; + return {GrayscaleResult::GRAY_OK, std::nullopt}; } explicit Image(std::string const &path) { @@ -151,8 +157,8 @@ struct Image { if (options.inputSlice.width % 8 == 0 && options.inputSlice.height % 8 == 0) { fprintf( stderr, - "note: Did you mean the slice \"%" PRIu32 ",%" PRIu32 ":%" PRId32 ",%" PRId32 - "\"? (width and height are in tiles, not pixels!)\n", + " (Did you mean the slice \"%" PRIu32 ",%" PRIu32 ":%" PRId32 ",%" PRId32 + "\"? The width and height are in tiles, not pixels!)\n", options.inputSlice.left, options.inputSlice.top, options.inputSlice.width / 8, @@ -181,7 +187,7 @@ struct Image { if (uint32_t css = color.toCSS(); ambiguous.find(css) == ambiguous.end()) { error( "Color #%08x is neither transparent (alpha < %u) nor opaque (alpha >= " - "%u) [first seen at x: %" PRIu32 ", y: %" PRIu32 "]", + "%u) (first seen at (%" PRIu32 ", %" PRIu32 "))", css, Rgba::transparency_threshold, Rgba::opacity_threshold, @@ -195,8 +201,8 @@ struct Image { if (std::pair fused{color.toCSS(), other->toCSS()}; fusions.find(fused) == fusions.end()) { warnx( - "Fusing colors #%08x and #%08x into Game Boy color $%04x [first seen " - "at x: %" PRIu32 ", y: %" PRIu32 "]", + "Colors #%08x and #%08x both reduce to the same Game Boy color $%04x " + "(first seen at (%" PRIu32 ", %" PRIu32 "))", fused.first, fused.second, color.cgbColor(), @@ -381,7 +387,7 @@ static std::pair, std::vector> "Sorting palette colors by PNG's embedded PLTE chunk without '-c/--colors embedded'" ); sortIndexed(palettes, image.png.palette); - } else if (image.isSuitableForGrayscale()) { + } else if (image.isSuitableForGrayscale().first == Image::GRAY_OK) { sortGrayscale(palettes, image.colors.raw()); } else { sortRgb(palettes); @@ -939,15 +945,25 @@ void process() { // LCOV_EXCL_STOP if (options.palSpecType == Options::DMG) { + char const *prefix = + "Image is not compatible with a DMG palette specification: it contains"; if (options.hasTransparentPixels) { + fatal("%s transparent pixels", prefix); + } + switch (auto const [result, color] = image.isSuitableForGrayscale(); result) { + case Image::GRAY_OK: + break; + case Image::GRAY_TOO_MANY: + fatal("%s too many colors (%zu)", prefix, image.colors.size()); + case Image::GRAY_NONGRAY: + fatal("%s a non-gray color #%08x", prefix, color->toCSS()); + case Image::GRAY_CONFLICT: fatal( - "Image contains transparent pixels, not compatible with a DMG palette specification" + "%s a color #%08x that reduces to the same gray shade as another one", + prefix, + color->toCSS() ); } - if (!image.isSuitableForGrayscale()) { - fatal("Image contains too many or non-gray colors, not compatible with a DMG palette " - "specification"); - } } // Now, iterate through the tiles, generating color sets as we go diff --git a/test/gfx/ambiguous.err b/test/gfx/ambiguous.err new file mode 100644 index 000000000..d7817a14f --- /dev/null +++ b/test/gfx/ambiguous.err @@ -0,0 +1,2 @@ +error: Color #ff800080 is neither transparent (alpha < 16) nor opaque (alpha >= 240) (first seen at (0, 8)) +Conversion aborted after 1 error diff --git a/test/gfx/ambiguous.png b/test/gfx/ambiguous.png new file mode 100644 index 0000000000000000000000000000000000000000..7a84892d20ddc8d697fd6cbc5b194edcfdbed62f GIT binary patch literal 124 zcmeAS@N?(olHy`uVBq!ia0vp^5+KaM1|%Pp+x`GjEX7WqAsj$Z!;#VfZf0g@c3oYGUydeO2RSw*n8^2lB$$~Qw7)X%){%N~ Q0;rwA)78&qol`;+03g;OX#fBK literal 0 HcmV?d00001 diff --git a/test/gfx/bad_slice.err b/test/gfx/bad_slice.err index eaea9ef04..f10cd23e4 100644 --- a/test/gfx/bad_slice.err +++ b/test/gfx/bad_slice.err @@ -1,3 +1,3 @@ error: Image slice ((2, 2) to (130, 130)) is outside the image bounds (20x20) -note: Did you mean the slice "2,2:2,2"? (width and height are in tiles, not pixels!) + (Did you mean the slice "2,2:2,2"? The width and height are in tiles, not pixels!) Conversion aborted after 1 error diff --git a/test/gfx/bg_fuse.err b/test/gfx/bg_fuse.err index 9e8052b0e..439c4aa5b 100644 --- a/test/gfx/bg_fuse.err +++ b/test/gfx/bg_fuse.err @@ -1,3 +1,3 @@ -warning: Fusing colors #a9b9c9ff and #aabbccff into Game Boy color $66f5 [first seen at x: 1, y: 1] +warning: Colors #a9b9c9ff and #aabbccff both reduce to the same Game Boy color $66f5 (first seen at (1, 1)) FATAL: Tile (0, 0) contains the background color (#aabbccff) Conversion aborted after 1 error diff --git a/test/gfx/gray_alpha.err b/test/gfx/gray_alpha.err new file mode 100644 index 000000000..4d8fd7dd7 --- /dev/null +++ b/test/gfx/gray_alpha.err @@ -0,0 +1,2 @@ +FATAL: Image is not compatible with a DMG palette specification: it contains transparent pixels +Conversion aborted after 1 error diff --git a/test/gfx/gray_alpha.flags b/test/gfx/gray_alpha.flags new file mode 100644 index 000000000..9440ac74c --- /dev/null +++ b/test/gfx/gray_alpha.flags @@ -0,0 +1 @@ +-c dmg diff --git a/test/gfx/gray_alpha.png b/test/gfx/gray_alpha.png new file mode 100644 index 0000000000000000000000000000000000000000..e9806b476b488b3d2d59508bdcd4d989c5ba43f4 GIT binary patch literal 111 zcmeAS@N?(olHy`uVBq!ia0vp^93afW1|*O0@9PFqEX7WqAsj$Z!;#Vfj( literal 0 HcmV?d00001 diff --git a/test/gfx/gray_conflict.err b/test/gfx/gray_conflict.err new file mode 100644 index 000000000..daee49d0f --- /dev/null +++ b/test/gfx/gray_conflict.err @@ -0,0 +1,2 @@ +FATAL: Image is not compatible with a DMG palette specification: it contains a color #111111ff that reduces to the same gray shade as another one +Conversion aborted after 1 error diff --git a/test/gfx/gray_conflict.flags b/test/gfx/gray_conflict.flags new file mode 100644 index 000000000..9440ac74c --- /dev/null +++ b/test/gfx/gray_conflict.flags @@ -0,0 +1 @@ +-c dmg diff --git a/test/gfx/gray_conflict.png b/test/gfx/gray_conflict.png new file mode 100644 index 0000000000000000000000000000000000000000..3845f7050f0d8c772104eecad8e3a60bd7319abb GIT binary patch literal 116 zcmeAS@N?(olHy`uVBq!ia0vp^93afW1|*O0@9PFqEX7WqAsj$Z!;#VfF#IEF|} zo!Y-qu)%=mh)AEPt^b?SH`8HXBR*^Q sdfsFn2BXjJ3s;35Jk#*}Y}iAl=vXmMmk)B1KvNk!UHx3vIVCg!0CJBn%K!iX literal 0 HcmV?d00001 From 9c6e4d3f8cd8358f2932ade7cc25210f3ed878a8 Mon Sep 17 00:00:00 2001 From: Rangi42 Date: Fri, 19 Dec 2025 12:47:40 -0500 Subject: [PATCH 2/2] Fix assertion failure on ambiguous transparent/opaque pixels --- src/gfx/process.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/gfx/process.cpp b/src/gfx/process.cpp index 136e628f2..157d32c62 100644 --- a/src/gfx/process.cpp +++ b/src/gfx/process.cpp @@ -68,7 +68,7 @@ class ImagePalette { size_t size() const { return std::count_if(RANGE(_colors), [](std::optional const &slot) { - return slot.has_value() && !slot->isTransparent(); + return slot.has_value() && slot->isOpaque(); }); } decltype(_colors) const &raw() const { return _colors; } @@ -402,7 +402,7 @@ static std::pair, std::vector> for (auto [spec, pal] : zip(options.palSpec, palettes)) { for (size_t i = 0; i < options.nbColorsPerPal; ++i) { // If the spec has a gap, there's no need to copy anything. - if (spec[i].has_value() && !spec[i]->isTransparent()) { + if (spec[i].has_value() && spec[i]->isOpaque()) { pal[i] = spec[i]->cgbColor(); } } @@ -981,7 +981,7 @@ void process() { for (uint32_t y = 0; y < 8; ++y) { for (uint32_t x = 0; x < 8; ++x) { if (Rgba color = tile.pixel(x, y); - !color.isTransparent() || !options.hasTransparentPixels) { + color.isOpaque() || !options.hasTransparentPixels) { tileColors.insert(color.cgbColor()); } }