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.

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

Add Rounded Corners #1795

Merged
merged 14 commits into from
Nov 2, 2022
Merged

Add Rounded Corners #1795

merged 14 commits into from
Nov 2, 2022

Conversation

johnny-duo
Copy link
Contributor

@johnny-duo johnny-duo commented Nov 2, 2022

This PR resolves issue #1523:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-11-02.at.12.09.03.mp4

@calda
Copy link
Member

calda commented Nov 2, 2022

Thank you very much for working on this!

When the implementation is done and ready for review, could you also add a test case for this to the regression suite? You'll just need to:

  1. add the animation json file to the Tests/Samples, ideally as Tests/Samples/Issues/issue_1523.json
  2. run the unit test target, either in Xcode or by running bundle exec rake test:package

The code will also need to be formatted according to Airbnb's Swift Style Guide. You can do this automatically by running bundle exec rake format:swift.

Thanks!

@johnny-duo johnny-duo marked this pull request as ready for review November 2, 2022 17:54
@calda
Copy link
Member

calda commented Nov 2, 2022

I also see you've only implemented this feature for the Main Thread rendering engine and not the new Core Animation rendering engine. That's fine for this PR -- I'll port this to the Core Animation rendering engine in a follow-up.

@johnny-duo
Copy link
Contributor Author

@calda Hi! Thanks for looking at this PR! I'm having some issues with snapshot tests in LottieFiles/gradient_shapes and TypeFace/Apostrophe. It looks like they have "ty": "rd" inside of the jsons so I think this is expected.

Is it possible to view the Snapshot results?

@johnny-duo
Copy link
Contributor Author

Yup! This PR is only for the main thread engine for now.

// RoundedCornersNode.swift
// Lottie
//
// Created by Duolingo on 10/31/22.
Copy link
Member

Choose a reason for hiding this comment

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

You work at Duolingo? Cool!

I have a 1083 day streak learning Spanish 😄 Really love what Duolingo has been doing with Lottie lately -- cutting edge stuff. The new feature where the characters lip-sync with the text-to-speech synth honestly blew my mind. Is that using Lottie or some sort of custom lip sync renderer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! Glad you're enjoying that! I implemented that on iOS actually :D. We couldn't figure out a way to do that with Lottie so we ended up having to use a different more state-machine based animation Library

Copy link
Member

Choose a reason for hiding this comment

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

So cool, awesome work! (Not surprised that isn't possible with Lottie)

Are you on Twitter? Would love to give you a follow if so -- I'm https://twitter.com/calstephens98

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't use it much for work-related stuff but gave you a follow!

@calda
Copy link
Member

calda commented Nov 2, 2022

Is it possible to view the Snapshot results?

Yes, there are a few ways to do this:

  1. Run the snapshot tests in Xcode. The snapshots are added as attachments to the test results in the Test navigator.
  2. Add isRecording = true to the top of testMainThreadRenderingEngine in SnapshotTests.swift and re-run the tests. This updates the on-disk snapshots (which you'll need to do to make the tests pass, if this affects existing animations).


extension BezierPath {
// Round corners of a single bezier
func roundCorners(radius: CGFloat) -> BezierPath {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for implementing this algorithm in Swift!

Copy link
Member

@calda calda left a comment

Choose a reason for hiding this comment

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

Thank you!

@calda calda enabled auto-merge (squash) November 2, 2022 19:05
auto-merge was automatically disabled November 2, 2022 19:27

Head branch was pushed to by a user without write access

@calda calda enabled auto-merge (squash) November 2, 2022 19:30
@calda calda disabled auto-merge November 2, 2022 19:30
@calda calda enabled auto-merge (squash) November 2, 2022 19:31
@calda calda merged commit 982a8f1 into airbnb:master Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants