Skip to content

Commit

Permalink
Reland: Update Color to do all calculations with floating point compo…
Browse files Browse the repository at this point in the history 
…nents (#55231)

Reason for revert: Broke customer tests
Reland depends on flutter/flutter#155113

This transforms the rest of Color to use the floating point parameters.  This will likely break existing tests very subtly.  For example, colors will be slightly different in golden tests if `lerp` was ever used.

issue: flutter/flutter#127855

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
  • Loading branch information
@gaaclarke
gaaclarke authored Sep 23, 2024
1 parent 4180426 commit 95c5a09
Show file tree
Hide file tree
Showing 12 changed files with 177 additions and 138 deletions.
3 changes: 2 additions & 1 deletion flutter_frontend_server/test/to_string_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ void main() {
]));
final runResult = io.Process.runSync(dart, <String>[regularDill]);
checkProcessResult(runResult);
var paintString = '"Paint.toString":"Paint(Color(0xffffffff))"';
var paintString =
'"Paint.toString":"Paint(Color(alpha: 1.0000, red: 1.0000, green: 1.0000, blue: 1.0000, colorSpace: ColorSpace.sRGB))"';
if (buildDir.contains('release')) {
paintString = '"Paint.toString":"Instance of \'Paint\'"';
}
Expand Down
12 changes: 0 additions & 12 deletions lib/ui/lerp.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,3 @@ double _lerpDouble(double a, double b, double t) {
double _lerpInt(int a, int b, double t) {
return a + (b - a) * t;
}

/// Same as [num.clamp] but specialized for non-null [int].
int _clampInt(int value, int min, int max) {
assert(min <= max);
if (value < min) {
return min;
} else if (value > max) {
return max;
} else {
return value;
}
}
96 changes: 48 additions & 48 deletions lib/ui/painting.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ bool _radiusIsValid(Radius radius) {
return true;
}

Color _scaleAlpha(Color a, double factor) {
return a.withAlpha((a.alpha * factor).round().clamp(0, 255));
Color _scaleAlpha(Color x, double factor) {
return x.withValues(alpha: clampDouble(x.a * factor, 0, 1));
}

/// An immutable 32 bit color value in ARGB format.
Expand Down Expand Up @@ -310,10 +310,11 @@ class Color {
///
/// See <https://en.wikipedia.org/wiki/Relative_luminance>.
double computeLuminance() {
assert(colorSpace != ColorSpace.extendedSRGB);
// See <https://www.w3.org/TR/WCAG20/#relativeluminancedef>
final double R = _linearizeColorComponent(red / 0xFF);
final double G = _linearizeColorComponent(green / 0xFF);
final double B = _linearizeColorComponent(blue / 0xFF);
final double R = _linearizeColorComponent(r);
final double G = _linearizeColorComponent(g);
final double B = _linearizeColorComponent(b);
return 0.2126 * R + 0.7152 * G + 0.0722 * B;
}

Expand All @@ -339,28 +340,26 @@ class Color {
///
/// Values for `t` are usually obtained from an [Animation<double>], such as
/// an [AnimationController].
static Color? lerp(Color? a, Color? b, double t) {
// TODO(gaaclarke): Update math to use floats. This was already attempted
// but it leads to subtle changes that change test results.
assert(a?.colorSpace != ColorSpace.extendedSRGB);
assert(b?.colorSpace != ColorSpace.extendedSRGB);
if (b == null) {
if (a == null) {
static Color? lerp(Color? x, Color? y, double t) {
assert(x?.colorSpace != ColorSpace.extendedSRGB);
assert(y?.colorSpace != ColorSpace.extendedSRGB);
if (y == null) {
if (x == null) {
return null;
} else {
return _scaleAlpha(a, 1.0 - t);
return _scaleAlpha(x, 1.0 - t);
}
} else {
if (a == null) {
return _scaleAlpha(b, t);
if (x == null) {
return _scaleAlpha(y, t);
} else {
assert(a.colorSpace == b.colorSpace);
return Color._fromARGBC(
_clampInt(_lerpInt(a.alpha, b.alpha, t).toInt(), 0, 255),
_clampInt(_lerpInt(a.red, b.red, t).toInt(), 0, 255),
_clampInt(_lerpInt(a.green, b.green, t).toInt(), 0, 255),
_clampInt(_lerpInt(a.blue, b.blue, t).toInt(), 0, 255),
a.colorSpace,
assert(x.colorSpace == y.colorSpace);
return Color.from(
alpha: clampDouble(_lerpDouble(x.a, y.a, t), 0, 1),
red: clampDouble(_lerpDouble(x.r, y.r, t), 0, 1),
green: clampDouble(_lerpDouble(x.g, y.g, t), 0, 1),
blue: clampDouble(_lerpDouble(x.b, y.b, t), 0, 1),
colorSpace: x.colorSpace,
);
}
}
Expand All @@ -377,32 +376,30 @@ class Color {
static Color alphaBlend(Color foreground, Color background) {
assert(foreground.colorSpace == background.colorSpace);
assert(foreground.colorSpace != ColorSpace.extendedSRGB);
// TODO(gaaclarke): Update math to use floats. This was already attempted
// but it leads to subtle changes that change test results.
final int alpha = foreground.alpha;
if (alpha == 0x00) { // Foreground completely transparent.
final double alpha = foreground.a;
if (alpha == 0) { // Foreground completely transparent.
return background;
}
final int invAlpha = 0xff - alpha;
int backAlpha = background.alpha;
if (backAlpha == 0xff) { // Opaque background case
return Color._fromARGBC(
0xff,
(alpha * foreground.red + invAlpha * background.red) ~/ 0xff,
(alpha * foreground.green + invAlpha * background.green) ~/ 0xff,
(alpha * foreground.blue + invAlpha * background.blue) ~/ 0xff,
foreground.colorSpace,
final double invAlpha = 1 - alpha;
double backAlpha = background.a;
if (backAlpha == 1) { // Opaque background case
return Color.from(
alpha: 1,
red: alpha * foreground.r + invAlpha * background.r,
green: alpha * foreground.g + invAlpha * background.g,
blue: alpha * foreground.b + invAlpha * background.b,
colorSpace: foreground.colorSpace,
);
} else { // General case
backAlpha = (backAlpha * invAlpha) ~/ 0xff;
final int outAlpha = alpha + backAlpha;
assert(outAlpha != 0x00);
return Color._fromARGBC(
outAlpha,
(foreground.red * alpha + background.red * backAlpha) ~/ outAlpha,
(foreground.green * alpha + background.green * backAlpha) ~/ outAlpha,
(foreground.blue * alpha + background.blue * backAlpha) ~/ outAlpha,
foreground.colorSpace,
backAlpha = backAlpha * invAlpha;
final double outAlpha = alpha + backAlpha;
assert(outAlpha != 0);
return Color.from(
alpha: outAlpha,
red: (foreground.r * alpha + background.r * backAlpha) / outAlpha,
green: (foreground.g * alpha + background.g * backAlpha) / outAlpha,
blue: (foreground.b * alpha + background.b * backAlpha) / outAlpha,
colorSpace: foreground.colorSpace,
);
}
}
Expand All @@ -423,16 +420,19 @@ class Color {
return false;
}
return other is Color &&
other.value == value &&
other.a == a &&
other.r == r &&
other.g == g &&
other.b == b &&
other.colorSpace == colorSpace;
}

@override
int get hashCode => Object.hash(value, colorSpace);
int get hashCode => Object.hash(a, r, g, b, colorSpace);

// TODO(gaaclarke): Make toString() print out float values.
@override
String toString() => 'Color(0x${value.toRadixString(16).padLeft(8, '0')})';
String toString() =>
'Color(alpha: ${a.toStringAsFixed(4)}, red: ${r.toStringAsFixed(4)}, green: ${g.toStringAsFixed(4)}, blue: ${b.toStringAsFixed(4)}, colorSpace: $colorSpace)';
}

/// Algorithms to use when painting on the canvas.
Expand Down
7 changes: 0 additions & 7 deletions lib/web_ui/lib/lerp.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,3 @@ double? lerpDouble(num? a, num? b, double t) {
double _lerpDouble(double a, double b, double t) {
return a * (1.0 - t) + b * t;
}

/// Linearly interpolate between two integers.
///
/// Same as [lerpDouble] but specialized for non-null `int` type.
double _lerpInt(int a, int b, double t) {
return a * (1.0 - t) + b * t;
}
92 changes: 48 additions & 44 deletions lib/web_ui/lib/painting.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ void _validateColorStops(List<Color> colors, List<double>? colorStops) {
}
}

Color _scaleAlpha(Color a, double factor) {
return a.withAlpha(engine.clampInt((a.alpha * factor).round(), 0, 255));
Color _scaleAlpha(Color x, double factor) {
return x.withValues(alpha: (x.a * factor).clamp(0, 1));
}

class Color {
Expand Down Expand Up @@ -141,32 +141,32 @@ class Color {

double computeLuminance() {
// See <https://www.w3.org/TR/WCAG20/#relativeluminancedef>
final double R = _linearizeColorComponent(red / 0xFF);
final double G = _linearizeColorComponent(green / 0xFF);
final double B = _linearizeColorComponent(blue / 0xFF);
final double R = _linearizeColorComponent(r);
final double G = _linearizeColorComponent(g);
final double B = _linearizeColorComponent(b);
return 0.2126 * R + 0.7152 * G + 0.0722 * B;
}

static Color? lerp(Color? a, Color? b, double t) {
assert(a?.colorSpace != ColorSpace.extendedSRGB);
assert(b?.colorSpace != ColorSpace.extendedSRGB);
if (b == null) {
if (a == null) {
static Color? lerp(Color? x, Color? y, double t) {
assert(x?.colorSpace != ColorSpace.extendedSRGB);
assert(y?.colorSpace != ColorSpace.extendedSRGB);
if (y == null) {
if (x == null) {
return null;
} else {
return _scaleAlpha(a, 1.0 - t);
return _scaleAlpha(x, 1.0 - t);
}
} else {
if (a == null) {
return _scaleAlpha(b, t);
if (x == null) {
return _scaleAlpha(y, t);
} else {
assert(a.colorSpace == b.colorSpace);
return Color._fromARGBC(
engine.clampInt(_lerpInt(a.alpha, b.alpha, t).toInt(), 0, 255),
engine.clampInt(_lerpInt(a.red, b.red, t).toInt(), 0, 255),
engine.clampInt(_lerpInt(a.green, b.green, t).toInt(), 0, 255),
engine.clampInt(_lerpInt(a.blue, b.blue, t).toInt(), 0, 255),
a.colorSpace,
assert(x.colorSpace == y.colorSpace);
return Color.from(
alpha: _lerpDouble(x.a, y.a, t).clamp(0, 1),
red: _lerpDouble(x.r, y.r, t).clamp(0, 1),
green: _lerpDouble(x.g, y.g, t).clamp(0, 1),
blue: _lerpDouble(x.b, y.b, t).clamp(0, 1),
colorSpace: x.colorSpace,
);
}
}
Expand All @@ -175,30 +175,30 @@ class Color {
static Color alphaBlend(Color foreground, Color background) {
assert(foreground.colorSpace == background.colorSpace);
assert(foreground.colorSpace != ColorSpace.extendedSRGB);
final int alpha = foreground.alpha;
if (alpha == 0x00) {
final double alpha = foreground.a;
if (alpha == 0) { // Foreground completely transparent.
return background;
}
final int invAlpha = 0xff - alpha;
int backAlpha = background.alpha;
if (backAlpha == 0xff) {
return Color._fromARGBC(
0xff,
(alpha * foreground.red + invAlpha * background.red) ~/ 0xff,
(alpha * foreground.green + invAlpha * background.green) ~/ 0xff,
(alpha * foreground.blue + invAlpha * background.blue) ~/ 0xff,
foreground.colorSpace,
final double invAlpha = 1 - alpha;
double backAlpha = background.a;
if (backAlpha == 1) { // Opaque background case
return Color.from(
alpha: 1,
red: alpha * foreground.r + invAlpha * background.r,
green: alpha * foreground.g + invAlpha * background.g,
blue: alpha * foreground.b + invAlpha * background.b,
colorSpace: foreground.colorSpace,
);
} else {
backAlpha = (backAlpha * invAlpha) ~/ 0xff;
final int outAlpha = alpha + backAlpha;
assert(outAlpha != 0x00);
return Color._fromARGBC(
outAlpha,
(foreground.red * alpha + background.red * backAlpha) ~/ outAlpha,
(foreground.green * alpha + background.green * backAlpha) ~/ outAlpha,
(foreground.blue * alpha + background.blue * backAlpha) ~/ outAlpha,
foreground.colorSpace,
} else { // General case
backAlpha = backAlpha * invAlpha;
final double outAlpha = alpha + backAlpha;
assert(outAlpha != 0);
return Color.from(
alpha: outAlpha,
red: (foreground.r * alpha + background.r * backAlpha) / outAlpha,
green: (foreground.g * alpha + background.g * backAlpha) / outAlpha,
blue: (foreground.b * alpha + background.b * backAlpha) / outAlpha,
colorSpace: foreground.colorSpace,
);
}
}
Expand All @@ -216,15 +216,19 @@ class Color {
return false;
}
return other is Color &&
other.value == value &&
other.a == a &&
other.r == r &&
other.g == g &&
other.b == b &&
other.colorSpace == colorSpace;
}

@override
int get hashCode => Object.hash(value, colorSpace);
int get hashCode => Object.hash(a, r, g, b, colorSpace);

@override
String toString() => 'Color(0x${value.toRadixString(16).padLeft(8, '0')})';
String toString() =>
'Color(alpha: ${a.toStringAsFixed(4)}, red: ${r.toStringAsFixed(4)}, green: ${g.toStringAsFixed(4)}, blue: ${b.toStringAsFixed(4)}, colorSpace: $colorSpace)';
}

enum StrokeCap {
Expand Down
Loading

0 comments on commit 95c5a09

Please sign in to comment.