Bug 23689 - Add support for 3D to TransformationMatrix and WebKitCSSMatrix
Summary: Add support for 3D to TransformationMatrix and WebKitCSSMatrix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on: 6868
Blocks: 23359
  Show dependency treegraph
 
Reported: 2009-02-02 12:21 PST by Chris Marrin
Modified: 2009-02-09 21:22 PST (History)
4 users (show)

See Also:


Attachments
IDL of composite 2D/3D CSSMatrix (2.57 KB, text/plain)
2009-02-02 12:22 PST, Chris Marrin
no flags Details
Patch (27.71 KB, patch)
2009-02-06 09:51 PST, Chris Marrin
simon.fraser: review-
Details | Formatted Diff | Diff
Replacement patch (37.27 KB, patch)
2009-02-09 17:56 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Replacement patch addressing one more issue (37.24 KB, patch)
2009-02-09 17:58 PST, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2009-02-02 12:21:06 PST
The current CSS Transform proposal specifies 3D functions in WebKitCSSMatrix. In talking to Darin, he sees no problem having this 3D support in WebKit separately from support of 3D transforms in CSS.

Here is a link to the 2D version (which is currently in TOT):
 
    http://webkit.org/specs/CSSVisualEffects/CSSTransforms.html#cssmatrix-interface

and here is a link to the 3D version:

    http://webkit.org/specs/CSSVisualEffects/CSSTransforms3D.html#cssmatrix-interface

Attached is a composite IDL encompassing the functionality of both. I believe we will replace the two in the current specs with this one.
Comment 1 Chris Marrin 2009-02-02 12:22:12 PST
Created attachment 27253 [details]
IDL of composite 2D/3D CSSMatrix
Comment 2 Chris Marrin 2009-02-06 09:51:23 PST
Created attachment 27401 [details]
Patch

        Added 3D functions to WebKitCSSMatrix. This depends on the 3D functions
        added to TransformationMatrix in https://bugs.webkit.org/show_bug.cgi?id=6868
Comment 3 Simon Fraser (smfr) 2009-02-09 10:59:41 PST
Comment on attachment 27401 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================

> +        * css/WebKitCSSMatrix.cpp:
> +        (WebCore::WebKitCSSMatrix::translate):
> +        (WebCore::WebKitCSSMatrix::scale):
> +        (WebCore::WebKitCSSMatrix::rotate):
> +        (WebCore::WebKitCSSMatrix::rotateAxisAngle):
> +        (WebCore::WebKitCSSMatrix::toString):
> +        * css/WebKitCSSMatrix.h:
> +        (WebCore::WebKitCSSMatrix::a):
> +        (WebCore::WebKitCSSMatrix::b):
> +        (WebCore::WebKitCSSMatrix::c):
> +        (WebCore::WebKitCSSMatrix::d):
> +        (WebCore::WebKitCSSMatrix::e):
...

It's OK to remove useless lines from the changes files/functions list when they don't add
any useful info.

> Index: WebCore/css/WebKitCSSMatrix.cpp
> ===================================================================

> +PassRefPtr<WebKitCSSMatrix> WebKitCSSMatrix::translate(double x, double y, double z)
>  {
> +    // Passing a NaN (or undefined in JavaScript) will use default values
> +    // This allows the 3D form to used for 2D operations

This comment should be in the header, and maybe the idl?

>      if (isnan(x))
>          x = 0; 

Does isnan() work on all platforms?

> +    if (isnan(z))
> +        z = 0;
> +    return WebKitCSSMatrix::create(m_matrix.translate3d(x, y, z));

This seems wrong; it will change |this|.

> +    // Passing a NaN (or undefined in JavaScript) will use default values
> +    // This allows the 3D form to used for 2D operations
> +    // In the case of scale, passing scaleX or scaleZ as NaN uses 1, but
> +    // scaleY of NaN makes it the same as scaleX

Ditto.



> +    // Passing a NaN (or undefined in JavaScript) will use default values
> +    // This allows the 3D form to used for 2D operations
> +    // In the case of rotate, if rotY and rotZ are NaN, it rotates about Z.
> +    // Otherwise use a rotation value of 0.

Ditto.

>  String WebKitCSSMatrix::toString()
>  {
> -    return String::format("matrix(%f, %f, %f, %f, %f, %f)",
> -                            m_matrix.a(), m_matrix.b(), m_matrix.c(), m_matrix.d(), m_matrix.e(), m_matrix.f());
> +    if (m_matrix.isAffine())
> +        return String::format("matrix(%f, %f, %f, %f, %f, %f)",
> +                                m_matrix.a(), m_matrix.b(), m_matrix.c(), m_matrix.d(), m_matrix.e(), m_matrix.f());
> +    else
> +        return String::format("matrix3d(%f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f)",
> +                                m_matrix.m11(), m_matrix.m12(), m_matrix.m13(), m_matrix.m14(), 
> +                                m_matrix.m21(), m_matrix.m22(), m_matrix.m23(), m_matrix.m24(), 
> +                                m_matrix.m31(), m_matrix.m32(), m_matrix.m33(), m_matrix.m34(), 
> +                                m_matrix.m41(), m_matrix.m42(), m_matrix.m43(), m_matrix.m44());
>  }

Please add a FIXME comment pointing to  https://bugs.webkit.org/show_bug.cgi?id=20674


> Index: WebCore/css/WebKitCSSMatrix.h
> ===================================================================

>      void setMatrixValue(const String& string, ExceptionCode&);
>      
>      // this = this * secondMatrix
>      PassRefPtr<WebKitCSSMatrix> multiply(WebKitCSSMatrix* secondMatrix);
>      

>      PassRefPtr<WebKitCSSMatrix> inverse(ExceptionCode&);
> -    PassRefPtr<WebKitCSSMatrix> translate(float x, float y);
> -    PassRefPtr<WebKitCSSMatrix> scale(float scaleX, float scaleY);
> -    PassRefPtr<WebKitCSSMatrix> rotate(float rot);
> +    PassRefPtr<WebKitCSSMatrix> translate(double x, double y, double z);
> +    PassRefPtr<WebKitCSSMatrix> scale(double scaleX, double scaleY, double scaleZ);
> +    PassRefPtr<WebKitCSSMatrix> rotate(double rotX, double rotY, double rotZ);
> +    PassRefPtr<WebKitCSSMatrix> rotateAxisAngle(double x, double y, double z, double angle);

I'd like to more comments here (in particular, being very clear about the order of operations,
on |this| being modified, and on what the return value points to.

> Index: WebCore/css/WebKitCSSMatrix.idl
> ===================================================================

> +        attribute double a;
...
> +        attribute double m11;

Needs a comment explaining why both a-f and m11-m44 are exposed, and what 
a-f contain when the matrix has 3d in it.


> +        void setMatrixValue(in DOMString string) raises (DOMException);
>          WebKitCSSMatrix multiply(in WebKitCSSMatrix secondMatrix);
>          WebKitCSSMatrix inverse() raises (DOMException);
> -        WebKitCSSMatrix translate(in float x, in float y);
> -        WebKitCSSMatrix scale(in float scaleX, in float scaleY);
> -        WebKitCSSMatrix rotate(in float rot);
> +        WebKitCSSMatrix translate(in double x, in double y, in double z);
> +        WebKitCSSMatrix scale(in double scaleX, in double scaleY, in double scaleZ);
> +        WebKitCSSMatrix rotate(in double rotX, in double rotY, in double rotZ);
> +        WebKitCSSMatrix rotateAxisAngle(in double x, in double y, in double z, in double angle);
>          
>          [DontEnum] DOMString toString();

These need copious comments for the documentation folks.
Should use [Immutable].
Should make the order of operations very clear.

Before making this wholesale float -> double conversion, we need to find out if it's OK.

> Index: LayoutTests/ChangeLog
> ===================================================================

> +        Since the CSS parser does not yet handle any of the 3D
> +        functions for -webkit-transform, tests for these features
> +        are currently commented out.

Don't talk about future features. Just say that these are tests for
3D functionality of CSSMatrix.

> Index: LayoutTests/transforms/3d/cssmatrix-3d-interface.xhtml
> ===================================================================

We should also test
* the immutability of CSSMatrix
* order of operations
* round-tripping through a string (as far as we can).

r- for now, as I think we have a bit of research to do before this can land.
Comment 4 Darin Adler 2009-02-09 11:14:10 PST
(In reply to comment #3)
> Does isnan() work on all platforms?

I believe it does as long as you include <wtf/MathExtras.h>, not just <math.h>.

Also, no else after return.
Comment 5 Chris Marrin 2009-02-09 17:56:13 PST
Created attachment 27509 [details]
Replacement patch

Replacement patch addressing all issues
Comment 6 Chris Marrin 2009-02-09 17:58:02 PST
Created attachment 27510 [details]
Replacement patch addressing one more issue
Comment 7 Simon Fraser (smfr) 2009-02-09 18:05:31 PST
Comment on attachment 27510 [details]
Replacement patch addressing one more issue

> Index: WebCore/css/WebKitCSSMatrix.cpp
> ===================================================================

> +PassRefPtr<WebKitCSSMatrix> WebKitCSSMatrix::multiply(WebKitCSSMatrix* secondMatrix) const
>  {
>      TransformationMatrix tmp(m_matrix);
>      tmp.multiply(secondMatrix->m_matrix);

We should probably null-check secondMatrix here.

r=me with that fix.
Comment 8 Simon Fraser (smfr) 2009-02-09 21:22:20 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/transforms/2d/cssmatrix-interface-expected.txt
	M	LayoutTests/transforms/2d/cssmatrix-interface.xhtml
	M	WebCore/ChangeLog
	M	WebCore/css/WebKitCSSMatrix.cpp
	M	WebCore/css/WebKitCSSMatrix.h
	M	WebCore/css/WebKitCSSMatrix.idl
Committed r40807