Bug 113317 – [GTK][AC] Use transform property of ClutterActor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ChangSeok Oh]

ClutterActor has new properties 'transform' & 'child-transform' since its version 1.12.
So we don't need to keep and use a matrix variable of GraphicsLayerClutter anymore.

[ChangSeok Oh]

Created attachment 195122 [details]
Patch

[ChangSeok Oh]

Applied to webkit-clutter as well
http://cgit.collabora.com/git/webkit-clutter.git/commit/?h=wip/changseok/unreviewed&id=4b683474307b94db706b3a926c8a5a152506d06f

[Gustavo Noronha (kov)]

Comment on attachment 195122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195122&action=review

Looks OK in general, but I have a question for you:

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:361
> +    if (matrix && cogl_matrix_is_identity(matrix))
> +        return;

This looks wrong, unles… what happens if you have a transformed actor and wants to reset it to identity? Would you pass NULL as a matrix in that case?

[ChangSeok Oh]

Created attachment 195588 [details]
Patch

[ChangSeok Oh]

Comment on attachment 195122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195122&action=review

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:361
>> +        return;
> 
> This looks wrong, unles… what happens if you have a transformed actor and wants to reset it to identity? Would you pass NULL as a matrix in that case?

I concerned stopping actor's animation. If we set a transform value explicitly to actor, the actor sticks the value for its modelView matrix while animations. So I wanted to block meaningless setting transform. As you mentioned, we can release transform value by setting NULL instead of an identity matrix.
But I haven't faced such a real case, so let's remove graphicsLayerActorSetTransform and then use clutter_actor_set_transform api directly :)

[ChangSeok Oh]

Created attachment 195590 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 195590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195590&action=review

> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:614
> +    // We should release the transform value of actor before starting actor's animations.

Say clear instead of release here.

> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:618
> +    // Otherwise, animations of actor will not run as expected. It seems actor sticks
> +    // its modelView matrix according to the transform value which we set to actor explicitly.
> +    // It may be related with the 'transform-set' property of ClutterActor, but not sure.
> +    // https://developer.gnome.org/clutter/stable/ClutterActor.html#ClutterActor--transform-set

I don't understand this very well. So if the actor already has a transformation when the animation starts, the animation runs twice (as described in the previous comment)? Setting the matrix to NULL does indeed set transform-set to FALSE.

[ChangSeok Oh]

Created attachment 195602 [details]
TestCase

kov. please find attached a testcase.
You can see a z-spin animating blue square with it.
Its keyframes are like below..

@-webkit-keyframes z-spin {                                               
0%    { -webkit-transform: rotateZ(350deg); }                           
50%    { -webkit-transform: rotateZ(180deg); }                          
100%  { -webkit-transform: rotateZ(0deg); }                             
}

WebKit sets a transform for the first frame in GraphicsLayerClutter::updateTransform so we set a transform value for an actor by using clutter_actor_set_transform before adding the transition to the actor. After that, the actor keeps 350deg while the animation, not rotating 180deg and 0deg unless we clear it by clutter_actor_set_transform(actor, 0). This is the symptom.
In my theory, if we set the transform value of actor explicitly by clutter_actor_set_transform, then actor seems to keep the property "transform-set" true to check actor has a user defined transform. If so, actor seems to stick the transform for its modelView matrix. In other words, the user defined transform value seems to have higher priority than other transitions. so if we don't clear it before staring animation(actually adding transition to an actor), we'll see only first frame while animation time.

I'm worried if you don't understand yet :\
Please run the test case without clutter_actor_set_transform(actor, NULL)

[Gustavo Noronha (kov)]

Comment on attachment 195590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195590&action=review

r- so we can get this bug reported in clutter, thanks for explaining the problem, hopefully my suggested comment is easier to understand =)

>> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:618
>> +    // https://developer.gnome.org/clutter/stable/ClutterActor.html#ClutterActor--transform-set
> 
> I don't understand this very well. So if the actor already has a transformation when the animation starts, the animation runs twice (as described in the previous comment)? Setting the matrix to NULL does indeed set transform-set to FALSE.

OK, so the problem is if there is an existing transform the animation does not happen. I think we should replace this whole comment with that description: "Work-around the fact that if there is a transform already set the animation fails to start." This is a bug in clutter so I think we should report it in bugzilla.gnome.org and add the bug URI to the comment as well.

[ChangSeok Oh]

Created attachment 195610 [details]
Patch

[ChangSeok Oh]

Comment on attachment 195590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195590&action=review

Thank you~

>>> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:618
>>> +    // https://developer.gnome.org/clutter/stable/ClutterActor.html#ClutterActor--transform-set
>> 
>> I don't understand this very well. So if the actor already has a transformation when the animation starts, the animation runs twice (as described in the previous comment)? Setting the matrix to NULL does indeed set transform-set to FALSE.
> 
> OK, so the problem is if there is an existing transform the animation does not happen. I think we should replace this whole comment with that description: "Work-around the fact that if there is a transform already set the animation fails to start." This is a bug in clutter so I think we should report it in bugzilla.gnome.org and add the bug URI to the comment as well.

Done.

[ChangSeok Oh]

Created attachment 195612 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 195612 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 195612 [details]
Patch

Clearing flags on attachment: 195612

Committed r147145: <http://trac.webkit.org/changeset/147145>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.