Page MenuHomePhabricator

efl_ui_spotlight: Introduce animation manager
ClosedPublic

Authored by bu5hm4n on Feb 16 2020, 4:37 AM.

Details

Summary

the manager is basically not new, its just the moved fade manager, with
a little bit more utilization. The manager now can be equipt with 3
animaton objects that are played when the correct reason happens.

For now the fade manager is the only thing that uses that.

Depends on D11357

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.Feb 16 2020, 4:37 AM
bu5hm4n requested review of this revision.Feb 16 2020, 4:37 AM
bu5hm4n added a comment.EditedFeb 16 2020, 4:38 AM

The result can be seen here: https://phab.enlightenment.org/F3847090

You can specify your animation object that can be used to move in or out or jump a little bit of content.

The amount of code needed for something like this basically boils down to:

jump_animation = efl_new(EFL_CANVAS_ALPHA_ANIMATION_CLASS);
efl_animation_alpha_set(jump_animation, 0.0, 1.0);
efl_animation_duration_set(jump_animation, 0.5);

push_animation = efl_new(EFL_CANVAS_TRANSLATE_ANIMATION_CLASS);
efl_animation_translate_set(push_animation, EINA_POSITION2D(0, 100), EINA_POSITION2D(0, 0));
efl_animation_duration_set(push_animation, 0.5);

pop_animation = efl_new(EFL_CANVAS_TRANSLATE_ANIMATION_CLASS);
efl_animation_translate_set(pop_animation, EINA_POSITION2D(0, -100), EINA_POSITION2D(0, 0));
efl_animation_duration_set(pop_animation, 0.5);

custom_animation_manager = efl_new(EFL_UI_SPOTLIGHT_ANIMATION_MANAGER_CLASS,
                                   efl_ui_spotlight_manager_animation_push_setup_set(efl_added, push_animation),
                                   efl_ui_spotlight_manager_animation_pop_setup_set(efl_added, pop_animation),
                                   efl_ui_spotlight_manager_animation_jump_setup_set(efl_added, jump_animation, jump_animation));

Which is not very verbose, but looks like a good mixture between functionality and shortness. Additionally, compared to something that uses edje states and animations for that, this way of doing animations is a lot more performance friendly, as it uses mapping instead of real geometry changes.

CI: https://travis-ci.org/Enlightenment/efl/builds/651130577

segfaultxavi requested changes to this revision.Feb 17 2020, 8:31 AM

Worth mentioning that the above setup example can also be done with a one-liner :)

I'd like to discuss the class name:
Animation_Manager makes it look like this manager handles animations, which is not true. I think Animated_Manager or Animating_Manager are closer to its purpose (but then we'll also have to rename Fade_Manager and Scroll_Manager).
Another option would be to call this one Customizable_Manager.

Also, the *_setup properties should end with a noun, and "setup" is usually understood as a verb.
How about we call them *_animation instead?

Opinions?

I'll take care of docs in a child patch.

This revision now requires changes to proceed.Feb 17 2020, 8:31 AM
bu5hm4n requested review of this revision.Feb 17 2020, 8:39 AM

You will hate me, but i would take the API how it is right now, this is @beta for now, we still can discuss these names later on. (Also, together with the name for the two other managers). I would keep the _setup prefix as well, as the idea is to call them at restart.

segfaultxavi resigned from this revision.Feb 19 2020, 1:09 AM

OK, as long as we don't forget and we come back later to review this API...

I'll let someone else review the code :)

bu5hm4n added a comment.EditedFeb 19 2020, 6:18 AM

@Jaehyun_Cho Can you have a look ?

@bu5hm4n

Sorry I missed this one.

I think this patch is necessary because this patch enables users apply their own transition effects easily. :)
I have an idea about the APIs.

It seems that each API's animation works as follows.
push_setup -> "to" content's animation for push()
pop_setup -> "from" content's animation for pop()
jump_setup -> in : "to" content's animation / out : "from" content's animation

The current implementation is useful if "to" content's animations for push() and pop() are the same because we just need to call jump_setup.
But it seems that the following use case is more commonly used.
"to" content's animation for push() is the reverse version of "from" content's animation for pop(). ("from" content's animation for push() is the reverse version of "to" content's animation for pop())
In this use case, users should use push_setup and pop_setup with jump_setup.

I think it would be better if we separate increasing index case (move forward) and decreasing index case (move backward).
e.g.
forward_animation -> in : "to" content's animation / out : "from" content's animation (this also covers push_setup)
backward_animation -> in : "to" content's animation / out : "from" content's animation (this also covers pop_setup)
jump_setup is also covered by forward_animation and backward_animation.

Hi!

I am not sure if i understood your explanation. How it works is: You have the switch_to calls, which came with a "reason" the reason is either push or pop or switch.
When the reason is:

  • push, to animation of push_setup is taken, and from animation of jump_setup
  • pop, to animation of jump_setup is taken, and from animation of pop_setup
  • jump, to and from animation are taken from jump_setup

If I understood you correctly you want to have that in the first two cases also a custom to and from animation are setted, and not the ones form jump are taken, correct? I am not 100% sure if this is what you/we want, Efl.Ui.Spotlight is like a paper stack where you can navigate through, each operation has a assosiated "visual appearance", and laying something onto the stack looks like something, and "doing away" the element below has something. breaking this up, makes it possible that the operation cannot be specified from the "visual appearance", is that what we *really* want ?

I am not sure I understood your comment correctly.
So I want to explain my previous comment with more precisely.

As far as I know, we have push, pop, and switch cases.

  • push is done by push()
  • pop is done by pop()
  • switch is done by active_element_set()

My point is as follows.

  • push animation and switch (from lower index to higher index) animation are the same. (no need to be different)
  • pop animation and switch (from higher index to lower index) animation are the same. (no need to be different)

In this patch, we have push_setup, pop_setup, and jump_setup.
This means that we can support different animations between push and switch (from lower index to higher index) in this patch. (pop and switch (from higher index to lower index) as well)
But my point is that we do not need to support them.

Because I think as follows.

  • push() == (pack_end() and then active_element_set())
  • pop() == (active_element_set() and then unpack())

So I think we do not need to support different animations between push and switch (from lower index to higher index). (pop and switch (from higher index to lower index) as well)

To generalize push and switch (from lower index to higher index), I suggest "forward".
In the same way, I suggest "backward" to generalize pop and switch (from higher to lower index).

In short, I suggest "forward_animation(in, out)" and "backward_animation(in, out)" instead of "push_setup(animation)", "pop_setup(animation)", and "jump_setup(in, out)".

Ah - I wanted to support exactly that case to make things possible like the elm_test example we have. However, you do not *have* to call setup_push or setup_pop. If you do not set there a animation object, the ones from setup_jump are automatically taken.
In your API example i still do not understand when in/out from forward is played, and when in/out from backward are played.

I made a patch onto this patch on my dev branch (devs/jaehyun/animation_manager) as a test version.
https://git.enlightenment.org/core/efl.git/commit/?h=devs/jaehyun/animation_manager&id=72da9ec8c6ef9e6081979c1caae3e4749d0a9965

Here is a simple example with forward_animation and backward_animation.

Case 1. Moving forward (including push() case)
Current active index = 0
New active index = 1
-> forward_animation is started.
-> in forward_animation is applied to index 1 page
-> out forward_animation is applied to index 0 page

Case 2. Moving backward (including pop() case)
Current active index = 1
New active index = 0
-> backward_animation is started.
-> in backward_animation is applied to index 0 page
-> out backward_animation is applied to index 1 page

Okay yeah, that is i think what i said in my first comment.

The idea here is that you can associate a animation with a operation, and when you push something in, the object that is currently active, is simply faded out. Is there really the demand that someone wants to have a different "move out" animation depending on if its a jump or a push?

Okay yeah, that is i think what i said in my first comment.

The idea here is that you can associate a animation with a operation, and when you push something in, the object that is currently active, is simply faded out. Is there really the demand that someone wants to have a different "move out" animation depending on if its a jump or a push?

I understood your point.
So this patch wants to support different animation between push() and active_element_set().

We can think about naviframe for this case.
As you know, in naviframe, we have push(), pop(), insert_after(), elm_object_item_del() to change active_element like Spotlight.Container.

For insert_after() and elm_object_item_del(), there is no animation.
For push() and pop(), there are several animations based on naviframe item style. (the transition animation is defined in naviframe item style in edc)
Although there are several animations for push() and pop(), it does not depend on which API is used but it depends on naviframe item style.

Consequently, I would say there is no demand that someone wants to have a different "move out" animation depending on if its a jump or a push.
(no animation is supported by animated_transition false)

Okay, then lets say we go with what we have for now ? We can still merge the patch you proposed above in a none API changing manner, when there is demand for it. I am always a bit worried when we are "overdoing" something where we dont have a real usecase, would that be okay for you ?

bu5hm4n planned changes to this revision.Wed, Mar 11, 5:39 AM

After a little meeting, we agree that push_setup and pop_setup should also get 2 objects, but they will get the same semantic as jump_setup, and hence have the possibility to only pass one object.

bu5hm4n updated this revision to Diff 29479.Fri, Mar 13, 3:18 AM
bu5hm4n edited the summary of this revision. (Show Details)

add in and out to pop and push animation

Thank you for adding parameters to push_setup and pop_setup.

I have one more thing to discuss about backwards animation of "out" parameter.

In this patch, "out" animation works backwards.
e.g. if alpha animation (alpha from 0 to 1) is set to "out" parameter, then "out" animation sets alpha from 1 to 0.

In some cases, it may be useful and helpful to memory consumption.
But in many cases, "out" animation is not same with backwards animation of "in" animation.
e.g. in watch case, push "in" is zoom 0.x to 1.0 and push "out" is zoom 1.0 to 1.y

Instead, push()'s "out" animation is similar to backwards animation of pop()'s "in" animation. (in the same way, push()'s "in" animation is similar to backwards animation of pop()'s "out" animation)

In this case, users should imagine the backwards animation and set the backwards animation to "out" parameters of push_setup(), pop_setup(), jump_setup().
e.g. users should set alpha animation (alpha from 0 to 1) to "out" parameter to show alpha animation (alpha from 1 to 0).

In short, if "in" animation is not the exact backwards animation of "out" animation, then the current backwards logic might not be convenient.

So how about setting exact animation to "out" parameters of push_setup, pop_setup, and jump_setup instead of backwards animation?
e.g. if alpha animation (alpha from 1 to 0) is set to "out" parameter, then "out" animation shows alpha animation (alpha from 1 to 0)

If the backwards animation logic is not important in this patch, then it would be fine to submit this patch now and later I will make a patch to remove backwards animation logic.

What do you think?

Mhm, I would really like to keep the semantik that the out animation is played backwards, as that saves for the cases we have right now *a lot* of memory and duplication. What we could do, to support easier creation of the out animation, we could introduce a backward interpolator which maps [0..1] -> [1..0], would that be fine for you ?

If the backwards animation logic is not important in this patch, then it would be fine to submit this patch now and later I will make a patch to remove backwards animation logic.

What do you think?

Lets keep discussing that on IRC or somewhere in a ticket, and move on for now ?

Mhm, I would really like to keep the semantik that the out animation is played backwards, as that saves for the cases we have right now *a lot* of memory and duplication. What we could do, to support easier creation of the out animation, we could introduce a backward interpolator which maps [0..1] -> [1..0], would that be fine for you ?

I understand your point as a framework developer.
But as you know, users like intuitive usage and they would not understand why they should use backwards animation or backwards interpolator for framework's needs (less memory consumption and removing duplicate codes).

Jaehyun_Cho accepted this revision.Mon, Mar 16, 3:39 AM
This revision is now accepted and ready to land.Mon, Mar 16, 3:39 AM
Closed by commit rEFL561906399c88: efl_ui_spotlight: Introduce animation manager (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyTue, Mar 17, 2:32 AM
This revision was automatically updated to reflect the committed changes.