-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Fix parsing issue, add support for DotLottieConfiguration in SwiftUI LottieView #2277
Conversation
@@ -51,6 +51,9 @@ extension LayerModel { | |||
case (.null, _): | |||
return TransformLayer(layerModel: self) | |||
|
|||
case (.unknown, _): | |||
return 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.
This fixes an issue where a layer with an unknown type would log a compatibility error and fall back to the main thread rendering engine, even though the main thread engine wouldn't be able to handle it either
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.
Should we add a this comment to the code?
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.
Should we also do something about the default
case? It seems like without it the compiler would have warned us about this issue.
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.
Sure, I updated this to switch exhaustively
decodedAssets[precompAsset.id] = precompAsset | ||
precompAssets[precompAsset.id] = precompAsset | ||
} else { | ||
let imageAsset = try container.decode(ImageAsset.self) | ||
} else if let imageAsset = try? container.decode(ImageAsset.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.
If an image asset fails to parse for some reason, we can recover gracefully instead of throwing an error and rejecting the animation
@@ -8,7 +8,7 @@ | |||
// MARK: - InitializableError | |||
|
|||
enum InitializableError: Error { | |||
case invalidInput | |||
case invalidInput(file: StaticString = #file, line: UInt = #line) |
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.
Adding in file and line context makes it easier to debug where these errors are coming from
@@ -93,7 +93,7 @@ class LayerModel: Codable, DictionaryInitializable { | |||
inFrame = try container.decode(Double.self, forKey: .inFrame) | |||
outFrame = try container.decode(Double.self, forKey: .outFrame) | |||
startTime = try container.decode(Double.self, forKey: .startTime) | |||
transform = try container.decode(Transform.self, forKey: .transform) | |||
transform = try container.decodeIfPresent(Transform.self, forKey: .transform) ?? .default |
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.
All of the individual properties in a Transform
are optional and have default values, so we can easily make the Transform
itself optional with a default
/// - Defaults to `[.imageProvider]` | ||
/// - If a component is specified here, that value in the `DotLottieConfiguration` | ||
/// of an active dotLottie animation will override any value provided via other methods. | ||
public func dotLottieConfigurationComponents( |
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.
Since the DotLottieConfiguration
overrides any value you pass in via .imageProvider(...)
, .loopMode(...)
, or .speed(...)
, it seems like a good idea to make this behavior configurable. Otherwise it would be impossible to use a custom loop mode or speed when playing some dotLottie animations.
1 build increased size
SizeTest 1.0 (1)
|
Item | Install Size Change |
---|---|
📝 Lottie.Dictionary.value(for,file,line) | ⬆️ 8.2 kB |
🗑 Lottie.Dictionary.value(for) | ⬇️ -7.2 kB |
Strings.Swift File Paths | ⬆️ 4.4 kB |
📝 Lottie.LottieView.applyCurrentAnimationConfiguration(to) | ⬆️ 1.9 kB |
DYLD.Fixups | ⬆️ 1.3 kB |
🛸 Powered by Emerge Tools
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.
LGTM
@@ -51,6 +51,9 @@ extension LayerModel { | |||
case (.null, _): | |||
return TransformLayer(layerModel: self) | |||
|
|||
case (.unknown, _): | |||
return 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.
Should we also do something about the default
case? It seems like without it the compiler would have warned us about this issue.
let precompAsset = try container.decode(PrecompAsset.self) | ||
while | ||
!container.isAtEnd, | ||
let keyContainer = try? containerForKeys.nestedContainer(keyedBy: PrecompAsset.CodingKeys.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.
Should we add resilient decoder https://github.com/airbnb/ResilientDecoding in case we need the error at another time?
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.
We don't use the Codable
decoding implementation by default (the dictionary-based implementation is the default in Lottie 4.0+ since it's a lot faster), so no need to invest much in the Codable
implementation
return dotLottieFile.animation()?.animation | ||
return dotLottieFile.animation() |
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 PR contains the following updates: | Package | Update | Change | |---|---|---| | [airbnb/lottie-spm](https://github.com/airbnb/lottie-spm) | minor | `from: "4.3.4"` -> `from: "4.4.0"` | --- ### Release Notes <details> <summary>airbnb/lottie-spm (airbnb/lottie-spm)</summary> ### [`v4.4.0`](https://github.com/airbnb/lottie-spm/releases/tag/4.4.0) [Compare Source](https://github.com/airbnb/lottie-spm/compare/4.3.4...4.4.0) #### New features - Add privacy manifest ([https://github.com/airbnb/lottie-ios/pull/2252](https://github.com/airbnb/lottie-ios/pull/2252)) - Codesign Lottie.xcframework ([https://github.com/airbnb/lottie-ios/pull/2259](https://github.com/airbnb/lottie-ios/pull/2259)) - Add time remapping support to Core Animation rendering engine ([https://github.com/airbnb/lottie-ios/pull/2286](https://github.com/airbnb/lottie-ios/pull/2286)) - Add official visionOS support to lottie-ios repo ([https://github.com/airbnb/lottie-ios/pull/2287](https://github.com/airbnb/lottie-ios/pull/2287)) - lottie-spm now supports visionOS ([https://github.com/airbnb/lottie-spm/pull/12](https://github.com/airbnb/lottie-spm/pull/12)) - Adopt policy on minimum supported Swift / Xcode version, update minimum versions to Swift 5.7 / Xcode 14.1 ([https://github.com/airbnb/lottie-ios/pull/2260](https://github.com/airbnb/lottie-ios/pull/2260)) #### Bug fixes - Update LottieView to display placeholder using `overlay` instead of `ZStack` ([https://github.com/airbnb/lottie-ios/pull/2289](https://github.com/airbnb/lottie-ios/pull/2289)) - Fix issue where Core Animation rendering engine couldn't display last frame of animation when paused ([https://github.com/airbnb/lottie-ios/pull/2254](https://github.com/airbnb/lottie-ios/pull/2254)) - Do not create `DotLottieImageProvider` instance if there's no image files ([https://github.com/airbnb/lottie-ios/pull/2271](https://github.com/airbnb/lottie-ios/pull/2271)) - Mark DotLottieCache as Sendable ([https://github.com/airbnb/lottie-ios/pull/2245](https://github.com/airbnb/lottie-ios/pull/2245)) - Fix issue where AnimationKeypath in SolidLayer could be incorrect ([https://github.com/airbnb/lottie-ios/pull/2278](https://github.com/airbnb/lottie-ios/pull/2278)) - Fix issue where Repeater could be displayed incorrectly ([https://github.com/airbnb/lottie-ios/pull/2276](https://github.com/airbnb/lottie-ios/pull/2276)) - Include dSYMs in xcframework build ([https://github.com/airbnb/lottie-ios/pull/2284](https://github.com/airbnb/lottie-ios/pull/2284)) - Fix parsing issue, add support for DotLottieConfiguration in SwiftUI LottieView ([https://github.com/airbnb/lottie-ios/pull/2277](https://github.com/airbnb/lottie-ios/pull/2277)) - Fix issue where DotLottieImageProvider didn't handle base64 images ([https://github.com/airbnb/lottie-ios/pull/2283](https://github.com/airbnb/lottie-ios/pull/2283)) - Fix issue where manually interpolated keyframes could animate incorrectly ([https://github.com/airbnb/lottie-ios/pull/2285](https://github.com/airbnb/lottie-ios/pull/2285)) **Full Changelog**: airbnb/lottie-ios@4.3.4...4.4.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjM2LjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
This PR fixes two issues:
For #2269, we were failing to parse the animation. It's missing some fields that iOS requires, but that web doesn't require. We can relax the parsing a bit to allow this animation to parse successfully.
When testing the animation after fixing the parsing issue, I ran in to #2228. I bundled that animation and its image assets in a dotLottie file, but it was completely blank because the
DotLottieConfiguration.imageProvider
was being ignored.