BendingSpoons / katana-swift

Swift Apps in a Swoosh! A modern framework for creating iOS apps, inspired by Redux.
http://bendingspoons.com
MIT License
2.25k stars 90 forks source link

Refactor animations #35

Closed bolismauro closed 7 years ago

bolismauro commented 7 years ago

The current animation system is not powerful enough. We should improve it.

More details coming soon

bolismauro commented 7 years ago

Animation Reworking

Current state: how it works

The current animation system is quite simple. It basically introduces the possibility of performing the updates that are triggered by a change of state/props in a UIKit animation block. This animation is returned by a specific method of NodeDescription that takes as inputs the current props, the current state, the next props and the next state. With next props/state we mean the props/state that will be used in the next update (so renderChildren and applyPropsToNativeView) cycle.

public static func childrenAnimation(...) -> Animation {
    // return your animation here
}

The workflow of an animated update is the following:

There is one more thing to notice about this system. A NodeDescription basically manages a native view and some children. The animation returned by the childrenAnimation is applied only to changes relative to the native views of the direct children. This animation won't be applied to the children of the children of the node description. Each NodeDescription should define what animations should be performed. The default implementation of childrenAnimation is parentAnimation which means that the returned animation is the same of the parent. In this way animations are propagated down in the hierarchy.

Current state: issues

The current system is just not powerful enough for our needs. Let's see the main problem with an example. Think about how a NavigationController can be implemented when it comes to the push animation.

We know that the pushed node description should appear from the right edge of the screen. One way we can implement this is to trigger a sequence of (internal) states to perform the animation. Let's say we received in the props the list of node descriptions of the stack. We noticed that we have the previous descriptions plus 1: this means we need to perform a push animation.

State 1: render the new node description outside the screen

We render "old" node descriptions (the ones that we already had) in the screen and the new one just outside the screen, with already the current size. This UI update is performed without animation

State 2: move the node description in the screen

We trigger a new render and move the new node description in the screen. We animate this change and we obtain a very basic push animation.

The current APIs don't allow to:

One of the two would allow to implement the push animation we need.

A proposal for a new animation system

By discussing this issue we thought that this was a symptom of a general limitation of the animation approach we have. Moreover what we manually need to do for the NavigationController can be abstract in the system and automated.

In particoular we defined that, for a specific element (let's use a view for simplicity) we should manage 3 kinds of animations when we transition from state A to state B:

So basically we need to automate animations for entry elements (that is, elements that are created during an animated transition) and leave elements (that is, elements that are destroyed during an animated transition).

In order to implement this system we need to add some (hidden) extra states. Let's say we need to go from A to B. Instead of going directly from A to B we do the following A —> A' —> A'' —> B

Here is wha each state is about:

The only animated transition is the one from A' to A''. The other two are not animated.

By using this approach we can achieve transition, enter and leave animations. In this way we can solve the problems highlighted in the first section and also implement a really poweful animation system

API

The idea is that childrenAnimation returns something that contains all the information that are needed to perform the animation. In particoular we need 3 types of information:

Of course when it comes to entry and leave props, we will need them only when elements are effectively added or removed during an animation.

My proposal is to have a protocol like this

protocol NodeDescriptionAnimation {
  associatedtype Key
  func animation(for key: Key) -> Animation
  func entryPropsTransformers(for key: Key) -> [PropsTransformer]
  func leavePropsTransformers(for key: Key) -> [PropsTransformer]
}

A few notes:

The PropsTransformer type needs a proper explanation. As we said before, we need to calculate the entry/leave props. The idea is to calculate them as a transformation of what we already know:

So, how PropsTransformer looks like? It is very simple

typealias PropsTransformer = (_ props: Any) -> Any

It is a closure that takes as input some props, it applies some changes and then they return the new props.

This is a very simple implementation of the animation that:

struct SimpleAnimation: NodeDescriptionAnimations {
 // we don't really need a specific type of key here, the struct is generic
  typealias Key = Any
  func animation(for key: Any) -> Animation {
    return .linear(0.3)
  }

  func entryPropsTransformers(for key: Any) -> [PropsTransformer] {
    return [toggleAlpha]
  }

  func leavePropsTransformers(for key: Any) -> [PropsTransformer] {
    return [toggleAlpha]
  }  
}

protocol Alphable {
  var alpha: CGFloat { get set }
}

let toggleAlpha: PropsTransformer = { props in
  var p = props as! Alphable
  p.alpha = p.alpha == 1 ? 0 : 1 // we change the alpha value
  return p
}

We can add very complex animations for very specific NodeDescription by providing very specific transformers, or we can just use standard animations.

The idea is to add in Katana built-in transformers and maybe NodeDescriptionAnimations.

We then change the NodeDescription childrenAnimation method in the following way

public static func childrenAnimation(...) -> NodeChildrenAnimation? {
    // return your animation here
}

if the method returns nil, the animations are disabled for the children. This method will have the same signature as the current animation system. It will receive current and next props, current and next state and parent animation.

For consistency reasons with the current systems we use in Bending Spoons, I'd change the default return value to nil and basically disable the propagation of the animations.

Open topics

The main issue with this approach is that it may be hard to create the transformers (we are not sure yet) since the starting point of the transformer changes from the entry to leave case.

We should find a way to improve it. An idea may be to extend the PropsTransformer concept, with a entry and leave case.

Notes

Note that the actual implementation may change because of implementation details. For instance we can try to improve the PropsTransformer when it comes to typings

bolismauro commented 7 years ago

An alternative propsal for PropsTransformer

protocol PropsTransformer {
 static func transform(enterProps: Any) -> Any
 static func transform(leaveProps: Any) -> Any
}

(naming should be definitively improved) The idea is to have two functions. In this way whoever implements the transformer knows what is the context in which the transformation is performed. The downside is that some transitions may not have sense in either leave or enter context. For instance fadeIn doesn't make sense in the leave scenario

bolismauro commented 7 years ago

LF suggested birth props and death props instead of enter / leave. I have no strong opinion

lucaquerella commented 7 years ago

I'm not convinced on the API proposed. I'd rather do something similar to what we have for plastic

  static func childrenAnimations(nodes: NodesContainer<__Keys>, props: __Props, state: EmptyState) {

    let title = nodes[.title]!

    title.animationType = .linear
    title.animationDuration = 0.3
    title.leavePropsTransformers = [tootleAlpha]

    //we can also add an helper
    nodes.allNodes.animationDuration = 4
    //or even
    nodes[[.title, .button]].animationType = .Linear

  }
bolismauro commented 7 years ago

I still prefer the initial solution for the the following reasons:

In general I understand your point about having a single approach (or similar approaches). But I feel that this is way to forced. I can argue that we should treat childrenDescriptions in the same way then. But we don't, because it doesn't fit well. This is the same for me.

Update

I've also updated the initial proposal and addressed your concern about the fact that the keys in the methods were String. I think it is possible to have an associated type for the Key type. It is also possible to create container with a generic type of key, like the following

protocol AKeyProtocol {
  var isButton: Bool { get }
}

// animates buttons linearly and other elements with a spring animation
struct LinearOrSpringAnimation: NodeDescriptionAnimation {
  func animation(for key: AKeyProtocol) -> Animation {
    if key.isButton {
      return .linear(0.3)

    } else {
      // random numbers for spring params
      return .sprint(0.3, 10, 20)
    }
  }

  // other code is not relevant for this example
}
lucaquerella commented 7 years ago

I don't see why we should pass a container to update. We don't have to provide any information upfront, so the container is useless

What's the difference with the layout? they reason why I propose to use the container is a) consistency with the rest of the framework; b) to void a lot of if/else if/else if/else, for each sub-node. Readability.

you are decreasing the safety of the animation by separating the type of the animation from the parameters. What happens if I define a spring animation and I don't define the dumping factor? (this can be easily fixed by just using an enum, like in the initial proposal)

Agreed.

I change my proposal to:

  static func childrenAnimations(nodes: NodesContainer<__Keys>, props: __Props, state: EmptyState) {

    let title = nodes[.title]!

    title.animation = .linear(.3)
    title.leavePropsTransformers = [tootleAlpha]

    //we can also add an helper
    nodes.allNodes.animation = .linear(.5)
    //or even
    nodes[[.title, .button]].animation = .linear(.4)

  }

I have the feeling that this solution doesn't really promote reusable animations.

The structure you propose is strongly tight to the node, so there is no difference in the two solutions.

Overall I'm still convinced that this is a better option, I find it simpler and more elegant. I also understand that when it comes to model API there is often a strong component of personal taste.

bolismauro commented 7 years ago

What's the difference with the layout? In the layout we must offer 1) upfront information (here we don't) and 2) a structure/methods to use (here we don't)

a) consistency with the rest of the framework;

As I said, this is consistent with Plastic, but not with childrenDescription for instance. To be honest I'd like to improve the Plastic interface.

b) to void a lot of if/else if/else if/else, for each sub-node. Readability.

I don't see how you can avoid if/else . Can you give an example?

Other reasons I've found after I've wrote the answer:

smaramba commented 7 years ago

as a general rule, we should limit the use of classes as much as we can, at least for critical operations. Structs that include all value types data are stored in stack, while classes and structs with reference semantics inside are allocated in heap and also need to opt in to the locking mechanism to deal with possible multiple access. Of course the real difference in performance in our case is still to measure.

lucaquerella commented 7 years ago

I've reflect on this and longly discuss it offline with @bolismauro. I also asked to so external input (LF).

Here my new proposal, hopefully it brings the best of the two words. I'm looking forward to know your opinion.

static func childrenAnimations(nodes: NodesContainer<__Keys>, props: __Props, state: EmptyState) {

    let title = nodes[.title]!
    let button = node[.button]!

    title.animation = Animation(type: .linear(.3), entryPropsTransformers: [...], leavePropsTransformers: [...])
    button.animation = Animation(type: .linear(.4), entryPropsTransformers: [...], leavePropsTransformers: [...])

 }

and once again, we can add an helper:

static func childrenAnimations(nodes: NodesContainer<__Keys>, props: __Props, state: EmptyState) {

    nodes.all.animation = Animation(type: .linear(.4), entryPropsTransformers: [...], leavePropsTransformers: [...])
 }

or even:

static func childrenAnimations(nodes: NodesContainer<__Keys>, props: __Props, state: EmptyState) {

    nodes[[.title,.button]].animation = Animation(type: .linear(.4), entryPropsTransformers: [...], leavePropsTransformers: [...])
 }

If you guys like the approach I have some ideas on how to address the technical concerns expressed on and offline about this solution.

More or less on topic, since we are discussing stract/class I do agree that we should leverage value types.