Update Color to do all calculations with floating point components #54981

gaaclarke · 2024-09-05T22:26:38Z

Note: this is expected to create slight golden diff changes

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

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

issue: #127855 This is a forward fix to help flutter/engine#54981 land. It makes the following changes: 1) Removes hardcoded `Color.toString()` assumptions in asserts 1) Switches some lerp tests to use numbers that are more friendly to uint8_t and floating point numbers 1) implements custom matchers for color 1) removes word wrapping for some asserts since it makes them fragile to changes in `Color.toString()` invocations ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

…54934) This migrates the last failing test for flutter/engine#54981. In order to effectively resolve that test I had to make `equalsIgnoringHashCodes` more usable by printing out the line that differs instead of just a huge blob of "expected" vs "actual. ## example Here's the output after the change. ### test ``` test('equalsIgnoringHashCodes - wrong line', () { expect( '1\n2\n3\n4\n5\n6\n7\n8\n9\n10', equalsIgnoringHashCodes('1\n2\n3\n4\n5\n6\na\n8\n9\n10'), ); }); ``` ### output ``` Expected: normalized value matches '1\n' '2\n' '3\n' '4\n' '5\n' '6\n' 'a\n' '8\n' '9\n' '10' Actual: '1\n' '2\n' '3\n' '4\n' '5\n' '6\n' '7\n' '8\n' '9\n' '10' Which: Lines 7 differed, expected: 'a' but got '7' ```

gaaclarke · 2024-09-11T21:42:07Z

lib/web_ui/test/ui/filters_test.dart

@@ -121,22 +121,22 @@ Future<void> testMain() async {

  test('color filter as image filter', () async {
    const ui.ColorFilter colorFilter = ui.ColorFilter.mode(
-      ui.Color.fromRGBO(0, 0, 255, 128),
+      ui.Color.fromARGB(128, 0, 0, 255),


This test was incorrectly using the fromRGBO method, so I fixed it. The opacity component was supposed to be in [0,1.0].

gaaclarke · 2024-09-11T21:51:35Z

lib/ui/painting.dart

  @override
-  String toString() => 'Color(0x${value.toRadixString(16).padLeft(8, '0')})';
+  String toString() =>


4 sig figs was chosen since hdmi 1.3 can support 12 bits per color component and 1/2^12 = 0.00024

Scientific notation could have been another option if we wanted ( 2.4e-4 ).

gaaclarke · 2024-09-11T22:20:54Z

flutter_frontend_server/test/to_string_test.dart

@@ -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))"';


typically I'd call Color.toString() here but I'm unable to import dart:ui here for some reason.

This is a VM test, so that is why.

But it's weird that I'm asserting behavior for dart:ui, so it's there, it's just not visible to me.

Its basically a compiler test though

jonahwilliams

LGTM

flar · 2024-09-12T00:21:23Z

lib/ui/painting.dart

-    final double R = _linearizeColorComponent(red / 0xFF);
-    final double G = _linearizeColorComponent(green / 0xFF);
-    final double B = _linearizeColorComponent(blue / 0xFF);
+    final double R = _linearizeColorComponent(r);


Now that we have colorspaces, should these be taught to clamp or convert to a non-extended CS as well?

I added an assert that extended srgb is in't used. I don't think it will make sense with that.

…ponents (flutter/engine#54981)

…nents" (#55153) Reverts #54981 This is causing failures in customer tests that need to be updated: https://github.com/flutter/flutter/pull/155107/checks?check_run_id=30069840819

…ponents (flutter/engine#54981)

…155128) flutter/engine@48ddaf5...8609af6 2024-09-12 [email protected] Revert "Update Color to do all calculations with floating point components" (flutter/engine#55153) 2024-09-12 [email protected] Migrate more tests from `litetest` to `package:test` (flutter/engine#55119) 2024-09-12 [email protected] Roll Skia from acff7f24ddbe to 26b048c6a53b (1 revision) (flutter/engine#55151) 2024-09-12 [email protected] Roll Dart SDK from a438066d634f to aa27c61f5859 (8 revisions) (flutter/engine#55147) 2024-09-12 [email protected] Update Skia build for Vulkan headers (flutter/engine#55143) 2024-09-12 [email protected] Roll Skia from 2b40b50ea423 to acff7f24ddbe (1 revision) (flutter/engine#55144) 2024-09-12 [email protected] Update Color to do all calculations with floating point components (flutter/engine#54981) 2024-09-12 [email protected] Roll Fuchsia Linux SDK from fKNT8lbGh8JzxjE6m... to 3YH1DEYJ-s93fHBw5... (flutter/engine#55142) 2024-09-12 [email protected] Migrate `const_finder_test` to use `package:test` (flutter/engine#55132) 2024-09-12 [email protected] [engine] make UI thread the platform thread for Android. Still allows opt out as g3 escape hatch. (flutter/engine#55111) 2024-09-12 [email protected] Roll Skia from b750cbedc114 to 2b40b50ea423 (1 revision) (flutter/engine#55141) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from fKNT8lbGh8Jz to 3YH1DEYJ-s93 If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

## _Description of what this PR is changing or adding, and why:_ Adding more details about the changing of Color equality. ## _Issues fixed by this PR (if any):_ flutter/flutter#127855 ## _PRs or commits this PR depends on (if any):_ flutter/engine#54981 ## Presubmit checklist - [x] This PR is marked as draft with an explanation if not meant to land until a future stable release. - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]>

…lutter#54981) 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

github-actions bot added the platform-web Code specifically for the web engine label Sep 5, 2024

gaaclarke changed the title ~~Wide gamut framework breaker~~ Update Color to do all calculations with floating point components Sep 5, 2024

This was referenced Sep 5, 2024

Made some pixel tests fuzzy flutter/flutter#154680

Merged

Removes the int storage from Color #54714

Merged

gaaclarke force-pushed the wide-gamut-framework-breaker branch from 64bd33e to 4880770 Compare September 6, 2024 16:15

gaaclarke mentioned this pull request Sep 9, 2024

Update color assertions flutter/flutter#154752

Merged

9 tasks

gaaclarke mentioned this pull request Sep 10, 2024

Migrate Color.toString() test, improves equalsIgnoringHashCodes flutter/flutter#154934

Merged

9 tasks

gaaclarke added 9 commits September 11, 2024 10:58

Makes Color take advantage of the wider bit depth.

36ca48a

updated lerp

66f8393

migrated alphaBlend

138f388

moved equality and toString

7493253

brought over web changes

745a8a7

fixed fragile "toString" test

94e4b23

fixed fragile text test

2aebceb

removed unused functions

26595ac

fixed front end test, moved tests to abstract away color.tostring

aa290db

gaaclarke force-pushed the wide-gamut-framework-breaker branch from a2d1430 to aa290db Compare September 11, 2024 18:02

gaaclarke added 3 commits September 11, 2024 12:23

removed verbotin import

4466d89

fixed some web tests

e45bb0c

fixed more web tests

dadd6c9

gaaclarke commented Sep 11, 2024

View reviewed changes

removed num clamp

72dca37

gaaclarke commented Sep 11, 2024

View reviewed changes

gaaclarke marked this pull request as ready for review September 11, 2024 22:13

gaaclarke commented Sep 11, 2024

View reviewed changes

doh

19adc88

gaaclarke requested review from jonahwilliams and flar September 11, 2024 22:39

jonahwilliams approved these changes Sep 11, 2024

View reviewed changes

flar reviewed Sep 12, 2024

View reviewed changes

jim feedback

512d0a1

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 12, 2024

auto-submit bot merged commit e24aa4d into flutter:main Sep 12, 2024
32 checks passed

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter Engine from 48ddaf578fb0 to 0a6104386392 (7 revisions) flutter/flutter#155107

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2024

e24aa4d17 Update Color to do all calculations with floating point com…

42e0149

…ponents (flutter/engine#54981)

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Sep 12, 2024

Partial revert of flutter#54981

a4a1219

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Sep 12, 2024

Partial revert of flutter#54981

0b52af1

This was referenced Sep 12, 2024

Revert "Update Color to do all calculations with floating point components" #55153

Merged

Zulip test needs to migrate color assertions flutter/flutter#155113

Closed

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter Engine from 48ddaf578fb0 to c38b3e7fac11 (8 revisions) flutter/flutter#155117

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2024

e24aa4d17 Update Color to do all calculations with floating point com…

805e9e9

…ponents (flutter/engine#54981)

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter Engine from 48ddaf578fb0 to fabc3d99e802 (9 revisions) flutter/flutter#155120

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2024

e24aa4d17 Update Color to do all calculations with floating point com…

6efab96

…ponents (flutter/engine#54981)

engine-flutter-autoroll mentioned this pull request Sep 12, 2024

Roll Flutter Engine from 48ddaf578fb0 to 8609af642725 (11 revisions) flutter/flutter#155128

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2024

e24aa4d17 Update Color to do all calculations with floating point com…

1dfb939

…ponents (flutter/engine#54981)

gaaclarke mentioned this pull request Sep 12, 2024

Adds note about color equality after wide gamut. flutter/website#11132

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Color to do all calculations with floating point components #54981

Update Color to do all calculations with floating point components #54981

gaaclarke commented Sep 5, 2024 •

edited

Loading

gaaclarke Sep 11, 2024

gaaclarke Sep 11, 2024

gaaclarke Sep 11, 2024

gaaclarke Sep 11, 2024

jonahwilliams Sep 11, 2024

gaaclarke Sep 11, 2024

jonahwilliams Sep 11, 2024

jonahwilliams left a comment

flar Sep 12, 2024

gaaclarke Sep 12, 2024

Update Color to do all calculations with floating point components #54981

Update Color to do all calculations with floating point components #54981

Conversation

gaaclarke commented Sep 5, 2024 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Sep 5, 2024 •

edited

Loading