-
Notifications
You must be signed in to change notification settings - Fork 1
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
Migrate Swift code from thorvg-swift #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andyf-canva for amazing contribution. Let's start a review :)
Hello @andyf-canva I think new commits created during the review, other contributors don't need to care of this history. |
.gitmodules
Outdated
@@ -0,0 +1,4 @@ | |||
[submodule "thorvg"] | |||
path = thorvg | |||
url = [email protected]:andyf-canva/thorvg-swift.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we now switch repo to official one?
I understand additional code from thorvg inside is not necessary.
History: thorvg/thorvg#2420
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do. 😄 And as per your comment, I'll also try to move us onto the latest release version of ThorVG as well 👍
Once I've got that going, I'll report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tinyjin - see my comment below, I've now moved this onto the latest ThorVG version (v0.14.7), yet had to exclude the tools
directory due to some duplicate symbols errors.
.gitmodules
Outdated
[submodule "thorvg"] | ||
path = thorvg | ||
url = [email protected]:andyf-canva/thorvg-swift.git | ||
branch = release-v0.13.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to ask you to update ThorVG version to latest(v0.14.7), and check whether it's nicely working.
Ultimately, we'll need to align with the latest version whenever it's ready for merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea. As per my comment above (and below), I've moved the repo onto v0.14.7
and all the tests are passing and everything is working as expected.
@tinyjin - an update regarding some changes I've made over the past few days. We're getting so close now! 🙏 💯 Updating the ThorVG versionI've updated the submodule reference to point to the ThorVG upstream ( This introduced some "duplicate symbols" errors, as there are some functions inside the Seeming the Swift Package isn't using these tools as of yet, I've just added the Handling the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Please check comments. I'll also test this library in my local, once guidance is ready.
@tinyjin I've just updated the README and And apologies for the delay in getting back to you, have had a few busy weeks! |
@andyf-canva I have a question, Is there a component like based on UIView or SwiftUI? I expect we provide users with simple component is easily used. Currently, I only see API bridge between Swift and C++. We need to manually draw UIView(or Canvas) and attach buffer from LottieRenderer by looping the frames. Is this correct? Confirmed that each frames is computed well and return frame buffers well. Still we need something like a component. If users have to define custom view and loop by theirselves, probably this would make developers struggle using the package. |
@tinyjin no worries! So great to see this come to life ❤️ And glad to hear you were able to compute frames and read the buffer successfully on your side! 🎉
Unfortunately not 😢. And you're correct, if people want to use this API then they need to manually iterate through the frames and create a Here's an idea: shall we merge this PR as we're quite happy with the state of the code we wanted to migrate from thorvg-swift, and then before we release the initial version of this repo we can add another PR to introduce UIKit and SwiftUI views that make it more convenient to access the |
Got you @andyf-canva. Now clear and we can do like that. As you mentioned, we can merge this PR first and then bring another one for the simpler interface for Lottie, like SwiftUI or UIView. Thanks for awesome job here :) |
Last question for the package thing. I know this package will be used with Swift Package Manager. From my simple experience with Swift, it doesn't require publishing to specific hosted zone like NPM. Am I correct? I was wondering if we should cost further things for packaging. |
@tinyjin sounds like a plan! 😄 I'll merge this PR and then open up another with the SwiftUI and UIKit changes 👍 Regarding Swift Package Manager, yep no need to worry when it comes to costs for packaging. Swift Package Manager just reads GitHub directly just using a URL, and you can pass in other parameters such as release versions or branches you wish to pull from. There's some good docs from Apple here if you wish to take a look 😄 |
As mentioned in the GitHub issue #1, this PR includes all of the Swift code that wraps the ThorVG API from thorvg-swift, including the Swift Package manifest containing details on how to build the package. The thorvg core has been added as a submodule to the repo.