Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rgbgfx -c modulo to accept embedded palettes with extra used colors #1067

Open
pinobatch opened this issue Sep 29, 2022 · 17 comments
Open
Labels
enhancement Typically new features; lesser priority than bugs rgbgfx This affects RGBGFX

Comments

@pinobatch
Copy link
Member

pinobatch commented Sep 29, 2022

Say I have an indexed image with 16 input colors, and I want each input color to be reduced modulo 2**bit_depth in the output. For example, input colors 1, 5, 9, and 13 would become 1 in the output. I have relied on this behavior of a different tool in a previous project targeting an 8-bit console with 2bpp character graphics. Would it be reasonable to add -c modulo, which is -c embedded without the fatal error on an out-of-bounds color value?

If there's no serious objection, I plan to attempt the PR myself.

Test case: testcase1.png has 8 colors and uses all 8. testcase2.png has 8 colors and uses the first 4; all color index values have been reduced modulo 4. testcase3.png has 4 colors. Converting them with -c modulo should produce the same character data as converting testcase3.png with -c embedded. (I've used two of them as test cases for #1064 as well.)

testcase1
testcase2
testcase3

@ISSOtm
Copy link
Member

ISSOtm commented Sep 29, 2022

Isn't it sufficient to generate the palette with -c embedded, ignore the attributes, and truncate the palette data?

$ rgbgfx -c embedded -p >(dd of=testcase1.pal bs=8 count=1 status=none) -o testcase1.2bpp testcase1.png

@pinobatch
Copy link
Member Author

Most tiles in the test case contain both pixels of color 0 and pixels of color 4. I had planned to try the command line snippet in ISSOtm's reply to observe how rgbgfx master reacts to testcase1.png. However, I do not understand the snippet.

  1. What does >( do? I don't know how to search the manual for punctuation. Web search engines strip punctuation, and searching within man bash on Ubuntu 22.04 gives Invalid pattern (press RETURN).
  2. Why does this snippet appear to overwrite testcase1.png? Or am I misunderstanding the of= option of dd?

@ISSOtm
Copy link
Member

ISSOtm commented Sep 29, 2022

  1. What does >( do? I don't know how to search the manual for punctuation. Web search engines strip punctuation, and searching within man bash on Ubuntu 22.04 gives Invalid pattern (press RETURN).

The first result for Ctrl+F'ing >( in the single-page Bash manual explains "process substitution".

man pipes to a pager, and modern pagers tend to treat the provided search pattern as a regex. Entering >\( should work.

  1. Why does this snippet appear to overwrite testcase1.png? Or am I misunderstanding the of= option of dd?

Yes, I had a brainfart. Edited the line to be correct.

@pinobatch
Copy link
Member Author

It is not sufficient, at least with this particular test case.

$ /path/to/rgbgfx --version
rgbgfx v0.6.0-rc2-12-g023884d2
$ /path/to/rgbgfx -c embedded -p >(dd of=testcase1.pal bs=8 count=1 status=none) -o testcase1.2bpp testcase1.png
error: Could not fit tile colors [$0000, $6400] in specified palettes
error: Could not fit tile colors [$0000, $0019, $6400] in specified palettes
error: Could not fit tile colors [$0000, $0320, $6400] in specified palettes
error: Could not fit tile colors [$0000, $0339, $6400] in specified palettes
error: Could not fit tile colors [$0000, $6400, $6419] in specified palettes
error: Could not fit tile colors [$0000, $6400, $6720] in specified palettes
error: Could not fit tile colors [$0000, $6400, $6739] in specified palettes
note: The following palette was specified:
        [$0000, $0019, $0320, $0339]
Conversion aborted after 7 errors

I have made an additional testcase4.png, in which color 4's RGB value has been changed to equal that of color 0 and the values have not been changed. This gives fewer errors though still aborts the conversion.

$ /path/to/rgbgfx -c embedded -p >(dd of=testcase4.pal bs=8 count=1 status=none) -o testcase4.2bpp testcase4.png
error: Could not fit tile colors [$0000, $6419] in specified palettes
error: Could not fit tile colors [$0000, $6720] in specified palettes
error: Could not fit tile colors [$0000, $6739] in specified palettes
note: The following palette was specified:
        [$0000, $0019, $0320, $0339]
Conversion aborted after 3 errors

testcase4

@pinobatch
Copy link
Member Author

Two additional test cases:

  • testcase5.png has all color 4 pixels overwritten with color 0, where colors 0 and 4 still have distinct RGB values
  • testcase6.png has all color 4 pixels overwritten with color 0, and color 4's RGB values have been changed to those of color 0

testcase5.png is probably the most similar statistically to the images that I encountered when building character-replacement animation in the previous project.
testcase5
testcase6

@ISSOtm
Copy link
Member

ISSOtm commented Sep 29, 2022

I think the errors are caused by -c embedded only accepting 4 input colours, instead of all. I'll have to look into that.

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbgfx This affects RGBGFX labels Sep 30, 2022
@ISSOtm
Copy link
Member

ISSOtm commented Sep 30, 2022

This is why:

rgbds/src/gfx/process.cpp

Lines 567 to 569 in a47da5f

if (embPalSize > options.maxOpaqueColors()) { // Ignore extraneous colors if they are unused
embPalSize = options.maxOpaqueColors();
}

So if I'm understanding correctly, this should instead be:

if (embPalSize > options.maxOpaqueColors() * options.nbPalettes) {
    fatal("Too many colors");
}

, right?

@ISSOtm
Copy link
Member

ISSOtm commented Sep 30, 2022

I think my rationale for this condition was that it's unclear what should be done with more than 4 colours but the number cannot be divided by 4.

@pinobatch
Copy link
Member Author

If there are 7 colors 0-6, then colors 4-6 should become colors 0-2 in the output.
If there are 7 colors 0-3 and 5-7, as in test cases 5 and 6, then colors 5-7 should become 1-3 in the output.

@pinobatch
Copy link
Member Author

Or to put it another way: -c embedded is like -Werror=excess-colors and -c modulo is like -Wno-excess-colors

@ISSOtm
Copy link
Member

ISSOtm commented Oct 1, 2022

The problem is that we don't know where the colours are being used at this point, so "0–3 and 5–7" is a non-starter.

@Rangi42
Copy link
Contributor

Rangi42 commented Oct 1, 2022

The three test cases, for reference:

image

@Rangi42
Copy link
Contributor

Rangi42 commented Oct 1, 2022

-c embedded without -n 1 should fit this use case. Edit: Apparently it doesn't.

@Rangi42 Rangi42 closed this as completed Oct 1, 2022
@pinobatch
Copy link
Member Author

I am using -c embedded without -n 1, and it is still raising errors for four of the six test cases attached to this issue.

$ /path/to/rgbgfx --version
rgbgfx v0.6.0-rc2-27-gbbe28faa
$ /path/to/rgbgfx -c embedded -o testcase1.2bpp testcase1.png
error: Could not fit tile colors [$0000, $6400] in specified palettes
error: Could not fit tile colors [$0000, $0019, $6400] in specified palettes
error: Could not fit tile colors [$0000, $0320, $6400] in specified palettes
error: Could not fit tile colors [$0000, $0339, $6400] in specified palettes
error: Could not fit tile colors [$0000, $6400, $6419] in specified palettes
error: Could not fit tile colors [$0000, $6400, $6720] in specified palettes
error: Could not fit tile colors [$0000, $6400, $6739] in specified palettes
note: The following palette was specified:
        [$0000, $0019, $0320, $0339]
Conversion aborted after 7 errors
$ /path/to/rgbgfx -c embedded -o testcase2.2bpp testcase2.png
$ /path/to/rgbgfx -c embedded -o testcase3.2bpp testcase3.png
$ /path/to/rgbgfx -c embedded -o testcase4.2bpp testcase4.png
error: Could not fit tile colors [$0000, $6419] in specified palettes
error: Could not fit tile colors [$0000, $6720] in specified palettes
error: Could not fit tile colors [$0000, $6739] in specified palettes
note: The following palette was specified:
        [$0000, $0019, $0320, $0339]
Conversion aborted after 3 errors
$ /path/to/rgbgfx -c embedded -o testcase5.2bpp testcase5.png
error: Could not fit tile colors [$0000, $6419] in specified palettes
error: Could not fit tile colors [$0000, $6720] in specified palettes
error: Could not fit tile colors [$0000, $6739] in specified palettes
note: The following palette was specified:
        [$0000, $0019, $0320, $0339]
Conversion aborted after 3 errors
$ /path/to/rgbgfx -c embedded -o testcase6.2bpp testcase6.png
error: Could not fit tile colors [$0000, $6419] in specified palettes
error: Could not fit tile colors [$0000, $6720] in specified palettes
error: Could not fit tile colors [$0000, $6739] in specified palettes
note: The following palette was specified:
        [$0000, $0019, $0320, $0339]
Conversion aborted after 3 errors

Thus I don't understand "closed this as completed".

@Rangi42 Rangi42 reopened this Oct 2, 2022
@Rangi42 Rangi42 added this to the 0.6.0 milestone Oct 2, 2022
@Rangi42
Copy link
Contributor

Rangi42 commented Oct 2, 2022

@pinobatch Okay, looking at this again I see what the intended goal is.

I think "-c collapse" would be a more clear name than "-c modulo" for users.

This would require PNG pixels to initially be read as their palette indexes, collapsed with index %= 4, and only then mapped to the corresponding RGBA palette color. After that it would work just like -c embedded. (So in your testcase1.png, the -p output would include both palettes.)

That's possible to add of course, but nontrivial since currently indexed PNGs are entirely converted with libpng's png_set_palette_to_rgb before iterating over their pixels.

@ISSOtm
Copy link
Member

ISSOtm commented Oct 2, 2022

This is further rendered difficult by the palette being determined before any image processing is done. Doing so being inherent to RGBGFX's processing structure (how do you even read pixel indices if you don't even have palettes yet?), this is not something that can be changed.

@Rangi42 Rangi42 removed this from the 0.6.0 milestone Oct 2, 2022
@Rangi42
Copy link
Contributor

Rangi42 commented Oct 2, 2022

A preprocessing script could modulo all the PNG's palette indexes by 4 before applying rgbgfx -c embedded:

#!/usr/bin/env python

import sys, png # PyPNG

if len(sys.argv) != 3:
	print('Usage:', sys.argv[0], 'in.png out.png', file=sys.stderr)
	sys.exit(1)
with open(sys.argv[1], 'rb') as file:
	reader = png.Reader(file)
	width, height, rows, info = reader.read()
	rows = list(rows)
if 'palette' not in info:
	print(sys.argv[1], 'does not have an indexed palette!', file=sys.stderr)
	sys.exit(1)
rows = [[idx % 4 for idx in row] for row in rows]
writer = png.Writer(width, height,
	palette=info['palette'], bitdepth=info['bitdepth'], compression=9)
with open(sys.argv[2], 'wb') as file:
	writer.write(file, rows)

Then in your Makefile:

%.mod.png: %.png
	python modulo.py $< $@

%.mod.2bpp: %.mod.png
	rgbgfx -c embedded -o $@ $<

@Rangi42 Rangi42 added this to the 0.6.1 milestone Oct 3, 2022
@Rangi42 Rangi42 modified the milestones: v0.6.1, v0.6.2 Nov 15, 2022
@Rangi42 Rangi42 removed this from the v0.7.0 milestone Oct 31, 2023
@Rangi42 Rangi42 added this to the v0.8.0 milestone Oct 31, 2023
@Rangi42 Rangi42 removed this from the v0.9.0 milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbgfx This affects RGBGFX
Projects
None yet
Development

No branches or pull requests

3 participants