Fix #358 #430

merged 5 commits into from Mar 7, 2015


None yet

2 participants

scurest commented Mar 2, 2015

This fixes #358.

The cause was pretty obvious: transparency was being decided based on color (ie. is this pixel's index's color the color of index 0?) instead of index (ie. is this pixel's index 0?). Unfortunately, since EasyRPG never saw the indices (libpng handled it) this required a new path for handling indexed images with transparency, and then I wanted a new path for grayscale images to handle their transparency, and then I just pulled the rest of the image types out for the sake of symmetry, and that's how I got such an ugly commit :( Changes welcome.

  • I know there's a lot of repeated code, but I'm not sure what the best way to refactor it is.
  • Does ReadPalettedData need the if (bit_depth == 16) check?
  • I used malloc for the temporary buffer in ReadPalettedData just because that's what pixels used, but should I prefer new[]?
  • 8026ee4 gets rid of the temporary array, but it's a little tricky, so I'd appreciate if someone could check it (details in commit message).
  • I didn't test a grayscale image. Someone should probably do that. Seems fine.


(This is me reading the libpng docs)

scurest added some commits Mar 1, 2015
@scurest scurest Fix #358.
Changes the PNG reader for paletted images to use the index of a
pixel to decide its transparency, rather than its color. This caused
pixels with non-zero index to be transparent if their index's color
happened to be the same as index 0's color.

Unfortunately, the indexed color -> RGBA conversion was being done
in libpng, so EasyRPG never saw the indices to begin with. So it
requires fairly invasive changes. For the sake of symmetry, I pulled
out functions for non-indexed images too. Not very pretty.
@scurest scurest Change 'PaletteData' to 'PalettedData' to prevent confusion. cc1cd5b
@scurest scurest Avoid allocating temporary array.
When writing a row of pixel data, instead of using a temporary
buffer of width w to hold the indices, put them in the last
w bytes of the row of pixel data. Then the index->RGBA loop looks

    read nth index (from offset 3w+n)
    convert to color
    write color to nth pixel (offsets 4n..4n+3)

To check we don't overwrite a byte holding an index we'll later
need, we need to know that the offset of the last byte written,
4n+3, is less than the offset of the next byte read, 3w+(n+1).
4n+3 < 3w+(n+1) is equivalent to n < w-2/3, which is true since
n <= w-1.

Thanks scurest, looks a clever way to fix this. I was thinking on getting a transparent mask then applying them after but I was feeling it dirtier as it was requiring to modify the transparent palette calculation code when the issue was really in the PNG handling. Not tested but I guess this should work fine with BMP and XYZ image readers.

About the temporary array allocation removal adds some extra operations on a nested for I think won't make much slower the conversion, so looks good to me 👍.

About bit_depth I guess there are really very few images with 16 bits per channel, they are rare and limited for professional image use (raw conversion from cameras, medical images...), I don't think we will get a game with this, as RPG_RT does not support more than indexed PNGs with 1 channel palette, which don't support 16 bit depth. Only when a game is being designed specifically for EasyRPG Player may use 3-4 channel with 8 bit depth per channel. Using 16 bit depth would be extremely rare. I have never seen a PNG with 16 bit per channel yet.

scurest commented Mar 4, 2015

fdelapena, thanks. About 16-bit depth, it's probably just me being finicky. You're right about indexed images not having 16-bit depths, so I took those out along, with the <8-bit depths for GRAY_ALPHA, RGB, and RGBA, which were also impossible.


Thanks, tested and working fine, now the new methods don't require the parameter:

src/image_png.cpp:108:36: warning: unused parameter 'bit_depth' [-Wunused-parameter]
  png_uint_32 w, png_uint_32 h, int bit_depth, bool transparent,
src/image_png.cpp:162:36: warning: unused parameter 'bit_depth' [-Wunused-parameter]
  png_uint_32 w, png_uint_32 h, int bit_depth, bool transparent,
src/image_png.cpp:190:36: warning: unused parameter 'bit_depth' [-Wunused-parameter]
  png_uint_32 w, png_uint_32 h, int bit_depth,
src/image_png.cpp:206:36: warning: unused parameter 'bit_depth' [-Wunused-parameter]
  png_uint_32 w, png_uint_32 h, int bit_depth,
src/image_png.cpp:221:36: warning: unused parameter 'bit_depth' [-Wunused-parameter]
  png_uint_32 w, png_uint_32 h, int bit_depth,

By the way, if you want to silence these warnings in image_png.cpp in the same pull request would be nice:

src/image_png.cpp: In function 'bool ImagePNG::WritePNG(std::ostream&, int, int, uint32_t*)':
src/image_png.cpp:241:23: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (size_t i = 0; i < width * height; ++i) {
src/image_png.cpp:269:23: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (size_t i = 0; i < height; ++i) {

Other tested formats for this bug (BMP, XYZ) are working fine.

@scurest scurest Remove unused parameters, and other warnings.
Removes unused parameters, some alignment, and signed/unsigned
comparison. Also adds a missing cleanup case to WritePNG.
scurest commented Mar 7, 2015

Oops, that was careless. Should be fixed. Also added some cleanup on the return true path in WritePNG.

@fdelapena fdelapena merged commit 84c82e9 into EasyRPG:master Mar 7, 2015

1 check passed

default Merged build finished.
@scurest scurest deleted the scurest:iss358 branch Mar 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment