-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Lottie 3.0 - A complete rewrite of Lottie in Swift! #777
Conversation
Please see #780 (a Pull Request targeting your branch), with some impact on public API:
|
A little bit sad that:
But happy that it's now on Swift and happy of the changelog. |
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.
OK, just a quick partial overall overview. I wouldn't have enough time to review it fully.
extension MaskMode { | ||
var usableMode: MaskMode { | ||
switch self { | ||
case .add: |
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.
Maybe just need one case for lighten
, one case for difference
, and the rest as default: return self
.
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.
This is the full list of mask modes supported by body movin. If we add it, code will come 👍
|
||
extension CGRect { | ||
static var veryLargeRect: CGRect { | ||
return CGRect(x: -100000000, |
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.
SwiftLint would recommend to separate thousands as in -100_000_000
.
|
||
let frameRate: CGFloat | ||
let remappingNode: NodeProperty<Vector1D>? | ||
fileprivate var animationLayers: [CompositionLayer] |
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.
with fileprivate var animationLayers: [CompositionLayer] = []
, we wouldn't need to explicitly set it in init
functions.
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 prefer it this way, so if additional initializers are added the compiler will tell you to handle animationLayers
init(imageProvider: AnimationImageProvider, assets: [String : ImageAsset]?) { | ||
self.imageProvider = imageProvider | ||
self.imageLayers = [ImageCompositionLayer]() | ||
if let assets = assets { |
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.
Simply use the nil coalescing operator: self.imageAssets = assets ?? [:]
|
||
var image: CGImage? = nil { | ||
didSet { | ||
if let image = image { |
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.
No need for if/else, directly write: contentsLayer.contents = image
|
||
|
||
// Scale | ||
self.scale = try container.decodeIfPresent(KeyframeGroup<Vector3D>.self, forKey: .scale) ?? KeyframeGroup(Vector3D(x: Double(100), y: 100, z: 100)) |
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.
There aren't any overload for this initializer of Vector3D, so the explicit cast to Double is not needed.
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.
Theres one for CGFloat and Double
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.
Oh, right, I didn't notice the private extension earlier.
Note that personally I would have wrote it 100 as Double
, but it's fine. ;)
|
||
- Returns: Deseralized `Animation`. Optional. | ||
*/ | ||
public static func named(_ name: String, |
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.
Xcode 10.2 will warn for all users: 'public' modifier is redundant for static method declared in a public extension.
} | ||
|
||
/// Make sure the file exists. | ||
guard FileManager.default.fileExists(atPath: filepath) else { |
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.
Using fileExists
is against Apple recommendations. Apple advices to directly attempt the file operation without it. See the note of https://developer.apple.com/documentation/foundation/filemanager/1415645-fileexists
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.
👍
|
spelling
Oh, I wasn't aware of the private repo with the tests. Then it's all good. Changes approved. :) Just try to fix the "'public' modifier is redundant" warnings from Xcode 10.2 before releasing. |
@Coeur thanks so much for giving this a review! Just wanted to let you know that Airbnb is now using this version of Lottie in our production app and I've tested all of our animations using it. Really appreciate all of your feedback :) (and +1 to fixing the public modifier warnings) |
This fixes warnings in Xcode 10.2 related to access modifiers
Fix warnings in Xcode 10.2
fatalError("init(layer:) Wrong Layer Class") | ||
} | ||
self.imageReferenceID = layer.imageReferenceID | ||
self.image = nil |
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.
Given that image
is already defaulted to nil, would this line be necessary?
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.
Definitely not necessary.
} | ||
|
||
func addImageLayers(_ layers: [ImageCompositionLayer]) { | ||
for layer in layers { |
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.
Not sure if it's too unreadable but this can be written with a two-liner:
let foundLayers = layers.filter { imageAssets[$0.imageReferenceID] != nil }
imageLayers.append(contentsOf: foundLayers)
self.markers = try container.decodeIfPresent([Marker].self, forKey: .markers) | ||
|
||
if let markers = markers { | ||
var markerMap: [String : Marker] = [:] |
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.
Any thoughts on writing this whole if-let-else statement to this?
self.markerMap = markers?.reduce([String : Marker](), { markerMap, marker, in
markerMap[marker.name] = marker
}
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.
reduce(::)
will copy the dictionary at each iteration. You should use reduce(into::)
for this!
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.
You’re right, I did mean reduce(into::)
as my code probably won’t compile.
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.
👍
HI, Im trying to add Lottie (animated sticker) in video. I can export it but I can't play it in runtime AVSynchronizedLayer. Is there any way to use Lottie view/layer in avsynchronizedlayer? Im using Lottie - 2.5.2 (Objective C). |
@skborhan please fill a new GitHub issue or a StackOverflow question for a matter that isn't directly related to the review of the pull request, thank you. |
Lottie 3.0 - A complete rewrite of Lottie in Swift!
Lottie 3.0 - A complete rewrite of Lottie in Swift!
Lottie 3.0 Now in Swift!
This PR removes the old Objective-C implementation of Lottie and replaces it, in its entirety, with a Swift implementation. With this comes many bug fixes, and new features.
Migrating
Lottie 3.0 is a complete rewrite of Lottie in Swift 4.0! Please Note that the Api and Class names have changed slightly. (No more LOT prefix). Read About Migrating Here
Legacy Objective C Support
To continue using, and contributing to, the Obj-c version of Lottie point to this branch objectiveC
New Features
In addition to completely rebuilding the entire animation engine, several new features have been added:
Codable
. You can decode and encode JSON data.New Documentation
Documentation has also been completely revamped here. A new contributor documentation has been written that runs through the tech side of Lottie here