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

Support timing function for animation. Fix #12 #13 #478

Merged
merged 9 commits into from May 17, 2017

Conversation

Projects
None yet
5 participants
@phimage
Member

phimage commented May 11, 2017

Re pull #456
Timing function enum added TiminFunctionType

  • Add it to Animatable objects and AnimationConfiguration
  • Use this enum when possible instead of CAMediaTimingFunction

TODO

  • add more type in enum (see extension CAMediaTimingFunction)
  • add more type in enum see CAMediaTimingFunction( contructors in code
  • add custom type with control points in enum ?
  • manage spring animation or not, animation default value
  • Add an example
  • Update CHANGELOG
  • Documentation

@phimage phimage requested review from JakeLin and tbaranes May 11, 2017

@IBAnimatableBot

This comment has been minimized.

Show comment
Hide comment
@IBAnimatableBot

IBAnimatableBot May 11, 2017

2 Warnings
⚠️ Big PR
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 Danger

IBAnimatableBot commented May 11, 2017

2 Warnings
⚠️ Big PR
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.

Generated by 🚫 Danger

@phimage phimage changed the title from Support timing function for animation. Fix #12 #13 to [WIP] Support timing function for animation. Fix #12 #13 May 11, 2017

phimage added some commits May 11, 2017

Fix moteBy and moveTo animation
With TimingFunction position is animated using centre point

@phimage phimage changed the title from [WIP] Support timing function for animation. Fix #12 #13 to Support timing function for animation. Fix #12 #13 May 11, 2017

phimage added some commits May 11, 2017

@phimage phimage referenced this pull request May 15, 2017

Merged

Add ellipse mask type #481

@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes May 15, 2017

Member

@phimage Sorry it takes me time to review it, it's quite big and a lot to digest!
I have already a first comment: can you update the docs? I think you will the one to update is Documentation/APIs.md

Member

tbaranes commented May 15, 2017

@phimage Sorry it takes me time to review it, it's quite big and a lot to digest!
I have already a first comment: can you update the docs? I think you will the one to update is Documentation/APIs.md

@phimage

This comment has been minimized.

Show comment
Hide comment
@phimage

phimage May 15, 2017

Member

No problem!

API doc has not been reverted https://github.com/IBAnimatable/IBAnimatable/blob/master/Documentation/APIs.md
I add timingFunction in "Animatable protocol" section.
If you think I can add something elsewhere, I will do it :)

I see x, y, repeat.. attributes in the same section, maybe old parameters, now directly in animationType

Member

phimage commented May 15, 2017

No problem!

API doc has not been reverted https://github.com/IBAnimatable/IBAnimatable/blob/master/Documentation/APIs.md
I add timingFunction in "Animatable protocol" section.
If you think I can add something elsewhere, I will do it :)

I see x, y, repeat.. attributes in the same section, maybe old parameters, now directly in animationType

@tbaranes

That's a HUGE work, thanks for doing this, it's an awesome feature.
I let a few comments, mainly typos / minor stuff, it looks pretty good to me 👍

Can you add the docs update once more? I think that's enough.

Show outdated Hide outdated IBAnimatable/TimingFunctionType.swift
Show outdated Hide outdated IBAnimatable/TimingFunctionType.swift
Show outdated Hide outdated IBAnimatable/TimingFunctionType.swift
extension TimingFunctionType {
public var caType: CAMediaTimingFunction {
switch self {

This comment has been minimized.

@tbaranes

tbaranes May 15, 2017

Member

Same as above, if you can add the comments to separate the main groups

@tbaranes

tbaranes May 15, 2017

Member

Same as above, if you can add the comments to separate the main groups

This comment has been minimized.

@tbaranes

tbaranes May 16, 2017

Member

Github didn't see as fixed, but it's fixed!

@tbaranes

tbaranes May 16, 2017

Member

Github didn't see as fixed, but it's fixed!

Show outdated Hide outdated IBAnimatable/TimingFunctionType.swift
}
internal extension CAAnimation {

This comment has been minimized.

@tbaranes

tbaranes May 15, 2017

Member

Is there a reason to explicitly make it internal?

@tbaranes

tbaranes May 15, 2017

Member

Is there a reason to explicitly make it internal?

This comment has been minimized.

@phimage

phimage May 15, 2017

Member

It's a way for me to mark dangerous extension
I want to use it internally but never let it public, or someone else does it.

With internal explicitly marked, people ask themselves why and edit carefully

@phimage

phimage May 15, 2017

Member

It's a way for me to mark dangerous extension
I want to use it internally but never let it public, or someone else does it.

With internal explicitly marked, people ask themselves why and edit carefully

Show outdated Hide outdated IBAnimatable/TimingFunctionType.swift
Show outdated Hide outdated IBAnimatableApp/AnimationsTimingFunctionViewController.swift
Show outdated Hide outdated IBAnimatableApp/AnimationsViewController.swift
@tbaranes

This comment has been minimized.

Show comment
Hide comment
@tbaranes

tbaranes May 15, 2017

Member

Just tested the example, great one. A few feedbacks:

  • At first, I though that the timing function will update the animation. Since it doesn't, you should either do it, or refactoring makes a subsections to "Animations" to have two possibilities: "View Animations", and "Timing functions"
  • The image in the navigation bar isn't explicit enough (I didn't get it at first), if you choose option 1 from the previous point, we should replace the image by a text, or move the button in a place even more visible / explicit
  • Since some timingFunctions are quite long, they are truncated in the picker. Can we add the selected name somewhere? Below the graph for example. I think we have this issue in others picker as well, time to take care of this by starting with this one? 😬
  • I know there's a few incoherence about this in the playground: should we have a "back" text and no title, or no "back" text and a title? By using "TimingFunctions", I would go for option 2 to make it more explicit, but it's up to you. Some day it would be great to uniformise that behaviour
Member

tbaranes commented May 15, 2017

Just tested the example, great one. A few feedbacks:

  • At first, I though that the timing function will update the animation. Since it doesn't, you should either do it, or refactoring makes a subsections to "Animations" to have two possibilities: "View Animations", and "Timing functions"
  • The image in the navigation bar isn't explicit enough (I didn't get it at first), if you choose option 1 from the previous point, we should replace the image by a text, or move the button in a place even more visible / explicit
  • Since some timingFunctions are quite long, they are truncated in the picker. Can we add the selected name somewhere? Below the graph for example. I think we have this issue in others picker as well, time to take care of this by starting with this one? 😬
  • I know there's a few incoherence about this in the playground: should we have a "back" text and no title, or no "back" text and a title? By using "TimingFunctions", I would go for option 2 to make it more explicit, but it's up to you. Some day it would be great to uniformise that behaviour
@phimage

This comment has been minimized.

Show comment
Hide comment
@phimage

phimage May 15, 2017

Member

The animation is updated in the previous view, TimingFunctionPickDelegate do it

But not all animation are compatible ie. Spring animation are not compatible
In the same way velocity, dumping are only for spring animation and update it will not affect a lot of animations
Maybe a documentation file with a list of animation with two columns: Spring animation and timing function animation(or cubic animation) will be necessary, to check option compatibility

I edit moveTo and moveBy to be compliant with the two modes.


Yes, the image is not explicit
I take an available image, a timer. I think about putting a timing function graph but failed to create it using my TimingFunctionView in navigation bar items.

Member

phimage commented May 15, 2017

The animation is updated in the previous view, TimingFunctionPickDelegate do it

But not all animation are compatible ie. Spring animation are not compatible
In the same way velocity, dumping are only for spring animation and update it will not affect a lot of animations
Maybe a documentation file with a list of animation with two columns: Spring animation and timing function animation(or cubic animation) will be necessary, to check option compatibility

I edit moveTo and moveBy to be compliant with the two modes.


Yes, the image is not explicit
I take an available image, a timer. I think about putting a timing function graph but failed to create it using my TimingFunctionView in navigation bar items.

In demo app timing function button is enabled is animation is configu…
…rable by timing function

Redesign back button and title for timing function picker controller
Typo, comment on TimingFunctiontType
@phimage

This comment has been minimized.

Show comment
Hide comment
@phimage

phimage May 15, 2017

Member

So new change added

  • back button and navigation title for the timing function picker
  • icon changed to a Gear, and the button is enabled only if the selected animation is configurable using the timing function

Added on AnimationType two compute boolean variables to know is the animation use velocity and damping or use timing function
This method could help for future animation documentations

Member

phimage commented May 15, 2017

So new change added

  • back button and navigation title for the timing function picker
  • icon changed to a Gear, and the button is enabled only if the selected animation is configurable using the timing function

Added on AnimationType two compute boolean variables to know is the animation use velocity and damping or use timing function
This method could help for future animation documentations

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 May 15, 2017

Member

@phimage this is great work! I may be able to leverage this approach to do more cleanup or adopt a similar strategy. love it!

Member

SD10 commented May 15, 2017

@phimage this is great work! I may be able to leverage this approach to do more cleanup or adopt a similar strategy. love it!

@tbaranes

tbaranes requested changes May 16, 2017 edited

@phimage Awesome! I agree with you that a docs about timing function compatibility would be great, but we can merged that one first. So, just the missing entry in the docs and that's ready to go 👍

@phimage

This comment has been minimized.

Show comment
Hide comment
@phimage

phimage May 16, 2017

Member

For the doc, as I say, I do already something in main branch, see Animatable protocol
https://github.com/IBAnimatable/IBAnimatable/blob/master/Documentation/APIs.md

Member

phimage commented May 16, 2017

For the doc, as I say, I do already something in main branch, see Animatable protocol
https://github.com/IBAnimatable/IBAnimatable/blob/master/Documentation/APIs.md

@tbaranes

My bad, I misunderstood 😶

@JakeLin

All good, great work 👍👍👍. Look forward to using it.

opacityAnimation.keyTimes = [0, 0.45, 1]
ring1ScaleAnimation.timingFunctions = [CAMediaTimingFunction(name: kCAMediaTimingFunctionLinear), timingFunction]
opacityAnimation.timingFunctionsType = [.linear, .easeOutExpo]

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

good to encapsulate to an extension for timing function.

@JakeLin

JakeLin May 17, 2017

Member

good to encapsulate to an extension for timing function.

}
public extension Animatable where Self: UIView {
public extension Animatable {

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

I hope Swift can support the syntax like public extension Animatable where Self: UIView or UIBarItem later.

@JakeLin

JakeLin May 17, 2017

Member

I hope Swift can support the syntax like public extension Animatable where Self: UIView or UIBarItem later.

path.move(to: position)
path.addLine(to: CGPoint(x: xToMove, y: yToMove))
animatePosition(path: path, configuration: configuration, completion: completion)

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

Cool, before iOS 10, only Core Animation can apply custom timing.

@JakeLin

JakeLin May 17, 2017

Member

Cool, before iOS 10, only Core Animation can apply custom timing.

}
/// This animation use timing function parameter.
public var isCubic: Bool {

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

Should it call something like doesSupportTimingFunction? I am not good at naming though 😂

@JakeLin

JakeLin May 17, 2017

Member

Should it call something like doesSupportTimingFunction? I am not good at naming though 😂

Show outdated Hide outdated IBAnimatable/AnimatableBarButtonItem.swift
@@ -47,8 +47,7 @@ public extension AnimatedTransitioning {
transition.subtype = subtype
}
transition.duration = self.transitionDuration(using: transitionContext)
// Use `EaseOutQubic` for system built-in transition animations. Thanks to @lexrus
transition.timingFunction = CAMediaTimingFunction(controlPoints: 0.215, 0.61, 0.355, 1)
transition.timingFunctionType = .easeOutCubic

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

Great 🎉

@JakeLin

JakeLin May 17, 2017

Member

Great 🎉

case custom((x: Float, y: Float), (x: Float, y: Float))
case spring(damping: Float)
// from http://easings.net/

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

👍

@JakeLin
@available(iOS 10.0, *)
extension TimingFunctionType {
var cubicTimingParameters: UICubicTimingParameters {

This comment has been minimized.

@JakeLin

JakeLin May 17, 2017

Member

Supper cool to support UIPropertyAnimator API.

@JakeLin

JakeLin May 17, 2017

Member

Supper cool to support UIPropertyAnimator API.

@JakeLin

This comment has been minimized.

Show comment
Hide comment
@JakeLin

JakeLin May 17, 2017

Member

@phimage thanks for such a great PR. I think PR is ready to go. Please have a look at this comment #478 (comment) for the _timingFunction. It should be internal like the other _ properties.

Member

JakeLin commented May 17, 2017

@phimage thanks for such a great PR. I think PR is ready to go. Please have a look at this comment #478 (comment) for the _timingFunction. It should be internal like the other _ properties.

@phimage phimage merged commit 91a8338 into master May 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@phimage phimage deleted the revert-473-revert-456-feature/timingfunction branch May 17, 2017

@phimage phimage added this to the 5.0 milestone May 17, 2017

@JakeLin

This comment has been minimized.

Show comment
Hide comment
@JakeLin

JakeLin May 18, 2017

Member

@phimage Well done 🎉🎉🎉🎉🎉🎉

Member

JakeLin commented May 18, 2017

@phimage Well done 🎉🎉🎉🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment