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

Migrate Swift code from thorvg-swift #2

Merged
merged 24 commits into from
Oct 1, 2024

Conversation

andyf-canva
Copy link
Collaborator

@andyf-canva andyf-canva commented Jun 19, 2024

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.

Copy link
Member

@tinyjin tinyjin left a 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 :)

.gitmodules Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
swift-tests/SnapshotTests/LottieSnapshotTests.swift Outdated Show resolved Hide resolved
swift-tests/SnapshotTests/LottieSnapshotTests.swift Outdated Show resolved Hide resolved
swift/Engine.swift Outdated Show resolved Hide resolved
swift/Engine.swift Show resolved Hide resolved
swift/LottieRenderer.swift Outdated Show resolved Hide resolved
swift/ThorVGError.swift Outdated Show resolved Hide resolved
swift/Picture.swift Outdated Show resolved Hide resolved
@tinyjin
Copy link
Member

tinyjin commented Aug 23, 2024

Hello @andyf-canva
Can we squash commits all in one?

I think new commits created during the review, other contributors don't need to care of this history.
We can skip redundant commits, and leave meaningful one.

.gitmodules Outdated
@@ -0,0 +1,4 @@
[submodule "thorvg"]
path = thorvg
url = [email protected]:andyf-canva/thorvg-swift.git
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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
Copy link
Member

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.

Copy link
Collaborator Author

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.

@andyf-canva
Copy link
Collaborator Author

@tinyjin - an update regarding some changes I've made over the past few days. We're getting so close now! 🙏 💯

Updating the ThorVG version

I've updated the submodule reference to point to the ThorVG upstream ([email protected]:thorvg/thorvg.git), checking out the latest version (v0.14.7).

This introduced some "duplicate symbols" errors, as there are some functions inside the tools directory (i.e. lottie2gif, svg2png, svg2tvg) that have the same names as functions inside the src directory.

Seeming the Swift Package isn't using these tools as of yet, I've just added the tools directory to the Swift Package's file exclusion list.

Handling the config.h file

As mentioned in my other PR, generating this config.h file is tricky in an SPM context. To avoid needing to make changes in the thorvg repo, I've simply made a bash script that copies the config.h file into the correct destination to make the code compile.

SPM is so strict I couldn't see many other ways around it. I tried to use a Swift BuildToolPlugin, but these were very strict also on permissions to write to file. So I think a bash script will suffice for the time being. Keen to hear if you have any thoughts on this. I've added documentation in the README to help future contributors here.

And that's it! The Package is building, it's on the latest version and all the tests are passing. Looking forward to your next review ❤️

Copy link
Member

@tinyjin tinyjin left a 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.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
config.h Outdated Show resolved Hide resolved
@andyf-canva
Copy link
Collaborator Author

@tinyjin I've just updated the README and config.h file based on your last comments. Let me know what you think and if there's anymore detail you'd like me to go into. 👍

And apologies for the delay in getting back to you, have had a few busy weeks!

@tinyjin
Copy link
Member

tinyjin commented Sep 24, 2024

@andyf-canva
Thanks you so much :)

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.

@andyf-canva
Copy link
Collaborator Author

@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! 🎉

I have a question, Is there a component like based on UIView or SwiftUI?

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 LottieRenderer themselves. Which is a good point to make: if most consumers of Lottie expect to be using this in a UIKit/SwiftUI context, they may struggle to use the package.

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 LottieRenderer?

@tinyjin
Copy link
Member

tinyjin commented Sep 27, 2024

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 :)

@tinyjin
Copy link
Member

tinyjin commented Sep 27, 2024

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.

@andyf-canva
Copy link
Collaborator Author

andyf-canva commented Oct 1, 2024

@tinyjin sounds like a plan! 😄 I'll merge this PR and then open up another with the SwiftUI and UIKit changes 👍
I've also updated the issue to include those changes we've discussed.

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 😄

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