fix: Unmultiply before brightening #3407
Conversation
spydon
changed the title
| newPixelData[i + 1] = (color.g * 255).round(); | ||
| newPixelData[i + 2] = (color.b * 255).round(); | ||
| newPixelData[i + 3] = (color.a * 255).round(); | ||
| final a = pixelData[i + 3] / 255; |
There was a problem hiding this comment.
do we need to do this on darken as well? [even though the issue is not visible with the example image on darken I imagine there could be similar thing going on]
or even put it on pixelsInUint8() so everyone gets it
There was a problem hiding this comment.
Yes, I believe you want to operate on unmultiplied alpha to be accurate. I'll peek at the CI checks later.
There was a problem hiding this comment.
I did want to ask: why not use HSL and increase L by the amount? I know the stack overflow comment makes it sound incorrect, but its more correct because it shouldn't be changing the hue or saturation. That being said, these functions are "brighten" and "darken" which I'm not clear on what the desired results are (white-out of colors?)
There was a problem hiding this comment.
Also; what is "ammount" saying here? Are you wanting each pixel's individual's brightness to increase by some percentage amount (0 to 1) or are you trying to level all the brightness to a set level? If its individual; then 0 to 1 needs to be over the range of -<1.0> with 0 to 1 covering that.
There was a problem hiding this comment.
I did want to ask: why not use HSL and increase L by the amount? I know the stack overflow comment makes it sound incorrect, but its more correct because it shouldn't be changing the hue or saturation. That being said, these functions are "brighten" and "darken" which I'm not clear on what the desired results are (white-out of colors?)
Sounds good to be, I think this was just our naive attempt at the implementation without doing any research. 😅
But maybe we should just be redirecting to those painting methods you mentioned?
There was a problem hiding this comment.
I switched to HSL color from Painting and updated the darken call. The test errors and everything else has nothing to do with this code 😅
There was a problem hiding this comment.
Also; what is "ammount" saying here? Are you wanting each pixel's individual's brightness to increase by some percentage amount (0 to 1) or are you trying to level all the brightness to a set level? If its individual; then 0 to 1 needs to be over the range of -<1.0> with 0 to 1 covering that.
Makes sense, I think we could also do a breaking change here since it hasn't really worked before. Or deprecate this method and create a new one if we have some name that could fit.
There was a problem hiding this comment.
I think I captured what was previously done here lighten = (color + (1 - color) * amount; darken = color * amount.
|
Thank you so much @jtmcdole ! |
luanpotter
merged commit 2d6c8c1
into
flame-engine:fix/colors-darken
Fix brighten and darken alpha issue. This is all thanks to @jtmcdole's insight & fixes [here](#3407). --------- Co-authored-by: Lukas Klingsbo <[email protected]> Co-authored-by: Erick Zanardo <[email protected]> Co-authored-by: Lukas Klingsbo <[email protected]> Co-authored-by: John McDole <[email protected]>
The image data read back is in a pre-multiplied state. Convert it back to straight alpha before brightening and then premultiply.
This just makes the colors go to white though:
You could alternatively use the HSV color space and increase the Value (brightness); this would be more subtle and keep the colors correct.