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

Fix issue where layers with negative X scale values could be pixelated #2067

Merged
merged 8 commits into from
May 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
140 changes: 64 additions & 76 deletions Sources/Private/CoreAnimation/Animations/TransformAnimations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ protocol TransformModel {

/// The rotation of the transform on Z axis.
var rotationZ: KeyframeGroup<LottieVector1D> { get }

/// The skew of the transform (only present on `ShapeTransform`s)
var _skew: KeyframeGroup<LottieVector1D>? { get }

/// The skew axis of the transform (only present on `ShapeTransform`s)
var _skewAxis: KeyframeGroup<LottieVector1D>? { get }
}

// MARK: - Transform + TransformModel
Expand All @@ -40,6 +46,8 @@ extension Transform: TransformModel {
var _position: KeyframeGroup<LottieVector3D>? { position }
var _positionX: KeyframeGroup<LottieVector1D>? { positionX }
var _positionY: KeyframeGroup<LottieVector1D>? { positionY }
var _skew: KeyframeGroup<LottieVector1D>? { nil }
var _skewAxis: KeyframeGroup<LottieVector1D>? { nil }
}

// MARK: - ShapeTransform + TransformModel
Expand All @@ -49,6 +57,8 @@ extension ShapeTransform: TransformModel {
var _position: KeyframeGroup<LottieVector3D>? { position }
var _positionX: KeyframeGroup<LottieVector1D>? { nil }
var _positionY: KeyframeGroup<LottieVector1D>? { nil }
var _skew: KeyframeGroup<LottieVector1D>? { skew }
var _skewAxis: KeyframeGroup<LottieVector1D>? { skewAxis }
}

// MARK: - CALayer + TransformModel
Expand All @@ -66,15 +76,18 @@ extension CALayer {
context: LayerAnimationContext)
throws
{
// CALayers don't support animating skew with its own set of keyframes.
// If the transform includes a skew, we have to combine all of the transform
// components into a single set of keyframes.
// Only `ShapeTransform` supports skews.
if
let shapeTransform = transformModel as? ShapeTransform,
shapeTransform.hasSkew
// CALayers don't support animating skew with its own set of keyframes.
// If the transform includes a skew, we have to combine all of the transform
// components into a single set of keyframes.
transformModel.hasSkew
// Negative `scale.x` values aren't applied correctly by Core Animation when animating
// `transform.scale.x` and `transform.scale.y` using separate `CAKeyframeAnimation`s
// (https://openradar.appspot.com/FB9862872). If the transform includes negative `scale.x`
// values, we have to combine all of the transform components into a single set of keyframes.
|| transformModel.hasNegativeXScaleValues
{
try addCombinedTransformAnimation(for: shapeTransform, context: context)
try addCombinedTransformAnimation(for: transformModel, context: context)
}

else {
Expand Down Expand Up @@ -159,75 +172,17 @@ extension CALayer {
// Lottie animation files express scale as a numerical percentage value
// (e.g. 50%, 100%, 200%) so we divide by 100 to get the decimal values
// expected by Core Animation (e.g. 0.5, 1.0, 2.0).
// - Negative `scale.x` values aren't applied correctly by Core Animation.
// This appears to be because we animate `transform.scale.x` and `transform.scale.y`
// as separate `CAKeyframeAnimation`s instead of using a single animation of `transform` itself.
// https://openradar.appspot.com/FB9862872
// - To work around this, we set up a `rotationY` animation below
// to flip the view horizontally, which gives us the desired effect.
abs(CGFloat(scale.x) / 100)
CGFloat(scale.x) / 100
},
context: context)

/// iOS 14 and earlier doesn't properly support rendering transforms with
/// negative `scale.x` values: https://github.com/airbnb/lottie-ios/issues/1882
let osSupportsNegativeScaleValues: Bool = {
#if os(iOS) || os(tvOS)
if #available(iOS 15.0, tvOS 15.0, *) {
return true
} else {
return false
}
#else
// We'll assume this works correctly on macOS until told otherwise
return true
#endif
}()

lazy var hasNegativeXScaleValues = transformModel.scale.keyframes.contains(where: { $0.value.x < 0 })

// When `scale.x` is negative, we have to rotate the view
// half way around the y axis to flip it horizontally.
// - We don't do this in snapshot tests because it breaks the tests
// in surprising ways that don't happen at runtime. Definitely not ideal.
// - This isn't supported on iOS 14 and earlier either, so we have to
// log a compatibility error on devices running older OSs.
if TestHelpers.snapshotTestsAreRunning {
if hasNegativeXScaleValues {
context.logger.warn("""
Negative `scale.x` values are not displayed correctly in snapshot tests
""")
}
} else {
if !osSupportsNegativeScaleValues, hasNegativeXScaleValues {
try context.logCompatibilityIssue("""
iOS 14 and earlier does not support rendering negative `scale.x` values
""")
}

try addAnimation(
for: .rotationY,
keyframes: transformModel.scale,
value: { scale in
if scale.x < 0 {
return .pi
} else {
return 0
}
},
context: context)
}

try addAnimation(
for: .scaleY,
keyframes: transformModel.scale,
value: { scale in
// Lottie animation files express scale as a numerical percentage value
// (e.g. 50%, 100%, 200%) so we divide by 100 to get the decimal values
// expected by Core Animation (e.g. 0.5, 1.0, 2.0).
// - Negative `scaleY` values are correctly applied (they flip the view
// vertically), so we don't have to apply an additional rotation animation
// like we do for `scaleX`.
CGFloat(scale.y) / 100
},
context: context)
Expand Down Expand Up @@ -291,26 +246,38 @@ extension CALayer {

/// Adds an animation for the entire `transform` key by combining all of the
/// position / size / rotation / skew animations into a single set of keyframes.
/// This is necessary when there's a skew animation, since skew can only
/// be applied via a transform.
/// This is more expensive that animating each component separately, since
/// it may require manually interpolating the keyframes at each frame.
private func addCombinedTransformAnimation(
for transformModel: ShapeTransform,
for transformModel: TransformModel,
context: LayerAnimationContext)
throws
{
let combinedTransformKeyframes = Keyframes.combined(
transformModel.anchor,
transformModel.position,
transformModel.anchorPoint,
transformModel._position ?? KeyframeGroup(LottieVector3D(x: 0.0, y: 0.0, z: 0.0)),
transformModel._positionX ?? KeyframeGroup(LottieVector1D(0)),
transformModel._positionY ?? KeyframeGroup(LottieVector1D(0)),
transformModel.scale,
transformModel.rotationX,
transformModel.rotationY,
transformModel.rotationZ,
transformModel.skew,
transformModel.skewAxis,
makeCombinedResult: { anchor, position, scale, rotationX, rotationY, rotationZ, skew, skewAxis in
CATransform3D.makeTransform(
transformModel._skew ?? KeyframeGroup(LottieVector1D(0)),
transformModel._skewAxis ?? KeyframeGroup(LottieVector1D(0)),
makeCombinedResult: {
anchor, position, positionX, positionY, scale, rotationX, rotationY, rotationZ, skew, skewAxis
-> CATransform3D in

let transformPosition: CGPoint
if transformModel._positionX != nil, transformModel._positionY != nil {
transformPosition = CGPoint(x: positionX.cgFloatValue, y: positionY.cgFloatValue)
} else {
transformPosition = position.pointValue
}

return CATransform3D.makeTransform(
anchor: anchor.pointValue,
position: position.pointValue,
position: transformPosition,
scale: scale.sizeValue,
rotationX: rotationX.cgFloatValue,
rotationY: rotationY.cgFloatValue,
Expand All @@ -327,3 +294,24 @@ extension CALayer {
}

}

extension TransformModel {
/// Whether or not this transform has a non-zero skew value
var hasSkew: Bool {
guard
let _skew = _skew,
let _skewAxis = _skewAxis,
!_skew.keyframes.isEmpty,
!_skewAxis.keyframes.isEmpty
else {
return false
}

return _skew.keyframes.contains(where: { $0.value.cgFloatValue != 0 })
}

/// Whether or not this `TransformModel` has any negative X scale values
var hasNegativeXScaleValues: Bool {
scale.keyframes.contains(where: { $0.value.x < 0 })
}
}
40 changes: 40 additions & 0 deletions Sources/Private/CoreAnimation/Extensions/Keyframes+combined.swift
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,46 @@ enum Keyframes {
})
}

/// Combines the given keyframe groups of `Keyframe<T>`s into a single keyframe group of of `Keyframe<[T]>`s
/// - If all of the `KeyframeGroup`s have the exact same animation timing, the keyframes are merged
/// - Otherwise, the keyframes are manually interpolated at each frame in the animation
static func combined<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, CombinedResult>(
Copy link
Member Author

Choose a reason for hiding this comment

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

are variadic generics coming in Swift 5.9?????????

_ k1: KeyframeGroup<T1>,
_ k2: KeyframeGroup<T2>,
_ k3: KeyframeGroup<T3>,
_ k4: KeyframeGroup<T4>,
_ k5: KeyframeGroup<T5>,
_ k6: KeyframeGroup<T6>,
_ k7: KeyframeGroup<T7>,
_ k8: KeyframeGroup<T8>,
_ k9: KeyframeGroup<T9>,
_ k10: KeyframeGroup<T10>,
makeCombinedResult: (T1, T2, T3, T4, T5, T6, T7, T8, T9, T10) -> CombinedResult)
-> KeyframeGroup<CombinedResult>
where T1: AnyInterpolatable, T2: AnyInterpolatable, T3: AnyInterpolatable, T4: AnyInterpolatable,
T5: AnyInterpolatable, T6: AnyInterpolatable, T7: AnyInterpolatable, T8: AnyInterpolatable,
T9: AnyInterpolatable, T10: AnyInterpolatable
{
Keyframes.combined(
[k1, k2, k3, k4, k5, k6, k7, k8, k9, k10],
makeCombinedResult: { untypedValues in
guard
let t1 = untypedValues[0] as? T1,
let t2 = untypedValues[1] as? T2,
let t3 = untypedValues[2] as? T3,
let t4 = untypedValues[3] as? T4,
let t5 = untypedValues[4] as? T5,
let t6 = untypedValues[5] as? T6,
let t7 = untypedValues[6] as? T7,
let t8 = untypedValues[7] as? T8,
let t9 = untypedValues[8] as? T9,
let t10 = untypedValues[9] as? T10
else { return nil }

return makeCombinedResult(t1, t2, t3, t4, t5, t6, t7, t8, t9, t10)
})
}

// MARK: Private

/// Combines the given `[KeyframeGroup]` of `Keyframe<T>`s into a single `KeyframeGroup` of `Keyframe<CombinedResult>`s
Expand Down
2 changes: 2 additions & 0 deletions Sources/Private/CoreAnimation/Layers/RepeaterLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,6 @@ extension RepeaterTransform: TransformModel {
var _position: KeyframeGroup<LottieVector3D>? { position }
var _positionX: KeyframeGroup<LottieVector1D>? { nil }
var _positionY: KeyframeGroup<LottieVector1D>? { nil }
var _skew: KeyframeGroup<LottieVector1D>? { nil }
var _skewAxis: KeyframeGroup<LottieVector1D>? { nil }
}
9 changes: 0 additions & 9 deletions Sources/Private/Model/ShapeItems/ShapeTransform.swift
Original file line number Diff line number Diff line change
Expand Up @@ -164,15 +164,6 @@ final class ShapeTransform: ShapeItem {
/// Skew Axis
let skewAxis: KeyframeGroup<LottieVector1D>

/// Whether or not this transform has a non-zero skew value
var hasSkew: Bool {
guard !skew.keyframes.isEmpty, !skewAxis.keyframes.isEmpty else {
return false
}

return skew.keyframes.contains(where: { $0.value.cgFloatValue != 0 })
}

override func encode(to encoder: Encoder) throws {
try super.encode(to: encoder)
var container = encoder.container(keyedBy: CodingKeys.self)
Expand Down
1 change: 1 addition & 0 deletions Tests/Samples/Issues/issue_2066.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions Tests/SnapshotConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ extension SnapshotConfiguration {
"Nonanimating/FirstText": .precision(0.99),
"Nonanimating/verifyLineHeight": .precision(0.99),
"Nonanimating/blend_mode_test": .precision(0.99),
"Issues/issue_2066": .precision(0.9),

/// Test cases for the `AnimationKeypath` / `AnyValueProvider` system
"Nonanimating/keypathTest": .customValueProviders([
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Supports Core Animation engine
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.