Bug 112824 - Provide 2D Matrix decomposition for animation
Summary: Provide 2D Matrix decomposition for animation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.8
: P2 Normal
Assignee: Dean Jackson
URL: http://jsfiddle.net/56T5V/light/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-20 11:10 PDT by Rik Cabanier
Modified: 2013-09-27 10:27 PDT (History)
9 users (show)

See Also:


Attachments
test file (8.25 KB, text/html)
2013-03-20 11:11 PDT, Rik Cabanier
no flags Details
Not for review. Have to add test file. (2.84 KB, patch)
2013-04-02 22:02 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Nearly ready patch (33.97 KB, patch)
2013-09-25 19:36 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (37.07 KB, patch)
2013-09-26 18:16 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (37.94 KB, patch)
2013-09-27 09:34 PDT, Dean Jackson
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rik Cabanier 2013-03-20 11:10:38 PDT
When you get the matrix of an object that is animated with matrices, it sometimes doesn't match what Core Animation calculated,
The attached test file shows the problem. Open the HTML, and hover over any element in row 2 or 3. The matrix of the hovered elememt is applied to the bottom object.
You will see that it doesn't match the rendering.

This also affects hit testing. If you hover over an object, the hit testing is done using the internal animation. Since the internal one scale to a point, you sometimes don't get a hit.
Comment 1 Rik Cabanier 2013-03-20 11:11:01 PDT
Created attachment 194089 [details]
test file
Comment 2 Rik Cabanier 2013-04-02 22:02:40 PDT
Created attachment 196277 [details]
Not for review.
Have to add test file.
Comment 3 Dirk Schulze 2013-04-02 22:36:16 PDT
Comment on attachment 196277 [details]
Not for review.
Have to add test file.

The patch looks really interesting. Do you think you can add more descriptions what you changed and how it affected the behavior? Can you add some tests please?
Comment 4 Rik Cabanier 2013-04-02 23:33:16 PDT
(In reply to comment #3)
> (From update of attachment 196277 [details])
> The patch looks really interesting. Do you think you can add more descriptions what you changed and how it affected the behavior? Can you add some tests please?

Yeah. It's not ready for review yet.
I have to think a bit and maybe simplify the code. I also need to check if z-animations match
Comment 5 Dean Jackson 2013-09-25 19:36:18 PDT
Created attachment 212654 [details]
Nearly ready patch

Patch mostly works. I think there is a typo somewhere.

Not for review.

Basically, swap to a 2d decomposition if we know we're 2d.
Comment 6 Radar WebKit Bug Importer 2013-09-26 14:48:24 PDT
<rdar://problem/15091882>
Comment 7 Dean Jackson 2013-09-26 18:16:26 PDT
Created attachment 212777 [details]
Patch
Comment 8 Dean Jackson 2013-09-27 09:34:44 PDT
Created attachment 212817 [details]
Patch
Comment 9 Eric Carlson 2013-09-27 09:52:35 PDT
Comment on attachment 212817 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:298
> +static bool decompose2(const TransformationMatrix::Matrix4& mat, TransformationMatrix::Decomposed2Type& result)

Nit: is there any reason not to spell out "matrix" completely?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:308
> +

Nit: you can lose the extra blank line.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:324
> +    // Renormalize matrix to remove scale.
> +
> +    if (result.scaleX) {

Nit: you don't need the extra blank line.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:335
> +    // Compute rotation and renormalize matrix.
> +
> +    result.angle = atan2(row0y, row0x);

Ditto.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:342
> +        // Thanks to the normalization above.
> +        
> +        double sn = -row0y;

Ditto.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1528
> +    if (!WebCore::decompose2(m_matrix, decomp))
> +        return false;
> +    return true;

Nit: this can be just "return WebCore::decompose2(m_matrix, decomp)"

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1543
> +    if (!WebCore::decompose4(m_matrix, decomp))
>          return false;
>      return true;

And this can be "return WebCore::decompose4(m_matrix, decomp)"
Comment 10 Dean Jackson 2013-09-27 10:20:17 PDT
Comment on attachment 212817 [details]
Patch

Fixed all comments. Thanks.
Comment 11 Dean Jackson 2013-09-27 10:27:05 PDT
Committed r156553: <http://trac.webkit.org/changeset/156553>