Bug 112824

Summary: Provide 2D Matrix decomposition for animation
Product: WebKit Reporter: Rik Cabanier <cabanier>
Component: CSSAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dino, esprehn+autocc, glenn, kondapallykalyan, krit, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.8   
URL: http://jsfiddle.net/56T5V/light/
Attachments:
Description Flags
test file
none
Not for review. Have to add test file.
none
Nearly ready patch
none
Patch
none
Patch eric.carlson: review+

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>