Fix rgbgfx -Z palette overgeneration on merged color sets by vulcandth · Pull Request #1912 · gbdev/rgbds

vulcandth · 2026-04-03T21:26:32Z

Fix rgbgfx -Z palette overgeneration on merged color sets

Closes #1887.

Summary

This fixes the RGBGFX bug where automatic palette generation could produce more palettes under -Z than without it for the same image.

The issue-reproducer Gloom image now generates 3 palettes with and without -Z, matching the intended behavior.

Root Cause

The remaining bug was not in ColorSet::compare; that part of the color-sets branch was already correct.

The problem was in color-set generation after a strict superset was found.
When a tile introduced a color set that was a strict superset of an existing one, RGBGFX only overrode the first matching subset and then stopped. Any other existing strict subsets were left behind as redundant color sets.

That made the final set of color sets depend on tile traversal order. In the Gloom -Z case, those redundant subset classes survived long enough to force a fourth palette later in the packing step.

Changes

Collapse all existing strict-subset color sets when inserting a new strict superset.
Remap earlier attrmap entries to the surviving color-set ID as redundant subsets are removed.
Move the temporary tile/color-set debug logging behind VERB_DEBUG and off stdout, which also fixes the known write_stdout regression caused by the debug instrumentation.
Add a Gloom-based test/gfx regression fixture for the -Z reproduction.

Testing

Ran the direct reproduction with verbose output and confirmed that rgbgfx -P -Z on the Gloom fixture now builds 3 color sets and 3 palettes instead of 4 palettes.
Ran focused regression checks for the existing base_ids gfx case and the write_stdout path.
Ran the full test/gfx suite: All 235 tests passed!

ISSOtm · 2026-04-03T21:49:26Z

src/gfx/color_set.cpp

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


Might as well use a spaceship at this point, since we use C++20, huh?

Some older system doesn't support <=>: #1228 (comment)

This unfortunately doesn't record which old system that would be. And since two years have passed, we can give it a new try, I think. But I don't care much, anyway.

ISSOtm

The logic added in that third commit looks correct, but I'm feeling wary of it.

ISSOtm · 2026-04-03T22:14:16Z

src/gfx/process.cpp

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


I'm a little wary of this attrmap rescanning, as I fear it could blow up in some cases. rsgbgfx handles this image correctly even without this logic, so I feel it's preferable to avoid it if possible.

ISSOtm · 2026-04-03T22:14:49Z

src/gfx/process.cpp

+				for (size_t m = n + 1; m < colorSets.size();) {
+					if (colorSet.compare(colorSets[m]) != ColorSet::STRICT_SUPERSET) {
+						++m;
+						continue;


I'd rather make the rest of the loop a else, for clarity.

Rangi42 · 2026-04-04T17:53:57Z

src/gfx/process.cpp

+				// 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))


This logic is deeply indented and fairly self-contained, so I'd extract it into a static helper function.

Rangi42 · 2026-04-04T17:54:47Z

src/gfx/process.cpp

+				// 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);


If we're keeping these debug logs, may as well format them more like the rest.

Suggested change

fprintf(stderr, "- tile (%" PRIu32 ", %" PRIu32 ") overrides color set #%zu: [", tile.x, tile.y, n);

fprintf(stderr, "- Tile (%" PRIu32 ", %" PRIu32 ") overrides color set #%zu: [", tile.x, tile.y, n);

Rangi42 · 2026-04-04T17:54:59Z

src/gfx/process.cpp

 		}
+
+		if (checkVerbosity(VERB_DEBUG)) {
+			fprintf(stderr, "- tile (%" PRIu32 ", %" PRIu32 ") adds color set #%zu: [", tile.x, tile.y, colorSets.size());


Suggested change

fprintf(stderr, "- tile (%" PRIu32 ", %" PRIu32 ") adds color set #%zu: [", tile.x, tile.y, colorSets.size());

fprintf(stderr, "- Tile (%" PRIu32 ", %" PRIu32 ") adds color set #%zu: [", tile.x, tile.y, colorSets.size());

Rangi42 · 2026-04-04T17:55:29Z

test/gfx/issue1887_gloom_z.flags

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


Suggested change

-Z

-Z

Rangi42 · 2026-04-04T17:56:21Z

test/gfx/issue1887_gloom_z.png

The individual tiles of this could have their pixels scrambled without changing the colors of each tile or the overall size, so we don't have a poketcg image sitting in the repo.

Rangi42 · 2026-04-04T17:57:29Z

make format will run clang-format and fix the failing "Code format checking" CI step.

Rangi42 and others added 3 commits April 3, 2026 16:46

[in progress] Fix color set generation depending on iteration order (-Z)

d7902b2

Match compare logic output with Rust

1afc0fe

Fix rgbgfx color-set superset collapsing

3c0d042

Rangi42 added bug Unexpected behavior / crashes; to be fixed ASAP! rgbgfx This affects RGBGFX refactoring This PR is intended to clean up code more than change functionality labels Apr 3, 2026

Rangi42 added this to the 1.0.2 milestone Apr 3, 2026

Rangi42 self-requested a review April 3, 2026 21:33

Rangi42 self-assigned this Apr 3, 2026

ISSOtm reviewed Apr 3, 2026

View reviewed changes

ISSOtm requested changes Apr 3, 2026

View reviewed changes

Rangi42 removed their assignment Apr 3, 2026

Rangi42 requested changes Apr 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix rgbgfx -Z palette overgeneration on merged color sets#1912

Fix rgbgfx -Z palette overgeneration on merged color sets#1912
vulcandth wants to merge 3 commits intogbdev:masterfrom
vulcandth:color-sets

vulcandth commented Apr 3, 2026

Uh oh!

ISSOtm Apr 3, 2026

Uh oh!

Rangi42 Apr 3, 2026

Uh oh!

ISSOtm Apr 3, 2026

Uh oh!

ISSOtm left a comment

Uh oh!

ISSOtm Apr 3, 2026

Uh oh!

ISSOtm Apr 3, 2026

Uh oh!

Rangi42 Apr 4, 2026

Uh oh!

Rangi42 Apr 4, 2026

Uh oh!

Rangi42 Apr 4, 2026

Uh oh!

Rangi42 Apr 4, 2026

Uh oh!

Rangi42 Apr 4, 2026

Uh oh!

Rangi42 commented Apr 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// 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))

	fprintf(stderr, "- tile (%" PRIu32 ", %" PRIu32 ") overrides color set #%zu: [", tile.x, tile.y, n);
	fprintf(stderr, "- Tile (%" PRIu32 ", %" PRIu32 ") overrides color set #%zu: [", tile.x, tile.y, n);

		@@ -0,0 +1 @@
		-Z No newline at end of file

Uh oh!

Conversation

vulcandth commented Apr 3, 2026

Summary

Root Cause

Changes

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ISSOtm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rangi42 commented Apr 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants