Page MenuHomePhabricator

introduce efl_canvas_object_animation
ClosedPublic

Authored by bu5hm4n on Nov 7 2019, 2:29 AM.

Details

Summary

this brings the animation to the canvas object. All the controls of the
animation now do now require a player object anymore, you can just use
the API that is directly on the Efl.Canvas.Object object.

wip animation player replacement

Depends on D10645

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n created this revision.Nov 7 2019, 2:29 AM
bu5hm4n requested review of this revision.Nov 7 2019, 2:29 AM
bu5hm4n updated this revision to Diff 26738.Nov 7 2019, 5:52 AM
bu5hm4n retitled this revision from wip animation player replacement to introduce efl_canvas_object_animation.
bu5hm4n edited the summary of this revision. (Show Details)

add a example

segfaultxavi added inline comments.Nov 7 2019, 9:39 AM
src/examples/evas/evas-animation.c
2 ↗(On Diff #26738)

You'll need to update this whole header :)

bu5hm4n updated this revision to Diff 26778.Nov 10 2019, 3:51 AM

fix animation. position is now returned *always* from 0.0 to 1.0

bu5hm4n updated this revision to Diff 26786.Nov 10 2019, 2:14 PM

more tests, and a simple fix

segfaultxavi added inline comments.Mon, Nov 11, 1:25 AM
src/lib/evas/canvas/efl_canvas_object_animation.eo
5

Why not just animation?

I fear animation_object can be confused with "the object being animated".

16

I think this should be animation_progress. Otherwise it can be confusing for position animations.

bu5hm4n planned changes to this revision.Mon, Nov 11, 1:54 AM
bu5hm4n added inline comments.
src/lib/evas/canvas/efl_canvas_object_animation.eo
5

animation is already used on a few different objects. Property names are colliding.

16

Yeah I can do that,

I am afraid I need some more time (1 or 2 days) to give you opinion about this animation refactorying work.

I am still collecting opinions and feedback but need more time to conclude them.

I will give you opinion as soon as possible.

segfaultxavi added inline comments.Mon, Nov 11, 2:58 AM
src/lib/evas/canvas/efl_canvas_object_animation.eo
5

Oh, damn. animation was short and nice to have here.

What are the other usages? I can only find Efl.Canvas.Layout and I think animation is not correctly used there...

bu5hm4n added inline comments.Mon, Nov 11, 3:45 AM
src/lib/evas/canvas/efl_canvas_object_animation.eo
5

Independence from is its correctly or not there, its stable, we cannot change it there.

I propose an unification of all animation flags in D10645 which should clarify things.
If you make this patch depend on D10645 (and rebase) you should be able to rename animation_object to just animation. How does that sound, @bu5hm4n @Jaehyun_Cho @zmike @cedric?

src/lib/evas/canvas/efl_canvas_object_animation.eo
5

Efl.Canvas.Layout is not stable :)

I share the collected opinions about refactoring of Canvas Animation as follows.

Let's define each approach as follows.

Strategy 1 : "Efl.Canvas.Object class provides animation control methods by using new mixin".
Strategy 2 : "Efl.Canvas.Animation class provides animation control methods".

  1. From class design aspect
    • If class extension is required to be considered, Strategy 2 is recommended.
      • If new animation control methods are added?
      • If animation is applied to a certain class which does not inherit from Efl.Canvas.Object? (e.g. that certain class also needs to add animation control methods)
      • If a new class similar to animation is added and applied to Efl.Canvas.Object? (e.g. a new class whose methods look similar to animation control methods)
    • Otherwise, Strategy 1 would be fine.
      • But Efl.Ui.Animation_View's methods should be re-designed to distinguish from Efl.Canvas.Object's animation control methods.
  1. From usability
    • If Strategy 1 is applied,
      • It is recommended to simplify @method animation_start because parameters speed and start_pos are not familiar to users.
      • To set a single Animation instance to multiple Efl.Canvas.Object's animation_start, Animation instance should not be owned by Efl.Canvas.Object instance.
    • If Strategy 2 is applied,
      • A single Animation instance should not be applied to multiple Efl.Canvas.Object instance. So method "Clone" is required to clone Animation instance with same properties.
      • If an Animation instance is called by animation_start of Efl.Canvas.Object while already it is called by other Efl.Canvas.Object's animation_start, then it might cause malfunction. (by changing target of Animation instance)
    • For both cases,
      • Animation class properties such as from_degree, to_degree may need to be set in constructor time. Otherwise, the change of animation properties of running animation might cause malfunction.
      • Animation instance is deleted by developer manually.
  1. From performance / memory consumption aspect
    • If Strategy 1 is applied,
      • Efl.Canvas.Object's class table size is increased and it affects to the class table sizes of all classes related to Efl.Canvas.Object class.
    • If Strategy 2 is applied,
      • Efl.Canvas.Animation is required for each animation effect control. So that number of eo size is more consumed.
      • Strategy 2 consumes relatively more memory than Strategy 1 but the memory consumption does not look big enough.
    • For both cases,
      • Performances are the same.

Thanks for this analysis of the situation. @bu5hm4n and I have been talking about it and we agree on almost everything, but we'd like to make some clarifications:

1. Class design
  • In Strategy 1, if new methods are needed, they can be added to the mixin, and all Canvas objects will automatically benefit from them.
  • Why would anybody want to apply a Canvas.Animation to an object which is NOT a Canvas.Object? Can this work?
  • Regarding Efl.Ui.Animation_View: This class has two different playbacks going on, the external Efl.Canvas.Object animation and the internal Lottie animation. Therefore, we need two sets of controls, which need to be clearly separated. All mixin methods are prefixed with animation_ whereas Animation_View methods have no prefix. I think this (plus documentation) should be enough to separate them.
  • Additionally, as you already said, Efl.Ui.Animation_View needs some refactoring. It should implement the Efl.Player interface (and the Efl.Player interface can be extended with any missing functionality, like Sectors). This should also separate the two playback controls. Also, the word View is reserved for MVVM classes.
  • Regarding the need to add another class with “methods that look similar to Animation”, I suppose you are talking about Animation_View. If another class similar to animation needs to be added, won’t it be more convenient to extend Canvas.Animation with some more methods instead?
2. Usability
  • When providing an Animation object to a Canvas object, the caller only passes a reference to animation_start. So if N different callers are calling animation_start(efl_ref(animation_obj)), then everything will be fine.
  • If the caller does not care about the Animation object anymore, it can pass its reference to animation_start(animation_obj) (without calling efl_ref). In this way when the animation finishes and removes its reference the Animation object will automatically be destroyed (because nobody else is holding references).
  • Transferring all the animation properties to constructor properties, for convenience, has always been the plan.
3. Performance / memory consumption
  • The performance of both Strategies is not equal. The method "clone", heavily used in Strategy 2, will create a lot of objects. Creation and destruction of these objects will cost time. For complex animations (multiple groups chained into each other) this is even going to be heavier. And due to the complexity of these animation objects (different types, tree-style hierarchy), this is not going to be easily optimizable.
  • However, when we are going for the mixin, we can optimize the class size allocation (which will also improve quite a lot of other things).
  • Moreover, the mixin only requires one additional pointer in the private data (All the data that is needed during the animation is stored only when animation is really happening). So we do not really have an overhead in private data.
  • The extra methods required for Canvas objects in Strategy 2 are stored in the class vtable, so having more objects does not increase the amount of memory.

Given all the above, we think that Strategy 1 (the mixin) is the most convenient. Plus, it's already implemented :)

bu5hm4n updated this revision to Diff 26844.Wed, Nov 13, 7:20 AM
bu5hm4n edited the summary of this revision. (Show Details)

renames

bu5hm4n updated this revision to Diff 26847.Wed, Nov 13, 8:31 AM

its efl canvas now.

so we now only need evas ecore and ecore-evas eina in pkg-config

bu5hm4n updated this revision to Diff 26850.Wed, Nov 13, 9:08 AM

asdfasdf

I think API-wise I have no more comments for this patch (pending the end of the discussion).

I think this is pretty fine like it is now, there are 2 more things that need to migrate away from player, but that does not block this revision here.

@segfaultxavi @bu5hm4n
Thank you very much for the comments! :)

==== 1. Class design

  • Why would anybody want to apply a Canvas.Animation to an object which is NOT a Canvas.Object? Can this work?

This comment makes sense because we named Efl.Canvas.Animation and we support animation to canvas object.

  • Regarding Efl.Ui.Animation_View: This class has two different playbacks going on, the external Efl.Canvas.Object animation and the internal Lottie animation. Therefore, we need two sets of controls, which need to be clearly separated. All mixin methods are prefixed with animation_ whereas Animation_View methods have no prefix. I think this (plus documentation) should be enough to separate them.
  • Additionally, as you already said, Efl.Ui.Animation_View needs some refactoring. It should implement the Efl.Player interface (and the Efl.Player interface can be extended with any missing functionality, like Sectors). This should also separate the two playback controls. Also, the word View is reserved for MVVM classes.

@jsuya @Hermet
Could you consider implementing Efl.Player interface to Efl.Ui.Animation_View? Or could you consider change the current APIs names of Efl.Ui.Animation_View?

  • Regarding the need to add another class with “methods that look similar to Animation”, I suppose you are talking about Animation_View. If another class similar to animation needs to be added, won’t it be more convenient to extend Canvas.Animation with some more methods instead?

The example is Animation class and Animator class supported by View class in Android.
Animation class and Animator class are similar but they have distinct features each other. So the original comment was about introducing a new class which supports new animation features and requires different interface (i.e. properties and methods) from Canvas.Animation.
BTW, since we don't know the future demands, there is no absolutely right solution and we can just expect~ :)

==== 2. Usability

  • When providing an Animation object to a Canvas object, the caller only passes a reference to animation_start. So if N different callers are calling animation_start(efl_ref(animation_obj)), then everything will be fine.
  • If the caller does not care about the Animation object anymore, it can pass its reference to animation_start(animation_obj) (without calling efl_ref). In this way when the animation finishes and removes its reference the Animation object will automatically be destroyed (because nobody else is holding references).

The original collected feedback was also based on C# usages because now we focus on C# bindings very much.
So from C# usage, 'animation_start(efl_ref(animation_obj))' is not supported and not common way. C# users cannot understand this.
It seems that this is designed for the convenience of C users.
However, animation is more likely used multiple time. So usually the animation pointer (handle) is kept and re-used again. (even in C as well)
Therefore, I think we should let developers delete their animation instance by themselves like other widgets or objects.

  • Transferring all the animation properties to constructor properties, for convenience, has always been the plan.

It's great! :)

==== 3. Performance / memory consumption

  • The performance of both Strategies is not equal. The method "clone", heavily used in Strategy 2, will create a lot of objects. Creation and destruction of these objects will cost time. For complex animations (multiple groups chained into each other) this is even going to be heavier. And due to the complexity of these animation objects (different types, tree-style hierarchy), this is not going to be easily optimizable.

Good point about cloning complex animation objects. It makes sense.

Given all the above, we think that Strategy 1 (the mixin) is the most convenient. Plus, it's already implemented :)

"it's already implemented :)" This is really powerful and what I was scared most! ;)

Thank you very much for the comments and considerations! :)
Only one thing I need to ask you in Strategy 1.
Could you consider not automatically delete animation instance in animation_start? (The comments of 2. Usability)
As far as I know, there is no such way like "efl_ref" in C# and it seems that the auto deletion concept would not be easy to be accepted by C# users.

snip

==== 2. Usability

  • When providing an Animation object to a Canvas object, the caller only passes a reference to animation_start. So if N different callers are calling animation_start(efl_ref(animation_obj)), then everything will be fine.
  • If the caller does not care about the Animation object anymore, it can pass its reference to animation_start(animation_obj) (without calling efl_ref). In this way when the animation finishes and removes its reference the Animation object will automatically be destroyed (because nobody else is holding references).

The original collected feedback was also based on C# usages because now we focus on C# bindings very much.
So from C# usage, 'animation_start(efl_ref(animation_obj))' is not supported and not common way. C# users cannot understand this.
It seems that this is designed for the convenience of C users.
However, animation is more likely used multiple time. So usually the animation pointer (handle) is kept and re-used again. (even in C as well)
Therefore, I think we should let developers delete their animation instance by themselves like other widgets or objects.

The C# should be something like (ignoring the other parameters for brevity):

var anim = new AnimObj();

widget.StartAnimation(anim);
anotherWidget.StartAnimation(anim);
yetAnotherWidget.StartAnimation(anim);
....

As StartAnimation receives their animation object by @move, the C# binding will automatically call efl_ref for it.

As StartAnimation receives their animation object by @move, the C# binding will automatically call efl_ref for it.

@lauromoura
It would be a good idea! We need to check if it would be fine for all other @move cases. (e.g. @property manager of Efl.Ui.Spotlight.Container)
But I think it would be fine for other @move cases as well :)

As StartAnimation receives their animation object by @move, the C# binding will automatically call efl_ref for it.

@lauromoura
It would be a good idea! We need to check if it would be fine for all other @move cases. (e.g. @property manager of Efl.Ui.Spotlight.Container)
But I think it would be fine for other @move cases as well :)

This is already done for all @move Efl.Object parameters, like setting the manager in the Container and setting the animation in this patch.

@segfaultxavi @bu5hm4n
I missed asking you one more thing in the original comment.

Is it OK if Efl.Canvas.Object provides animation_start with single parameter Efl.Canvas.Animation instance? i.e. animation_start(Efl.Canvas.Animation animation)

To cover pause and revert(reverse) animation with above, how about the following way?

Adds @method animation_revert(void) to Efl.Canvas.Animation

  • If animation is already started, then this method pauses the animation and starts the animation in reverse way from the current progress.
  • If animation is not started yet, then this method does nothing.

What do you think?

This is already done for all @move Efl.Object parameters, like setting the manager in the Container and setting the animation in this patch.

Sorry I forgot that was already implemented. Thank you for the information! :)

segfaultxavi added a comment.EditedThu, Nov 14, 7:43 AM

Adds @method animation_revert(void) to Efl.Canvas.Animation

This would be very useful, but I think it is too specific. Being able to provide a speed and a starting point would allow to "revert" animations and many other things.

I am OK with adding animation_start(anim) if we also add animation_start_full(anim, speed, start_progress).
However, if we finish task T8474, we could have only one animation_start() method with default values, so in C# you could use StartAnimation(anim) or StartAnimation(anim, speed, starting_pos). I think this would be the nicest solution.

To revert an animation, in C#, you just need to do:

obj.StartAnimationFull(old_anim, -1.0, obj.AnimationProgress);

However, if we finish task T8474, we could have only one animation_start() method with default values, so in C# you could use StartAnimation(anim) or StartAnimation(anim, speed, starting_pos). I think this would be the nicest solution.

To revert an animation, in C#, you just need to do:

obj.StartAnimationFull(old_anim, -1.0, obj.AnimationProgress);

If T8474 is completed, it would be wonderful!
Then, as you said, we should not separate animation_start(anim) at all.

Let's use animation_start(anim, speed, progress) as the current patch and let's discuss if each parameter is good enough or not to be used later. Because the patch is already completed!

I am going to merge this on the 20th of November, if there is no voice raised against this.

bu5hm4n updated this revision to Diff 26961.Mon, Nov 18, 5:53 AM

more docs

segfaultxavi added inline comments.Mon, Nov 18, 7:31 AM
src/lib/evas/canvas/efl_canvas_object_animation.eo
27

This parameter should be progress now (and the docs).

40

Start a new animation.

42

And what happens to the previous animation object?

46

Does this sentence make sense to you?

It should be something like:

Playback speed. `1.0` is normal playback. Negative values mean backward playback.
47

starting_progress?

bu5hm4n updated this revision to Diff 26982.Tue, Nov 19, 9:08 AM

make xavi happy

segfaultxavi resigned from this revision.Tue, Nov 19, 9:40 AM

Making Xavi happy is VERY important!

I have no further API concerns, I'll let somebody else review the implementation.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Nov 20, 1:05 AM
Closed by commit rEFLfa9389354885: introduce efl_canvas_object_animation (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.