Interpolated transforms

Added interpolated transforms

Landed @ 91145

BUG=
TEST=ui_unittests

[wjmaclean, 2011-06-27 19:13:17]

Minor nits, toherwise LGTM

http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
File ui/gfx/interpolated_transform.cc (right):

http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:22: end_time_(end_time) {
DCHECK(end_time > start_time)

http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:37: if (time <= start_time_)
time < start_time_ ?

http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
File ui/gfx/interpolated_transform_unittest.cc (right):

http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:12: const static float EPSILON = 1e-6;
This seems a little large, even for float. How did you chose this value?

http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:19: return fabs(lhs - rhs) /
std::max(fabs(rhs), fabs(rhs)) < EPSILON;
Why fabs(rhs) twice?

[Ian Vollick, 2011-06-27 19:48:48]

On 2011/06/27 19:13:17, wjmaclean wrote:
> Minor nits, toherwise LGTM
> 
>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:22: end_time_(end_time) {
> DCHECK(end_time > start_time)
>
Added -- although I let it be ok for end_time == start_time so that you could
effectively have a step function. 

> ui/gfx/interpolated_transform.cc:37: if (time <= start_time_)
> time < start_time_ ?
>
Done. 
>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:12: const static float EPSILON =
1e-6;
> This seems a little large, even for float. How did you chose this value?
> 

Reading
http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm , 1e-6
seems like an ok tolerance for relative error. Might be wrong on this -- it's an
unfamiliar area for me.

>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:19: return fabs(lhs - rhs) /
> std::max(fabs(rhs), fabs(rhs)) < EPSILON;
> Why fabs(rhs) twice?

Typo. Fixed.

[wjmaclean, 2011-06-27 20:05:46]

On 2011/06/27 19:48:48, vollick wrote:
> On 2011/06/27 19:13:17, wjmaclean wrote:
> > Minor nits, toherwise LGTM
> > 
> >
>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
> > ui/gfx/interpolated_transform.cc:22: end_time_(end_time) {
> > DCHECK(end_time > start_time)
> >
> Added -- although I let it be ok for end_time == start_time so that you could
> effectively have a step function. 

Your code will handle +inf on line 53 then?

> > ui/gfx/interpolated_transform.cc:37: if (time <= start_time_)
> > time < start_time_ ?
> >
> Done. 
> >
>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
> > ui/gfx/interpolated_transform_unittest.cc:12: const static float EPSILON =
> 1e-6;
> > This seems a little large, even for float. How did you chose this value?
> > 
> 
> Reading
> http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm ,
1e-6
> seems like an ok tolerance for relative error. Might be wrong on this -- it's
an
> unfamiliar area for me.
> 

I've often used 1E-8 for this kind of work, but we can experiment.

[Ian Vollick, 2011-06-27 20:19:03]

>
http://codereview.chromium.org/7262024/diff/2001/ui/gfx/interpolated_transfor...
> > > ui/gfx/interpolated_transform.cc:22: end_time_(end_time) {
> > > DCHECK(end_time > start_time)
> > >
> > Added -- although I let it be ok for end_time == start_time so that you
could
> > effectively have a step function. 
> 
> Your code will handle +inf on line 53 then?
> 

Should never get there. If none of the values are NaN, then if start_time ==
end_time == x, then time < x or time >= x, so we'll bail early. Added comments
to this effect and added more NaN checks.

[Ian Vollick, 2011-06-28 13:07:22]

+sky

[sky, 2011-06-28 15:39:14]

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
File ui/gfx/interpolated_transform.cc (right):

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:17: : start_time_(0.0f),
nit: 4 space indent (same comment for all constructors you have).

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:38: {
nit: { on previous line.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:44: if (time != time ||
nit: fit as much as you can on one line.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:91: 
nit: remove newline.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:195: ui::Transform toPivot;
unix hacker style: to_pivot/from_pivot.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:200: scoped_ptr<InterpolatedTransform>
preTransform(
pre_transform/post_transform.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transform.h
File ui/gfx/interpolated_transform.h (right):

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:15: class InterpolatedTransform {
Add a description of this class. Same comment for all classes.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:18: InterpolatedTransform(float start_time,
float end_time);
Document what start_time and end_time are.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:19: virtual ~InterpolatedTransform() {}
Don't inline this.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:25: void Compose(InterpolatedTransform* other) {
child_.reset(other); }
Don't inline this since it may run destructors. Also, the method doesn't really
indicate it takes ownership. SetChild is much clearer.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:28: virtual ui::Transform
InterpolateButDoNotCompose(float t) const = 0;
Description.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:30: float ValueBetween(float time, float
start_value, float end_value) const;
description.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:36: scoped_ptr<InterpolatedTransform> child_;
Description.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:118: InterpolatedTransform* transform);
Document ownership of transform (and for next method).

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
File ui/gfx/interpolated_transform_unittest.cc (right):

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:35: 0, 100, 100, 200);
nit: indent 4.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:50: 0, 100, 100, 200);
nit: indent 4.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:67: gfx::Point(0, 0), gfx::Point(100,
100), 100, 200);
nit: indent 4.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:84: pivot,
nit: indent 4.

http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform_unittest.cc:101: pivot,
nit: indent 4.

[Ian Vollick, 2011-06-28 18:06:06]

Summary:
 - Added description of all classes and the functions in the base class.
 - Renamed Compose ==> SetChild
 - fixed formatting

>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:17: : start_time_(0.0f),
> nit: 4 space indent (same comment for all constructors you have).
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:38: {
> nit: { on previous line.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:44: if (time != time ||
> nit: fit as much as you can on one line.
> 
All on one line.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:91: 
> nit: remove newline.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:195: ui::Transform toPivot;
> unix hacker style: to_pivot/from_pivot.
> 
No more camel case.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.cc:200: scoped_ptr<InterpolatedTransform>
> preTransform(
> pre_transform/post_transform.
> 
No more camel case.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:15: class InterpolatedTransform {
> Add a description of this class. Same comment for all classes.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:18: InterpolatedTransform(float start_time,
> float end_time);
> Document what start_time and end_time are.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:19: virtual ~InterpolatedTransform() {}
> Don't inline this.
> 
Destructor definitions are in the .cc file.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:25: void Compose(InterpolatedTransform* other)
{
> child_.reset(other); }
> Don't inline this since it may run destructors. Also, the method doesn't
really
> indicate it takes ownership. SetChild is much clearer.
> 
Renamed Compose ==> SetChild
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:28: virtual ui::Transform
> InterpolateButDoNotCompose(float t) const = 0;
> Description.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:30: float ValueBetween(float time, float
> start_value, float end_value) const;
> description.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:36: scoped_ptr<InterpolatedTransform> child_;
> Description.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform.h:118: InterpolatedTransform* transform);
> Document ownership of transform (and for next method).
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:35: 0, 100, 100, 200);
> nit: indent 4.
> 
Done
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:50: 0, 100, 100, 200);
> nit: indent 4.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:67: gfx::Point(0, 0),
gfx::Point(100,
> 100), 100, 200);
> nit: indent 4.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:84: pivot,
> nit: indent 4.
> 
Done.
>
http://codereview.chromium.org/7262024/diff/7002/ui/gfx/interpolated_transfor...
> ui/gfx/interpolated_transform_unittest.cc:101: pivot,
> nit: indent 4.

Done.

[sky, 2011-06-28 18:43:46]

LGTM

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
File ui/gfx/interpolated_transform.cc (right):

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.cc:148: //TODO(vollick) 3d xforms.
nit: '//TODO' -> '// TODO'

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transform.h
File ui/gfx/interpolated_transform.h (right):

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:47: // If time \in (start_time_, end_time_],
this function linearly interpolates
\in -> in?

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:56: float start_time_;
const

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:87: float start_degrees_;
const

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:112: float start_scale_;
const

http://codereview.chromium.org/7262024/diff/9009/ui/gfx/interpolated_transfor...
ui/gfx/interpolated_transform.h:156: ui::Transform transform_;
const

[Paweł Hajdan Jr., 2011-07-18 23:14:18]

Drive-by (chrome/test/OWNERS, returning from vacation).

http://codereview.chromium.org/7262024/diff/11001/ui/gfx/interpolated_transfo...
File ui/gfx/interpolated_transform_unittest.cc (right):

http://codereview.chromium.org/7262024/diff/11001/ui/gfx/interpolated_transfo...
ui/gfx/interpolated_transform_unittest.cc:14: bool ApproximatelyEqual(float lhs,
float rhs) {
Did you possibly just want
http://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Floating-Point_Co...
?

That could help you avoid re-inventing the wheel. Could you please do a
follow-up CL?