Skip to content

Include transform in static Gradient lerp methods #149624

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

Jump to bottom
Merged
merged 2 commits into from
Jun 14, 2024
Merged

Include transform in static Gradient lerp methods #149624

merged 2 commits into from
Jun 14, 2024

Conversation

Zabadam
Copy link
Contributor

@Zabadam Zabadam commented Jun 3, 2024

Fixes #149534 Gradient subclasses' static lerp methods drop the GradientTransform of both a and b

LinearGradient.lerp(), RadialGradient.lerp() and SweepGradient.lerp() no longer drop the GradientTransform of a and/or b.

The primitive interpolation is performed the same way TileMode is handled: transform: t < 0.5 ? a.transform : b.transform.

Media

Video demonstration

Before

Gradient.lerp.Loses.GradientTransform.mp4

After

Gradient.lerp.Preserved.Transform.mp4

Pre-launch Checklist

Zabadam added 2 commits June 3, 2024 15:04
@Zabadam
Include transform in static Gradient lerp methods
042fd1b 
LinearGradient.lerp(), RadialGradient.lerp() and SweepGradient.lerp() no longer drop the GradientTransform of `a` and/or `b`.
@Zabadam
Added gradient lerp tests with transforms
eb7ad38
@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 3, 2024
@goderbauer goderbauer requested a review from Hixie June 4, 2024 22:04
Hixie
Hixie reviewed Jun 6, 2024
View reviewed changes
packages/flutter/lib/src/painting/gradient.dart
Comment on lines 509 to +510
tileMode: t < 0.5 ? a.tileMode : b.tileMode, // TODO(ianh): interpolate tile mode
transform: t < 0.5 ? a.transform : b.transform,
Copy link
Contributor

Choose a reason for hiding this comment

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

not a big deal, but if you feel like it, you could remove the TODO on the line above (or alternatively, add one on the new line, with a link to #443)

(same for all three gradients)

Hixie
Hixie approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@Hixie Hixie left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer goderbauer requested a review from nate-thegrate June 12, 2024 22:09
nate-thegrate
nate-thegrate approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Thanks for noticing the bug and implementing a fix!

packages/flutter/test/painting/gradient_test.dart
Comment on lines +215 to +217
expect(testGradient1, equals(actual0));
expect(testGradient2, equals(actual1));
expect(testGradient2, equals(actual2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(testGradient1, equals(actual0));
expect(testGradient2, equals(actual1));
expect(testGradient2, equals(actual2));
expect(actual0, equals(testGradient1));
expect(actual1, equals(testGradient2));
expect(actual2, equals(testGradient2));

Tiny nit: "actual" should be passed as the first argument for expect() (here and below). Not a big deal though, since in this case the test works fine either way.

@nate-thegrate
Copy link
Contributor

We can probably merge this now, and then make the aforementioned tweaks the next time we update these functions.

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 14, 2024
@auto-submit auto-submit bot merged commit 43e71ae into flutter:master Jun 14, 2024
72 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 14, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2024
@engine-flutter-autoroll
43e71ae Include transform in static Gradient lerp methods (flutter/fl…
64deb12 
…utter#149624)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 14, 2024
@engine-flutter-autoroll
Roll Flutter from 01db23b to 349ec71 (19 revisions) (#6926)
cab9c86 
flutter/flutter@01db23b...349ec71

2024-06-14 32538273+ValentinVignal@users.noreply.github.com Add tests for navigator.0.dart (flutter/flutter#150034)
2024-06-14 parlough@gmail.com Switch to `Iterable.cast` instance method (flutter/flutter#150185)
2024-06-14 65806473+Zabadam@users.noreply.github.com Include transform in static Gradient lerp methods (flutter/flutter#149624)
2024-06-14 parlough@gmail.com Validate the `contrastLevel` during `ColorScheme` creation (flutter/flutter#150176)
2024-06-14 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.9 to 3.25.10 (flutter/flutter#150228)
2024-06-13 polinach@google.com Fix leaky test. (flutter/flutter#150235)
2024-06-13 109111084+yaakovschectman@users.noreply.github.com Document CIPD role & login for upgrading Android engine (flutter/flutter#149433)
2024-06-13 36861262+QuncCccccc@users.noreply.github.com Update doc for `ColorScheme.surface` (flutter/flutter#150212)
2024-06-13 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#150206)
2024-06-13 47866232+chunhtai@users.noreply.github.com Bump new release for a11y_assessment (flutter/flutter#150213)
2024-06-13 kustermann@google.com Use --(no-)strip-wams instead of --(no-)-name-section in `dart compile wasm` (flutter/flutter#149641)
2024-06-13 34871572+gmackall@users.noreply.github.com Reland "Identify and re-throw our dependency checking errors in flutter.groovy" (flutter/flutter#150128)
2024-06-13 kustermann@google.com Use --(no-)strip-wams instead of --(no-)-name-section in `dart compile wasm` (flutter/flutter#150180)
2024-06-13 matanlurey@users.noreply.github.com Suppress Flutter update check if `--machine` is present at all. (flutter/flutter#150138)
2024-06-13 tessertaha@gmail.com [Reland] Introduce `ChipAnimationStyle` to override default chips animations durations (flutter/flutter#149876)
2024-06-13 parlough@gmail.com Update framework and flutter fix flutter.dev/docs links (flutter/flutter#150174)
2024-06-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4cb3025d3abf to 8167dffd1914 (2 revisions) (flutter/flutter#150208)
2024-06-13 leroux_bruno@yahoo.fr Replace InputDecorator M3 golden test (flutter/flutter#150111)
2024-06-13 engine-flutter-autoroll@skia.org Roll Packages from 260102b to 7805455 (2 revisions) (flutter/flutter#150198)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jun 14, 2024
@Zabadam @victorsanni
Include transform in static Gradient lerp methods (flutter#149624)
b87e18c 
*Fixes flutter#149534 Gradient subclasses' static lerp methods drop the GradientTransform of both `a` and `b`*

LinearGradient.lerp(), RadialGradient.lerp() and SweepGradient.lerp() no longer drop the GradientTransform of `a` and/or `b`.

The primitive interpolation is performed the same way TileMode is handled:  `transform: t < 0.5 ? a.transform : b.transform`.

## Media

<details>
<summary>Video demonstration</summary>

### Before
https://github.com/flutter/flutter/assets/65806473/de14e201-b1a7-44ba-95ff-e62587c7f6ac

### After
https://github.com/flutter/flutter/assets/65806473/d48f40e2-1b0f-4ac8-a45c-b3c423e3fd64

</details>

  - Changed no documentation.
  - Non-breaking change.
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jun 14, 2024
@Zabadam @victorsanni
Include transform in static Gradient lerp methods (flutter#149624)
29a7879 
*Fixes flutter#149534 Gradient subclasses' static lerp methods drop the GradientTransform of both `a` and `b`*

LinearGradient.lerp(), RadialGradient.lerp() and SweepGradient.lerp() no longer drop the GradientTransform of `a` and/or `b`.

The primitive interpolation is performed the same way TileMode is handled:  `transform: t < 0.5 ? a.transform : b.transform`.

## Media

<details>
<summary>Video demonstration</summary>

### Before
https://github.com/flutter/flutter/assets/65806473/de14e201-b1a7-44ba-95ff-e62587c7f6ac

### After
https://github.com/flutter/flutter/assets/65806473/d48f40e2-1b0f-4ac8-a45c-b3c423e3fd64

</details>

  - Changed no documentation.
  - Non-breaking change.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
43e71ae Include transform in static Gradient lerp methods (flutter/fl…
21a394d 
…utter#149624)
@Zabadam Zabadam mentioned this pull request Aug 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hixie Hixie Hixie approved these changes

@nate-thegrate nate-thegrate nate-thegrate approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gradient subclasses' static lerp methods drop the GradientTransform of both a and b
3 participants
@Zabadam @nate-thegrate @Hixie