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

Add support for mixed isHold / !isHold keyframes #1644

Merged
merged 9 commits into from
Jul 11, 2022

Conversation

calda
Copy link
Member

@calda calda commented Jul 11, 2022

This PR add support for mixed isHold / !isHold keyframes. Fixes #1632.

isHold corresponds to CAAnimationCalculationMode.discrete and !isHold corresponds to .linear. An individual CAKeyframeAnimation can only have a single CAAnimationCalculationMode, so previously the Core Animation engine only supported sets of keyframes that all used the same mode.

Now, we create a separate CAKeyframeAnimation for each segment of keyframes with the same calculationMode, and then combine them into a single CAAnimationGroup.

Supports Core Animation engine
Copy link
Member Author

Choose a reason for hiding this comment

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

🎉

{
guard !keyframes.isEmpty else { return nil }

// If there is exactly one keyframe value, we can improve performance
// by applying that value directly to the layer instead of creating
// a relatively expensive `CAKeyframeAnimation`.
if keyframes.count == 1 {
let keyframeValue = try keyframeValueMapping(keyframes[0].value)
Copy link
Member Author

Choose a reason for hiding this comment

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

I factored this out into a separate method

/// Creates an animation that applies a single keyframe to this layer property
/// - In many cases this animation can be omitted entirely, and the underlying
/// property can be set directly. In that case, no animation will be created.
private func singleKeyframeAnimation<ValueRepresentation>(
Copy link
Member Author

Choose a reason for hiding this comment

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


/// Creates a `CAAnimationGroup` that wraps a `CAKeyframeAnimation` for each
/// of the given `animationSegments`
private func animationGroup<KeyframeValue, ValueRepresentation>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is new:

@calda calda force-pushed the cal--mixed-discrete-keyframes branch from df94b37 to 930a1cd Compare July 11, 2022 20:11

/// We can't do `extension Array where Element == Keyframe` since we don't have a way
/// to specify the generic type of the keyframe itself. That would require a syntax
/// like `extension <T> Array where Element == Keyframe<T>`. Instead we can use
Copy link
Member Author

Choose a reason for hiding this comment

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

generic extensions when 🤪

Copy link
Member Author

Choose a reason for hiding this comment

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

remember in like Swift 3 or earlier when you had to do this any time you wanted to extend an array with a specific Element (since there wasn't an Element == Foo syntax yet)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh man, those were the days

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this doesn't work as the following? Or are you trying to avoid a function and have this be a computed property that doesn't support where clauses:

extension Array {
  func splitByCalculationModeSegment<Value>() 
    -> [[Keyframe<Value>]] 
    where
    Element == Keyframe<Value>
  {
    
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

ooooohhh good idea, let me try that

Copy link
Member Author

Choose a reason for hiding this comment

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

much better, thanks for the clever suggestion! 3f14b41

Copy link
Collaborator

@erichoracek erichoracek left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!


/// We can't do `extension Array where Element == Keyframe` since we don't have a way
/// to specify the generic type of the keyframe itself. That would require a syntax
/// like `extension <T> Array where Element == Keyframe<T>`. Instead we can use
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh man, those were the days


/// We can't do `extension Array where Element == Keyframe` since we don't have a way
/// to specify the generic type of the keyframe itself. That would require a syntax
/// like `extension <T> Array where Element == Keyframe<T>`. Instead we can use
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this doesn't work as the following? Or are you trying to avoid a function and have this be a computed property that doesn't support where clauses:

extension Array {
  func splitByCalculationModeSegment<Value>() 
    -> [[Keyframe<Value>]] 
    where
    Element == Keyframe<Value>
  {
    
  }
}

@calda calda enabled auto-merge (squash) July 11, 2022 22:07
@calda calda merged commit 48915a4 into master Jul 11, 2022
@calda calda deleted the cal--mixed-discrete-keyframes branch July 11, 2022 22:26
calda added a commit that referenced this pull request Nov 28, 2022
calda added a commit that referenced this pull request Dec 1, 2022
MoroziOS pushed a commit to MoroziOS/tmg-lottie-ios that referenced this pull request May 22, 2024
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.

Mixed isHold / !isHold keyframes are currently unsupported by the Core Animation engine.
2 participants