Page MenuHomePhabricator

Animation API
Open, Incoming QueuePublic

Description

  • efl_canvas_animation:
    • duration is the same as Efl.Playable.length ?
    • interpolator should this object be owned, or just a enum telling the type of the interpolation
  • efl_canvas_animation_rotate: (D10350)
    • rotate should take a Vector instead of cx, cy.
    • rotate_absolute should take a Position2D
  • efl_canvas_animation_scale
    • scale should take 3 Vectors instead of *x, *y
    • scale_absolute should take 3 Vectors instead of *x, *y
  • efl_canvas_animation_translate
    • translate should take 2 Position2D
    • translate should take 2 Position2D
  • efl_canvas_animation_player:
    • animation should take ownership ?
    • auto_del should this rather be solved by the fact that playing would automatically take a ref, if you want to get rid of the object, stop it. When the animation is done, this reference is killed.
    • Events should be moved to Efl.Player ?
bu5hm4n created this task.Sep 30 2019, 5:28 AM

@Jaehyun_Cho what do you think about those things ?

@bu5hm4n Thank you for the description :) I replied as follows.

  • efl_canvas_animation:
    • I think duration is the same as Efl.Playable.length.
    • Since interpolator has its own factors, I think it should be an object. Can we make interpolator not ownable? Because I think there are some demands that app wants to implement animations with same interpolator. (e.g. Cubic Bezier with the same factors. If Interpolator is ownable, then app should create and set interpolator again and again.)
  • efl_canvas_animation_rotate:
    • Current rotate's cx and cy is double value relative to the pivot object. (e.g. 0.0 ~ 1.0, center is 0.5) So I am not sure if Vector is the correct type for this relative values although value type is the same.
    • yes, rotate_absolute should use Position2D
  • efl_canvas_animation_scale
    • Current efl_canvas_animations are designed for 2D animation. So I think 3 Vectors are over the current 2D animation. Moreover, I am not sure if Vector is the right type for this case.
  • efl_canvas_animation_translate
    • yes, translate should use 2 Position2D
  • efl_canvas_animation_player
    • I think animation should NOT take ownership. Because same animations are used for different target objects (i.e. different players).
    • About automatic ref, I think it's good idea. Do you mean that auto_del_set(false) causes unref the automatic ref?
    • yes, events should move to Efl.Player.

@bu5hm4n Thank you for the description :) I replied as follows.

  • efl_canvas_animation:
    • I think duration is the same as Efl.Playable.length.
    • Since interpolator has its own factors, I think it should be an object. Can we make interpolator not ownable? Because I think there are some demands that app wants to implement animations with same interpolator. (e.g. Cubic Bezier with the same factors. If Interpolator is ownable, then app should create and set interpolator again and again.)

Okay.

  • efl_canvas_animation_rotate:
    • Current rotate's cx and cy is double value relative to the pivot object. (e.g. 0.0 ~ 1.0, center is 0.5) So I am not sure if Vector is the correct type for this relative values although value type is the same.

I think Vector2D sounds like the type we want to have for that. Maybe its just me, but i am prefering to have a simple container over two values for the same semantical thing.

  • yes, rotate_absolute should use Position2D

Okay.

  • efl_canvas_animation_scale
    • Current efl_canvas_animations are designed for 2D animation. So I think 3 Vectors are over the current 2D animation. Moreover, I am not sure if Vector is the right type for this case.

I think Vector2D is the correct thing here, my idea would be that the parameters would be in the end (from : Vector2D, to : Vector2D, pivot : Efl.Canvas.Object, c : Vector2D)

  • efl_canvas_animation_translate
    • yes, translate should use 2 Position2D

Okay

  • efl_canvas_animation_player
    • I think animation should NOT take ownership. Because same animations are used for different target objects (i.e. different players).
    • About automatic ref, I think it's good idea. Do you mean that auto_del_set(false) causes unref the automatic ref?

Mhm, after reading it a second time, i think i am a bit confused here. What is autodel deleting, the animated object, or the player ?

  • yes, events should move to Efl.Player.

Okay.

I am actaully a little bit concerend about the amount of objects required something easy as a animation.... esp. with the autodel thing, so you create 2 objects just to delete them. If we publish that like this. Then we have no good way to refactor that later on ... :(

  • efl_canvas_animation_rotate:
    • Current rotate's cx and cy is double value relative to the pivot object. (e.g. 0.0 ~ 1.0, center is 0.5) So I am not sure if Vector is the correct type for this relative values although value type is the same.

I think Vector2D sounds like the type we want to have for that. Maybe its just me, but i am prefering to have a simple container over two values for the same semantical thing.

Like you said, it may be related to preference. The reason why I am not sure about this is because these parameters have similar values of the parameters of hint_align of efl_gfx_hint.eo.
BTW, I don't have any preference about this case.

I think Vector2D is the correct thing here, my idea would be that the parameters would be in the end (from : Vector2D, to : Vector2D, pivot : Efl.Canvas.Object, c : Vector2D)

Oh, I misunderstood. Now I see. So it is the same as the preference of using Vector2D type.

Mhm, after reading it a second time, i think i am a bit confused here. What is autodel deleting, the animated object, or the player ?

If auto_del is true, then player is deleted when animation ends.

I am actaully a little bit concerend about the amount of objects required something easy as a animation.... esp. with the autodel thing, so you create 2 objects just to delete them. If we publish that like this. Then we have no good way to refactor that later on ... :(

I totally agree with you that we need to reduce the number of objects which are created for easy animation.
I'm sorry I don't understand what you mean about autodel in the later comment. Could you explain please?

bu5hm4n updated the task description. (Show Details)Oct 10 2019, 6:48 AM

@Jaehyun_Cho Can you review D10350 ?

For Now i would focus on the animation classes. When this is done we can discuss the player object.

So, all of the things raised about Efl.Canvas.Animation.* APIs are solved as far as i can see in the attached revisions.

Which leaves Efl.Canvas.Animation.Player.

RFC Replacement of Efl.Canvas.Animation.Player

I think that the player object in there is a little bit unneccessary.
For me it seems that we could have a animation player object directly attached to these Efl.Canvas.Object that want to play a animation (for example as a mixin). Which means, a efl.canvas.animation.player would get rid of the target and auto_del property.
The playable implementation of the player is just a direct passthrough to the animation object. So this is also not neccessary.
The player itself here is only usefull for playing however, why would you want to pause a animation? That does not seem like something you want to do for me, as it will leave the object in a weird state, the only reason why you want to do that, is directly deleting it entirely, which then also does not really require a call to playing. The only reason where you want to abort a animation is the moment where its repeated infinitly, in this case we could handle that by simply unsetting the animation.

A new mixin

What i would like to do here is create a new mixin Efl.Canvas.Object with a single method "play_animation" which takes a Efl.Canvas.Animation. In the calling of this method, the animation will automatically start to play. If you want to stop it again, call the method again with $null (or a seperate unset method). The method call returns a promise that gets fullfilled when the animation is finished.

This mixin can also be the place for adding simple "casual" operations, like "rotate(90)" or "fade_out()", which might makes applying animations a lot easier for a normal developer.

@zmike @Jaehyun_Cho @woohyun @stefan_schmidt @segfaultxavi @cedric Does this sound fine ?

zmike added a comment.Oct 30 2019, 5:51 AM

I like the idea of the mixin, though I think we probably want to go with something more like animation as a property that can be read and changed to trigger the animations.

This all sounds very sensible. How would you call the mixin? Something like Efl.Canvas.Playable, Animable or Animatable?
I also prefer the animation property instead of the play_animation method. It's exactly the same, but it allows the user to read back the current value.
Also, why return a future instead of emitting an event? We now have two equivalent mechanisms (futures and events) and we risk creating a lot of confusion if we don't use them in a consistent manner.

Efl.Canvas.Animatable Looks good to me, however, it sounds very made up for this. But it would be totally acceptable for me.
Would everyone be okay with that:

(P) animation //the currently played animation. Re-setting a animation will stop the old one. The value here is @owned (but the parent is never touched).
(E) animation,end //after this is called animation will automatically be unset.

Does that sounds sensible ?

Future are useful and best when it is an one off trigger than can be used to chain a series of other action. In this case, I think animation,end is a valid case of using a future as you might want to trigger another animation or use it as a timeout to do something else. Event are best for something that is repeated with the same action expected as a consequences of that event every time.

Basically I like the idea (P) animation :)

I agree with (P) animation as a simplest way of support animation. But I think we need to provide a way to "pause" animation as well.
e.g. If an animation starts by user's action and user cancelled their action before the animation is finished, then animation may need to be paused and we may need to show the reverse animation from the pause moment.

Can this new mixin Efl.Canvas.Animatable also implement the Efl.Player interface? In this way, the playing, paused, playback_speed etc. properties will all be accessible in the canvas object itself.

bu5hm4n added a comment.EditedNov 5 2019, 1:09 PM

I am a little bit worried to have this mixin implement Efl.Player that would mean that *every* Efl.Canvas.Object has a playing paused and playback_speed method, which might be very confusing. Additionally, this might collide with other implementations in the Video Canvas Object. So i do not think this is an option tbh.

How about this: We make the animation Property read only, and have three additional methods:

  • animation_start which takes a simple animation.
  • animation_start2 (Working title) which takes a animation, the position to start(, and maybe the playback speed). Where the position can be from -1.0 to 1.0 a negative position will mean the animation will be played backwards and it will automatically play towards to position -1.0.
  • animation_stop which will remove the animation from being played, which will return the position it is currently in.

How does that sound @Jaehyun_Cho @segfaultxavi ? (The semantic with positon < 0 or > 0 might be a bit confusing, maybe a forth method is the way to go, not to sure tbh.)

I am worried that adding additional methods to the new mixin would make the new mixin complicated like Efl.Canvas.Animation_Player unlike the initial purpose.

How about keeping the new mixin simple as @bu5hm4n proposed initially and having one class that takes care of complicated animation control (Efl.Player's methods)?
I think there are 2 ways to do it as follows.

  1. Preserve Efl.Canvas.Animation_Player and Introduce the new mixin as well.
    • Efl.Canvas.Animation_Player is preserved and used for the complicated animation control. (Efl.Player's methods)
    • The new mixin only sets or starts animation as @bu5hm4n proposed.

OR

  1. Efl.Canvas.Animation implements Efl.Player and Remove Efl.Canvas.Animation_Player and Introduce the new mixin.
    • Efl.Canvas.Animation implements Efl.Player and it works like Efl.Canvas.Animation_Player.
    • It would make Efl.Canvas.Animation complicated but we can remove Efl.Canvas.Animation_Player.
    • The new mixin only sets or starts animation as @bu5hm4n proposed.

What do you think? @bu5hm4n @segfaultxavi

After a lot of discussing on IRC with @bu5hm4n I have a better knowledge of the requirements for this.

We all agree that the Efl.Canvas.Animation_Player object is weird and should disappear.

The problem with implementing Efl.Player in Efl.Canvas.Object is that is it has much more options than we support in the current implementation (like changing the playback_speed).
We currently only support setting the playback speed and direction before starting playback, therefore, it makes sense that these values are actually parameters to a start method, as @bu5hm4n was proposing above.

Working on top of his proposal, I would merge start and start2, for simplicity. Unneeded parameters can be just NULL, or -1 or whatever. I would not complicate things with negative positions. I would not return the current position from stop, since it is a bit weird.

So, in summary:

  • Efl_Animation *animation: Readonly property.
  • double animation_position: Readonly property.
  • void animation_start(Efl.Animation *animation, double start_pos, double end_pos, double speed): Sets the animation and starts it. When the animation finishes it can be automatically unset, or an event can be emitted. Or the method can return a Future.
  • void animation_stop(): Stops the animation and unsets it.

With this API, multiple canvas objects can have the same animation, animations can be stopped and "rolled back"... would this work for all use cases, @Jaehyun_Cho ?

@Hermet @jsuya

Please could you give your thoughts about this discussion? (i.e. Introducing new mixin with animation_start/animation_stop for Efl.Canvas.Object)

Because @Hermet and @jsuya have been contributed vector graphics and Efl.Ui.Animation_View class and this discussion might be related to vector graphics and Efl.Ui.Animation_View class as well.

I implemented the RFC from Xavi above here: D10615 (So we have something to talk about).

@segfaultxavi

I totally understand the reason of your idea. I think it implies the followings.

  1. Set one animation to multiple canvas object.
    • To support this, animation should not support animation controls such as "Start" and "Stop".
  2. Support animation simpler.
    • To support this, Efl.Canvas.Animation_Player is removed because it implements way too much methods. And new mixin is introduced with small number of methods.

However, I think that the new approach may not fully support "pause" or "rolled back" as well.

Based on the new approach, animation_start() -> animation_stop() -> animation_start() would be called to support "pause" or "rolled back".
But when animation_stop() is called, the animation is unset and the map effect would not be applied to the canvas object.
Therefore, the canvas object can be displayed without applying the map effect during the moment between animation_stop() and animation_start().

Moreover, to my mind, the method animation_start() does not look simple as it was initially introduced.. Users may not like the parameters as a simple animation start function..

I thought that providing 2 ways for animation (1. simple method by the new mixin 2. complicated methods by other class) but I don't have a new idea now. I will think about this more..

  1. If pause like this is a *must* have, then we can introduce it to this mixin and support this case. No problem with that. We just have 0 usages of that right now so I saw no purpose in carrying it over (further more, the pause feature in the player object does not work at all like pausing, it will restart the animation from the begining when you unpause it, which is why I said this is totally equivalent to start stop. The problem you are describing is also happening right now in the player class, pausing is resetting the mapping.).
  1. What do you mean with start is not simple right now, you just pass a animation, the speed and the start position, speed is 1.0 by default, start is 0.0. If these two doubles are too complex, then we can introduce a second method that only takes a animation. However, Xavi said above that we should merge the complex and the simple one, and there have been 0 voices against it.
  1. The usecase with roll back does not even work with the player object we have right now, as speed is never taken into account in the animation itself. With the mixin I have linked above, it is easily possible. by that:
double position = efl_canvas_object_animation_posistion_get(obj);
Eo *animation = efl_canvas_object_animation_object_get(obj);
efl_canvas_object_animation_start(animation, -1.0, position);

Do you think this is convenient enough?

  1. If pause like this is a *must* have, then we can introduce it to this mixin and support this case. No problem with that. We just have 0 usages of that right now so I saw no purpose in carrying it over (further more, the pause feature in the player object does not work at all like pausing, it will restart the animation from the begining when you unpause it, which is why I said this is totally equivalent to start stop. The problem you are describing is also happening right now in the player class, pausing is resetting the mapping.).

I think "pause" and "resume" are basic functions for animation. It is hard to find framework that support animation without "pause". So I think that we need to support it.
Even though "pause" is not perfectly supported now, I think it would not be a reason why we do not need to consider "pause".
BTW, for now, it seems that "pause" works with Efl.Canvas.Animation_Player without resetting mapping. (elementary_test -> Efl.Animation.Pause ->Pause Animation/Resume Animation)
If we add "pause" to the new mixin, then I am worried if it would look fine by design aspect that supporting animation control functions to canvas object (by implementing new mixin).

  1. What do you mean with start is not simple right now, you just pass a animation, the speed and the start position, speed is 1.0 by default, start is 0.0. If these two doubles are too complex, then we can introduce a second method that only takes a animation. However, Xavi said above that we should merge the complex and the simple one, and there have been 0 voices against it.

I thought that you proposed the new mixin with only one method animation_start(animation) or only one property animation to accomplish the beauty of simplicity. I really like that idea.
In the new proposal, animation_start() has 2 more double values. For someone, it is not big deal but for others, it would be better use animation_start(animation) like the initial version.
Personally, I have heard some complaints many times from app developers like this : "Why this API has these specific parameters?", "Is there no simple version of this API?", etc..

  1. The usecase with roll back does not even work with the player object we have right now, as speed is never taken into account in the animation itself. With the mixin I have linked above, it is easily possible. by that: ` double position = efl_canvas_object_animation_posistion_get(obj); Eo *animation = efl_canvas_object_animation_object_get(obj); efl_canvas_object_animation_start(animation, -1.0, position); `

    Do you think this is convenient enough?

About "speed", I am also not sure if it is necessary.
I couldn't checked it yet but supporting "roll back" that easily is great. :)

The revision i have linked above now contains pause. You can check the example that got added in this revision and press p for pause (press twice for toggle) or press r for reverse, which will reverse the animation, just as you wished to do that. IMO the code in there look super easy and nice. The speed parameter is essential for the reversing, if i remove it again, the reversing code will be way harder. Would something like the revision be acceptable for you ? If yes, then i would continue removing the player object, as this usage as we have it right now is way easier.