From 8a6b1946e3a6431cdddfada9eb97a792ca42b45d Mon Sep 17 00:00:00 2001 From: Rangi <35663410+Rangi42@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:54:24 -0400 Subject: [PATCH] Parse inline palette specs after getting CLI palette size limit (#2054) --- include/gfx/main.hpp | 5 +- include/gfx/pal_spec.hpp | 2 - src/gfx/main.cpp | 82 ++++++++++++++++------------ src/gfx/pal_spec.cpp | 17 +++--- src/gfx/reverse.cpp | 4 +- test/gfx/long_inline_pal_spec.err | 4 ++ test/gfx/long_inline_pal_spec.flags | 1 + test/gfx/long_inline_pal_spec.png | Bin 0 -> 71 bytes 8 files changed, 67 insertions(+), 48 deletions(-) create mode 100644 test/gfx/long_inline_pal_spec.err create mode 100644 test/gfx/long_inline_pal_spec.flags create mode 100644 test/gfx/long_inline_pal_spec.png diff --git a/include/gfx/main.hpp b/include/gfx/main.hpp index f41118b8..237a9831 100644 --- a/include/gfx/main.hpp +++ b/include/gfx/main.hpp @@ -25,7 +25,8 @@ struct Options { std::array baseTileIDs{0, 0}; // -b enum { NO_SPEC, - EXPLICIT, + INLINE, + EXTERNAL, EMBEDDED, DMG, } palSpecType = NO_SPEC; // -c @@ -59,6 +60,8 @@ struct Options { uint16_t maxNbColors() const { return nbColorsPerPal * nbPalettes; } + bool hasExplicitPalSpec() const { return palSpecType == INLINE || palSpecType == EXTERNAL; } + uint8_t dmgColors[4] = {}; uint8_t dmgValue(uint8_t i) const { assume(i < 4); diff --git a/include/gfx/pal_spec.hpp b/include/gfx/pal_spec.hpp index 04fff542..d19d7fe3 100644 --- a/include/gfx/pal_spec.hpp +++ b/include/gfx/pal_spec.hpp @@ -8,8 +8,6 @@ void parseInlinePalSpec(char const * const rawArg); void parseExternalPalSpec(char const *arg); void parseDmgPalSpec(char const * const rawArg); -void parseDmgPalSpec(uint8_t palSpecDmg); - void parseBackgroundPalSpec(char const *arg); #endif // RGBDS_GFX_PAL_SPEC_HPP diff --git a/src/gfx/main.cpp b/src/gfx/main.cpp index 45aabefb..3fc56f03 100644 --- a/src/gfx/main.cpp +++ b/src/gfx/main.cpp @@ -36,13 +36,13 @@ Options options; // Flags which must be processed after the option parsing finishes static struct LocalOptions { - std::optional externalPalSpec; // -c - bool autoAttrmap; // -A - bool autoTilemap; // -T - bool autoPalettes; // -P - bool autoPalmap; // -Q - bool groupOutputs; // -O - bool reverse; // -r + std::optional palSpec; // -c + bool autoAttrmap; // -A + bool autoTilemap; // -T + bool autoPalettes; // -P + bool autoPalmap; // -Q + bool groupOutputs; // -O + bool reverse; // -r bool autoAny() const { return autoAttrmap || autoTilemap || autoPalettes || autoPalmap; } } localOptions; @@ -193,10 +193,10 @@ static void parseArg(int ch, char *arg) { break; case 'c': - localOptions.externalPalSpec = std::nullopt; // Allow overriding a previous pal spec + localOptions.palSpec = std::nullopt; // Allow overriding a previous pal spec if (arg[0] == '#') { - options.palSpecType = Options::EXPLICIT; - parseInlinePalSpec(arg); + options.palSpecType = Options::INLINE; + localOptions.palSpec = arg; } else if (strcasecmp(arg, "embedded") == 0) { // Use PLTE, error out if missing options.palSpecType = Options::EMBEDDED; @@ -204,13 +204,13 @@ static void parseArg(int ch, char *arg) { options.palSpecType = Options::NO_SPEC; } else if (strcasecmp(arg, "dmg") == 0) { options.palSpecType = Options::DMG; - parseDmgPalSpec(0xE4); // Same darkest-first order as `sortGrayscale` + localOptions.palSpec = "e4"; // Same darkest-first order as `sortGrayscale` } else if (strncasecmp(arg, "dmg=", literal_strlen("dmg=")) == 0) { options.palSpecType = Options::DMG; - parseDmgPalSpec(&arg[literal_strlen("dmg=")]); + localOptions.palSpec = &arg[literal_strlen("dmg=")]; } else { - options.palSpecType = Options::EXPLICIT; - localOptions.externalPalSpec = arg; + options.palSpecType = Options::EXTERNAL; + localOptions.palSpec = arg; } break; @@ -514,24 +514,13 @@ static void verboseOutputConfig() { // -s/--palette-size fprintf(stderr, "\tPalettes contain %" PRIu8 " colors\n", options.nbColorsPerPal); // -c/--colors - if (options.palSpecType == Options::NO_SPEC) { + switch (options.palSpecType) { + case Options::NO_SPEC: fputs("\tAutomatic palette generation\n", stderr); - } else { - fprintf(stderr, "\t%s palette spec\n", [] { - switch (options.palSpecType) { - case Options::EXPLICIT: - return "Explicit"; - case Options::EMBEDDED: - return "Embedded"; - case Options::DMG: - return "DMG"; - default: - return "???"; - } - }()); - } - if (options.palSpecType == Options::EXPLICIT) { - fputs("\t[\n", stderr); + break; + case Options::INLINE: + case Options::EXTERNAL: + fputs("\tExplicit palette spec\n\t[\n", stderr); for (auto const &pal : options.palSpec) { fputs("\t\t", stderr); for (auto const &color : pal) { @@ -544,6 +533,13 @@ static void verboseOutputConfig() { putc('\n', stderr); } fputs("\t]\n", stderr); + break; + case Options::EMBEDDED: + fputs("\tEmbedded palette spec from PNG indexed PLTE chunk\n", stderr); + break; + case Options::DMG: + fprintf(stderr, "\tDMG palette spec $%02" PRIx8 "\n", options.palSpecDmg); + break; } // -L/--slice if (options.inputSlice.width || options.inputSlice.height || options.inputSlice.left @@ -671,9 +667,24 @@ int main(int argc, char *argv[]) { autoOutPath(localOptions.autoPalettes, options.palettes, ".pal"); autoOutPath(localOptions.autoPalmap, options.palmap, ".palmap"); - // Execute deferred external pal spec parsing, now that all other params are known - if (localOptions.externalPalSpec) { - parseExternalPalSpec(localOptions.externalPalSpec->c_str()); + // Execute deferred pal spec parsing, now that all other params are known + switch (options.palSpecType) { + case Options::NO_SPEC: + case Options::EMBEDDED: + assume(!localOptions.palSpec); + break; + case Options::INLINE: + assume(localOptions.palSpec); + parseInlinePalSpec(localOptions.palSpec->c_str()); + break; + case Options::EXTERNAL: + assume(localOptions.palSpec); + parseExternalPalSpec(localOptions.palSpec->c_str()); + break; + case Options::DMG: + assume(localOptions.palSpec); + parseDmgPalSpec(localOptions.palSpec->c_str()); + break; } verboseDo(VERB_CONFIG, verboseOutputConfig); @@ -687,8 +698,7 @@ int main(int argc, char *argv[]) { } else { process(); } - } else if (!options.palettes.empty() && options.palSpecType == Options::EXPLICIT - && !localOptions.reverse) { + } else if (!options.palettes.empty() && options.hasExplicitPalSpec() && !localOptions.reverse) { processPalettes(); } else { usage.printAndExit("No input file specified (pass \"-\" to read from standard input)"); diff --git a/src/gfx/pal_spec.cpp b/src/gfx/pal_spec.cpp index 5251b4af..9ac1fa95 100644 --- a/src/gfx/pal_spec.cpp +++ b/src/gfx/pal_spec.cpp @@ -29,6 +29,7 @@ #include "gfx/rgba.hpp" #include "gfx/warning.hpp" +using namespace std::literals; using namespace std::string_view_literals; static char const *hexDigits = "0123456789ABCDEFabcdef"; @@ -131,8 +132,14 @@ void parseInlinePalSpec(char const * const rawArg) { if (n == arg.length()) { break; } else if (arg[n] != ';' && arg[n] != ':') { - if (nbColors == 4) { - parseError(n, 1, "Each palette can only contain up to 4 colors"); + if (nbColors == options.nbColorsPerPal) { + // `parseError` cannot take variadic arguments, since `format_` and + // `-Wformat-security` would complain about passing a template parameter pack + // to the C-style variadic `error` function, so we format the error message + // before passing it to `parseError`. + std::string msg = "Each palette can only contain up to "s + + std::to_string(options.nbColorsPerPal) + " colors"; + parseError(n, 1, msg.c_str()); return; } break; @@ -664,11 +671,7 @@ void parseDmgPalSpec(char const * const rawArg) { return; } - parseDmgPalSpec(toHex(arg[0], arg[1])); -} - -void parseDmgPalSpec(uint8_t palSpecDmg) { - options.palSpecDmg = palSpecDmg; + options.palSpecDmg = toHex(arg[0], arg[1]); // Map gray shades to their DMG color indexes for fast lookup by `Rgba::grayIndex` for (uint8_t i = 0; i < 4; ++i) { diff --git a/src/gfx/reverse.cpp b/src/gfx/reverse.cpp index 1206b698..f0e3e7d6 100644 --- a/src/gfx/reverse.cpp +++ b/src/gfx/reverse.cpp @@ -249,7 +249,7 @@ void reverse() { ); } - if (options.palSpecType == Options::EXPLICIT && palettes != options.palSpec) { + if (options.hasExplicitPalSpec() && palettes != options.palSpec) { warnx("Colors in the palette file do not match those specified with '-c'"); // This spacing aligns "...versus with `-c`" above the column of `-c` palettes fputs("Colors specified in the palette file: ...versus with '-c':\n", stderr); @@ -273,7 +273,7 @@ void reverse() { } else if (options.palSpecType == Options::EMBEDDED) { warnx("An embedded palette was requested, but no palette file was specified; ignoring " "request"); - } else if (options.palSpecType == Options::EXPLICIT) { + } else if (options.hasExplicitPalSpec()) { palettes = std::move(options.palSpec); // We won't be using it again. } diff --git a/test/gfx/long_inline_pal_spec.err b/test/gfx/long_inline_pal_spec.err new file mode 100644 index 00000000..13d00e2d --- /dev/null +++ b/test/gfx/long_inline_pal_spec.err @@ -0,0 +1,4 @@ +error: Each palette can only contain up to 2 colors +In inline palette spec: "#ffffff,#888888,#000000" + ^ +Conversion aborted after 1 error diff --git a/test/gfx/long_inline_pal_spec.flags b/test/gfx/long_inline_pal_spec.flags new file mode 100644 index 00000000..1acfa5e5 --- /dev/null +++ b/test/gfx/long_inline_pal_spec.flags @@ -0,0 +1 @@ +-c #ffffff,#888888,#000000 -s 2 diff --git a/test/gfx/long_inline_pal_spec.png b/test/gfx/long_inline_pal_spec.png new file mode 100644 index 0000000000000000000000000000000000000000..3233c135eef064b25509a4d998b5b91b87f6ab9b GIT binary patch literal 71 zcmeAS@N?(olHy`uVBq!ia0vp^93adDBp6m-`S}e<@p-y9hE&{2{_%f4qcju4{s{h$ TmlgGXfK+(8`njxgN@xNAHMtWS literal 0 HcmV?d00001