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

@@ -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;

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 {

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:;
 	}

test/gfx/column_major_palette.flags

@@ -0,0 +1 @@
+-Z

test/gfx/column_major_palette.out.pal

@@ -0,0 +1 @@
+"XH9g礎pIH9gS"g