Bug 113317

Summary: [GTK][AC] Use transform property of ClutterActor
Product: WebKit Reporter: ChangSeok Oh <kevin.cs.oh>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, joone, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 105699, 113318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
TestCase
none
Patch
none
Patch none

Description ChangSeok Oh 2013-03-26 10:03:52 PDT
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.
Comment 1 ChangSeok Oh 2013-03-26 11:13:04 PDT
Created attachment 195122 [details]
Patch
Comment 3 Gustavo Noronha (kov) 2013-03-27 12:38:31 PDT
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?
Comment 4 ChangSeok Oh 2013-03-28 10:07:37 PDT
Created attachment 195588 [details]
Patch
Comment 5 ChangSeok Oh 2013-03-28 10:18:43 PDT
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 :)
Comment 6 ChangSeok Oh 2013-03-28 10:22:05 PDT
Created attachment 195590 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2013-03-28 10:33:46 PDT
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.
Comment 8 ChangSeok Oh 2013-03-28 11:14:50 PDT
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)
Comment 9 Gustavo Noronha (kov) 2013-03-28 11:29:29 PDT
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.
Comment 10 ChangSeok Oh 2013-03-28 11:55:21 PDT
Created attachment 195610 [details]
Patch
Comment 11 ChangSeok Oh 2013-03-28 11:57:17 PDT
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.
Comment 12 ChangSeok Oh 2013-03-28 12:10:27 PDT
Created attachment 195612 [details]
Patch
Comment 13 Gustavo Noronha (kov) 2013-03-28 12:35:35 PDT
Comment on attachment 195612 [details]
Patch

Thanks!
Comment 14 WebKit Review Bot 2013-03-28 12:57:23 PDT
Comment on attachment 195612 [details]
Patch

Clearing flags on attachment: 195612

Committed r147145: <http://trac.webkit.org/changeset/147145>
Comment 15 WebKit Review Bot 2013-03-28 12:57:26 PDT
All reviewed patches have been landed.  Closing bug.