Created attachment 210719 [details] [review]
actor: Invalidate the current state when popping easing states

Created attachment 210720 [details] [review]
actor: Clear outstanding transitions when setting fixed values

Review of attachment 210719 [details] [review]:

looks good to me

Review of attachment 210720 [details] [review]:

one question: why?

(In reply to comment #4)
> Review of attachment 210720 [details] [review]:
> 
> one question: why?

If I add a 500ms duration on the actor's x position, and then 250ms later set the x directly, it should stop the current animation and warp to the position.

(In reply to comment #5)
> (In reply to comment #4)
> > Review of attachment 210720 [details] [review] [details]:
> > 
> > one question: why?
> 
> If I add a 500ms duration on the actor's x position, and then 250ms later set
> the x directly, it should stop the current animation and warp to the position.

no, it should not, unless you set the easing duration to 0, and set the position - or you get the transition for the "x" property and remove it yourself.

to be even more explicit: the *intent* of the API is that changing the state of the actor should always be a transition - unless you go out of your way to make it "jump" to the final state. any change that allows you to do a jump without explicitly telling the actor to do so is in direct opposition to the intent of the API, and as such it won't be accepted.

Then we need to update the transition in both cases, otherwise with something like:

    clutter_actor_save_easing_state (actor);
    clutter_actor_set_x (actor, 200);
    clutter_actor_restore_easing_state (actor);
    clutter_actor_set_x (actor, 100);

the second set_x will be ignored, as the easing duration is back to 0, which means that it just updates fixed_x, and not the transition.

Created attachment 210771 [details] [review]
actor: Simplify setters of animatable properties

Instead of checking the duration of the current easing state we should
check if there's a transition in progress, and update it
unconditionally.

If there is no easing state, or the easing state has a duration of zero
milliseconds, then create_transition() should bail out early and set the
requested final state.

This allows us to write:

  clutter_actor_save_easing_state (actor);
  clutter_actor_set_x (actor, 200);
  clutter_actor_restore_easing_state (actor);

  [...]

  clutter_actor_set_x (actor, 100);

and have the second set_x() update the easing in progress, instead of
being ignored.

Attachment 210719 [details] pushed as 628ffa7 - actor: Invalidate the current state when popping easing states
Attachment 210771 [details] pushed as fa8d431 - actor: Simplify setters of animatable properties

(In reply to comment #9)
> This allows us to write:
> 
>   clutter_actor_save_easing_state (actor);
>   clutter_actor_set_x (actor, 200);
>   clutter_actor_restore_easing_state (actor);
> 
>   [...]
> 
>   clutter_actor_set_x (actor, 100);
> 
> and have the second set_x() update the easing in progress, instead of
> being ignored.

I still think that this is a bad decision, and will break existing behavior. Here's a code sample:

    static void
    button_press_cb (...)
    {
      if (button == 1)
        {
          clutter_actor_save_easing_state (actor);
          clutter_actor_set_x (actor, 200);
          clutter_actor_restore_easing_state (actor);
        }
      else
        {
          clutter_actor_set_x (actor, 50);
        }
    }

If you left-mouse-click, you get a nice animation to 200. If you right-mouse-click fast, you get a transition back to 50, otherwise the actor gets warped back to 50.

As an example, see test-scrolling. If you re-grab the actor as it's animating the drag-end, the drag is now animating as well.

Using the implicit animation API like this is just going to introduce race conditions. Just stop.

Additionally, the comment that you added that says that actors are going to have a default easing state. That's going to make it hard to just "set the x" of an actor without animating, which is very useful to do, especially if we use Tweener in the shell -- now we have *two* competing animation frameworks.

( Rant: The one thing I'll suggest is to sit down and write a complex application like GNOME Shell, or go through and try and hack on GNOME Shell and port it to the new API for a day or two, and hopefully you'll realize how your API changes are really, really inconvenient. You're blindly writing an API without use cases or existing applications, so you're yourself distant from the users of the API, and you're designing something that looks nice on paper. )

(In reply to comment #11)
> (In reply to comment #9)
> > This allows us to write:
> > 
> >   clutter_actor_save_easing_state (actor);
> >   clutter_actor_set_x (actor, 200);
> >   clutter_actor_restore_easing_state (actor);
> > 
> >   [...]
> > 
> >   clutter_actor_set_x (actor, 100);
> > 
> > and have the second set_x() update the easing in progress, instead of
> > being ignored.
> 
> I still think that this is a bad decision, and will break existing behavior.

there is no existing behaviour that uses implicit animations with easing states, so how can it break?

> Here's a code sample:
> 
>     static void
>     button_press_cb (...)
>     {
>       if (button == 1)
>         {
>           clutter_actor_save_easing_state (actor);
>           clutter_actor_set_x (actor, 200);
>           clutter_actor_restore_easing_state (actor);
>         }
>       else
>         {
>           clutter_actor_set_x (actor, 50);
>         }
>     }
> 
> If you left-mouse-click, you get a nice animation to 200. If you
> right-mouse-click fast, you get a transition back to 50, otherwise the actor
> gets warped back to 50.
> 
> As an example, see test-scrolling. If you re-grab the actor as it's animating
> the drag-end, the drag is now animating as well.
> 
> Using the implicit animation API like this is just going to introduce race
> conditions. Just stop.

it's a race only insofar as you try to mix both immediate and tweened behaviours at the same time; it's not a good plan, so stop doing that.

> Additionally, the comment that you added that says that actors are going to
> have a default easing state. That's going to make it hard to just "set the x"
> of an actor without animating, which is very useful to do, especially if we use
> Tweener in the shell -- now we have *two* competing animation frameworks.

> ( Rant: The one thing I'll suggest is to sit down and write a complex
> application like GNOME Shell, or go through and try and hack on GNOME Shell and
> port it to the new API for a day or two, and hopefully you'll realize how your
> API changes are really, really inconvenient. You're blindly writing an API
> without use cases or existing applications, so you're yourself distant from the
> users of the API, and you're designing something that looks nice on paper. )

the use of Tweener in Shell is inconsequential: you are using a random API that is based on the assumption that setting a value on an actor's state will always yield an immediate jump there, which is a guarantee we never made, and that I only respect out of the kindness of my heart. your usage of an external API and mixing it with the Clutter animation API (*any* animation API)  is something I have no intention or inclination to consider a valid use case.

the implicit animation API is just a basic evolution of clutter_actor_animate(): it works under the same assumptions and following the same rules. if you're using clutter_actor_animate() and Tweenet at the same time you're going to fall into the same issues - same thing applies for State or Animator.

the real solution is to stop moaning and use the Clutter animation API for Clutter elements, instead of trying to bend Clutter into the wrong shape against the intent of the Clutter maintainer.

(In reply to comment #12)
> it's a race only insofar as you try to mix both immediate and tweened
> behaviours at the same time; it's not a good plan, so stop doing that.

    static void
    button_press_cb (...)
    {
      if (button == 1)
        {
          clutter_actor_save_easing_state (actor);
          clutter_actor_set_easing_duration (actor, 500);
          clutter_actor_set_x (actor, 400);
          clutter_actor_restore_easing_state (actor);
        }
      else
       {
          clutter_actor_save_easing_state (actor);
          clutter_actor_set_easing_duration (actor, 5000);
          clutter_actor_set_x (actor, 50);
          clutter_actor_restore_easing_state (actor);
       }
    }

Left click, watch the fast transition to the right. Right-click after the animation is finished, and you get a slow transition. Perfect.

Now right-click while animating to the right. A fast transition back. And now left-click while animating to the left. A slow transition back to the right. More race conditions.

Of course, you can fix these bugs like whack-a-mole as they come up, but I'd just suggest to quit while you're ahead and realize that this API as you want it implemented is going to be full of bugs and worms. You shouldn't care if a transition exists, you should always stop any existing transitions and always use the current easing state of the actor... Yes, I know my patch didn't account for this.

Review of attachment 210771 [details] [review]:

This broke the Shell. Please revert. I am currently investigating why.

these commits are on master; gnome-shell should be using the clutter-1.10 branch, given that we branched for GNOME 3.4.

I guess this can be closed.