From 998f636495e7f5dbccb81d99a2fa52fbb6d58226 Mon Sep 17 00:00:00 2001 From: vulcandth Date: Sun, 7 Jun 2026 08:55:05 -0500 Subject: [PATCH] Fix `rgbgfx -Z` palette overgeneration on merged color sets (#1912) - Fix logic for color set comparison (which affects sorting them) - Prune color sets which are proper subsets of newly-encountered ones (a comment implied we were already doing this, but we weren't) - Add more verbose logging to debug this behavior --- include/gfx/color_set.hpp | 6 +-- src/gfx/color_set.cpp | 47 ++++++++++------ src/gfx/process.cpp | 74 +++++++++++++++++++++++--- test/gfx/column_major_palette.flags | 1 + test/gfx/column_major_palette.out.pal | 1 + test/gfx/column_major_palette.png | Bin 0 -> 1226 bytes 6 files changed, 100 insertions(+), 29 deletions(-) create mode 100644 test/gfx/column_major_palette.flags create mode 100644 test/gfx/column_major_palette.out.pal create mode 100644 test/gfx/column_major_palette.png diff --git a/include/gfx/color_set.hpp b/include/gfx/color_set.hpp index c49d67c0..13c473ff 100644 --- a/include/gfx/color_set.hpp +++ b/include/gfx/color_set.hpp @@ -21,11 +21,7 @@ public: // Adds the specified color to the set, or **silently drops it** if the set is full. void add(uint16_t color); - enum ComparisonResult { - NEITHER, - WE_BIGGER, - THEY_BIGGER = -1, - }; + enum ComparisonResult { INCOMPARABLE, SUBSET_OR_EQUAL, STRICT_SUPERSET }; ComparisonResult compare(ColorSet const &other) const; size_t size() const; diff --git a/src/gfx/color_set.cpp b/src/gfx/color_set.cpp index 2b61309b..f0fb4cf4 100644 --- a/src/gfx/color_set.cpp +++ b/src/gfx/color_set.cpp @@ -42,29 +42,44 @@ void ColorSet::add(uint16_t color) { } ColorSet::ComparisonResult ColorSet::compare(ColorSet const &other) const { - // This works because the sets are sorted numerically + // This algorithm works because the sets are sorted numerically assume(std::is_sorted(RANGE(_colorIndices))); assume(std::is_sorted(RANGE(other._colorIndices))); - auto ours = _colorIndices.begin(), theirs = other._colorIndices.begin(); - bool weBigger = true, theyBigger = true; + auto self_item = begin(), other_item = other.begin(); + auto const self_end = end(), other_end = other.end(); + bool self_has_unique = false, other_has_unique = false; - while (ours != end() && theirs != other.end()) { - if (*ours == *theirs) { - ++ours; - ++theirs; - } else if (*ours < *theirs) { - ++ours; - theyBigger = false; - } else { // *ours > *theirs - ++theirs; - weBigger = false; + while (self_item != self_end && other_item != other_end) { + if (*self_item < *other_item) { + // *self_item is not in other, so self cannot be a strict subset of other + self_has_unique = true; + ++self_item; + } else if (*self_item > *other_item) { + // *other_item is not in self, so self cannot be a strict superset of other + other_has_unique = true; + ++other_item; + } else { + // *self_item == *other_item, so continue comparing + ++self_item; + ++other_item; + } + + // Early return optimization: we already know self and other are incomparable + if (self_has_unique && other_has_unique) { + return INCOMPARABLE; } } - weBigger &= theirs == other.end(); - theyBigger &= ours == end(); - return theyBigger ? THEY_BIGGER : (weBigger ? WE_BIGGER : NEITHER); + // Check if either color set has unique items remaining after one set has been fully iterated + if (self_item != self_end) { + self_has_unique = true; + } + if (other_item != other_end) { + other_has_unique = true; + } + + return self_has_unique ? other_has_unique ? INCOMPARABLE : STRICT_SUPERSET : SUBSET_OR_EQUAL; } size_t ColorSet::size() const { diff --git a/src/gfx/process.cpp b/src/gfx/process.cpp index 4d0673ba..c963ee96 100644 --- a/src/gfx/process.cpp +++ b/src/gfx/process.cpp @@ -1027,22 +1027,65 @@ void process() { // Insert the color set, making sure to avoid overlaps for (size_t n = 0; n < colorSets.size(); ++n) { switch (colorSet.compare(colorSets[n])) { - case ColorSet::WE_BIGGER: - colorSets[n] = colorSet; // Override them - // Remove any other color sets that we encompass - // (Example [(0, 1), (0, 2)], inserting (0, 1, 2)) + case ColorSet::STRICT_SUPERSET: + // Override the previous color set that this one is a strict superset of + + if (checkVerbosity(VERB_DEBUG)) { + fprintf( + stderr, + "- Tile (%" PRIu32 ", %" PRIu32 ") overrides color set #%zu: [", + tile.x, + tile.y, + n + ); + for (uint16_t color : colorSets[n]) { + fprintf(stderr, "$%04x, ", color); + } + fputs("] becomes [", stderr); + for (uint16_t color : colorSet) { + fprintf(stderr, "$%04x, ", color); + } + fputs("]\n", stderr); + } + + colorSets[n] = colorSet; + // Remove any other color sets that we are also a strict superset of + // (example: we have [(0, 1), (0, 2)] and are inserting (0, 1, 2)) + for (size_t m = n + 1; m < colorSets.size();) { + if (colorSet.compare(colorSets[m]) != ColorSet::STRICT_SUPERSET) { + ++m; + } else { + // We are about to remove a set, which will shift sets that may be + // already referenced in the attrmap: re-number to keep it consistent + for (size_t i = 0; i + 1 < attrmap.size(); ++i) { + AttrmapEntry &entry = attrmap[i]; + if (entry.colorSetID == AttrmapEntry::transparent + || entry.colorSetID == AttrmapEntry::background) { + continue; + } + if (entry.colorSetID == m) { + entry.colorSetID = n; + } else if (entry.colorSetID > m) { + --entry.colorSetID; + } + } + colorSets.erase(colorSets.begin() + m); + } + } [[fallthrough]]; - case ColorSet::THEY_BIGGER: - // Do nothing, they already contain us + case ColorSet::SUBSET_OR_EQUAL: + // Use the previous color set that this one is a subset or duplicate of attrs.colorSetID = n; goto continue_visiting_tiles; // Can't `continue` from within a nested loop - case ColorSet::NEITHER: - break; // Keep going + case ColorSet::INCOMPARABLE: + // This color set is incomparable so far, so keep going + break; } } + // This color set is incomparable with all previous ones, so add it as a new one attrs.colorSetID = colorSets.size(); if (colorSets.size() == AttrmapEntry::background) { // Check for overflow fatal( @@ -1050,6 +1093,21 @@ void process() { AttrmapEntry::transparent ); } + + if (checkVerbosity(VERB_DEBUG)) { + fprintf( + stderr, + "- Tile (%" PRIu32 ", %" PRIu32 ") adds color set #%zu: [", + tile.x, + tile.y, + colorSets.size() + ); + for (uint16_t color : colorSet) { + fprintf(stderr, "$%04x, ", color); + } + fputs("]\n", stderr); + } + colorSets.push_back(colorSet); continue_visiting_tiles:; } diff --git a/test/gfx/column_major_palette.flags b/test/gfx/column_major_palette.flags new file mode 100644 index 00000000..2e52e40e --- /dev/null +++ b/test/gfx/column_major_palette.flags @@ -0,0 +1 @@ +-Z diff --git a/test/gfx/column_major_palette.out.pal b/test/gfx/column_major_palette.out.pal new file mode 100644 index 00000000..193f8e5c --- /dev/null +++ b/test/gfx/column_major_palette.out.pal @@ -0,0 +1 @@ +"XH9g‘bpIH9gS"g \ No newline at end of file diff --git a/test/gfx/column_major_palette.png b/test/gfx/column_major_palette.png new file mode 100644 index 0000000000000000000000000000000000000000..d4176ca8a034639de3b30cc499fed1c146d480f7 GIT binary patch literal 1226 zcmeAS@N?(olHy`uVBq!ia0vp^4nSanMprB-lYeY$Kep*R+Vo@qXKw@TIiJqTph=Qq} zp`M|UFr#}3P|=Rm2+uT6Pb~%xAcvJfijkFp5y41z#9 z3Wzh?S-|2sKsE>@05M2Ej7GPWfq|KUVFEh?3s8-Lk+A{e0*JXFJ**2LW=#XKL4XNp z5))WukfjBX1=VF}U;vW6xqQ~tliRuDfhLxDx;TbpIA5J5!1vuwQK6tpB@!&mlTE;F+%#YMf{^`)zHeW&|faFW)2_o?XF z-14+{X`3x9ufCan@?81zx4X@)XYMqpzod5h+NC)&@BaOwe1iS&x|y+e?wtJbt-C$# zT6C;@ce&B4kJ-gDj6Z6;`zEvfp~)vF##gt_%&ZWxFZm5-PK?=QbpOP~x*M_H$1^r> z_?#0Hr`e!uduHm=N5PxcuVIL;nxXA|ba{Gyn(@8X8<%#o7xP5b)$EUcBC+N80@ZoK z59dmGAGJLd^s&QM?S#9wKcZgVzssYy|0e@PZw{n zVZU*Hv6a;C$}788eogHAcC-J9jK;iMX}`S~v)otRT~@aC*2h)ihW)qa@2{;1aX9_f z{99Yygq!_g3v9m~oo@Oq&Fu5{lTTfyN|y8V8wFIm&7J&l(ckCC>xF#2O|@N6mU2W+ zc;j)GceZSAUq550*09@s!+7)j_-*`WF57Bt)8Dt~xVl>jpT;}0ITrJe*y%e&YMnEi zXEFbRoe8^$`|Q=)*ZB@S{+2Pvc6P=L=B~|WckId7|L5Htd6u`*&l(=(6dTy>+&feD z_rHWM*)yN+jZW|W^Lpm&mlC;U^>Zb<=gaNWxn*DV&&<&zZs*21eV?B@$(gc?oPU?< zH9`EX;j7;cpNkdei&*@$`C82Iw{HK>XYA|QXJ6kQCu&)@?vl@qs9BGHzPNt$J@b$6 zL3Zq$ET;>-Uy>pDFr)v@;p=Dr-C8YD@Z^m)>w>?3gSlTy+-J34b0>Pr(np2W|Nm_G zTgMo@X@2Y*&A2W8*47#OH=4hZ>8kZgyLqDdo94{DGfVrn)TZ(;uw8az$?}x`&KuL8 zAK3A6`!lPbvmGW$lwJSuTk+GQ`qZk`uWhcMI^SxuYQE8N>E@N&`wOzopr08!Np8UO$Q literal 0 HcmV?d00001