Bug 35520 – [Qt] GraphicsLayer: matrix interpolations in transform-animations don't behave correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Noam Rosenthal]

GraphicsLayer.h has an API that helps realizing whether AC should avoid taking control of the animation.
The Mac implementation, for example, avoids > 180 deg. rotations. Right now Qt tries to do those rotations anyway, and it doesn't look right.

[Kim Grönholm]

Created attachment 49889 [details]
Test page for testing transitions and animations with empty source list

The >180deg rotation animations don't work properly (meaning that they "take the shortest way") if the rotation is not interpolated as an angle value but as a transformation matrix. This also happens without AC. 

However, there were actually two bugs in determining whether to fall back to matrix interpolation in case of AC:
1. When the source transformation is empty, AC still falls to matrix interpolation even though it should not.
2. If the source and target transformationlists are of same size but mismatch, AC doesn't fall to matrix interpolation even though it should

Then there also was a bug in the actual interpolation of matrices (target matrix was not taken as is but first combined with source matrix).

I'll attach test cases for both situations and a patch to fix all the mentioned issues.

[Kim Grönholm]

Created attachment 49890 [details]
Test page for testing animations and transitions with same sized but mismatching transform lists

[Kim Grönholm]

Created attachment 49891 [details]
Fixes animations behavior with AC to match non-AC in some special cases

Submitted a patch that solves the issues making the test cases behave similarly with AC than without AC.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component

[Eric Seidel (no email)]

Comment on attachment 49891 [details]
Fixes animations behavior with AC to match non-AC in some special cases

Needs a test.

[Noam Rosenthal]

This is a good test for >180 degree rotations: http://qt.gitorious.org/qt-labs/scxml/blobs/raw/statechartz/statechartz/demo.html

We should test if the patch fixes this site, and doesn't regress anything else.

[Kim Grönholm]

Created attachment 51010 [details]
Fixes the transform animations by correctly deciding whether to do matrix interpolation or not

[Noam Rosenthal]

Signed of by /me :)

[Simon Hausmann]

Comment on attachment 51010 [details]
Fixes the transform animations by correctly deciding whether to do matrix interpolation or not

You might consider using WebCore/manual-tests or manual-tests/qt for manual tests.

But we _really_ need layout tests for this code.

[WebKit Commit Bot]

Comment on attachment 51010 [details]
Fixes the transform animations by correctly deciding whether to do matrix interpolation or not

Clearing flags on attachment: 51010

Committed r56321: <http://trac.webkit.org/changeset/56321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Comment on attachment 51010 [details]
Fixes the transform animations by correctly deciding whether to do matrix interpolation or not


> +                else
> +                    for (size_t j = 0; j < sourceOperationCount && validTransformLists; ++j)
> +                        if (!sourceOperations.operations()[j]->isSameType(*targetOperations.operations()[j]))
> +                            validTransformLists = false;

Simon the patch you reviewed has wrong coding style.

The else and for has contents that spans multiple real lines in total. Both the else and the for should use braces. The if doesn't not need braces, thus it is correct.

[Noam Rosenthal]

ah, right Kenneth.
Should I submit a style-fix patch for that and a few other style issues?