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.

Sign up for GitHub

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

fix: Unmultiply before brightening #3407

Merged
merged 2 commits into from
Dec 16, 2024
Merged

fix: Unmultiply before brightening #3407

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
560b0f0
Unmultiply colors before brightening
jtmcdole Dec 14, 2024
fba0626
Update darken and switch to HSL.
jtmcdole Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 31 additions & 11 deletions packages/flame/lib/src/extensions/image.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,37 @@ extension ImageExtension on Image {
final newPixelData = Uint8List(pixelData.length);

for (var i = 0; i < pixelData.length; i += 4) {
final color = Color.fromARGB(
pixelData[i + 3],
pixelData[i + 0],
pixelData[i + 1],
pixelData[i + 2],
).brighten(amount);

newPixelData[i] = (color.r * 255).round();
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe you want to operate on unmultiplied alpha to be accurate. I'll peek at the CI checks later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I captured what was previously done here lighten = (color + (1 - color) * amount; darken = color * amount.


// Lets avoid division by zero.
if (a == 0) {
newPixelData[i + 0] = pixelData[i + 0];
newPixelData[i + 1] = pixelData[i + 1];
newPixelData[i + 2] = pixelData[i + 2];
newPixelData[i + 3] = pixelData[i + 3];
continue;
}

// Unmultiply the color
var r = (pixelData[i + 0] / 255) / a;
var g = (pixelData[i + 1] / 255) / a;
var b = (pixelData[i + 2] / 255) / a;

// Brighten in a color accurate way
r = r + (1.0 - r) * amount;
g = g + (1.0 - g) * amount;
b = b + (1.0 - b) * amount;

// Clamp
r = r.clamp(0, 1.0);
g = g.clamp(0, 1.0);
b = b.clamp(0, 1.0);

// Pre-multiply the new color.
newPixelData[i + 0] = (r * a * 255).round();
newPixelData[i + 1] = (g * a * 255).round();
newPixelData[i + 2] = (b * a * 255).round();
newPixelData[i + 3] = pixelData[i + 3];
}
return fromPixels(newPixelData, width, height);
}
Expand Down
Loading