Add support to `TextProviders` for `CoreAnimation` #1723

lostinstaches · 2022-08-19T08:10:24Z

What 🙋🏼

This PR is supposed to provide support for text providers, so it would be possible to pass text to animations running on the CoreAnimation engine. 🚀

This is my first attempt of doing the PR in this library and an open source project in general. So please bear with me a bit! 🤠
I have four questions/concerns that should be answered before this PR can be valid! (All questions are also presented in the code as comments).

Concerns 😬

Which keypath should we pass in the setupAnimations method?
Where do we get the text layer source text from?
How do we init LayerContextAnimations inside configureRenderLayer to call the self.setupAnimations method properly?
In HardcodedTextProvider, what do we want to return?

Related issue 🤖

Fixes #1722 ☝🏼

calda · 2022-08-19T13:19:40Z

Sources/Private/CoreAnimation/CoreAnimationLayer.swift

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


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.

Got you! Thanks!
Fixed in b542f66 ✅

calda · 2022-08-19T13:22:30Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

+    try super.setupAnimations(context: context)
+
+    // Question 1: Which keypath to pass?
+    _ = try context.currentKeypath.keys.map { key in


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

calda · 2022-08-19T13:24:55Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

+
+    // Question 1: Which keypath to pass?
+    _ = try context.currentKeypath.keys.map { key in
+      // Question 2: Where do we get the text layer text?


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().

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!" 🤔

calda · 2022-08-19T13:26:18Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

+    _ = try context.currentKeypath.keys.map { key in
+      // Question 2: Where do we get the text layer text?
+      let customText = context.textProvider.textFor(keypathName: key, sourceText: "text layer text")
+      if customText.isEmpty {


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

lottie-ios/Sources/Private/MainThread/LayerContainers/CompLayers/TextCompositionLayer.swift

Line 115 in 167812e

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.

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

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

Okay, then it is this way already in the code! 🎉

calda · 2022-08-19T13:27:17Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

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


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.

Right! Fixed in 01307aa ✅

calda · 2022-08-19T13:28:13Z

Tests/SnapshotConfiguration.swift

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


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

31f3bf9 ✅

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...

All good, I'll run it locally and push

Ruby can be a nightmare 🫠

calda · 2022-08-19T13:28:51Z

Tests/SnapshotConfiguration.swift

@@ -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()),


This should be:

Suggested change

"": .customTextProvider(HardcodedTextProvider()),

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

a053713 ✅

calda · 2022-08-19T13:30:07Z

Tests/Utils/HardcodedTextProvider.swift

+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
+  }


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

}

Alrighty! a053713 ✅

calda

Looking good!

calda · 2022-08-19T13:58:17Z

Sources/Private/CoreAnimation/CoreAnimationLayer.swift

+      // We need to rebuild the current animation after registering a value provider,
+      // since any existing `CAAnimation`s could now be out of date.


Suggested change

// We need to rebuild the current animation after registering a value provider,

// since any existing `CAAnimation`s could now be out of date.

// We need to rebuild the current animation after updating the text provider,

// since this is used in `TextLayer.setupAnimations(context:)`

7a71e9a ✅

calda · 2022-08-19T13:59:17Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

@@ -26,10 +26,11 @@ final class TextLayer: BaseCompositionLayer {
  override func setupAnimations(context: LayerAnimationContext) throws {
    try super.setupAnimations(context: context)

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


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

Got rid of it in c8a84c2 ✅

calda · 2022-08-19T14:00:27Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

+      // Question 2.1: Should we get from somewhere source text? What if the user has custom name?
+      let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: "text layer text")
+      // Question 2.2: If we don't have this .isEmpty check, then how do we decide when to set our custom text, and when to
+      // set the result of the `exactlyOneKeyFrame()`?


If you look at the behavior of the main thread rendering engine, it just unconditionally sets renderLayer.text to the result of textProvider.textFor(keypathName:sourceText:). Since that method doesn't return an optional, it looks like expected usage is to just return sourceText if you don't want to customize the text for that specific keypath. That's how DefaultTextProvider works, for example.

Updated the method in c8a84c2 🟡

Then we just set it like this? 🤔

calda · 2022-08-19T14:01:03Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

-      // Question 2: Where do we get the text layer text?
-      let customText = context.textProvider.textFor(keypathName: key, sourceText: "text layer text")
+      // Question 2.1: Should we get from somewhere source text? What if the user has custom name?
+      let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: "text layer text")


sourceText is just the text value specified in the animation json for this layer -- so, try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text").text

But what if the JSON will have a different name for this text layer?
Would there be a way to pass some specific value to configure animation for such behavior?

calda · 2022-08-19T14:04:18Z

Tests/SnapshotConfiguration.swift

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

+    // Test cases for `AnimatedTextProvider`
+    "Issues/issue_1722": .customTextProvider(HardcodedTextProvider(text: "Bounce-bounce")),


When you're done with the code changes you can generate the snapshot images for this by running the unit test target on an iPhone 8 simulator (or by running bundle exec rake test:package on the command line).

fyi I think the snapshot tests are sensitive to specific Xcode versions -- right now I think they run correctly on Xcode 13.4.1 but not on Xcode 14. If you get a lot of spurious failures when you run the tests let me know and I can pull you branch and run them for you.

Hm unfortunately it always return this message during the test run:
No reference was found on disk. Automatically recorded snapshot:

If you get a lot of spurious failures when you run the tests let me know and I can pull you branch and run them for you.

Yeah, Cal, that would be helpful, thanks a bunch! 🙏🏼

By the way the name of the text layer in the provided animation is Animation text layer 🙏🏼

Hm unfortunately it always return this message during the test run:
No reference was found on disk. Automatically recorded snapshot:

This is expected the first time you run the tests after you add a new sample JSON file. If you run the tests again this goes away, since now it compares the new snapshot with the previously-recorded one.

Hm true. However, the snapshots anyway don't contain the text. Could it be because the text layer has a different name?

I think this was because we had to call textRenderLayer.sizeToFit() after updating textRenderLayer.text.

calda · 2022-08-19T14:06:38Z

Tests/SnapshotConfiguration.swift

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


Make sure to actually apply this text provider to the AnimationView in SnapshotConfiguration.makeAnimationView: https://github.com/airbnb/lottie-ios/blob/master/Tests/SnapshotTests.swift#L255

Cool to know, thanks!
Done in c7a21ce ✅

campersau · 2022-08-19T14:37:48Z

Sources/Private/CoreAnimation/Layers/TextLayer.swift

@@ -23,6 +23,13 @@ final class TextLayer: BaseCompositionLayer {
    fatalError("init(coder:) has not been implemented")
  }

+  override func setupAnimations(context: LayerAnimationContext) throws {
+    try super.setupAnimations(context: context)
+    let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: "text layer text")


Would it be possible to pass in the actual source text like here:

lottie-ios/Sources/Private/MainThread/LayerContainers/CompLayers/TextCompositionLayer.swift

Line 115 in 167812e

let textString = textProvider.textFor(keypathName: keypathName, sourceText: text.text)

Suggested change

let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: "text layer text")

let sourceText = try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text")

let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: sourceText )

I think the textLayerModel.text.exactlyOneKeyframe code (currently on line 51) can be removed from configureRenderLayer.

Sure, I think it's possible.
Done in 8cb4ccd ✅

I think the textLayerModel.text.exactlyOneKeyframe code (currently on line 51) can be removed from configureRenderLayer.

But I am not sure about that. Since we use the text later on in the configureRenderLayer(:)

renderLayer.alignment = text.justification.textAlignment renderLayer.lineHeight = CGFloat(text.lineHeight) renderLayer.tracking = (CGFloat(text.fontSize) * CGFloat(text.tracking)) / 1000 renderLayer.fillColor = text.fillColorData?.cgColorValue renderLayer.strokeColor = text.strokeColorData?.cgColorValue renderLayer.strokeWidth = CGFloat(text.strokeWidth ?? 0) renderLayer.strokeOnTop = text.strokeOverFill ?? false renderLayer.preferredSize = text.textFrameSize?.sizeValue

Ah, haven't expanded far enough downwards, nevermind.

calda · 2022-08-19T15:12:22Z

Tests/Utils/HardcodedTextProvider.swift

+import QuartzCore
+#if os(iOS)
+import UIKit
+#endif


Are these necessary?

Suggested change

import QuartzCore

#if os(iOS)

import UIKit

#endif

8bbf3c9 ✅

calda

Nice work, I pushed some commits to make the tests pass etc

lostinstaches · 2022-08-19T15:44:24Z

Thanks a lot @calda! Superb experience to try out some code here together!
If everything works out with CI and it would be possible to merge the PR, do we know when the updated version could be released, so it would be possible to use the core engine for text animations? 👀

calda · 2022-08-19T15:46:10Z

Thanks for the contribution!

I turned on auto-merge so this PR will merge automatically as soon as CI finishes, assuming the checks are all green.

We just released 3.4.2 so I don't plan on releasing a new version immediately. We'd probably release 3.4.3 in a month or so. In the mean time your project could point directly to master / the commit that includes this change.

Co-authored-by: Cal Stephens <[email protected]>

…#1723) Co-authored-by: Cal Stephens <[email protected]>

lostinstaches added 4 commits August 19, 2022 09:54

chore: add animation json for the issue

5436bf7

feat: add textProvider to LC and LAC

ba40ed1

feat: override setupAnimations in TextLayer

f4fe478

feat: add hardcoded test for text provider (WIP)

ae4e887

lostinstaches mentioned this pull request Aug 19, 2022

Core Animation rendering engine doesn't support text providers #1722

Closed

chore: fix order of params

2632390

calda reviewed Aug 19, 2022

View reviewed changes

lostinstaches added 5 commits August 19, 2022 15:36

feat: call rebuildCurrentAnimations when set TP

b542f66

chore: remove renderLayer.text set in configureRL

01307aa

feat: update setupAnimations method

5055f10

chore: improve hardcoded text provider

a053713

chore: fix formatting

31f3bf9

calda reviewed Aug 19, 2022

View reviewed changes

chore: imrpove doc inside didSet textProvider

7a71e9a

calda reviewed Aug 19, 2022

View reviewed changes

feat: set custom text to renderLayer.text

c8a84c2

calda reviewed Aug 19, 2022

View reviewed changes

lostinstaches added 2 commits August 19, 2022 16:07

feat: apply text provider to the animation view

c7a21ce

chore: fix linter

ff77091

chore: add text provider to tests

77bb6ba

campersau reviewed Aug 19, 2022

View reviewed changes

feat: passing the actual source text

8cb4ccd

lostinstaches requested review from calda and campersau and removed request for calda and campersau August 19, 2022 14:43

calda reviewed Aug 19, 2022

View reviewed changes

lostinstaches and others added 6 commits August 19, 2022 17:18

chore: remove obsolete imports in HTP

8bbf3c9

chore: remove obsolete question-comment

1d50bef

lint

913e1da

Update snapshots

de65cbb

Clean up, make tests pass

b2309b6

Fix typo

8be6a68

calda approved these changes Aug 19, 2022

View reviewed changes

calda enabled auto-merge (squash) August 19, 2022 15:43

calda merged commit 91f46bf into airbnb:master Aug 19, 2022

calda added a commit that referenced this pull request Nov 28, 2022

Add TextProvider support to Core Animation rendering engine (#1723)

a1ba21f

Co-authored-by: Cal Stephens <[email protected]>

calda added a commit that referenced this pull request Dec 1, 2022

Add TextProvider support to Core Animation rendering engine (#1723)

3a3d6a1

Co-authored-by: Cal Stephens <[email protected]>

MoroziOS pushed a commit to MoroziOS/tmg-lottie-ios that referenced this pull request May 22, 2024

Add TextProvider support to Core Animation rendering engine (airbnb…

f3d4073

…#1723) Co-authored-by: Cal Stephens <[email protected]>

	var customTextProvider: AnimationTextProvider?
	var customTextProvider: AnimationTextProvider?

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

		// We need to rebuild the current animation after registering a value provider,
		// since any existing `CAAnimation`s could now be out of date.

	let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: "text layer text")
	let sourceText = try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text")
	let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: sourceText )

Add support to TextProviders for CoreAnimation #1723

Add support to TextProviders for CoreAnimation #1723

Conversation

lostinstaches commented Aug 19, 2022 • edited by calda Loading

What 🙋🏼

Concerns 😬

Related issue 🤖

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda left a comment

Choose a reason for hiding this comment

lostinstaches commented Aug 19, 2022

calda commented Aug 19, 2022

Add support to `TextProviders` for `CoreAnimation` #1723

Add support to `TextProviders` for `CoreAnimation` #1723

lostinstaches commented Aug 19, 2022 •

edited by calda

Loading

calda Aug 19, 2022 •

edited

Loading