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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Added: PathValueProvider #2376

wants to merge 11 commits into from

Conversation

kalugny
Copy link

@kalugny kalugny commented Apr 14, 2024

Hi,
Thank you for the great work on Lottie.

This PR adds the ability to replace a Path in the lottie, similarly to PointValueProvider.

In order to accomplish this I had to make BezierPath public.

Thanks,
Yuval

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants