Use SkMatrix44 for the underlying implementation of ui::Transform

Use SkMatrix44 for the underlying implementation of ui::Transform


BUG=
TEST=ui_unittest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91855

[sky, 2011-06-09 17:20:29]

At a minimum you'll also need to update compositor_win. Not sure about the GL
side.
I also don't know whats involved with skia rolls. You should ping Mike Reed or
Stephen White to make sure it's ok to do that.

  -Scott

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#newcode15
ui/gfx/transform.cc:15: static inline int SymmetricRound(float x) {
don't indent this.

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#newcode185
ui/gfx/transform.cc:185: point->SetPoint(SymmetricRound(p[0]),
Why do we want SymmetricRound and not floor?

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#newcode190
ui/gfx/transform.cc:190: if (!hasInverse_) {
Why do we bother storing the inverse?

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.h
File ui/gfx/transform.h (right):

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.h#newcode91
ui/gfx/transform.h:91: SkMatrix44 matrix_;
newline between 90 and 91.

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform_unittest.cc#ne...
ui/gfx/transform_unittest.cc:1: // Copyright (c) 2006-2008 The Chromium Authors.
All rights reserved.
2011

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform_unittest.cc#ne...
ui/gfx/transform_unittest.cc:28: static const struct TestCase {
Why are any of these 'static const'?

[wjmaclean, 2011-06-09 17:20:57]

I'm guessing this hasn't been compiled/tested with COMPOSITOR_2 defined, right?
If so, that will need to happen before this can land (e.g. TextureGL::Draw
relies on the matrix, and will require modification to use a 4x4 ...)

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#newcode46
ui/gfx/transform.cc:46: static_cast<SkMScalar>(1.0));
Should this be altering other scales?

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#newcode70
ui/gfx/transform.cc:70: static_cast<SkMScalar>(0.0));
Again, should this alter existing Ty components?

[reed1, 2011-06-09 19:22:47]

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (left):

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#oldcode22
ui/gfx/transform.cc:22: matrix_.setRotate(SkFloatToScalar(degree));
I think this can just be

matrix.setRotateDegreesAbout(degree, 0, 0, 1);
hasInverse_ = false;

SkMatrix44 only works if SkScalar==float, so there are no fixed-point
considerations. Plus, SkMScalar will either be float or double, either of which
will correctly handle pass a float (degree) and literals (0 and 1)

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#oldcode26
ui/gfx/transform.cc:26: matrix_.setScaleX(SkFloatToScalar(x));
For this and SetScaleY,Z, see my no-need-to-cast comments from SetRotate()

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7044062/diff/1/ui/gfx/transform.cc#newcode114
ui/gfx/transform.cc:114: SkMatrix44 translate;
setTranslate() by definition always overwrites the entire matrix, so there is no
need to call reset() first. The same is true for all of the setFoo() methods
(setScale, setRotate... , etc).

The methods beginning with pre... or post... by definition are modifying an
existing matrix, so their previous contents do matter.

All of that said, SkMatrix44 (unlike SkMatrix) is always initialized at
constructor time. Hence in this example...

SkMatrix44 matrix;
matrix.reset();

The reset() call is redundant.

[rjkroege, 2011-06-10 17:52:14]

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_gl.cc (right):

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_gl.cc:164: const SkMatrix44& matrix = t.matrix();
Given Ganesh, there should be a single call into SKMatrix44 that returs an array
pointer that you can pass to glUniformMatrix4fv.

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_win.cc:357: m.get(0,0), m.get(1,0), m.get(2,0),
m.get(3,0),
same comment as the GL case. Is there not a way to construct this without
copying all 16 values?

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/transform.cc#newcode89
ui/gfx/transform.cc:89: SkMatrix44 rot;
rot()? Make it clear you're invoking the default constructor? Same comment
elsewhere.

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/transform.cc#newcode145
ui/gfx/transform.cc:145: SkMatrix matrix = matrix_;
does this invoke the copy constructor? does it need to? Is mapRect const? Please
adjust its cont-ness as necessary

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/transform.h
File ui/gfx/transform.h (right):

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/transform.h#newcode60
ui/gfx/transform.h:60: void PreconcatTransform(const Transform& transform);
why did we lose the boolean prop?

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/transform.h#newcode93
ui/gfx/transform.h:93: SkMatrix44 inverse_;
time/space overhead of keeping the inverse?

[Ian Vollick, 2011-06-10 23:12:08]

Both sky and rjkroege mentioned the cached inverse. It was probably premature
optimization, but calculating the inverse isn't the cheapest operation, and it
seemed a waste not to cache it. A downside is that I don't think
TransformPointInverse and TransformRectInverse can be const unless hasInverse_
and inverse_ become mutable.

I'd be happy getting rid of the cached inverse -- please let me know if you
think that's the right thing to do.

Responses to the individual reviewers' comments:

sky:
 - fixed indenting of SymmetricRound
 - updated compositor_gl and compositor_win to use the raw data in SkMatrix44
 - Why SymmetricRound vs std::floor? I was trying to make it so that
TransformPoint returned the closest point represented with two integers.
 - Why static const arrays in the tests? No good reason (I was following another
example). Maybe it's that since they'd never change it'd be nice to only
initialize them once?  Not that these test functions are ever called more than
once..

wjmaclean:
 - I've updated compositor_gl.cc, and the code works with COMPOSITOR_2 defined
 - SetTranslateX no longer modifies the ty or tz elements. Same goes for the
other, related setters.

reed:
 - got rid of the unnecessary static casts and calls to SkMatrix44::reset

rjkroege:
 - gl and d3d are now passed the raw matrix data (this depends on a skia change
exposing the raw data)
 - [Pre]ConcatTransform no longer returns a bool because SkMatrix44's setConcat
doesn't do sanity checks to ensure that a 'perspective' matrix remains one, etc.
Also, since these functions just represent matrix multiplications, it seems
strange that they could fail

[sky, 2011-06-10 23:20:54]

On Fri, Jun 10, 2011 at 4:12 PM,  <vollick@chromium.org> wrote:
> Both sky and rjkroege mentioned the cached inverse. It was probably
> premature
> optimization, but calculating the inverse isn't the cheapest operation, and
> it
> seemed a waste not to cache it. A downside is that I don't think
> TransformPointInverse and TransformRectInverse can be const unless
> hasInverse_
> and inverse_ become mutable.
>
> I'd be happy getting rid of the cached inverse -- please let me know if you
> think that's the right thing to do.

Yes please. If we did want to cache then mutable is the way to go.
And, you would need to override the copy constructor and assignment
operator.

> Responses to the individual reviewers' comments:
>
> sky:
>  - fixed indenting of SymmetricRound
>  - updated compositor_gl and compositor_win to use the raw data in
> SkMatrix44
>  - Why SymmetricRound vs std::floor? I was trying to make it so that
> TransformPoint returned the closest point represented with two integers.

The problem is we're converting to an integer coordinate system
(views). I suspect if you do this we'll get incorrect hit detection.
I'm surprised this didn't break ConvertPointToViewWithTransform.

  -Scott

>  - Why static const arrays in the tests? No good reason (I was following
> another
> example). Maybe it's that since they'd never change it'd be nice to only
> initialize them once?  Not that these test functions are ever called more
> than
> once..
>
> wjmaclean:
>  - I've updated compositor_gl.cc, and the code works with COMPOSITOR_2
> defined
>  - SetTranslateX no longer modifies the ty or tz elements. Same goes for the
> other, related setters.
>
> reed:
>  - got rid of the unnecessary static casts and calls to SkMatrix44::reset
>
> rjkroege:
>  - gl and d3d are now passed the raw matrix data (this depends on a skia
> change
> exposing the raw data)
>  - [Pre]ConcatTransform no longer returns a bool because SkMatrix44's
> setConcat
> doesn't do sanity checks to ensure that a 'perspective' matrix remains one,
> etc.
> Also, since these functions just represent matrix multiplications, it seems
> strange that they could fail
>
>
> http://codereview.chromium.org/7044062/
>

[tomhudson, 2011-06-13 14:36:07]

On 2011/06/10 17:52:14, rjkroege wrote:

http://codereview.chromium.org/7044062/diff/2007/ui/gfx/compositor/compositor...
> ui/gfx/compositor/compositor_gl.cc:164: const SkMatrix44& matrix = t.matrix();
> Given Ganesh, there should be a single call into SKMatrix44 that returs an
array
> pointer that you can pass to glUniformMatrix4fv.

There's discussion is currently going on in response to the proposed Skia change
that is probably relevant to bring in here (or make sure reviewers to this patch
are cc'd on): http://codereview.appspot.com/4579050/.

Skia doesn't expose the internals. We're currently debating a change from floats
to doubles for 3x3 matrices, which would break lots of client code if we exposed
the internals. However, it looks like it'd save us about 2% of the total time in
representative Chrome profiles, because of the expense of converting from float
to double and back every time we do math where maintaining precision in
intermediate values is important. The time spent copying floats out of matrices
into GL doesn't even make a dent in any of the apps I've profiled in the last
two weeks.

[reed1, 2011-06-13 14:48:39]

skia rev. 1564 (soon to be rolled into chrome) provides getters for
row and col major floats and doubles. It also changes the default
storage to floats. Perhaps we could time against these APIs.

On Mon, Jun 13, 2011 at 10:36 AM,  <tomhudson@google.com> wrote:
> On 2011/06/10 17:52:14, rjkroege wrote:
>
>
http://codereview.chromium.org/7044062/diff/2007/ui/gfx/compositor/compositor...
>>
>> ui/gfx/compositor/compositor_gl.cc:164: const SkMatrix44& matrix =
>> t.matrix();
>> Given Ganesh, there should be a single call into SKMatrix44 that returs an
>
> array
>>
>> pointer that you can pass to glUniformMatrix4fv.
>
> There's discussion is currently going on in response to the proposed Skia
> change
> that is probably relevant to bring in here (or make sure reviewers to this
> patch
> are cc'd on): http://codereview.appspot.com/4579050/.
>
> Skia doesn't expose the internals. We're currently debating a change from
> floats
> to doubles for 3x3 matrices, which would break lots of client code if we
> exposed
> the internals. However, it looks like it'd save us about 2% of the total
> time in
> representative Chrome profiles, because of the expense of converting from
> float
> to double and back every time we do math where maintaining precision in
> intermediate values is important. The time spent copying floats out of
> matrices
> into GL doesn't even make a dent in any of the apps I've profiled in the
> last
> two weeks.
>
>
>
> http://codereview.chromium.org/7044062/
>

[Ian Vollick, 2011-06-13 17:47:51]

Using SkMatrix44::asColMajorf instead of a pointer to the internal data seems
like a good solution to me, too. I've updated compositor_gl.cc and
compositor_win.cc to use the new method.

[reed1, 2011-06-13 17:50:31]

I will try to roll the DEPS as soon as possible.

On Mon, Jun 13, 2011 at 1:47 PM,  <vollick@chromium.org> wrote:
> Using SkMatrix44::asColMajorf instead of a pointer to the internal data
> seems
> like a good solution to me, too. I've updated compositor_gl.cc and
> compositor_win.cc to use the new method.
>
> http://codereview.chromium.org/7044062/
>

[Ian Vollick, 2011-06-14 22:28:28]

reed:
 - this CL no longer contains code to roll skia

sky:
 - Was the concern with rounding that we might round outside of the screen's
boundaries? How would you feel about breaking the Transform interface so that
TransformPoint and TransformPointReverse operated on gfx::Point3f (a new point
class using floats that I would create) rather than gfx::Point? The consumers of
these functions could then decide to round them however is appropriate.

[sky, 2011-06-14 22:41:18]

On Tue, Jun 14, 2011 at 3:28 PM,  <vollick@chromium.org> wrote:
> reed:
>  - this CL no longer contains code to roll skia
>
> sky:
>  - Was the concern with rounding that we might round outside of the screen's
> boundaries? How would you feel about breaking the Transform interface so
> that
> TransformPoint and TransformPointReverse operated on gfx::Point3f (a new
> point
> class using floats that I would create) rather than gfx::Point? The
> consumers of
> these functions could then decide to round them however is appropriate.

Not the screens boundaries, but the views boundaries. Here's the case
I care about (from http://codereview.chromium.org/7066010):

'Lets say the root view has one child view at 50,50 with a
scale 5. Because the current code rounds down, anything in the range 46-49 of
the parent gets mapped to 0x0 in the child. This means that any mouse movements
from 46-49 get forwarded to the child even though it doesn't contain the point.
By rounding down instead of up for negative numbers we ensure 46-49 are treated
as -1 and don't get forwarded to the child.'

It seems like rounding like you're doing would reintroduce this problem.

  -Scott

> http://codereview.chromium.org/7044062/
>

[reed1, 2011-06-15 12:11:37]

SkMatrix44 has a companion SkVector4. Perhaps we could add a
Vector/Point3 as well to skia. Would that be as useful?

On Tue, Jun 14, 2011 at 6:28 PM,  <vollick@chromium.org> wrote:
> reed:
>  - this CL no longer contains code to roll skia
>
> sky:
>  - Was the concern with rounding that we might round outside of the screen's
> boundaries? How would you feel about breaking the Transform interface so
> that
> TransformPoint and TransformPointReverse operated on gfx::Point3f (a new
> point
> class using floats that I would create) rather than gfx::Point? The
> consumers of
> these functions could then decide to round them however is appropriate.
>
> http://codereview.chromium.org/7044062/
>

[vollick1, 2011-06-15 14:31:35]

I think we're in trouble if we convert to integers in object space at all,
either by flooring or rounding. Adding some more detail to your earlier
example, consider a square (top/left corner = (0,0), bottom right corner
(10, 10)) that is scaled up by five and translated by the vector (50,50).
Points (x,y) are in the transformed square if x,y \in [50, 100] in screen
space (or \in [0, 10] in object space). The transformation matrix is as
follows:
 [ 5 0 0 50 ]
 [ 0 5 0 50 ]
 [ 0 0 1 0  ]
 [ 0 0 0 1  ]
... and it's inverse is:
 [ .2 0  0 -10 ]
 [ 0  .2 0 -10 ]
 [ 0  0  1  0  ]
 [ 0  0  0  1  ]
If we do the usual thing and convert screen points into object space using
the inverse transform and then round, (48, 48) and (49, 49) will map to (0,
0) as you mentioned. The points (101, 101) and (102, 102) will map to (10,
10) and will be incorrectly placed in the rectangle, too. The problem gets a
bit worse on this side of the square if we use std::floor as (103, 103) and
(104, 104) will also map to (10, 10).

wjmaclean mentioned that rather than converting points to object space, we
could convert regions/views into screen space and do our hit testing in
screen coordinates. This gets around the rounding problem and it's very
reasonable especially if the regions started as rectangles -- even if they
are skewed or rotated.

That said, I think the easiest solution is to simply not round in object
space. This way, the hit testing logic can remain essentially the same.

If we were to go this route, we'd need a 3d Point/Vector of floats/doubles,
but I don't know if we should use a 3d point from Skia or add one in gfx::.
I thought that we'd want a 3d point in gfx:: since we're using gfx::Point
rather than a Skia point, but I could be wrong about that.

On Wed, Jun 15, 2011 at 8:11 AM, Mike Reed <reed@google.com> wrote:

> SkMatrix44 has a companion SkVector4. Perhaps we could add a
> Vector/Point3 as well to skia. Would that be as useful?
>
> On Tue, Jun 14, 2011 at 6:28 PM,  <vollick@chromium.org> wrote:
> > reed:
> >  - this CL no longer contains code to roll skia
> >
> > sky:
> >  - Was the concern with rounding that we might round outside of the
> screen's
> > boundaries? How would you feel about breaking the Transform interface so
> > that
> > TransformPoint and TransformPointReverse operated on gfx::Point3f (a new
> > point
> > class using floats that I would create) rather than gfx::Point? The
> > consumers of
> > these functions could then decide to round them however is appropriate.
> >
> > http://codereview.chromium.org/7044062/
> >
>

[sky, 2011-06-15 17:26:21]

On Wed, Jun 15, 2011 at 7:31 AM, Ian Vollick <vollick@google.com> wrote:
> I think we're in trouble if we convert to integers in object space at all,
> either by flooring or rounding. Adding some more detail to your earlier
> example, consider a square (top/left corner = (0,0), bottom right corner
> (10, 10)) that is scaled up by five and translated by the vector (50,50).
> Points (x,y) are in the transformed square if x,y \in [50, 100] in screen
> space (or \in [0, 10] in object space). The transformation matrix is as
> follows:
>  [ 5 0 0 50 ]
>  [ 0 5 0 50 ]
>  [ 0 0 1 0  ]
>  [ 0 0 0 1  ]
> ... and it's inverse is:
>  [ .2 0  0 -10 ]
>  [ 0  .2 0 -10 ]
>  [ 0  0  1  0  ]
>  [ 0  0  0  1  ]
> If we do the usual thing and convert screen points into object space using
> the inverse transform and then round, (48, 48) and (49, 49) will map to (0,
> 0) as you mentioned.

If you use floor, don't these end at -1,-1?

> The points (101, 101) and (102, 102) will map to (10,
> 10) and will be incorrectly placed in the rectangle, too.

Yep. If I remember correctly those points were rejected for some other
reason. I would have to look.

>  The problem gets a
> bit worse on this side of the square if we use std::floor as (103, 103) and
> (104, 104) will also map to (10, 10).
> wjmaclean mentioned that rather than converting points to object space, we
> could convert regions/views into screen space and do our hit testing in
> screen coordinates. This gets around the rounding problem and it's very
> reasonable especially if the regions started as rectangles -- even if they
> are skewed or rotated.
> That said, I think the easiest solution is to simply not round in object
> space. This way, the hit testing logic can remain essentially the same.
> If we were to go this route, we'd need a 3d Point/Vector of floats/doubles,
> but I don't know if we should use a 3d point from Skia or add one in gfx::.
> I thought that we'd want a 3d point in gfx:: since we're using gfx::Point
> rather than a Skia point, but I could be wrong about that.

Easier said than done. I think that'll be hard to integrated into the
current code.

I don't have a feel for what the right answer is here, but do agree
it's a bit broke right now.

  -Scott

> On Wed, Jun 15, 2011 at 8:11 AM, Mike Reed <reed@google.com> wrote:
>>
>> SkMatrix44 has a companion SkVector4. Perhaps we could add a
>> Vector/Point3 as well to skia. Would that be as useful?
>>
>> On Tue, Jun 14, 2011 at 6:28 PM,  <vollick@chromium.org> wrote:
>> > reed:
>> >  - this CL no longer contains code to roll skia
>> >
>> > sky:
>> >  - Was the concern with rounding that we might round outside of the
>> > screen's
>> > boundaries? How would you feel about breaking the Transform interface so
>> > that
>> > TransformPoint and TransformPointReverse operated on gfx::Point3f (a new
>> > point
>> > class using floats that I would create) rather than gfx::Point? The
>> > consumers of
>> > these functions could then decide to round them however is appropriate.
>> >
>> > http://codereview.chromium.org/7044062/
>> >
>
>

[Ian Vollick, 2011-06-16 21:43:30]

sky:
 - view now converts to floating point coordinates before transforming and then
converts back right after using floor, retaining the current behavior.

reed:
 - I've added gfx::Point3<T> to match gfx::Point, but it's not used anywhere
directly. Only the typedef'd Point3f and Point3d are used -- these typedefs
could be pointed at Skia points/vectors in the future, hopefully with minimal
impact.

[sky, 2011-06-16 23:06:56]

http://codereview.chromium.org/7044062/diff/16001/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/7044062/diff/16001/skia/skia.gyp#newcode512
skia/skia.gyp:512: '../third_party/skia/include/utils/SkMatrix44.h',
Don't include these on the mac.

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_win.cc:355:
transform.matrix().asColMajorf(transform_data_buffer);
I can't remember which is which. Did you make sure d3d uses column major?

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h
File ui/gfx/point3.h (right):

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode11
ui/gfx/point3.h:11: #include "ui/gfx/point.h"
newline between 10 and 11.

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode16
ui/gfx/point3.h:16: template <typename T>
Do we really need a template here? Do we plan on using both float and double?

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode22
ui/gfx/point3.h:22: data_[0] = x;
Use member initializer list.

http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode27
ui/gfx/point3.h:27: Point3(const Point3<T>& other) {
See style guide, we generally don't use copy constructor/assignment operator
(http://www.corp.google.com/eng/doc/cppguide.xml#Copy_Constructors) or operator
overloading
(http://www.corp.google.com/eng/doc/cppguide.xml#Operator_Overloading).

[vollick1, 2011-06-17 16:04:08]

>
> http://codereview.chromium.**org/7044062/diff/16001/skia/**
>
skia.gyp#newcode512<http://codereview.chromium.org/7044062/diff/16001/skia/skia.gyp#newcode512>
> skia/skia.gyp:512: '../third_party/skia/include/**utils/SkMatrix44.h',
> Don't include these on the mac.
>

Fixed (the source is only included when OS != 'mac')


> http://codereview.chromium.**org/7044062/diff/16001/ui/gfx/**
>
compositor/compositor_win.cc#**newcode355<http://codereview.chromium.org/7044062/diff/16001/ui/gfx/compositor/compositor_win.cc#newcode355>
> ui/gfx/compositor/compositor_**win.cc:355:
> transform.matrix().**asColMajorf(transform_data_**buffer);
> I can't remember which is which. Did you make sure d3d uses column
> major?
>
> Fixed. Thank you for catching this; d3d uses row major.


> http://codereview.chromium.**org/7044062/diff/16001/ui/gfx/**
>
point3.h#newcode11<http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode11>
> ui/gfx/point3.h:11: #include "ui/gfx/point.h"
> newline between 10 and 11.
>
Done.


>
> http://codereview.chromium.**org/7044062/diff/16001/ui/gfx/**
>
point3.h#newcode16<http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode16>
> ui/gfx/point3.h:16: template <typename T>
> Do we really need a template here? Do we plan on using both float and
> double?
>
Maybe not, but I thought it would be ok to avoid hard coding the type given
how simple the template is. I could certainly be wrong. Please let me know
if I should de-templatize the class.


>
> http://codereview.chromium.**org/7044062/diff/16001/ui/gfx/**
>
point3.h#newcode22<http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode22>
> ui/gfx/point3.h:22: data_[0] = x;
> Use member initializer list.
>
Done.


>
> http://codereview.chromium.**org/7044062/diff/16001/ui/gfx/**
>
point3.h#newcode27<http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode27>
> ui/gfx/point3.h:27: Point3(const Point3<T>& other) {
> See style guide, we generally don't use copy constructor/assignment
> operator
>
(http://www.corp.google.com/**eng/doc/cppguide.xml#Copy_**Constructors<http://...)
> or
> operator overloading
>
(http://www.corp.google.com/**eng/doc/cppguide.xml#Operator_**Overloading<http...
> ).
>
> Done -- copy and assign are disallowed and there is no operator
overloading.

[sky, 2011-06-17 16:38:21]

>> http://codereview.chromium.org/7044062/diff/16001/ui/gfx/point3.h#newcode16
>> ui/gfx/point3.h:16: template <typename T>
>> Do we really need a template here? Do we plan on using both float and
>> double?
>
> Maybe not, but I thought it would be ok to avoid hard coding the type given
> how simple the template is. I could certainly be wrong. Please let me know
> if I should de-templatize the class.

On the down side, going with the template makes the code harder to
read, and anyone that wants to use Point3 now has to have template
versions (Transform is now very hard to follow).  I say go with floats
until we find we need it.

  -Scott

[sky, 2011-06-17 16:38:23]

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h
File ui/gfx/point3.h (right):

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode21
ui/gfx/point3.h:21: : x_(0), y_(0), z_(0)
Move the member initializer list to 20.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode24
ui/gfx/point3.h:24: Point3(const T& x, const T& y, const T& z)
We generally don't use references for primitive types.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode25
ui/gfx/point3.h:25: : x_(x), y_(y), z_(z)
If you can't fit the constructor and member initializer list on a single line,
then each member goes on its own line (like 29). But, you need to indent 4 (not
2).

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode29
ui/gfx/point3.h:29: : x_(point.x())
trailing commas, not leading. And 4 space indent.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode36
ui/gfx/point3.h:36: T x() { return x_; }
These functions should be const.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode44
ui/gfx/point3.h:44: void SetPoint(const T& x, const T& y, const T& z) {
no reference.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode62
ui/gfx/point3.h:62: void Multiply(const T& s) {
no reference.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode68
ui/gfx/point3.h:68: T Dist2(const Point3<T>& other) const {
Add a description as what this does isn't obvious from the name.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode77
ui/gfx/point3.h:77: static_cast<int>(std::floor(x_)),
77/78 should be 4 space indented. Or better yet, align with ( on 76.

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode86
ui/gfx/point3.h:86: DISALLOW_COPY_AND_ASSIGN(Point3);
I know I mentioned not enabling assignment operator/copy constructor but as this
class is cheap you may as well remove the macro and allow the class to copied
(gfx::Point does this).

[wjmaclean, 2011-06-17 16:58:06]

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h
File ui/gfx/point3.h (right):

http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode68
ui/gfx/point3.h:68: T Dist2(const Point3<T>& other) const {
On 2011/06/17 16:38:23, sky wrote:
> Add a description as what this does isn't obvious from the name.

Rename "DistanceSquared"? "NormSquared"? (Or use "Sqr" where I've used
"Squared")

[vollick1, 2011-06-17 17:29:03]

I got rid of all templates. Hopefully the code is easier to read.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode21<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode21>
> ui/gfx/point3.h:21: : x_(0), y_(0), z_(0)
> Move the member initializer list to 20.
>
> Done.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode24<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode24>
> ui/gfx/point3.h:24: Point3(const T& x, const T& y, const T& z)
> We generally don't use references for primitive types.
>
> Fixed.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode25<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode25>
> ui/gfx/point3.h:25: : x_(x), y_(y), z_(z)
> If you can't fit the constructor and member initializer list on a single
> line, then each member goes on its own line (like 29). But, you need to
> indent 4 (not 2).
>
> Done.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode29<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode29>
> ui/gfx/point3.h:29: : x_(point.x())
> trailing commas, not leading. And 4 space indent.
>
> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode36<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode36>
> ui/gfx/point3.h:36: T x() { return x_; }
> These functions should be const.
>
> Done.

> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode44<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode44>
> ui/gfx/point3.h:44: void SetPoint(const T& x, const T& y, const T& z) {
> no reference.
>
> Done.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode62<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode62>
> ui/gfx/point3.h:62: void Multiply(const T& s) {
> no reference.
>
> Done


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode68<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode68>
> ui/gfx/point3.h:68: T Dist2(const Point3<T>& other) const {
> Add a description as what this does isn't obvious from the name.
>
>
Added a comment and renamed the function 'SquaredDistanceTo(other)'.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode77<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode77>
> ui/gfx/point3.h:77: static_cast<int>(std::floor(x_**)),
> 77/78 should be 4 space indented. Or better yet, align with ( on 76.
>
> Fixed.


> http://codereview.chromium.**org/7044062/diff/29001/ui/gfx/**
>
point3.h#newcode86<http://codereview.chromium.org/7044062/diff/29001/ui/gfx/point3.h#newcode86>
> ui/gfx/point3.h:86: DISALLOW_COPY_AND_ASSIGN(**Point3);
> I know I mentioned not enabling assignment operator/copy constructor but
> as this class is cheap you may as well remove the macro and allow the
> class to copied (gfx::Point does this).
>
> Can copy and assign.


>
>
http://codereview.chromium.**org/7044062/<http://codereview.chromium.org/7044...
>

[sky, 2011-06-17 18:43:41]

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/point3.h
File ui/gfx/point3.h (right):

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/point3.h#newcode19
ui/gfx/point3.h:19: {}
Either { on the previous line, or {} on the previous line, but not {} on this
line.

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/point3.h#newcode27
ui/gfx/point3.h:27: Point3f(const Point3f& other) : x_(other.x_), y_(other.y_),
z_(other.z_)
These aren't going to fit when you move { onto 27, so, make sure you do:
Point3f(const Point3f& other)
    : x_(other.x_),
      y_ ...

That said, do even need to define this? Won't the one generated for you work
fine?

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/point3.h#newcode32
ui/gfx/point3.h:32: Point3f& operator=(const Point3f& other) {
Same thing on this, can't you use the one generated by the compiler?

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/point3.h#newcode61
ui/gfx/point3.h:61: Point AsPoint() {
const

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform.cc#newcode109
ui/gfx/transform.cc:109: bool Transform::TransformRect(gfx::Rect* rect) const {
position of functions in .cc should match that of .h.

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform.cc#newcode132
ui/gfx/transform.cc:132: return true;
For the ones that always return true, make them void.

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform_unittest.c...
ui/gfx/transform_unittest.cc:23: #ifdef NDEBUG
Use #if defined().
I think you want:
#if !defined(NDEBUG)
though

What is the point of this though?

http://codereview.chromium.org/7044062/diff/25002/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

http://codereview.chromium.org/7044062/diff/25002/ui/ui_unittests.gypi#newcode52
ui/ui_unittests.gypi:52: 'gfx/transform_unittest.cc',
Make sure this isn't compiled on mac, same goes for transform.

[Ian Vollick, 2011-06-17 19:17:57]

sky: as you mentioned, the default copy ctor and assignment operator are fine.
Removed mine.

> http://codereview.chromium.org/7044062/diff/25002/ui/gfx/point3.h#newcode19
> ui/gfx/point3.h:19: {}
> Either { on the previous line, or {} on the previous line, but not {} on this
> line.

fixed

> 
>
http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform.cc#newcode109
> ui/gfx/transform.cc:109: bool Transform::TransformRect(gfx::Rect* rect) const
{
> position of functions in .cc should match that of .h.

fixed

> 
>
http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform.cc#newcode132
> ui/gfx/transform.cc:132: return true;
> For the ones that always return true, make them void.

fixed
>
http://codereview.chromium.org/7044062/diff/25002/ui/gfx/transform_unittest.c...
> ui/gfx/transform_unittest.cc:23: #ifdef NDEBUG
> Use #if defined().
> I think you want:
> #if !defined(NDEBUG)
> though
> 
> What is the point of this though?

Was using this to measure performance of Eigen when built in release mode. No
longer serves a purpose. Removed.

> 
> http://codereview.chromium.org/7044062/diff/25002/ui/ui_unittests.gypi
> File ui/ui_unittests.gypi (right):
> 
>
http://codereview.chromium.org/7044062/diff/25002/ui/ui_unittests.gypi#newcode52
> ui/ui_unittests.gypi:52: 'gfx/transform_unittest.cc',
> Make sure this isn't compiled on mac, same goes for transform.

Fixed: ui_unittests.gypi and ui_gfx.gyp only include transform related code on
non-mac OS's

[sky, 2011-06-17 19:41:24]

LGTM

[Ian Vollick, 2011-06-24 15:39:52]

Updated compositor_win.cc after testing on Windows. Also updated
views_unittest.cc. Note: this test will not pass presently because rotating by
multiples of 90 degrees doesn't result in a 'clean' rotation matrix (matrix ends
up with some entries that are very close to, but not quite, zero). Skia patch
4631068 should address this.

[sky, 2011-06-24 16:03:35]

Can you add operator==. Also, you'll need to sync up again and change
LayerAnimator. Sorry.

[Ian Vollick, 2011-06-24 20:18:47]

On 2011/06/24 16:03:35, sky wrote:
> Can you add operator==. Also, you'll need to sync up again and change
> LayerAnimator. Sorry.

Done.

[sky, 2011-06-24 20:58:19]

LGTM with the following fixed.

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_win.cc:344:
std::swap(transform_matrix._12,transform_matrix._21);
nit: space after ',' on all these lines.

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform.cc#newcode154
ui/gfx/transform.cc:154: point.x(),
Do these need to be wrapped in SkFloatToScalar()?

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform.cc#newcode162
ui/gfx/transform.cc:162: point.SetPoint(p[0]/p[3], p[1]/p[3], p[2]/p[3]);
nit: add whitespace before/after the '/'

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
ui/gfx/transform_unittest.cc:9: #include <iostream>
The order of includes should match that of other files:
header first (ui/gfx/transform.h)
newline
system includes
newline
other includes.

http://codereview.chromium.org/7044062/diff/41001/views/view_unittest.cc
File views/view_unittest.cc (right):

http://codereview.chromium.org/7044062/diff/41001/views/view_unittest.cc#newc...
views/view_unittest.cc:1863: SkMatrix matrix = transform.matrix();
const SkMatrix& here and other places.

[tfarina, 2011-06-25 00:19:19]

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
ui/gfx/transform_unittest.cc:1: // Copyright (c) 2006-2011 The Chromium Authors.
All rights reserved.
nit: 2006-2011 -> 2011

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
ui/gfx/transform_unittest.cc:12: typedef testing::Test XFormTest;
If I'm not wrong, you don't need this typedef. You can use XFormTest directly
with TEST() macro.

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
ui/gfx/transform_unittest.cc:22: } // anonymous namespace
Technically speaking this is not an anonymous namespace, but an unnamed
namespace. Please, just remove the "anonymous" part, i.e:
namespace {
...
}  // namespace

Even better, you can wrap the tests below into the unnamed namespace if you want
too.

http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
ui/gfx/transform_unittest.cc:24: TEST(XFormTest, Equality) {
Please, wrap the tests into an unnamed namespace as suggested above or into the
ui namespace.

[Ian Vollick, 2011-07-04 19:00:08]

>
http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
> ui/gfx/transform_unittest.cc:1: // Copyright (c) 2006-2011 The Chromium
Authors.
> All rights reserved.
> nit: 2006-2011 -> 2011

Fixed.

> 
>
http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
> ui/gfx/transform_unittest.cc:12: typedef testing::Test XFormTest;
> If I'm not wrong, you don't need this typedef. You can use XFormTest directly
> with TEST() macro.

Fixed.

> 
>
http://codereview.chromium.org/7044062/diff/41001/ui/gfx/transform_unittest.c...
> ui/gfx/transform_unittest.cc:22: } // anonymous namespace
> Technically speaking this is not an anonymous namespace, but an unnamed
> namespace. Please, just remove the "anonymous" part, i.e:
> namespace {
> ...
> }  // namespace
> 
> Even better, you can wrap the tests below into the unnamed namespace if you
want
> too.

Done. Tests are all in the unnamed namespace.

[tomhudson, 2011-07-06 14:18:15]

http://codereview.chromium.org/7044062/diff/62001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/7044062/diff/62001/ui/gfx/canvas.h#newcode217
ui/gfx/canvas.h:217: #if !defined(OS_MACOSX)
OK, after so much work has been done it feels like I'm coming late to the party,
but - why #ifdef out this one function on Mac? We know Skia-as-Mac-backend isn't
widely distributed yet (although it's in the codebase as a compile flag now and
we hope to ship it c. M15), but is there some reason the rest of the class
compiles and this one function doesn't? It seems like clients would have to
contort their use of this class in a way that they're going to have to rip out
when Skia is the Mac backend.

[vollick1, 2011-07-06 14:52:13]

Hi Tom,

Transform and SkMatrix44 were not to be built as part of the mac build and
#ifdef'ing out this function was a simple way of getting the canvas code to
compile on the mac (this function isn't used on the mac right now). When the
need arises, I think Transform, SkMatrix44 and this function can all be
pulled back in pretty easily.

Ian

On Wed, Jul 6, 2011 at 10:18 AM, <tomhudson@google.com> wrote:

>
>
http://codereview.chromium.**org/7044062/diff/62001/ui/gfx/**canvas.h<http://...
> File ui/gfx/canvas.h (right):
>
> http://codereview.chromium.**org/7044062/diff/62001/ui/gfx/**
>
canvas.h#newcode217<http://codereview.chromium.org/7044062/diff/62001/ui/gfx/canvas.h#newcode217>
> ui/gfx/canvas.h:217: #if !defined(OS_MACOSX)
> OK, after so much work has been done it feels like I'm coming late to
> the party, but - why #ifdef out this one function on Mac? We know
> Skia-as-Mac-backend isn't widely distributed yet (although it's in the
> codebase as a compile flag now and we hope to ship it c. M15), but is
> there some reason the rest of the class compiles and this one function
> doesn't? It seems like clients would have to contort their use of this
> class in a way that they're going to have to rip out when Skia is the
> Mac backend.
>
>
>
http://codereview.chromium.**org/7044062/<http://codereview.chromium.org/7044...
>

[tomhudson, 2011-07-06 15:43:11]

On 2011/07/06 14:52:13, vollick_google.com wrote:
> Hi Tom,
> 
> Transform and SkMatrix44 were not to be built as part of the mac build and
> #ifdef'ing out this function was a simple way of getting the canvas code to
> compile on the mac (this function isn't used on the mac right now). When the
> need arises, I think Transform, SkMatrix44 and this function can all be
> pulled back in pretty easily.

Ian,

I'd just like to understand *why* these things don't belong in the Mac build,
and how the interface makes sense if the Mac clients can use the class without
calling transform(), while non-Mac clients need to call transform().

[sky, 2011-07-06 15:49:21]

On Wed, Jul 6, 2011 at 8:43 AM,  <tomhudson@google.com> wrote:
> On 2011/07/06 14:52:13, vollick_google.com wrote:
>>
>> Hi Tom,
>
>> Transform and SkMatrix44 were not to be built as part of the mac build and
>> #ifdef'ing out this function was a simple way of getting the canvas code
>> to
>> compile on the mac (this function isn't used on the mac right now). When
>> the
>> need arises, I think Transform, SkMatrix44 and this function can all be
>> pulled back in pretty easily.
>
> Ian,
>
> I'd just like to understand *why* these things don't belong in the Mac
> build,
> and how the interface makes sense if the Mac clients can use the class
> without
> calling transform(), while non-Mac clients need to call transform().
>
> http://codereview.chromium.org/7044062/
>

The code that uses this is currently windows/linux specific, which is
why it is ifdef'd there. If someone has a need for it on the mac side,
then we can make it be built there.

  -Scott