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

Added: PathValueProvider #2376

Closed
wants to merge 11 commits into from
12 changes: 11 additions & 1 deletion Sources/Private/CoreAnimation/Animations/LayerProperty.swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ enum PropertyName: String, CaseIterable {
case rotation = "Rotation"
case strokeWidth = "Stroke Width"
case gradientColors = "Colors"
case path = "Path"
}

// MARK: CALayer properties
Expand Down Expand Up @@ -218,7 +219,7 @@ extension LayerProperty {
.init(
caLayerKeypath: #keyPath(CAShapeLayer.path),
defaultValue: nil,
customizableProperty: nil /* currently unsupported */ )
customizableProperty: .path)
}

static var fillColor: LayerProperty<CGColor> {
Expand Down Expand Up @@ -390,6 +391,15 @@ extension CustomizableProperty {
})
}

static var path: CustomizableProperty<CGPath> {
Copy link
Member

Choose a reason for hiding this comment

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

How have you been testing this? Can you add a snapshot test case for this functionality?

You'll need to add a case for this to SnapshotConfiguration.swift, you can follow the examples of the existing value provider test cases. If you add a new sample animation file, you can add it at Tests/Samples/Issues/pr_2376.json.

Copy link
Author

Choose a reason for hiding this comment

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

I encountered an issue with the tests, but I think it may be a problem with how the tests work.

recordHierarchyKeypath, which is used to log all the keypaths in use by the Core Animation engine, logs whatever is used in a ValueProvider. This now includes a Path property.

The test fails because the Path property seems to be missing in the Main Thread engine.

But the PathValueProvider works for both, so I think the test is at fault.

I will add Snapshot tests as you suggested.

.init(
name: [.path],
conversion: { typeErasedValue, _ in
guard let path = typeErasedValue as? BezierPath else { return nil }
return path.cgPath()
})
}

static func floatValue(_ name: PropertyName...) -> CustomizableProperty<CGFloat> {
.init(
name: name,
Expand Down
5 changes: 4 additions & 1 deletion Sources/Private/CoreAnimation/ValueProviderStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ final class ValueProviderStore {
context: LayerAnimationContext)
throws -> KeyframeGroup<Value>?
{
context.recordHierarchyKeypath?(keypath.fullPath)
// NOTE: Not recording Path because it messes up tests
Copy link
Member

Choose a reason for hiding this comment

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

Was this to work around the failure of testCoreAnimationEngineKeypathLogging?

Instead, please re-record the snapshots. You can add isRecording = true locally, and then re-run the test:

  func testCoreAnimationEngineKeypathLogging() async {
+   isRecording = true
    for animationKeypathTestAnimation in animationKeypathTestAnimations {
      await snapshotHierarchyKeypaths(
        animationName: animationKeypathTestAnimation,
        configuration: LottieConfiguration(renderingEngine: .coreAnimation))
    }
  }

We'll just want to make sure the updated snapshot output (which you'll have to commit) is reasonable.

Copy link
Member

@calda calda Apr 15, 2024

Choose a reason for hiding this comment

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

We shouldn't change the code to work around a test failure, unless the test failure is highlighting a genuine bug or compatibility issue. Instead we should adjust the test cases as necessary.

Copy link
Member

Choose a reason for hiding this comment

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

The test fails because the Path property seems to be missing in the Main Thread engine.

If this was the issue, then we need to fix how the Path property is set up. Although I didn't see that issue when I pulled your code and ran the tests (I only saw a failure in testCoreAnimationEngineKeypathLogging).

Copy link
Author

Choose a reason for hiding this comment

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

OK, then it seems I misunderstood how the test works.
I'll record the tests and make sure they pass as well as add tests as necessary.

if !(customizableProperty.name == [.path]) {
context.recordHierarchyKeypath?(keypath.fullPath)
}

guard let anyValueProvider = valueProvider(for: keypath) else {
return nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ extension CurveVertex: Interpolatable {
// MARK: - BezierPath + Interpolatable

extension BezierPath: Interpolatable {
func interpolate(to: BezierPath, amount: CGFloat) -> BezierPath {
public func interpolate(to: BezierPath, amount: CGFloat) -> BezierPath {
var newPath = BezierPath()
for i in 0..<min(elements.count, to.elements.count) {
let fromVertex = elements[i].vertex
Expand Down
41 changes: 22 additions & 19 deletions Sources/Private/Utility/Primitives/BezierPath.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import CoreGraphics
// MARK: - BezierPath

/// A container that holds instructions for creating a single, unbroken Bezier Path.
struct BezierPath {
public struct BezierPath {
calda marked this conversation as resolved.
Show resolved Hide resolved

// MARK: Lifecycle

Expand Down Expand Up @@ -289,7 +289,7 @@ extension BezierPath: Codable {

// MARK: Lifecycle

init(from decoder: Decoder) throws {
public init(from decoder: Decoder) throws {
let container: KeyedDecodingContainer<BezierPath.CodingKeys>

if let keyedContainer = try? decoder.container(keyedBy: BezierPath.CodingKeys.self) {
Expand Down Expand Up @@ -351,6 +351,26 @@ extension BezierPath: Codable {
elements = decodedElements
}

// MARK: Public

public func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: BezierPath.CodingKeys.self)
try container.encode(closed, forKey: .closed)

var vertexContainer = container.nestedUnkeyedContainer(forKey: .vertices)
var inPointsContainer = container.nestedUnkeyedContainer(forKey: .inPoints)
var outPointsContainer = container.nestedUnkeyedContainer(forKey: .outPoints)

/// If closed path, ignore the final element.
let finalIndex = closed ? elements.endIndex - 1 : elements.endIndex
for i in 0..<finalIndex {
let element = elements[i]
try vertexContainer.encode(element.vertex.point)
try inPointsContainer.encode(element.vertex.inTangentRelative)
try outPointsContainer.encode(element.vertex.outTangentRelative)
}
}

// MARK: Internal

/// The BezierPath container is encoded and decoded from the JSON format
Expand All @@ -371,23 +391,6 @@ extension BezierPath: Codable {
case vertices = "v"
}

func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: BezierPath.CodingKeys.self)
try container.encode(closed, forKey: .closed)

var vertexContainer = container.nestedUnkeyedContainer(forKey: .vertices)
var inPointsContainer = container.nestedUnkeyedContainer(forKey: .inPoints)
var outPointsContainer = container.nestedUnkeyedContainer(forKey: .outPoints)

/// If closed path, ignore the final element.
let finalIndex = closed ? elements.endIndex - 1 : elements.endIndex
for i in 0..<finalIndex {
let element = elements[i]
try vertexContainer.encode(element.vertex.point)
try inPointsContainer.encode(element.vertex.inTangentRelative)
try outPointsContainer.encode(element.vertex.outTangentRelative)
}
}
}

// MARK: AnyInitializable
Expand Down
47 changes: 47 additions & 0 deletions Sources/Private/Utility/Primitives/CGPathExtension.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
//
// CGPathExtension.swift
// Lottie
//
// Created by Yuval Kalugny on 13/04/2024.
//

import CoreGraphics

extension CGPath {

var bezierPath: BezierPath {
var path = BezierPath()

applyWithBlock { element in
switch element.pointee.type {
case .moveToPoint:
let point = element.pointee.points[0]
path.moveToStartPoint(.init(.zero, point, .zero))
case .addLineToPoint:
let point = element.pointee.points[0]
path.addLine(toPoint: point)
case .addQuadCurveToPoint:
let controlPoint = element.pointee.points[0]
let endPoint = element.pointee.points[1]
let controlPoint1 = CGPoint(
x: (2.0 / 3.0) * (controlPoint.x - element.pointee.points[-1].x) + element.pointee.points[-1].x,
y: (2.0 / 3.0) * (controlPoint.y - element.pointee.points[-1].y) + element.pointee.points[-1].y)
let controlPoint2 = CGPoint(
x: (2.0 / 3.0) * (controlPoint.x - endPoint.x) + endPoint.x,
y: (2.0 / 3.0) * (controlPoint.y - endPoint.y) + endPoint.y)
path.addCurve(toPoint: endPoint, outTangent: controlPoint1, inTangent: controlPoint2)
case .addCurveToPoint:
let controlPoint1 = element.pointee.points[0]
let controlPoint2 = element.pointee.points[1]
let endPoint = element.pointee.points[2]
path.addCurve(toPoint: endPoint, outTangent: controlPoint1, inTangent: controlPoint2)
case .closeSubpath:
path.close()
@unknown default:
break
}
}

return path
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//
// PathValueProvider.swift
// lottie-swift
//
// Created by Yuval Kalugny on 13/04/2024
//

import CoreGraphics
import Foundation

// MARK: - PathValueProvider

/// A `ValueProvider` that returns a CGPath Value
public final class PathValueProvider: ValueProvider {

// MARK: Lifecycle

/// Initializes with a block provider
public init(block: @escaping PathValueBlock) {
self.block = block
path = .init(rect: .zero, transform: nil)
identity = UUID()
}

/// Initializes with a single path.
public init(_ path: CGPath) {
self.path = path
block = nil
hasUpdate = true
identity = path.hashValue
}

// MARK: Public

/// Returns a CGPath for a CGFloat(Frame Time)
public typealias PathValueBlock = (CGFloat) -> CGPath

public var path: CGPath {
didSet {
hasUpdate = true
}
}

// MARK: ValueProvider Protocol

public var valueType: Any.Type {
BezierPath.self
}

public var storage: ValueProviderStorage<BezierPath> {
if let block {
return .closure { frame in
self.hasUpdate = false
return block(frame).bezierPath
}
} else {
hasUpdate = false
return .singleValue(path.bezierPath)
}
}

public func hasUpdate(frame _: CGFloat) -> Bool {
if block != nil {
return true
}
return hasUpdate
}

// MARK: Private

private var hasUpdate = true

private var block: PathValueBlock?
private let identity: AnyHashable
}

// MARK: Equatable

extension PathValueProvider: Equatable {
public static func ==(_ lhs: PathValueProvider, _ rhs: PathValueProvider) -> Bool {
lhs.identity == rhs.identity
}
}