Bug 113317

Created attachment 195122 [details] Patch

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

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?

Created attachment 195588 [details] Patch

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 :)

Created attachment 195590 [details] Patch

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.

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 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.

Created attachment 195610 [details] Patch

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.

Created attachment 195612 [details] Patch

Comment on attachment 195612 [details] Patch Thanks!

Comment on attachment 195612 [details] Patch Clearing flags on attachment: 195612 Committed r147145: <http://trac.webkit.org/changeset/147145>

All reviewed patches have been landed. Closing bug.