-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Added: PathValueProvider #2376
Changes from all commits
fc1bd8e
e7f5d0c
04df334
bbd54a1
eb7f18b
b5fe739
3dd5856
3249546
a733b71
2386bd0
42c666c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this to work around the failure of Instead, please re-record the snapshots. You can add 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, then it seems I misunderstood how the test works. |
||
if !(customizableProperty.name == [.path]) { | ||
context.recordHierarchyKeypath?(keypath.fullPath) | ||
} | ||
|
||
guard let anyValueProvider = valueProvider(for: keypath) else { | ||
return nil | ||
|
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 | ||
} | ||
} |
There was a problem hiding this comment.
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 atTests/Samples/Issues/pr_2376.json
.There was a problem hiding this comment.
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 aPath
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.