Provide approximate type functions for SkMatrix44.

Provide approximate type functions for SkMatrix44.

Skia's SkMatrix44 class provides methods for computing the type of a matrix
using exact computations, i.e. matrix values must be of exactly some value.
However, in some cases some inaccuracy can be tolerated.

This patch adds an implementation of IsApproximatelyIdentityOrTranslation()
as part of the gfx::Transform class. This replaces the same computation in
picture_layer_tiling.cc.

Should any other approximate type computations on gfx::Transform matrices be
necessary they should be added to gfx::Transform directly.

BUG=293787
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229609

[Dominik Grewe, 2013-10-15 14:03:39]

PTAL.

The approximate computation introduced by epenner@ in http://crrev.com/19271005
seems to have been removed. I've emailed him to check.

[danakj, 2013-10-15 15:34:35]

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode22
cc/base/math_util.cc:22: static inline bool
SkMScalarApproximatelyEqual(SkMScalar x, SkMScalar y,
can you drop the SkMScalar prefix from these methods? That makes them sound like
they live in the Skia namespace (which doesn't have an actual c++ namespace).

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode25
cc/base/math_util.cc:25: return SkTAbs<SkMScalar>(x-y) <= tolerance;
use std::abs()

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode28
cc/base/math_util.cc:28: static inline bool
SkMScalarApproximatelyInteger(SkMScalar x,
drop the anonymous namespace, or drop the "static" they are redundant.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode30
cc/base/math_util.cc:30: SkMScalar nearestInteger = std::floor(x +
SkDoubleToMScalar(0.5));
how about just "0.5f" instead of "SkDoubleToMScalar(0.5)"

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode30
cc/base/math_util.cc:30: SkMScalar nearestInteger = std::floor(x +
SkDoubleToMScalar(0.5));
nearest_integer

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode611
cc/base/math_util.cc:611: bool
MathUtil::IsMatrixApproximatelyPureTranslation(const SkMatrix44& matrix,
Can you make these methods take a gfx::Transform instead of a SkMatrix44? Then
the callsites do not need to unwrap it.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode614
cc/base/math_util.cc:614: SkMScalarApproximatelyEqual(matrix.get(0, 0), 1,
tolerance) &&
1.f (don't use integer literals for floating point) here and below.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.h
File cc/base/math_util.h (right):

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.h#newcode184
cc/base/math_util.h:184: static bool IsMatrixApproximatelyPureTranslation(const
SkMatrix44& matrix,
s/Pure//

[Dominik Grewe, 2013-10-15 17:04:19]

Thanks for the feedback!

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode22
cc/base/math_util.cc:22: static inline bool
SkMScalarApproximatelyEqual(SkMScalar x, SkMScalar y,
On 2013/10/15 15:34:35, danakj wrote:
> can you drop the SkMScalar prefix from these methods? That makes them sound
like
> they live in the Skia namespace (which doesn't have an actual c++ namespace).

Done.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode25
cc/base/math_util.cc:25: return SkTAbs<SkMScalar>(x-y) <= tolerance;
On 2013/10/15 15:34:35, danakj wrote:
> use std::abs()

Done.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode28
cc/base/math_util.cc:28: static inline bool
SkMScalarApproximatelyInteger(SkMScalar x,
On 2013/10/15 15:34:35, danakj wrote:
> drop the anonymous namespace, or drop the "static" they are redundant.

Done.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode30
cc/base/math_util.cc:30: SkMScalar nearestInteger = std::floor(x +
SkDoubleToMScalar(0.5));
On 2013/10/15 15:34:35, danakj wrote:
> nearest_integer

Done.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode30
cc/base/math_util.cc:30: SkMScalar nearestInteger = std::floor(x +
SkDoubleToMScalar(0.5));
On 2013/10/15 15:34:35, danakj wrote:
> how about just "0.5f" instead of "SkDoubleToMScalar(0.5)"

I wanted to keep it independent of whether SkMScalar is float or double (afaik
it's double for now).

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode611
cc/base/math_util.cc:611: bool
MathUtil::IsMatrixApproximatelyPureTranslation(const SkMatrix44& matrix,
On 2013/10/15 15:34:35, danakj wrote:
> Can you make these methods take a gfx::Transform instead of a SkMatrix44? Then
> the callsites do not need to unwrap it.

How about overloading it for both types? Would make testing easier as well.

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.cc#newcode614
cc/base/math_util.cc:614: SkMScalarApproximatelyEqual(matrix.get(0, 0), 1,
tolerance) &&
On 2013/10/15 15:34:35, danakj wrote:
> 1.f (don't use integer literals for floating point) here and below.

How about "SkFloatToMScalar(1.f)" to keep it independent of SkMScalar?

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.h
File cc/base/math_util.h (right):

https://codereview.chromium.org/27223008/diff/1/cc/base/math_util.h#newcode184
cc/base/math_util.h:184: static bool IsMatrixApproximatelyPureTranslation(const
SkMatrix44& matrix,
On 2013/10/15 15:34:35, danakj wrote:
> s/Pure//

I think "Pure" was chosen to indicate that the matrix is a translation matrix
but nothing else (it doesn't have a scale, for example). I'm not the expert in
this area though, so I'm happy to change it. Just thought I'd double check.

[Dominik Grewe, 2013-10-16 08:54:38]

It turns out that IsMatrixApproximatelyIntegerTransform isn't currently needed
any more. Shall I remove it from the patch?

[shawnsingh, 2013-10-16 11:43:34]

I'm OK with removing IsMatrixApproximatelyIntegerTransform if you wish =)

one comment below.  But otherwise looks good!

https://codereview.chromium.org/27223008/diff/10001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/27223008/diff/10001/cc/base/math_util.cc#newc...
cc/base/math_util.cc:22: inline bool ApproximatelyEqual(SkMScalar x, SkMScalar
y, SkMScalar tolerance) {
I'm wondering, did you have a strong reason not to use NearlyZero or NearlyOne
as they were before?

I think that we should not encourage this version of ApproximatelyEqual that
uses an absolute threshold.  The absolute thresholds were acceptable
specifically for checking for approximately == 1 or == 0, because when we
already know one of the values being compared (i.e. 0 or 1), we can literally
convert between absolute and relative thresholds, and they are effectively the
same.  But if we ever do need a general ApproximatelyEqual comparison, we would
probably want to encourage using a relative threshold or even a relative ulps
threshold.

As a secondary advantage, in my opinion the NearlyZero/NearlyOne functions are
more readable when looking at the implementations of the approximatelyXXX
functions.  It's error-prone to see 1's and 0's hidden among so many commas and
parentheses.

[Dominik Grewe, 2013-10-16 12:46:22]

https://codereview.chromium.org/27223008/diff/10001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/27223008/diff/10001/cc/base/math_util.cc#newc...
cc/base/math_util.cc:22: inline bool ApproximatelyEqual(SkMScalar x, SkMScalar
y, SkMScalar tolerance) {
On 2013/10/16 11:43:34, shawnsingh wrote:
> I'm wondering, did you have a strong reason not to use NearlyZero or NearlyOne
> as they were before?
> 
> I think that we should not encourage this version of ApproximatelyEqual that
> uses an absolute threshold.  The absolute thresholds were acceptable
> specifically for checking for approximately == 1 or == 0, because when we
> already know one of the values being compared (i.e. 0 or 1), we can literally
> convert between absolute and relative thresholds, and they are effectively the
> same.  But if we ever do need a general ApproximatelyEqual comparison, we
would
> probably want to encourage using a relative threshold or even a relative ulps
> threshold.
> 
> As a secondary advantage, in my opinion the NearlyZero/NearlyOne functions are
> more readable when looking at the implementations of the approximatelyXXX
> functions.  It's error-prone to see 1's and 0's hidden among so many commas
and
> parentheses.

The main motivation was to avoid code duplication. ApproximatelyInteger uses
that function with arbitrary numbers, not just 0 and 1. But if we get rid of
IsApproximatelyIntegerTransform we wouldn't need it any more.

I agree that NearlyZero and NearlyOne is less error prone.

[Dominik Grewe, 2013-10-16 16:00:23]

I removed IsApproximatelyIntegerTransform(). So there's no need for a generic
ApproximatelyEquals() method any more and I've replaced it with
ApproximatelyZero() and ApproximatelyOne().

[danakj, 2013-10-16 16:02:36]

Can you upload again? Getting chunk mismatch errors.


On Wed, Oct 16, 2013 at 12:00 PM, <dominikg@chromium.org> wrote:

> I removed IsApproximatelyIntegerTransfor**m(). So there's no need for a
> generic
> ApproximatelyEquals() method any more and I've replaced it with
> ApproximatelyZero() and ApproximatelyOne().
>
>
https://codereview.chromium.**org/27223008/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dominik Grewe, 2013-10-16 16:13:07]

On 2013/10/16 16:02:36, danakj wrote:
> Can you upload again? Getting chunk mismatch errors.

Done.

[danakj, 2013-10-16 19:04:14]

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util.cc#newc...
cc/base/math_util.cc:29: return std::abs(x-SkDoubleToMScalar(1.0)) <= tolerance;
spaces around -

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util.h
File cc/base/math_util.h (right):

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util.h#newco...
cc/base/math_util.h:184: static bool IsApproximatelyPureTranslation(
Thanks this API is more simple, how about moving this to the gfx::Transform()
class?

We already have IsIdentityOrTranslation() there. This would be
"IsApproximatelyIdentityOrTranslation(tolerance)".

The goal is to move everything in this class into ui/gfx/ and I don't see the
reason why we can't move this method there now instead of later.

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util.h#newco...
cc/base/math_util.h:186: static bool IsMatrixApproximatelyPureTranslation(
Drop this function from the public API, I think you can roll these two functions
into one.

[shawnsingh, 2013-10-16 19:05:36]

LGTM =)

I'm concerned that the DCHECK will have a perf hit since we're putting it in the
heart of the hot path, which means we will have 12 or so DCHECKS for every time
this function is called...   As I understand this will be compiled out for
production, but it will still persist in our own release builds, which could be
misleading.    I don't have a strong opinion about whether we should keep it or
not.   One option, as a compromise, would be to DCHECK once for the entire
matrix, instead of once per comparison.

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util_unittes...
File cc/base/math_util_unittest.cc (right):

https://codereview.chromium.org/27223008/diff/27001/cc/base/math_util_unittes...
cc/base/math_util_unittest.cc:177:
EXPECT_FALSE(MathUtil::IsMatrixApproximatelyPureTranslation(matrix, 0));

nice way to check that we are not accidentally an exact matrix =)  I like it.

[shawnsingh, 2013-10-16 19:09:51]

> The goal is to move everything in this class into ui/gfx/ and I don't see the
> reason why we can't move this method there now instead of later.

I thought our preference was "don't move things to more common code unless they
need to be used more commonly".  At this point, do we have any intention to use
these more generally outside of cc/ ?

[danakj, 2013-10-16 19:12:33]

On Wed, Oct 16, 2013 at 3:09 PM, <shawnsingh@chromium.org> wrote:

> The goal is to move everything in this class into ui/gfx/ and I don't see
>> the
>> reason why we can't move this method there now instead of later.
>>
>
> I thought our preference was "don't move things to more common code unless
> they
> need to be used more commonly".  At this point, do we have any intention
> to use
> these more generally outside of cc/ ?
>

My position is more "if it makes sense as part of the class then it should
be there". Like it makes sense to ask a Transform if it's approximately
translation as much as if it is exactly translation. I don't think it's a
controversial method.

I think this more applies to weird use cases that will impact the rest of
the code base, or new classes like cc::Region.


>
>
>
https://codereview.chromium.**org/27223008/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shawnsingh, 2013-10-16 19:14:08]

OK, SGTM  I'm happy with that.   In Dominik's defense, he was doing what several
of us had discussed offline many weeks ago. =)

[Dominik Grewe, 2013-10-17 16:57:47]

I've moved the code to the gfx::Transform class. Please have another look.

[danakj, 2013-10-17 17:43:17]

LGTM with some style nits

https://codereview.chromium.org/27223008/diff/37001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/27223008/diff/37001/ui/gfx/transform.cc#newco...
ui/gfx/transform.cc:225: ApproximatelyOne (matrix_.get(0, 0), tolerance) &&
no extra white space to line things up vertically. this just breaks style rules,
and makes future patches require larger diffs to re-align stuff.

https://codereview.chromium.org/27223008/diff/37001/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/27223008/diff/37001/ui/gfx/transform_unittest...
ui/gfx/transform_unittest.cc:2238: 
1 blank line only

https://codereview.chromium.org/27223008/diff/37001/ui/gfx/transform_unittest...
ui/gfx/transform_unittest.cc:2256: 
1 blank line only

[Dominik Grewe, 2013-10-17 18:27:41]

Thanks!

[commit-bot: I haz the power, 2013-10-17 18:35:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/27223008/42001

[commit-bot: I haz the power, 2013-10-17 22:07:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-18 21:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/27223008/67001

[commit-bot: I haz the power, 2013-10-19 18:26:09]

Change committed as 229609