Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@JsouLiang
Copy link
Contributor

Use new DlBlendMode enum to change SkBlendMode

@JsouLiang
Copy link
Contributor Author

cc @flar @chinmaygarde

@chinmaygarde
Copy link
Member

I think the patch looks fine but updating the non-opaque types at this time is not the most pressing priority.

@flar
Copy link
Contributor

flar commented Mar 15, 2022

As Chinmay says, the enums aren't the highest priority, but we should convert them as we encounter them in the larger objects. This potentially should have been done when I converted DlColorFilter.

In my most recent commit for my DlColorSource PR I converted DlTileMode as it was used by that object. Take a look at the way I did it with the conversion functions being inlined rather than declared as an actual function (the cast is a NOP, so it shouldn't force a function call).

PR: #31981
File: https://github.com/flutter/engine/blob/614d9a5967c60736c2cb1e2342d113750fa883a0/display_list/display_list_tile_mode.h

@chinmaygarde
Copy link
Member

Right. My comment about the priority was more towards convincing @JsouLiang that if they want to help out in tacking the items in flutter/flutter#95434 (comment) (which I would be super appreciative of), perhaps the right ones to tackle first would be the opaque and semi-opaque ones. But the direction here especially after @flar s last comment is sound.

@flar
Copy link
Contributor

flar commented Mar 16, 2022

The DlColorSource work has landed, which includes the enum rework for DlTileMode/SkTileMode. Take a look at that and update this for consistency (and please comment if you have suggestions on the way those are implemented). Note that I added an enum_unittests.cc to hold the (not very complicated) unit tests for the enums. They will help us discover if we ever run across a change in the Sk enums.

@JsouLiang JsouLiang force-pushed the use-dlblendmode-object branch 2 times, most recently from ec91dd6 to e37c645 Compare March 18, 2022 03:13
@JsouLiang
Copy link
Contributor Author

@flar hey, I have change the implement of DlBlendMode, please give me a review, so i can join other tasks. : )

@flar
Copy link
Contributor

flar commented Mar 18, 2022

@flar hey, I have change the implement of DlBlendMode, please give me a review, so i can join other tasks. : )

I was waiting for you to fix all of the broken checks, but I'll take a first pass look.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

It looks good except for a couple of minor suggestions. Also, there are changes required to pass all the submit checks, so I'll make this a comment review for now.

@flar
Copy link
Contributor

flar commented Mar 18, 2022

Note that the impeller directory in the engine tree is imported from github.com/flutter/impeller so if you've made local changes to that directory, they weren't included in this push. Instead, you have to make coordinated changes to both repos.

engine/src/flutter/impeller is one of the directories synchronized by gclient sync (yes, it is outside of the third_party directories, but it will eventually be merged into the engine so it's not technically "3rd party")

Fork the impeller repo and make the same changes to it as you made to your local impeller copy and push those changes to your fork and make a PR for that repo. You can mark it WIP, but this all shouldn't take too long to complete.

You can test if your impeller PR works by temporarily changing the entry in your engine's DEPS file to point to your commit in your forked impeller repo and executing gclient sync. (You'll need to undo the changes made to the local engine's impeller directory as it needs to be "clean" for the gclient to sync it.) If it then builds appropriately then you'll need to request a PR for your impeller changes, get that in and then update the DEPS to point to the commit hash in the official flutter/impeller repo.

Note that you still have other pieces broken in the current version of this PR before we can coordinate that dependency update. Particularly it looks like there are missing items in the licenses file?

@JsouLiang JsouLiang force-pushed the use-dlblendmode-object branch from e37c645 to 1e686a2 Compare March 19, 2022 09:42
@JsouLiang
Copy link
Contributor Author

@flar Hey bro, I have push change to the impeller project, and it have approved, so next please help me merge it, and I will change the DEPS commit id. Thank you very much :)

@JsouLiang JsouLiang force-pushed the use-dlblendmode-object branch 2 times, most recently from dd341eb to 6339a57 Compare March 19, 2022 15:29
@JsouLiang JsouLiang force-pushed the use-dlblendmode-object branch from 6339a57 to d92382b Compare March 19, 2022 15:54
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM

@flar flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 19, 2022
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 19, 2022
@flar
Copy link
Contributor

flar commented Mar 19, 2022

Ah, sorry, it needs a second reviewer.

@zanderso zanderso added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 19, 2022
@fluttergithubbot fluttergithubbot merged commit 1aa5255 into flutter:main Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants