From a9ab248fedb917cb57a89d0df16af09d351a2f7b Mon Sep 17 00:00:00 2001 From: Rangi <35663410+Rangi42@users.noreply.github.com> Date: Fri, 19 Dec 2025 13:00:05 -0500 Subject: [PATCH] Improve some RGBGFX error messages (#1876) * Improve some RGBGFX error messages * Fix assertion failure on ambiguous transparent/opaque pixels --- man/rgbgfx.1 | 6 ++-- src/gfx/process.cpp | 56 ++++++++++++++++++++++------------- 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, 56 insertions(+), 24 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 e129813d..12adba33 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 280f4924..157d32c6 100644 --- a/src/gfx/process.cpp +++ b/src/gfx/process.cpp @@ -68,7 +68,7 @@ public: 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; } @@ -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); @@ -396,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(); } } @@ -939,14 +945,24 @@ 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( - "Image contains transparent pixels, not compatible with a DMG palette specification" - ); + fatal("%s transparent pixels", prefix); } - if (!image.isSuitableForGrayscale()) { - fatal("Image contains too many or non-gray colors, not compatible with a DMG palette " - "specification"); + 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( + "%s a color #%08x that reduces to the same gray shade as another one", + prefix, + color->toCSS() + ); } } @@ -965,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()); } } diff --git a/test/gfx/ambiguous.err b/test/gfx/ambiguous.err new file mode 100644 index 00000000..d7817a14 --- /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 eaea9ef0..f10cd23e 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 9e8052b0..439c4aa5 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 00000000..4d8fd7dd --- /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 00000000..9440ac74 --- /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 00000000..daee49d0 --- /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 00000000..9440ac74 --- /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