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

Refactor animations #35

Closed
bolismauro opened this issue Nov 4, 2016 · 9 comments
Closed

Refactor animations #35

bolismauro opened this issue Nov 4, 2016 · 9 comments
Assignees

Comments

@bolismauro
Copy link
Contributor

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

More details coming soon

@bolismauro bolismauro self-assigned this Nov 4, 2016
@bolismauro
Copy link
Contributor Author

bolismauro commented Nov 4, 2016

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:

  • Either props or state is changed
  • childrenAnimation returns the animation to use
  • The new children are calculated
  • The UI changes, calculated by Katana, are performed in the returned animation block

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:

  • chain animations
  • set a new internal state, trigger the update and perform some operations when the update is completed

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:

  • The view exists both in the state A and in the state B. This case is already managed by the current animation system
  • The view doesn't exist in the state A but it exists in the state B. In this case we need to create initial props configuration (let's call it entry props) for the view, that will be used as starting point for the animation to the state B
  • The view exists in the state A but it doesn't exist in the state B. In this case we need to create a final props configuration (let's call it leave props) that will be used as final point of the animation 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:

  • A is the initial state
  • A' is the state where we render everything like in A, but it will have all the entry elements. We render them using the entry props
  • A'' is the state where we render everything like in B, but we well keep all the leave elements. We render them using the leave props

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:

  • the animation to perform for each children
  • the entry props for each children
  • the leave props for each children

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:

  • Animation is basically the same enum we have right now (it contains all the possible UIKit animations)
  • we use the children keys to reference

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:

  • For entry props, we calculate them as a transformation of the final props (props in state B)
  • For leave props, we calculate them as a transformation of the initial props (props in state A)

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:

  • applies a linear animation, 0.3 seconds, to every element
  • fade in all the entry elements
  • fade out all the leave elements
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@lucaquerella
Copy link
Contributor

lucaquerella commented Nov 4, 2016

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
Copy link
Contributor Author

bolismauro commented Nov 4, 2016

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

  • 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
  • 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)
  • I have the feeling that this solution doesn't really promote reusable animations. You can of course create a function (or equivalent) that takes as input all the parameters and contains the reusable logic, but my feeling is that it is way less elegant that having a protocol that manages the animation. It is also easier to swap between animations based on an external setting, or create reusable animations with a fixed set of parameters that can be define in another place.

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
Copy link
Contributor

lucaquerella commented Nov 4, 2016

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
Copy link
Contributor Author

bolismauro commented Nov 5, 2016

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:

  • Classes: we need to use them instead of structs. AFAIK they are heavier to use (@smaramba ?) and this process is done in a critical moment of the app (UI update)
  • What happens if you forgot to pass the information for a child? We can use a default of course, but why don't go for a way where we can force the definition of things we need? In general this is the bigger issue of Plastic too. There is no way to ensure there are all the information needed. Now with Plastic I've not found a way to fix this problem, since in that context you need to create relations between children, which means you should be able to access them all. Here we don't have this problem, and therefore I'd for for a solution where it is possible to enforce things we need
  • How do you implement parentAnimation? In general, how can you propagate the animation information down in the tree? We can't just pass down the container. Since the children are different, you need to create the container again and then find a way to copy the relevant information, it is just not really handy to implement. With NodeDescriptionAnimation you don't have this problem. Of course if you create very specific animations for a description, that doesn't work too. But for very generic animations, not tied to a description (like the one in the example), it works out of the box
  • How do you implement the no animation semantic? We need a very fast and easy way to check it, since it will be the 99% of the cases. We will need a fast way to detect and avoid to do useless operations. (This point is minor, I'm sure there is a way, but I want to push you to think about it)

@smaramba
Copy link
Contributor

smaramba commented Nov 6, 2016

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
Copy link
Contributor

lucaquerella commented Nov 6, 2016

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.

This was referenced Nov 7, 2016
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

No branches or pull requests

3 participants