From 68c334254529eff82a32f721d19f07c6433050fa Mon Sep 17 00:00:00 2001 From: Cal Stephens Date: Wed, 20 Sep 2023 11:25:33 -0700 Subject: [PATCH] Reduce log volume from warning about After Effects expressions (#2196) --- Lottie.xcodeproj/project.pbxproj | 4 + .../Animations/CALayer+addAnimation.swift | 7 +- .../CoreAnimation/CoreAnimationLayer.swift | 2 + .../CoreAnimation/Layers/AnimationLayer.swift | 19 ++++ Tests/LoggingTests.swift | 106 ++++++++++++++++++ Tests/SnapshotTests.swift | 8 +- Tests/ValueProvidersTests.swift | 1 + .../testAnimationWithNoIssues.LottieLogo1.txt | 1 + ...oMainThreadRenderingEngine.Boat_Loader.txt | 6 + ...EngineUnsupportedAnimation.Boat_Loader.txt | 6 + ...tMainThreadRenderingEngine.Boat_Loader.txt | 1 + ...sExpressionsWarning.LottieFiles-growth.txt | 1 + 12 files changed, 154 insertions(+), 8 deletions(-) create mode 100644 Tests/LoggingTests.swift create mode 100644 Tests/__Snapshots__/LoggingTests/testAnimationWithNoIssues.LottieLogo1.txt create mode 100644 Tests/__Snapshots__/LoggingTests/testAutomaticFallbackToMainThreadRenderingEngine.Boat_Loader.txt create mode 100644 Tests/__Snapshots__/LoggingTests/testCoreAnimationRenderingEngineUnsupportedAnimation.Boat_Loader.txt create mode 100644 Tests/__Snapshots__/LoggingTests/testExplicitMainThreadRenderingEngine.Boat_Loader.txt create mode 100644 Tests/__Snapshots__/LoggingTests/testUnsupportedAfterEffectsExpressionsWarning.LottieFiles-growth.txt diff --git a/Lottie.xcodeproj/project.pbxproj b/Lottie.xcodeproj/project.pbxproj index 29983a85e6..7e49913f0e 100644 --- a/Lottie.xcodeproj/project.pbxproj +++ b/Lottie.xcodeproj/project.pbxproj @@ -330,6 +330,7 @@ 0887347B28F0CCDD00458627 /* LottieAnimationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0887347428F0CCDD00458627 /* LottieAnimationView.swift */; }; 0887347C28F0CCDD00458627 /* LottieAnimationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0887347428F0CCDD00458627 /* LottieAnimationView.swift */; }; 0887347D28F0CCDD00458627 /* LottieAnimationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0887347428F0CCDD00458627 /* LottieAnimationView.swift */; }; + 089C50C22ABA0C6D007903D3 /* LoggingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 089C50C12ABA0C6D007903D3 /* LoggingTests.swift */; }; 08AB05552A61C20400DE86FD /* ReducedMotionOption.swift in Sources */ = {isa = PBXBuildFile; fileRef = 08AB05542A61C20400DE86FD /* ReducedMotionOption.swift */; }; 08AB05562A61C20400DE86FD /* ReducedMotionOption.swift in Sources */ = {isa = PBXBuildFile; fileRef = 08AB05542A61C20400DE86FD /* ReducedMotionOption.swift */; }; 08AB05572A61C20400DE86FD /* ReducedMotionOption.swift in Sources */ = {isa = PBXBuildFile; fileRef = 08AB05542A61C20400DE86FD /* ReducedMotionOption.swift */; }; @@ -1176,6 +1177,7 @@ 0887347228F0CCDD00458627 /* LottieAnimationHelpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LottieAnimationHelpers.swift; sourceTree = ""; }; 0887347328F0CCDD00458627 /* LottieAnimationViewInitializers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LottieAnimationViewInitializers.swift; sourceTree = ""; }; 0887347428F0CCDD00458627 /* LottieAnimationView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LottieAnimationView.swift; sourceTree = ""; }; + 089C50C12ABA0C6D007903D3 /* LoggingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LoggingTests.swift; sourceTree = ""; }; 08AB05542A61C20400DE86FD /* ReducedMotionOption.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ReducedMotionOption.swift; sourceTree = ""; }; 08AB05582A61C5B700DE86FD /* DecodingStrategy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DecodingStrategy.swift; sourceTree = ""; }; 08AB055C2A61C5CC00DE86FD /* RenderingEngineOption.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RenderingEngineOption.swift; sourceTree = ""; }; @@ -1755,6 +1757,7 @@ 08CB2680291ED2B700B4F071 /* AnimationViewTests.swift */, 2E70F79E295BB6D30089A0EF /* CompatibleAnimationViewTests.swift */, 080F5FDB2AB1075000ADC32C /* TextProviderTests.swift */, + 089C50C12ABA0C6D007903D3 /* LoggingTests.swift */, ); path = Tests; sourceTree = ""; @@ -3233,6 +3236,7 @@ 2EAF59A727A076BC00E00531 /* Bundle+Module.swift in Sources */, 2E70F79F295BB6D30089A0EF /* CompatibleAnimationViewTests.swift in Sources */, 2E8044AE27A07347006E74CB /* Snapshotting+presentationLayer.swift in Sources */, + 089C50C22ABA0C6D007903D3 /* LoggingTests.swift in Sources */, 36E57EAC28AF7ADF00B7EFDA /* HardcodedTextProvider.swift in Sources */, 2E72128527BB32DB0027BC56 /* PerformanceTests.swift in Sources */, 6DB3BDC328245AA2002A276D /* ParsingTests.swift in Sources */, diff --git a/Sources/Private/CoreAnimation/Animations/CALayer+addAnimation.swift b/Sources/Private/CoreAnimation/Animations/CALayer+addAnimation.swift index 0c4dd771f8..50711038c7 100644 --- a/Sources/Private/CoreAnimation/Animations/CALayer+addAnimation.swift +++ b/Sources/Private/CoreAnimation/Animations/CALayer+addAnimation.swift @@ -50,15 +50,14 @@ extension CALayer { guard !keyframes.isEmpty else { return nil } // Check if this set of keyframes uses After Effects expressions, which aren't supported. - if let unsupportedAfterEffectsExpression = keyframeGroup.unsupportedAfterEffectsExpression { + // - We only log this once per `CoreAnimationLayer` instance. + if keyframeGroup.unsupportedAfterEffectsExpression != nil, !context.loggingState.hasLoggedAfterEffectsExpressionsWarning { + context.loggingState.hasLoggedAfterEffectsExpressionsWarning = true context.logger.info(""" `\(property.caLayerKeypath)` animation for "\(context.currentKeypath.fullPath)" \ includes an After Effects expression (https://helpx.adobe.com/after-effects/using/expression-language.html), \ which is not supported by lottie-ios (expressions are only supported by lottie-web). \ This animation may not play correctly. - - \(unsupportedAfterEffectsExpression.replacingOccurrences(of: "\n", with: "\n ")) - """) } diff --git a/Sources/Private/CoreAnimation/CoreAnimationLayer.swift b/Sources/Private/CoreAnimation/CoreAnimationLayer.swift index 772e69a119..d5745a8449 100644 --- a/Sources/Private/CoreAnimation/CoreAnimationLayer.swift +++ b/Sources/Private/CoreAnimation/CoreAnimationLayer.swift @@ -209,6 +209,7 @@ final class CoreAnimationLayer: BaseAnimationLayer { private let valueProviderStore: ValueProviderStore private let compatibilityTracker: CompatibilityTracker private let logger: LottieLogger + private let loggingState = LoggingState() /// The current playback state of the animation that is displayed in this layer private var currentPlaybackState: PlaybackState? { @@ -265,6 +266,7 @@ final class CoreAnimationLayer: BaseAnimationLayer { valueProviderStore: valueProviderStore, compatibilityTracker: compatibilityTracker, logger: logger, + loggingState: loggingState, currentKeypath: AnimationKeypath(keys: []), textProvider: textProvider, recordHierarchyKeypath: configuration.recordHierarchyKeypath) diff --git a/Sources/Private/CoreAnimation/Layers/AnimationLayer.swift b/Sources/Private/CoreAnimation/Layers/AnimationLayer.swift index d1ef86629c..ed79358e45 100644 --- a/Sources/Private/CoreAnimation/Layers/AnimationLayer.swift +++ b/Sources/Private/CoreAnimation/Layers/AnimationLayer.swift @@ -38,6 +38,9 @@ struct LayerAnimationContext { /// The logger that should be used for assertions and warnings let logger: LottieLogger + /// Mutable state related to log events, stored on the `CoreAnimationLayer`. + let loggingState: LoggingState + /// The AnimationKeypath represented by the current layer var currentKeypath: AnimationKeypath @@ -84,3 +87,19 @@ struct LayerAnimationContext { return copy } } + +// MARK: - LoggingState + +/// Mutable state related to log events, stored on the `CoreAnimationLayer`. +final class LoggingState { + + // MARK: Lifecycle + + init() { } + + // MARK: Internal + + /// Whether or not the warning about unsupported After Effects expressions + /// has been logged yet for this layer. + var hasLoggedAfterEffectsExpressionsWarning = false +} diff --git a/Tests/LoggingTests.swift b/Tests/LoggingTests.swift new file mode 100644 index 0000000000..012130b979 --- /dev/null +++ b/Tests/LoggingTests.swift @@ -0,0 +1,106 @@ +// Created by Cal Stephens on 9/19/23. +// Copyright © 2023 Airbnb Inc. All rights reserved. + +import SnapshotTesting +import UIKit +import XCTest + +@testable import Lottie + +// MARK: - LoggingTests + +@MainActor +final class LoggingTests: XCTestCase { + + // MARK: Internal + + func testAnimationWithNoIssues() async { + await snapshotLoggedMessages( + animationName: "LottieLogo1", + configuration: LottieConfiguration(renderingEngine: .automatic)) + } + + func testAutomaticFallbackToMainThreadRenderingEngine() async { + // This animation is not supported by the Core Animation rendering engine + // because it uses time remapping + await snapshotLoggedMessages( + animationName: "Boat_Loader", + configuration: LottieConfiguration(renderingEngine: .automatic)) + } + + func testCoreAnimationRenderingEngineUnsupportedAnimation() async { + // This animation is not supported by the Core Animation rendering engine + // because it uses time remapping + await snapshotLoggedMessages( + animationName: "Boat_Loader", + configuration: LottieConfiguration(renderingEngine: .coreAnimation)) + } + + func testExplicitMainThreadRenderingEngine() async { + // This animation is not supported by the Core Animation rendering engine + // because it uses time remapping. Manually specifying the Main Thread + // rendering engine should silence the log messages. + await snapshotLoggedMessages( + animationName: "Boat_Loader", + configuration: LottieConfiguration(renderingEngine: .mainThread)) + } + + func testUnsupportedAfterEffectsExpressionsWarning() async { + // This animation has unsupported After Effects expressions, which triggers a log message + await snapshotLoggedMessages( + animationName: "LottieFiles/growth", + configuration: LottieConfiguration(renderingEngine: .automatic)) + } + + // MARK: Private + + private func snapshotLoggedMessages( + animationName: String, + configuration: LottieConfiguration, + function: String = #function, + line: UInt = #line) + async + { + let loggedMessages = await loggedMessages(for: animationName, configuration: configuration) + + assertSnapshot( + matching: loggedMessages.joined(separator: "\n"), + as: .description, + named: animationName, + testName: function, + line: line) + } + + private func loggedMessages(for animationName: String, configuration: LottieConfiguration) async -> [String] { + var logMessages = [String]() + + let logger = LottieLogger( + assert: { condition, message, _, _ in + if !condition() { + logMessages.append("[assertionFailure] \(message())") + } + }, + assertionFailure: { message, _, _ in + logMessages.append("[assertionFailure] \(message())") + }, + warn: { message, _, _ in + logMessages.append("[warning] \(message())") + }, + info: { message in + logMessages.append("[info] \(message())") + }) + + let animationView = await SnapshotConfiguration.makeAnimationView( + for: animationName, + configuration: configuration, + logger: logger)! + + animationView.forceDisplayUpdate() + + if logMessages.isEmpty { + return ["Animation setup did not emit any logs"] + } + + return logMessages + } +} diff --git a/Tests/SnapshotTests.swift b/Tests/SnapshotTests.swift index dcbe9a9be5..652f9079c9 100644 --- a/Tests/SnapshotTests.swift +++ b/Tests/SnapshotTests.swift @@ -127,6 +127,10 @@ class SnapshotTests: XCTestCase { #if os(iOS) for sampleAnimationName in Samples.sampleAnimationNames { for percent in progressPercentagesToSnapshot(for: SnapshotConfiguration.forSample(named: sampleAnimationName)) { + guard SnapshotConfiguration.forSample(named: sampleAnimationName).shouldSnapshot(using: configuration) else { + continue + } + guard let animationView = await SnapshotConfiguration.makeAnimationView( for: sampleAnimationName, @@ -282,10 +286,6 @@ extension SnapshotConfiguration { { let snapshotConfiguration = customSnapshotConfiguration ?? SnapshotConfiguration.forSample(named: sampleAnimationName) - guard snapshotConfiguration.shouldSnapshot(using: configuration) else { - return nil - } - let animationView: LottieAnimationView if let animation = Samples.animation(named: sampleAnimationName) { animationView = LottieAnimationView( diff --git a/Tests/ValueProvidersTests.swift b/Tests/ValueProvidersTests.swift index 94775252d3..a5d0889b96 100644 --- a/Tests/ValueProvidersTests.swift +++ b/Tests/ValueProvidersTests.swift @@ -42,6 +42,7 @@ final class ValueProvidersTests: XCTestCase { valueProviderStore: store, compatibilityTracker: .init(mode: .track, logger: .printToConsole), logger: .printToConsole, + loggingState: LoggingState(), currentKeypath: .init(keys: []), textProvider: DictionaryTextProvider([:])) diff --git a/Tests/__Snapshots__/LoggingTests/testAnimationWithNoIssues.LottieLogo1.txt b/Tests/__Snapshots__/LoggingTests/testAnimationWithNoIssues.LottieLogo1.txt new file mode 100644 index 0000000000..9cec40fd7e --- /dev/null +++ b/Tests/__Snapshots__/LoggingTests/testAnimationWithNoIssues.LottieLogo1.txt @@ -0,0 +1 @@ +Animation setup did not emit any logs \ No newline at end of file diff --git a/Tests/__Snapshots__/LoggingTests/testAutomaticFallbackToMainThreadRenderingEngine.Boat_Loader.txt b/Tests/__Snapshots__/LoggingTests/testAutomaticFallbackToMainThreadRenderingEngine.Boat_Loader.txt new file mode 100644 index 0000000000..7b444e732e --- /dev/null +++ b/Tests/__Snapshots__/LoggingTests/testAutomaticFallbackToMainThreadRenderingEngine.Boat_Loader.txt @@ -0,0 +1,6 @@ +[warning] Encountered Core Animation compatibility issue while setting up animation: +[Chest] The Core Animation rendering engine partially supports time remapping keyframes, but this is somewhat experimental and has some known issues. Since it doesn't work in all cases, we have to fall back to using the main thread engine when using `RenderingEngineOption.automatic`. +This animation may have additional compatibility issues, but animation setup was cancelled early to avoid wasted work. + +Automatically falling back to Main Thread rendering engine. This fallback comes with some additional performance +overhead, which can be reduced by manually specifying that this animation should always use the Main Thread engine. diff --git a/Tests/__Snapshots__/LoggingTests/testCoreAnimationRenderingEngineUnsupportedAnimation.Boat_Loader.txt b/Tests/__Snapshots__/LoggingTests/testCoreAnimationRenderingEngineUnsupportedAnimation.Boat_Loader.txt new file mode 100644 index 0000000000..0ab0f8ed28 --- /dev/null +++ b/Tests/__Snapshots__/LoggingTests/testCoreAnimationRenderingEngineUnsupportedAnimation.Boat_Loader.txt @@ -0,0 +1,6 @@ +[assertionFailure] Encountered Core Animation compatibility issues while setting up animation: +[Chest] The Core Animation rendering engine partially supports time remapping keyframes, but this is somewhat experimental and has some known issues. Since it doesn't work in all cases, we have to fall back to using the main thread engine when using `RenderingEngineOption.automatic`. + +This animation cannot be rendered correctly by the Core Animation engine. +To resolve this issue, you can use `RenderingEngineOption.automatic`, which automatically falls back +to the Main Thread rendering engine when necessary, or just use `RenderingEngineOption.mainThread`. diff --git a/Tests/__Snapshots__/LoggingTests/testExplicitMainThreadRenderingEngine.Boat_Loader.txt b/Tests/__Snapshots__/LoggingTests/testExplicitMainThreadRenderingEngine.Boat_Loader.txt new file mode 100644 index 0000000000..9cec40fd7e --- /dev/null +++ b/Tests/__Snapshots__/LoggingTests/testExplicitMainThreadRenderingEngine.Boat_Loader.txt @@ -0,0 +1 @@ +Animation setup did not emit any logs \ No newline at end of file diff --git a/Tests/__Snapshots__/LoggingTests/testUnsupportedAfterEffectsExpressionsWarning.LottieFiles-growth.txt b/Tests/__Snapshots__/LoggingTests/testUnsupportedAfterEffectsExpressionsWarning.LottieFiles-growth.txt new file mode 100644 index 0000000000..2e9af0b3dc --- /dev/null +++ b/Tests/__Snapshots__/LoggingTests/testUnsupportedAfterEffectsExpressionsWarning.LottieFiles-growth.txt @@ -0,0 +1 @@ +[info] `transform.rotation.z` animation for "men.man 1.hand 2.Transform" includes an After Effects expression (https://helpx.adobe.com/after-effects/using/expression-language.html), which is not supported by lottie-ios (expressions are only supported by lottie-web). This animation may not play correctly. \ No newline at end of file