Skip to content

[Impeller][DisplayList] Consolidate BlendMode definitions #165450

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
auto-submit merged 3 commits into from
Mar 20, 2025
Merged

[Impeller][DisplayList] Consolidate BlendMode definitions #165450

auto-submit merged 3 commits into from
Mar 20, 2025

Conversation

@flar
Copy link
Contributor

@flar flar commented Mar 19, 2025 •
edited
Loading

Fixes: #165412

The 2 enum classes impeller::BlendMode and flutter::DlBlendMode were essentially identical so they were merged.

The names were consolidated to the ui.BlendMode/DlBlendMode versions and the implementation is shared from Impeller to DisplayList via a using directive. No conversion is needed any more to describe blend modes between DisplayList and Impeller.

@github-actions github-actions bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Mar 19, 2025
flar
flar commented Mar 19, 2025
View reviewed changes
@flar flar force-pushed the merge-impeller-dl-blend-mode branch from e15a6ac to 10a5b5d Compare March 19, 2025 16:30
@flar flar requested a review from jonahwilliams March 19, 2025 17:50
@flar
Copy link
Contributor Author

flar commented Mar 19, 2025

It looks like google testing has identified some new failures, but they are in web tests - not sure how this PR could be the cause.

@flar flar requested a review from gaaclarke March 19, 2025 17:50
@flar
Copy link
Contributor Author

flar commented Mar 19, 2025

@gaaclarke I had to do something to restore the naming on the golden tests for blurs so the goldens would get matched right. We could also just let the goldens get renamed, but it was a simple fix to have them use the legacy naming.

@flar
Copy link
Contributor Author

flar commented Mar 19, 2025

Thoughts on the idea of including a kDefaultFoo = <default value> value in the enums rather than just hard-coding it everywhere? Is it more transparent if the sites that use the default say it by value, or is it safer to have the enum define it as a name? I migrated all instances of DlBlendMode::kDefaultMode to srcover, but I could have also added the kDefaultMode to Impeller's version...?

jonahwilliams
jonahwilliams approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

I think it would be good to keep a kDefaultMode, but otherwise LGTM

keeping the goldens matching is good but if they update names in this PR its not a huge deal.

@flar
Copy link
Contributor Author

flar commented Mar 19, 2025

Now that we know the golden tests pass with no diffs, I went ahead and allowed them to be renamed. There will be a bunch of "new goldens" to approve on this latest run...

@flutter-dashboard
Copy link

flutter-dashboard bot commented Mar 19, 2025

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #165450 at sha acc2f6b

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Mar 19, 2025
@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Mar 20, 2025
Merged via the queue into flutter:master with commit e6fbc63 Mar 20, 2025
175 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 25, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
2f02265 
…utter/flutter#165450)
@flar flar mentioned this pull request Mar 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 25, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
147027c 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
5957afd 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
27e525f 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
b4338fa 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
188c794 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
fea2ff2 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 26, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
0b18107 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
7b22ece 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
ea84955 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
2731dc6 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
bfd4aef 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
344ebdd 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
dcdbda7 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
5783148 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
a921a92 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
27d2e2e 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
e32f797 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
0bdfbe1 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
d3a2c23 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
74eb4d8 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
231e9ea 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
30f7bac 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
0ebea24 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
8c39062 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
00d2ff6 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
8b6c59b 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
d46be31 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
80de261 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
3e8b304 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
65d2511 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
4e31d4f 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
438efe5 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
015b541 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
39dfba0 
…utter/flutter#165450)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025
@engine-flutter-autoroll
e6fbc63 [Impeller][DisplayList] Consolidate BlendMode definitions (fl…
1259d66 
…utter/flutter#165450)
zhangyuang pushed a commit to zhangyuang/flutter-fork that referenced this pull request Jun 9, 2025
@flar
[Impeller][DisplayList] Consolidate BlendMode definitions (flutter#16…
8a9a0a8 
…5450)

The 2 enum classes `impeller::BlendMode` and `flutter::DlBlendMode` were
essentially identical so they were merged.

The names were consolidated to the ui.BlendMode/DlBlendMode versions and
the implementation is shared from Impeller to DisplayList via a `using`
directive. No conversion is needed any more to describe blend modes
between DisplayList and Impeller.
romanejaquez pushed a commit to romanejaquez/flutter that referenced this pull request Aug 14, 2025
@flar @romanejaquez
[Impeller][DisplayList] Consolidate BlendMode definitions (flutter#16…
224877b 
…5450)

The 2 enum classes `impeller::BlendMode` and `flutter::DlBlendMode` were
essentially identical so they were merged.

The names were consolidated to the ui.BlendMode/DlBlendMode versions and
the implementation is shared from Impeller to DisplayList via a `using`
directive. No conversion is needed any more to describe blend modes
between DisplayList and Impeller.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaaclarke gaaclarke Awaiting requested review from gaaclarke

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[DisplayList] Use impeller::BlendMode rather than a private DlBlendMode enum

2 participants

@flar @jonahwilliams