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

Add support to TextProviders for CoreAnimation #1723

Merged
merged 22 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Lottie.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@
2EAF5B0427A0798700E00531 /* AnimationFontProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2EAF59F227A0798700E00531 /* AnimationFontProvider.swift */; };
2EAF5B0527A0798700E00531 /* AnimationFontProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2EAF59F227A0798700E00531 /* AnimationFontProvider.swift */; };
2EAF5B0627A0798700E00531 /* AnimationFontProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2EAF59F227A0798700E00531 /* AnimationFontProvider.swift */; };
36E57EAC28AF7ADF00B7EFDA /* HardcodedTextProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 36E57EAB28AF7ADF00B7EFDA /* HardcodedTextProvider.swift */; };
6D0E635F28246BD0007C5DB6 /* Difference in Frameworks */ = {isa = PBXBuildFile; productRef = 6D0E635E28246BD0007C5DB6 /* Difference */; };
6D99D6432823790700E5205B /* LegacyGradientFillRenderer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6D99D6422823790700E5205B /* LegacyGradientFillRenderer.swift */; };
6D99D6442823790700E5205B /* LegacyGradientFillRenderer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6D99D6422823790700E5205B /* LegacyGradientFillRenderer.swift */; };
Expand Down Expand Up @@ -783,6 +784,7 @@
2EAF59EF27A0798700E00531 /* GradientValueProvider.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GradientValueProvider.swift; sourceTree = "<group>"; };
2EAF59F027A0798700E00531 /* PointValueProvider.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PointValueProvider.swift; sourceTree = "<group>"; };
2EAF59F227A0798700E00531 /* AnimationFontProvider.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AnimationFontProvider.swift; sourceTree = "<group>"; };
36E57EAB28AF7ADF00B7EFDA /* HardcodedTextProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HardcodedTextProvider.swift; sourceTree = "<group>"; };
6D99D6422823790700E5205B /* LegacyGradientFillRenderer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LegacyGradientFillRenderer.swift; sourceTree = "<group>"; };
6DB3BDB528243FA5002A276D /* ValueProvidersTests.swift */ = {isa = PBXFileReference; indentWidth = 2; lastKnownFileType = sourcecode.swift; path = ValueProvidersTests.swift; sourceTree = "<group>"; tabWidth = 2; };
6DB3BDB7282454A6002A276D /* DictionaryInitializable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DictionaryInitializable.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -876,6 +878,7 @@
2E8040BF27A07343006E74CB /* Snapshotting+presentationLayer.swift */,
2EAF59A627A076BC00E00531 /* Bundle+Module.swift */,
2E09FA0527B6CEB600BA84E5 /* HardcodedFontProvider.swift */,
36E57EAB28AF7ADF00B7EFDA /* HardcodedTextProvider.swift */,
);
path = Utils;
sourceTree = "<group>";
Expand Down Expand Up @@ -1876,6 +1879,7 @@
6DEF696E2824A76C007D640F /* BundleTests.swift in Sources */,
2EAF59A727A076BC00E00531 /* Bundle+Module.swift in Sources */,
2E8044AE27A07347006E74CB /* Snapshotting+presentationLayer.swift in Sources */,
36E57EAC28AF7ADF00B7EFDA /* HardcodedTextProvider.swift in Sources */,
2E72128527BB32DB0027BC56 /* PerformanceTests.swift in Sources */,
6DB3BDC328245AA2002A276D /* ParsingTests.swift in Sources */,
6DB3BDB628243FA5002A276D /* ValueProvidersTests.swift in Sources */,
Expand Down
26 changes: 14 additions & 12 deletions Sources/Private/CoreAnimation/CoreAnimationLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@ final class CoreAnimationLayer: BaseAnimationLayer {
init(
animation: Animation,
imageProvider: AnimationImageProvider,
textProvider: AnimationTextProvider,
fontProvider: AnimationFontProvider,
compatibilityTrackerMode: CompatibilityTracker.Mode,
logger: LottieLogger)
throws
{
self.animation = animation
self.imageProvider = imageProvider
self.textProvider = textProvider
self.fontProvider = fontProvider
self.logger = logger
compatibilityTracker = CompatibilityTracker(mode: compatibilityTrackerMode, logger: logger)
Expand All @@ -44,6 +46,7 @@ final class CoreAnimationLayer: BaseAnimationLayer {
animation = typedLayer.animation
currentAnimationConfiguration = typedLayer.currentAnimationConfiguration
imageProvider = typedLayer.imageProvider
textProvider = typedLayer.textProvider
fontProvider = typedLayer.fontProvider
didSetUpAnimation = typedLayer.didSetUpAnimation
compatibilityTracker = typedLayer.compatibilityTracker
Expand Down Expand Up @@ -92,10 +95,16 @@ final class CoreAnimationLayer: BaseAnimationLayer {
didSet { reloadImages() }
}

/// The `AnimationTextProvider` that `TextLayer`'s use to retrieve texts,
/// that they should use to render their text context
var textProvider: AnimationTextProvider {
didSet { reloadTexts() }
Copy link
Member

Choose a reason for hiding this comment

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

reloadFonts() / reloadTexts() doesn't cause setupAnimations(context:) to be called again. Instead I think we need to call rebuildCurrentAnimation(), like we do in setValueProvider(_:keypath:) below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you! Thanks!
Fixed in b542f66 βœ…

}

/// The `FontProvider` that `TextLayer`s use to retrieve the `CTFont`
/// that they should use to render their text content
var fontProvider: AnimationFontProvider {
didSet { reloadFonts() }
didSet { reloadTexts() }
}

/// Queues the animation with the given timing configuration
Expand Down Expand Up @@ -203,6 +212,7 @@ final class CoreAnimationLayer: BaseAnimationLayer {
LayerContext(
animation: animation,
imageProvider: imageProvider,
textProvider: textProvider,
fontProvider: fontProvider,
compatibilityTracker: compatibilityTracker,
layerName: "root layer")
Expand Down Expand Up @@ -234,6 +244,7 @@ final class CoreAnimationLayer: BaseAnimationLayer {
compatibilityTracker: compatibilityTracker,
logger: logger,
currentKeypath: AnimationKeypath(keys: []),
textProvider: textProvider,
logHierarchyKeypaths: configuration.logHierarchyKeypaths)

// Perform a layout pass if necessary so all of the sublayers
Expand Down Expand Up @@ -383,15 +394,6 @@ extension CoreAnimationLayer: RootAnimationLayer {
(sublayers ?? []).filter { $0 is AnimationLayer }
}

var textProvider: AnimationTextProvider {
get { DictionaryTextProvider([:]) }
set {
logger.assertionFailure("""
The Core Animation rendering engine currently doesn't support `textProvider`s")
""")
}
}

func reloadImages() {
// When the image provider changes, we have to update all `ImageLayer`s
// so they can query the most up-to-date image from the new image provider.
Expand All @@ -402,8 +404,8 @@ extension CoreAnimationLayer: RootAnimationLayer {
}
}

func reloadFonts() {
// When the text provider changes, we have to update all `TextLayer`s
func reloadTexts() {
// When the text (or font) provider changes, we have to update all `TextLayer`s
// so they can query the most up-to-date font from the new font provider.
for sublayer in allSublayers {
if let textLayer = sublayer as? TextLayer {
Expand Down
3 changes: 3 additions & 0 deletions Sources/Private/CoreAnimation/Layers/AnimationLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ struct LayerAnimationContext {
/// The AnimationKeypath represented by the current layer
var currentKeypath: AnimationKeypath

/// The `AnimationTextProvider`
var textProvider: AnimationTextProvider

/// Whether or not to log `AnimationKeypath`s for all of the animation's layers
/// - Used for `CoreAnimationLayer.logHierarchyKeypaths()`
var logHierarchyKeypaths: Bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import QuartzCore
struct LayerContext {
let animation: Animation
let imageProvider: AnimationImageProvider
let textProvider: AnimationTextProvider
let fontProvider: AnimationFontProvider
let compatibilityTracker: CompatibilityTracker
var layerName: String
Expand Down
19 changes: 19 additions & 0 deletions Sources/Private/CoreAnimation/Layers/TextLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,22 @@ final class TextLayer: BaseCompositionLayer {
fatalError("init(coder:) has not been implemented")
}

override func setupAnimations(context: LayerAnimationContext) throws {
try super.setupAnimations(context: context)

// Question 1: Which keypath to pass?
_ = try context.currentKeypath.keys.map { key in
Copy link
Member

Choose a reason for hiding this comment

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

We should pass the entire keypath here, e.g. context.currentKeypath.fullPath

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this context.currentKeypath.keys.map -- we should be able to just remove it and just have the code on the inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got rid of it in c8a84c2 βœ…

// Question 2: Where do we get the text layer text?
Copy link
Member

Choose a reason for hiding this comment

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

try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text").text is text value displayed in the text layer.

Due to limitations of Core Animation text rendering, this text layer only supports displaying a static text value over the entire animation. That's why we extract a single string from the keyframes using exaxtlyOneKeyframe().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the method in 5055f10 🟑

Due to limitations of Core Animation text rendering, this text layer only supports displaying a static text value over the entire animation. That's why we extract a single string from the keyframes using exaxtlyOneKeyframe().

Do you mean that text won't be able to be animated in the end anyway, or that just the text can't be changed during the animation? Like we can't start animation with text "Hell.." and finish it with "Hello!" πŸ€”

let customText = context.textProvider.textFor(keypathName: key, sourceText: "text layer text")
if customText.isEmpty {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this isEmpty check -- the Main Thread rendering engine doesn't have this check (

let textString = textProvider.textFor(keypathName: keypathName, sourceText: text.text)
) and it seems like a valid use case to hide a piece of text by returning an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the method in 5055f10 🟑

Okay, I see. But then I just don't understand how do we actually decide when to set this:
renderLayer.text = customText

and when do this:

renderLayer.text = try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text").text? πŸ€”

Copy link
Member

@calda calda Aug 19, 2022

Choose a reason for hiding this comment

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

we always just do renderLayer.text = customText πŸ˜„ https://github.com/airbnb/lottie-ios/pull/1723/files#r950221577

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, then it is this way already in the code! πŸŽ‰

renderLayer.text = customText
} else {
renderLayer.text = try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text").text
}
}
}


/// Called by CoreAnimation to create a shadow copy of this layer
/// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
override init(layer: Any) {
Expand All @@ -48,6 +64,9 @@ final class TextLayer: BaseCompositionLayer {
// - We may be able to support animating `fillColor` by getting clever with layer blend modes
// or masks (e.g. use `CoreTextRenderLayer` to draw black glyphs, and then fill them in
// using a `CAShapeLayer`).

// Question 3: Should we somehow initialize LayerContextAnimations at this stage
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the renderLayer.text = text.text call from this method -- it only needs to be done once, and setupAnimations is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! Fixed in 01307aa βœ…

// and pass it to `self.setupAnimations(context: layerAnimationsContext)`
if !textLayerModel.animators.isEmpty {
try context.logCompatibilityIssue("""
The Core Animation rendering engine currently doesn't support text animators.
Expand Down
2 changes: 2 additions & 0 deletions Sources/Public/Animation/AnimationView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,7 @@ final public class AnimationView: AnimationViewBase {
let coreAnimationLayer = try CoreAnimationLayer(
animation: animation,
imageProvider: imageProvider.cachedImageProvider,
textProvider: textProvider,
fontProvider: fontProvider,
compatibilityTrackerMode: .track,
logger: logger)
Expand Down Expand Up @@ -1030,6 +1031,7 @@ final public class AnimationView: AnimationViewBase {
let coreAnimationLayer = try CoreAnimationLayer(
animation: animation,
imageProvider: imageProvider.cachedImageProvider,
textProvider: textProvider,
fontProvider: fontProvider,
compatibilityTrackerMode: .abort,
logger: logger)
Expand Down
1 change: 1 addition & 0 deletions Tests/Samples/Issues/issue_1722.json

Large diffs are not rendered by default.

15 changes: 15 additions & 0 deletions Tests/SnapshotConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ struct SnapshotConfiguration {
/// A custom `AnimationImageProvider` to use when rendering this animation
var customImageProvider: AnimationImageProvider?

/// A custom `AnimationTextProvider` to use when rendering this animatino
var customTextProvider: AnimationTextProvider?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var customTextProvider: AnimationTextProvider?
var customTextProvider: AnimationTextProvider?

btw our readme has instructions on how you can run our code formatter (which will be necessary for CI to pass): https://github.com/airbnb/lottie-ios#contributing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

31f3bf9 βœ…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, I am experiencing some troubles with the ruby version and therefore can't run the linter.
So all the previous problems I found manually and fixed. But now I don't see any problem, and it still fails...

Copy link
Member

Choose a reason for hiding this comment

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

All good, I'll run it locally and push

Copy link
Member

Choose a reason for hiding this comment

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

Ruby can be a nightmare 🫠


/// A custom `AnimationFontProvider` to use when rendering this animation
var customFontProvider: AnimationFontProvider?

Expand Down Expand Up @@ -70,6 +73,9 @@ extension SnapshotConfiguration {
// Test cases for `AnimatedImageProvider`
"Nonanimating/_dog": .customImageProvider(HardcodedImageProvider(imageName: "Samples/Images/dog.png")),

// Test cases for `AnimatedTextProvider`
"": .customTextProvider(HardcodedTextProvider()),
Copy link
Member

Choose a reason for hiding this comment

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

This should be:

Suggested change
"": .customTextProvider(HardcodedTextProvider()),
"Issues/issue_1722": .customTextProvider(HardcodedTextProvider()),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a053713 βœ…


// Test cases for `AnimationFontProvider`
"Nonanimating/Text_Glyph": .customFontProvider(HardcodedFontProvider(font: UIFont(name: "Chalkduster", size: 36)!)),

Expand Down Expand Up @@ -128,6 +134,15 @@ extension SnapshotConfiguration {
return configuration
}

static func customTextProvider(
_ customTextProvider: AnimationTextProvider)
-> SnapshotConfiguration
{
var configuration = SnapshotConfiguration.default
configuration.customTextProvider = customTextProvider
return configuration
}

/// A `SnapshotConfiguration` value using the given custom value providers
static func customFontProvider(
_ customFontProvider: AnimationFontProvider)
Expand Down
22 changes: 22 additions & 0 deletions Tests/Utils/HardcodedTextProvider.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Created by Igor Katselenbogen on 08/19/22.
// Copyright Β© 2022 Airbnb Inc. All rights reserved.

import Lottie
import QuartzCore
#if os(iOS)
import UIKit
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Are these necessary?

Suggested change
import QuartzCore
#if os(iOS)
import UIKit
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8bbf3c9 βœ…


// MARK: - HardcodedImageProvider

/// An `AnimationTextProvider` that always returns a specific hardcoded text
class HardcodedTextProvider: AnimationTextProvider {
// Question 4: What should we actually return here?
func textFor(keypathName: String, sourceText: String) -> String {
#if os(iOS)
return "text layer text"
#else
return nil
#endif
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be:

Suggested change
class HardcodedTextProvider: AnimationTextProvider {
// Question 4: What should we actually return here?
func textFor(keypathName: String, sourceText: String) -> String {
#if os(iOS)
return "text layer text"
#else
return nil
#endif
}
class HardcodedTextProvider: AnimationTextProvider {
init(text: String) {
self.text = text
}
func textFor(keypathName _: String, sourceText _: String) -> String {
text
}
private let text: String
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alrighty! a053713 βœ…

}