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

Trim shape default type value #2478

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Conversation

alexallah
Copy link
Contributor

I created a .lottie animation using a third party tool and I tried to render it using lottie-ios. Unfortunately, it logged an error and didn't display the file.

However, my animation was rendering just fine in the browser. After some digging, it turns out that the the trim shape dictionary decoder requires it to have a type specified. The JavaScript implementation (and probably others) doesn't require it and sets default value when not found:
https://github.com/LottieFiles/lottie-js/blob/a1bc6c5668323161a08159a2e040ff0cf14ab46c/src/shapes/trim-shape.ts#L40

In this PR, I set default trim type to individual. Similar to the JavaScript implementation.

@@ -34,7 +34,7 @@ final class Trim: ShapeItem {
end = try KeyframeGroup<LottieVector1D>(dictionary: endDictionary)
let offsetDictionary: [String: Any] = try dictionary.value(for: CodingKeys.offset)
offset = try KeyframeGroup<LottieVector1D>(dictionary: offsetDictionary)
let trimTypeRawValue: Int = try dictionary.value(for: CodingKeys.trimType)
let trimTypeRawValue: Int = try dictionary.value(for: CodingKeys.trimType) ?? TrimType.individually.rawValue
Copy link
Member

Choose a reason for hiding this comment

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

Please also make this change in the Decodable init above

@calda
Copy link
Member

calda commented Sep 12, 2024

Thanks! Could you add a sample json animation using this functionality as Tests/Samples/Issues/pr_2478.json, and then generate snapshot images by re-running the tests with bundle exec rake test:package

@alexallah
Copy link
Contributor Author

@calda I've updated the decoder method and added a test sample.

I'm not sure why some of the tests have failed. Seems unrelated to this change.

@calda
Copy link
Member

calda commented Sep 13, 2024

You have to record snapshots for the new sample animation, that's why the tests are failing :)

@calda
Copy link
Member

calda commented Sep 13, 2024

I pushed a change that adds the snapshots.

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.

Thanks!

@calda calda enabled auto-merge (squash) September 13, 2024 13:41
@calda calda merged commit 81b2805 into airbnb:master Sep 13, 2024
12 checks passed
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