Fix 'Reorganize colors only' - #49
Conversation
|
Hi! I'll check this proper when I have some time available. For now, I've updated the remote runners to allow checks to run correctly, and fixed a formatting issue that was causing them to fail. You'll have to add these changes to your PR by merging or rebasing the master branch, then the checks can run properly. I can already tell the formatting one is going to fail because some of the lines you added are misaligned, so try to fix that as well before you push again. |
weird issues where the converter did not recognize, say, (255, 0, 0), because it was looking for (255, 0, 0, 255).
fb74c2a to
e5c0947
Compare
| self._transparent_color: tuple[int, int, int, int] | None = None | ||
| self._transparent_color: tuple[int, int, int] | None = None | ||
| if transparent_color is not None: | ||
| self._transparent_color = ( | ||
| transparent_color[0], | ||
| transparent_color[1], | ||
| transparent_color[2], | ||
| 255, |
There was a problem hiding this comment.
Why is this change needed again? I see this value is passed to a couple functions, and I can't be sure nothing will break if this is changed here.
For example, there's a check in TransparencyHandler that relies on the color from the image being an exact match of this color to replace it. Is that not working properly right now?
There was a problem hiding this comment.
Correct, that check breaks because the input image has its alpha channel removed entirely (see line 69), but the color it is matching on still has an alpha channel. See the below image for the results with this line unmodified.
When looking for #FF0000 as a transparency color, whilst it correctly is placed into the first entry of each sub-palette, all instances of it in the image itself are not correctly marked as transparency and instead take up additional palette entries.
There was a problem hiding this comment.
What if the image is in RGBA mode and its pixels do have an alpha channel?
There was a problem hiding this comment.
This has to be a change in Pillow. Look at the line right above. I convert images to RGBA and then to RGBa. RGBa still presented it's alpha channel before, now maybe it doesnt.
In that case the minimum supported Pillow version would need to be updated.
https://pillow.readthedocs.io/en/stable/handbook/concepts.html
There was a problem hiding this comment.
Ah, I believe I can see the cause of the issue. It's to do with the legacy converter in particular (which reorganize colors invokes);
Here we can see the legacy converter changes the image to RGB, not RGBa. That being the case, should the legacy converter be amended to use RGBa instead?
Or alternatively, in line 65 of the legacy converter, would it be easier to just strip the alpha channel from the transparent color? Just purely for when using "reorganize colors only"..?
There was a problem hiding this comment.
This has to be a change in Pillow. Look at the line right above. I convert images to RGBA and then to RGBa. RGBa still presented it's alpha channel before, now maybe it doesnt.
Interesting theory. If anyone can confirm this with a debugger (or just changing the code to print the RGBa color) and see what comes up, that would be great. @Deeshura could you test this, since you already have an editable setup?
Here we can see the legacy converter changes the image to RGB, not RGBa. That being the case, should the legacy converter be amended to use RGBa instead?
It seems like it uses both the original object (img) and the new one, which is in RGB mode (_img). Surely there's a reason for this distinction?
Or alternatively, in line 65 of the legacy converter, would it be easier to just strip the alpha channel from the transparent color? Just purely for when using "reorganize colors only"..?
The legacy converter is passed the transparency color, so it would already not have an alpha channel after your change (self._transparent_color is passed to do_simple_convert on line 182, which is defined here, and that calls the legacy converter passing that same transpareny color).
There was a problem hiding this comment.
Interesting theory. If anyone can confirm this with a debugger (or just changing the code to print the RGBa color) and see what comes up, that would be great. @Deeshura could you test this, since you already have an editable setup?
See below for two test cases using the non-legacy image converter set to "Dither: Floyd-Steinberg". In both cases, the transparency color is defined as #FF0000
Test 1, where self._transparent_color is defined with an alpha channel.
Test 2, where self._transparent_color is defined without one.
As we can see, the non-legacy converter, which stores the image in RGBa, needs the transparency channel defined in order to work correctly. My changes to remove the transparency channel from self._transparent_color actually, currently, breaks the non-legacy converter.
The legacy converter is passed the transparency color, so it would already not have an alpha channel after your change (self._transparent_color is passed to do_simple_convert on line 182, which is defined here, and that calls the legacy converter passing that same transpareny color).
Yes. My apologies, I don't think I explained myself clearly. What I meant to ask, is should I revert my changes to where self._transparent_color is defined here, (because this breaks the non-legacy converter) and instead edit where the transparency color is passed to self._transparent_color in the legacy converter here so that the defined transparency color only has its alpha channel removed when invoking the legacy converter with "Reorganize colors only"?
There was a problem hiding this comment.
That would make both versions work, right? If so, it seems like a better solution, yeah.
Also, did you check if the RGBa conversion makes the color lose its alpha channel or not?
There was a problem hiding this comment.
That would make both versions work, right? If so, it seems like a better solution, yeah.
Alright, I will make that edit and test to see how it goes then.
Also, did you check if the RGBa conversion makes the color lose its alpha channel or not?
Per the results above, whilst the image is in RGBa mode (as used by the non-legacy converter), including an alpha channel in the transparency color is required for the transparency color to be detected properly. So RGBa images do, in fact, properly retain their alpha channel. I was just mistaken as to the cause of the issue.
… causing" This reverts commit c402178.
the legacy converter
|
Okay, I believe this fixes the transparency issues. Now, the alpha channel is removed from the transparency color purely in the legacy converter. This means that both the main converter and the legacy converter now handle transparency colors correctly. The results of two tests with this code, in both cases with the transparency color set to #FF0000. Test 1, set to "Dither: Ordered". [see endnote] 
Test 2, set to "Reorganize colors only" (the legacy converter). 
Everything is working as expected, as far as I can tell. endnote: note that for demonstration purposes, i've manually changed the first color in each sub-palette to FF0000, to show that it has moved FF0000 to the first slot of each sub-palette as transparent colors require. this mode changes the transparency color to 000000 normally after moving it to the first slot of the sub-palette. There's no functional issue with this, however. |
Frostbyte0x70
left a comment
Frostbyte0x70
left a comment
There was a problem hiding this comment.
Looks good to me then! I'll admit I'm not very familiar with how the converter is supposed to work, so I can't tell with 100% confidence that this will work without issues, but since both converters are consistent with each other now and there's no errors anymore, it at least seems like an upgrade to me.
I'll merge it tomorrow if there's no objections.
3 participants
"Amends 'Reorganize colors only' to use raw indexing, fixing 'too many values to unpack (expected 3)' Also corrects an edge case with how transparent colors are handled."