Skip to content
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

Add support for layer blend mode "bm" for NORMAL, SCREEN, OVERLAY, DARKEN, LIGHTEN, and ADD #2408

Merged
merged 19 commits into from Nov 18, 2023
Merged

Add support for layer blend mode "bm" for NORMAL, SCREEN, OVERLAY, DARKEN, LIGHTEN, and ADD #2408

merged 19 commits into from Nov 18, 2023

Conversation

kudanai
Copy link
Contributor

@kudanai kudanai commented Oct 30, 2023

Adds support for "bm" (BlendMode) on layers. Possibly addresses #1055
Might need a little bit of a refactor. Relies on Paint.setBlendMode() available in android Q and up.

Sample before & after

IMAGE 2023-10-30 12:28:18
Screenshot 2023-10-30 at 12 27 39

gpeal
gpeal reviewed Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@gpeal gpeal left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for taking a stab at this!

I want to land this but in the past, I have been hesitant to launch feature that require new platforms until the usage of that platform is very high. About 75% of Android phones are on Q and above but most apps aren't there with their minSdk yet.

The use case I'm concerned about is somebody landing an animation that looks correct during development but looks completely broken for end users with old phones.

The only other time I've launched something like this was enableMergePathsForKitKatAndAbove in which it's opt-in and the API makes it very clear what the platform restrictions are.

Also, if you add animations to this folder, they will automatically be picked up by snapshot tests.

lottie/src/main/java/com/airbnb/lottie/animation/content/FillContent.java Outdated Show resolved Hide resolved
lottie/src/main/java/com/airbnb/lottie/model/content/LBlendMode.java Outdated Show resolved Hide resolved
lottie/src/main/java/com/airbnb/lottie/parser/LayerParser.java Outdated Show resolved Hide resolved
@github-actions
Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

kudanai and others added 3 commits October 30, 2023 21:38
@kudanai @gpeal
Update lottie/src/main/java/com/airbnb/lottie/animation/content/FillC…
0871413 
…ontent.java

Co-authored-by: Gabriel Peal <gpeal@users.noreply.github.com>
@kudanai @gpeal
Update lottie/src/main/java/com/airbnb/lottie/parser/LayerParser.java
a804ac6 
Co-authored-by: Gabriel Peal <gpeal@users.noreply.github.com>
@kudanai @gpeal
Update lottie/src/main/java/com/airbnb/lottie/model/content/LBlendMod…
eac4986 
…e.java

Co-authored-by: Gabriel Peal <gpeal@users.noreply.github.com>
@kudanai
Copy link
Contributor Author

kudanai commented Oct 30, 2023 •
edited

Thank you for the feedback.

I understand what you mean by the version check. I could suggest rolling back a little and only supporting the blend modes that have lower minsdk requirements, and continue working on it from there. Does that seem like a more prudent approach?

Also I wasn't entirely sure about the structure of that enum, it feels overly verbose to me. However since bm can also appear for shapes, figured it might be useful to leave the enum itself in place.

Done! Meanwhile, I will collect some sample lotties with specific blend modes and drop them in the directory.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 2, 2023

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@kudanai
Copy link
Contributor Author

kudanai commented Nov 7, 2023 •
edited

Pushing new changes:

  • Separate out the backwards compatible blend modes and ones that are only Q+
    • They've been moved out into a separate (unused for now) method
    • we'll be enabling 6/18 blend modes: NORMAL, SCREEN, OVERLAY, DARKEN, LIGHTEN, and ADD
  • Removed API version guard while actually applying the layer blend mode

@kudanai
feat: separate blendModes that have backwards support
cabfeff
@kudanai
Merge remote-tracking branch 'origin/layer-blend' into layer-blend
9abec0d 
# Conflicts:
#	lottie/src/main/java/com/airbnb/lottie/animation/content/FillContent.java
#	lottie/src/main/java/com/airbnb/lottie/model/content/LBlendMode.java
@kudanai
chore: lint
8bc5e98
@github-actions GitHub Actions
Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@kudanai kudanai changed the title Add support for "bm" (BlendMode) for Android Q and up Add support for layer blend mode "bm" for NORMAL, SCREEN, OVERLAY, DARKEN, LIGHTEN, and ADD Nov 14, 2023
@kudanai
Copy link
Contributor Author

kudanai commented Nov 15, 2023

The supported modes look consistent to me on 23 and 31 now

@xvrh xvrh mentioned this pull request Nov 17, 2023
Closed
@github-actions GitHub Actions
Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

gpeal
gpeal approved these changes Nov 18, 2023
View reviewed changes
Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@gpeal gpeal merged commit 1256d26 into airbnb:master Nov 18, 2023
6 checks passed
@gpeal gpeal mentioned this pull request Dec 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpeal gpeal gpeal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kudanai @gpeal