Sort Pokemon and trainer sprite palettes, with Makefile-specified exceptions #1137

Rangi42 · 2024-08-28T16:57:13Z

This avoids the need to define their order via indexed PNG palettes

It also avoids the need to use tools/palfix.py on custom sprites

Be sure to squash-merge this when/if doing so

…eptions This avoids the need to define their order via indexed PNG palettes It also avoids the need to use tools/palfix.py on custom sprites Fixes pret#1136

Rangi42 · 2024-08-28T17:01:46Z

The algorithm here is slightly different than what was described in the issue:

Both front.2bpp and back.2bpp are created using the same normal.gbcpal input. The normal.gbcpal is generated by combining front.gbcpal and back.gbcpal. This means that front and back can each use a subset of the overall palette (e.g. Jigglypuff's back doesn't use its eye-blue color).

tools/gbcpal.c checks the colors by sorting them, filtering out duplicates, also filtering our black and white, and then allowing zero, one, or two colors. This allows for any valid sprite, including "all black and white" (fills in the middle colors as white and black also) or "all color, no black or white" (if we checked for "first color is white" or "last color is black", this would fail).

mid-kid

I love this! It is clean and simple, and will be relatively unintrusive to current workflows.

...I just have a few nitpicks.

tools/gbcpal.c

mid-kid · 2024-08-28T21:52:06Z

tools/gbcpal.c

+}
+
+double luminance(struct Color color) {
+	return 0.299 * color.r * color.r + 0.587 * color.g * color.g + 0.114 * color.b * color.b;


Please break up this line, and while I believe these numbers, do you have any source/explanation for them? I've never looked into this, and I would've just naïvely summed the colors.

This is the standard conversion from RGB to Y (luminance), used by jpeg encoding and probably lots of other things.
I use the exact same coefficients here, for example.

What breakup do you have in mind? It's the same function I've used in a few Python helper scripts:

def luminance(c): r, g, b = c return 0.299 * r**2 + 0.587 * g**2 + 0.114 * b**2

Oh, actually I didn't notice at first that you are squaring each color component. That is not the standard luminance conversion used by jpeg etc. But it's probably the standard somewhere! I've just never seen it before. I'd be curious to hear the benefits.

I've used it for many years, quoting from some source that defined luminance as the sqrt of sum of squares (i.e. a Euclidean distance) but omitting the sqrt when it's unnecessary (e.g. for comparisons).

Google leads to https://stackoverflow.com/questions/596216/formula-to-determine-perceived-brightness-of-rgb-color leads to https://alienryderflex.com/hsp.html which looks familiar (and IMO based on the StackOverflow illustrations is the best choice of the given options).

tools/gbcpal.c

mid-kid · 2024-08-28T22:11:18Z

tools/gbcpal.c

+	uint16_t first, second;
+	if (num_colors == 0) {
+		// colors are white and black
+		first = 0x7fff;
+		second = 0x0000;
+	} else if (num_colors == 1) {
+		// colors are both the same one
+		first = pack_color(colors[0]);
+		second = first;
+	} else if (num_colors == 2) {
+		// colors are light and dark
+		first = pack_color(colors[0]);
+		second = pack_color(colors[1]);
+		if (reverse) {
+			// darker color is first
+			uint16_t tmp = first;
+			first = second;
+			second = tmp;
+		}
+	} else {
+		error_exit("%s: more than 2 colors besides black and white (%zu)\n", out_filename, num_colors);
+	}


I think the algorithm of white + reversed?(colors) + black * (1 + (max_colors - num_colors)) would make this cleaner, something like

if (num_colors > (MAX_COLORS - 2)) error; uint16_t colors[MAX_COLORS]; int c = 0; colors[c++] = 0x7fff; // First color is white for (int i = 0; i < num_colors; i++) colors[c++] = pack_color(colors[reverse ? num_colors - 1 - i : i]); // Add the maximum of 2 custom colors while (c < MAX_COLORS / 2) colors[c++] = 0x7fff; // If not enough colors are present, fill up to half with white while (c < MAX_COLORS) colors[c++] = 0; // Fill the remainder with black // Alternatively: fill the remaining colors from a predefined grayscale array, to make battle anims nicer? for (c < MAX_COLORS; c++) colors[c] = pack_color((struct Color){32/4*(3-c), 32/4*(3-c), 32/4*(3-c)});

This is less verbose and I like the option of expanding this for more colors/maybe gen3 in the future?

I find this a lot harder to understand and grasp "when my input was such-and-such, what order will my colors be in?" I think the current verbose code is useful for being able to point users to it and show just how this new approach works.

Extending it to support 4bpp is a cool idea, but that would involve more rewriting than just this section, and it's a short enough file that I don't think we need to future-proof it like that already. (YAGNI; a gbapal.c tool will be its own thing, not much harder to write than this was.)

fill the remaining colors from a predefined grayscale array

I did consider this, but it would be error-prone. Imagine a front sprite that uses one or two colors, and a back sprite using zero, just black+white. Each one gets gbapal.c run on it, so the back sprite now has two gray colors... and then generating normal.gbcpal fails because there are too many combined colors.

mid-kid · 2024-08-28T22:24:35Z

Makefile

@@ -315,7 +350,8 @@ gfx/mobile/stadium2_n64.2bpp: tools/gfx += --trim-whitespace
 		tools/gfx $(tools/gfx) --depth 1 -o $@ $@)

 %.gbcpal: %.png
-	$(RGBGFX) --colors embedded -p $@ $<
+	$(RGBGFX) -p $@ $<
+	tools/gbcpal $(tools/gbcpal) $@


Make this tools/gbcpal $(tools/gbcpal) $@ || { rm -f $@; false }, to remove the file and stop on error (or use a differently named temporary file/separate rule...).

Can you please explain further what this would be protecting against? Note that if tools/gbcpal has an error, it would exit before the opening the file for writing with write_u8 at the very end.

Makefile

Resolved

Rangi42 · 2024-09-07T05:13:34Z

@mid-kid Please leave an updated review/comments when you have time? (Also would welcome others' feedback on this.)

dannye

Seems like a good way to enforce that the front and back pics use essentially the same palette.

Sort Pokemon and trainer sprite palettes, with Makefile-specified exc…

5a9bfb4

…eptions This avoids the need to define their order via indexed PNG palettes It also avoids the need to use tools/palfix.py on custom sprites Fixes pret#1136

Rangi42 added refactoring tools labels Aug 28, 2024

Rangi42 requested a review from mid-kid August 28, 2024 16:57

Rangi42 requested a review from vulcandth August 28, 2024 17:11

Rangi42 added 3 commits August 28, 2024 13:17

Use long option flags for rgbgfx, same as tools/gfx

f99f0e6

Separate the usual rgbgfx flags from the --color specification

c0173db

Reorganize some Makefile rules

4783fff

mid-kid previously requested changes Aug 28, 2024

View reviewed changes

Pass a uint16_t to unpack_color

56f20b2

Rangi42 requested a review from mid-kid August 31, 2024 15:08

Rangi42 added 3 commits August 31, 2024 11:15

Use terser comparator definitions

97f7888

Require an explicit separate output and input file

900ce2d

Use even terser comparator definitions

4d462e9

Rangi42 requested a review from dannye September 6, 2024 02:10

dannye approved these changes Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort Pokemon and trainer sprite palettes, with Makefile-specified exceptions #1137

Sort Pokemon and trainer sprite palettes, with Makefile-specified exceptions #1137

Rangi42 commented Aug 28, 2024 •

edited

Loading

Rangi42 commented Aug 28, 2024

mid-kid left a comment •

edited

Loading

mid-kid Aug 28, 2024

dannye Aug 28, 2024

Rangi42 Aug 31, 2024

dannye Aug 31, 2024

Rangi42 Sep 1, 2024

mid-kid Aug 28, 2024

Rangi42 Aug 31, 2024

Rangi42 Aug 31, 2024

mid-kid Aug 28, 2024 •

edited

Loading

Rangi42 Aug 31, 2024

Rangi42 commented Sep 7, 2024

dannye left a comment

Sort Pokemon and trainer sprite palettes, with Makefile-specified exceptions #1137

Are you sure you want to change the base?

Sort Pokemon and trainer sprite palettes, with Makefile-specified exceptions #1137

Conversation

Rangi42 commented Aug 28, 2024 • edited Loading

Rangi42 commented Aug 28, 2024

mid-kid left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mid-kid Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rangi42 commented Sep 7, 2024

dannye left a comment

Choose a reason for hiding this comment

Rangi42 commented Aug 28, 2024 •

edited

Loading

mid-kid left a comment •

edited

Loading

mid-kid Aug 28, 2024 •

edited

Loading