Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23444049:
Implement transform snapping for gfx::Transforms. (Closed)
|
Created:
7 years, 3 months ago by avallee Modified:
7 years, 2 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement transform snapping for gfx::Transforms.
Implement SnapTransform which takes a decomposed transform and viewport
and tries to nudge the rotation, scale and translation values. It tries applying the inverse of
the transform to the viewport. (Objects in this rect having the
transform applied will end up in the viewport) If the transform maps the
corners onto integer points and the corners are no more than 1 pixel
away we will return success and the decomposed transform with the new
rotation value.
R=vollick@chromium.org
BUG=280280
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230849
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fix test case to back-map correctly. Address danajk@ comments. #
Total comments: 12
Patch Set 3 : Add a test and fixed comments by vollick@. #
Total comments: 10
Patch Set 4 : Rebase to master. #Patch Set 5 : Factor out build snapped rotation matrix. #Patch Set 6 : Remove std::round and make epsilon a float literal. #
Total comments: 6
Patch Set 7 : Snap *ALL* the components of the transform. #
Total comments: 5
Patch Set 8 : Address danakj@ comments. #
Total comments: 2
Patch Set 9 : Add unit tests for snapping translation, scaling and a composite of all components. #
Total comments: 10
Patch Set 10 : #
Messages
Total messages: 23 (0 generated)
Just some quick style nits before ian looks at it. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:18: const float NEAR_INT_ESPILON = 1e-10; kNearIntEpsilon I will also note this is at least the 3rd epsilon constant defined in ui/gfx/transformy stuff, and they are all different.. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:21: return (abs(f-round(f)) < NEAR_INT_ESPILON); std::abs std::round https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:361: point_a = point_b = Point3F(point.x(), point.y(), 0.0); 0.f https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:366: bool invertible = true; true || anything = true. did you mean false? https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:376: if ((point_b-point_a).Length() > 1.0) { spaces around operators https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:392: if (value < -0.5) { treat SkMScalar as a float. so compare to -0.5f. same for all other literals here.
Addressed Dana's nits. Discovered that my initial test case would map the (0,0) to something like 53.5 (because that's 1/2 the translation, going the other way should always give ints). https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:18: const float NEAR_INT_ESPILON = 1e-10; On 2013/09/10 20:29:51, danakj wrote: > kNearIntEpsilon > > I will also note this is at least the 3rd epsilon constant defined in > ui/gfx/transformy stuff, and they are all different.. I'll unify with what's in transform.cc. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:21: return (abs(f-round(f)) < NEAR_INT_ESPILON); On 2013/09/10 20:29:51, danakj wrote: > std::abs > std::round Done. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:361: point_a = point_b = Point3F(point.x(), point.y(), 0.0); On 2013/09/10 20:29:51, danakj wrote: > 0.f Done. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:366: bool invertible = true; On 2013/09/10 20:29:51, danakj wrote: > true || anything = true. did you mean false? Meant and_equals. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:376: if ((point_b-point_a).Length() > 1.0) { On 2013/09/10 20:29:51, danakj wrote: > spaces around operators Done. https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:392: if (value < -0.5) { uOn 2013/09/10 20:29:51, danakj wrote: > treat SkMScalar as a float. so compare to -0.5f. same for all other literals > here. Done.
https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:18: const float NEAR_INT_ESPILON = 1e-10; On 2013/09/10 20:29:51, danakj wrote: > kNearIntEpsilon > > I will also note this is at least the 3rd epsilon constant defined in > ui/gfx/transformy stuff, and they are all different.. This is a bummer, for sure. Is it possible to share or justify the need for the new constant? https://codereview.chromium.org/23444049/diff/1/ui/gfx/transform_util.cc#newc... ui/gfx/transform_util.cc:21: return (abs(f-round(f)) < NEAR_INT_ESPILON); On 2013/09/10 20:29:51, danakj wrote: > std::abs > std::round I think we want the relative error, not the absolute error here. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:19: const float kNearIntEpsilon = 1e-8; Can we expose this from SkMatrix44? I think we'll want a function static SkMScalar SkMatrix44::near_int_epsilon() since we'll want a different constant depending on whether SkMScalar is a float or a double. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:300: matrix->setDouble(3, i, decomp.perspective[i]); If we're going to be breaking things up into separate Apply* functions, let's separate perspective and translation, too. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:325: } Same with skew and scale. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:405: Transform original = ComposeTransform(in); This is wasteful. The caller will have the composed original on hand. Please take it as a parameter rather than recomposing it here. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:406: Transform snapped = ComposeWithRotation(in, rotation_matrix); This isn't going generalize well when we want to snap more than rotation. Compose works by concat'ing a bunch SkMatrix44's which are in turn built based on the values in the DecomposedTransforms. I suggest you create the following: static buildTranslationMatrix(SkMatrix44* out, const DecomposedTransform& decomp); static buildRotationMatrix(SkMatrix44* out, const DecomposedTransform& decomp); ...etc, and then add: ComposeTransform(const SkMatrix44& perspective, const SkMatrix44& translation, ...); ..and then you could do: ComposeTransform(const DecomposedTransform& decomp) { SkMatrix44 perspective(k-don't-initialize-thanks); ... buildPerspectiveMatrix(&perspective, decomp); ... ComposeTransform(perspective, translation, ...); } Once you've done this, it'll be easy to call the wide ComposeTransform with your snapped components (you should make helper functions to generate the snapped components, perhaps just with a bool snap parameter to the other build*Matrix fn's) -- rotation for now, but probably scale and others shortly. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util_unit... File ui/gfx/transform_util_unittest.cc (right): https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util_unit... ui/gfx/transform_util_unittest.cc:71: } Please add tests for a viewport that's far away from the center of rotation. I'm hoping you can find a case where snapping succeeds when the viewport is close to the center of rotation, but fails when it's far away, even though the matrix is the same.
Fixed issues. PTAL. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:19: const float kNearIntEpsilon = 1e-8; On 2013/09/11 12:19:48, Ian Vollick wrote: > Can we expose this from SkMatrix44? I think we'll want a function static > SkMScalar SkMatrix44::near_int_epsilon() since we'll want a different constant > depending on whether SkMScalar is a float or a double. Done. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:300: matrix->setDouble(3, i, decomp.perspective[i]); On 2013/09/11 12:19:48, Ian Vollick wrote: > If we're going to be breaking things up into separate Apply* functions, let's > separate perspective and translation, too. Followed the comment from the bottom. Create all the matrices and the fat compose just multiplies them all. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:325: } On 2013/09/11 12:19:48, Ian Vollick wrote: > Same with skew and scale. Done. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:405: Transform original = ComposeTransform(in); On 2013/09/11 12:19:48, Ian Vollick wrote: > This is wasteful. The caller will have the composed original on hand. Please > take it as a parameter rather than recomposing it here. Done. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util.cc#n... ui/gfx/transform_util.cc:406: Transform snapped = ComposeWithRotation(in, rotation_matrix); On 2013/09/11 12:19:48, Ian Vollick wrote: > This isn't going generalize well when we want to snap more than rotation. > Compose works by concat'ing a bunch SkMatrix44's which are in turn built based > on the values in the DecomposedTransforms. I suggest you create the following: > static buildTranslationMatrix(SkMatrix44* out, const DecomposedTransform& > decomp); > static buildRotationMatrix(SkMatrix44* out, const DecomposedTransform& decomp); > ...etc, and then add: > > ComposeTransform(const SkMatrix44& perspective, > const SkMatrix44& translation, ...); > > ..and then you could do: > ComposeTransform(const DecomposedTransform& decomp) { > SkMatrix44 perspective(k-don't-initialize-thanks); > ... > buildPerspectiveMatrix(&perspective, decomp); > ... > ComposeTransform(perspective, > translation, ...); > } > > Once you've done this, it'll be easy to call the wide ComposeTransform with your > snapped components (you should make helper functions to generate the snapped > components, perhaps just with a bool snap parameter to the other build*Matrix > fn's) -- rotation for now, but probably scale and others shortly. Done. https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util_unit... File ui/gfx/transform_util_unittest.cc (right): https://codereview.chromium.org/23444049/diff/9004/ui/gfx/transform_util_unit... ui/gfx/transform_util_unittest.cc:71: } On 2013/09/11 12:19:48, Ian Vollick wrote: > Please add tests for a viewport that's far away from the center of rotation. I'm > hoping you can find a case where snapping succeeds when the viewport is close to > the center of rotation, but fails when it's far away, even though the matrix is > the same. Done.
https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:198: bool CheckTansformPoint(const PointF& point, nit: typo, but even still, it would be nice if the fn name mentioned what in particular it was checking. InverseTransformsMapIntPointToWithinAPixel maybe? It's a mouthful, but it's a bit more meaningful. https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:202: point_a = point_b = Point3F(point.x(), point.y(), 0.f); nit: Use the Point3F ctor that takes a PointF. https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:209: invertible &= transform_b.TransformPointReverse(&point_b); It's a bummer that we recompute the inverse of these matrices 4 times for each corner. Please pass the inverted matrices as parameters. Also, should the point param be a Point rather than a PointF? https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:406: const Transform& transform, This is not the signature I expected. I was hoping for bool SnapTransform(Transform* out, const Transform& unsnapped, const Rect& viewport); I don't think it would be too much of a stretch from what you have here to snap the rest of the components and I think it would be more useful. https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:425: } Please make a BuildSnappedRotationMatrix helper (and similar helpers for translation, etc).
https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:198: bool CheckTansformPoint(const PointF& point, On 2013/09/14 23:45:31, Ian Vollick wrote: > nit: typo, but even still, it would be nice if the fn name mentioned what in > particular it was checking. InverseTransformsMapIntPointToWithinAPixel maybe? > It's a mouthful, but it's a bit more meaningful. Done. https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:202: point_a = point_b = Point3F(point.x(), point.y(), 0.f); On 2013/09/14 23:45:31, Ian Vollick wrote: > nit: Use the Point3F ctor that takes a PointF. Done. https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:209: invertible &= transform_b.TransformPointReverse(&point_b); On 2013/09/14 23:45:31, Ian Vollick wrote: > It's a bummer that we recompute the inverse of these matrices 4 times for each > corner. Please pass the inverted matrices as parameters. Also, should the point > param be a Point rather than a PointF? Done. https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:406: const Transform& transform, On 2013/09/14 23:45:31, Ian Vollick wrote: > This is not the signature I expected. I was hoping for > bool SnapTransform(Transform* out, const Transform& unsnapped, const Rect& > viewport); > > I don't think it would be too much of a stretch from what you have here to snap > the rest of the components and I think it would be more useful. Done. I don't think you want other snapping yet, right? https://codereview.chromium.org/23444049/diff/17001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:425: } On 2013/09/14 23:45:31, Ian Vollick wrote: > Please make a BuildSnappedRotationMatrix helper (and similar helpers for > translation, etc). Added Snapped rotation unless we also want snapped translation helper right away. However, only snapped rotation would be used here in SnapRotation.
Ping.
https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:26: return (std::abs(f - std::floor(f + 0.5f)) < kNearIntEpsilon); We should be able to ditch this, I think. See below. https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:236: if (!(NearInteger(point_b.x()) && NearInteger(point_b.y()))) { If we snap everything (not just rotation), then we will certainly map int points to int points, so this would be unnecessary. I'm nervous about the behavior of NearInteger with large floating point values. I think your second check below will be sufficient if we go full-snapping. https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:449: bool SnapRotation(Transform* out, s/SnapRotation/SnapTransform/ and snap everything but perspective, please.
PTAL. https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:26: return (std::abs(f - std::floor(f + 0.5f)) < kNearIntEpsilon); On 2013/10/18 14:54:46, Ian Vollick wrote: > We should be able to ditch this, I think. See below. Done. https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:236: if (!(NearInteger(point_b.x()) && NearInteger(point_b.y()))) { On 2013/10/18 14:54:46, Ian Vollick wrote: > If we snap everything (not just rotation), then we will certainly map int points > to int points, so this would be unnecessary. I'm nervous about the behavior of > NearInteger with large floating point values. I think your second check below > will be sufficient if we go full-snapping. Gone. Implemented full snapping, (skew snapped to identity, perspective untouched). https://codereview.chromium.org/23444049/diff/45001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:449: bool SnapRotation(Transform* out, On 2013/10/18 14:54:46, Ian Vollick wrote: > s/SnapRotation/SnapTransform/ and snap everything but perspective, please. Done.
Some driveby styleish things. https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:124: matrix->setTranslate(SkDoubleToMScalar(decomp.translate[0]), I find it a bit odd reading this that you have all thee BuildFooMatrix methods, and each one works differently. Some take a pointer and setIdentity() on it and then set some values. Some take a pointer and set values without setting identity. Some create a new matrix and return it. Do you think these could be more consistent or is this just required? Maybe not calling them all Build* would help then, and differentiate behaviour with names. https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:236: bool CheckViewportPointMapsWithinAPixel(const Point& point, bikeshed: WithinOnePixel https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:237: const Transform& transform) { git cl format
danakj@, fixed according to your comments. https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:124: matrix->setTranslate(SkDoubleToMScalar(decomp.translate[0]), On 2013/10/21 15:28:42, danakj wrote: > I find it a bit odd reading this that you have all thee BuildFooMatrix methods, > and each one works differently. > > Some take a pointer and setIdentity() on it and then set some values. > Some take a pointer and set values without setting identity. > Some create a new matrix and return it. > > Do you think these could be more consistent or is this just required? Maybe not > calling them all Build* would help then, and differentiate behaviour with names. Removed the pointers, makes it easier to read. Added comments when using uninitialized ctor followed by calls SkMatrix methods which will implicitly set identity. https://codereview.chromium.org/23444049/diff/74001/ui/gfx/transform_util.cc#... ui/gfx/transform_util.cc:236: bool CheckViewportPointMapsWithinAPixel(const Point& point, On 2013/10/21 15:28:42, danakj wrote: > bikeshed: WithinOnePixel Yeah, was mostly shortening to make the call sits all fit on one line. Clearer reading.
lgtm, modulo some unit tests for the new snapping functionality added in these latest patch sets. https://codereview.chromium.org/23444049/diff/134001/ui/gfx/transform_util_un... File ui/gfx/transform_util_unittest.cc (right): https://codereview.chromium.org/23444049/diff/134001/ui/gfx/transform_util_un... ui/gfx/transform_util_unittest.cc:35: TEST(TransformUtilTest, SnapTransform) { Need tests for snapping scale and translation.
https://codereview.chromium.org/23444049/diff/134001/ui/gfx/transform_util_un... File ui/gfx/transform_util_unittest.cc (right): https://codereview.chromium.org/23444049/diff/134001/ui/gfx/transform_util_un... ui/gfx/transform_util_unittest.cc:35: TEST(TransformUtilTest, SnapTransform) { On 2013/10/21 18:36:30, Ian Vollick wrote: > Need tests for snapping scale and translation. Done.
Some last style comments, otherwise LGTM when Ian is happy. https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:207: matrix.setScale(SkDoubleToMScalar(decomp.scale[0]), This doesn't setIdentity like the other setFoo()s? https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:240: Point3F point_original, point_transformed; From style guide "In particular, initialization should be used instead of declaration and assignment". Can this be instead: Point3F point_original(point); Point3F point_transformed(point); This also gives us one statement per line which is nice. https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util_un... File ui/gfx/transform_util_unittest.cc (right): https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util_un... ui/gfx/transform_util_unittest.cc:117: transform.Scale3d(5.0, 2.1, 1.0); This function takes SkMScalars. You can use float literals for SkFloat/DoubleToMScalar, but not double literals.
On 2013/10/23 14:57:19, danakj wrote: > Some last style comments, otherwise LGTM when Ian is happy. Oh I see they already are.
lgtm2 Thanks for the new tests. https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:133: std::floor(decomp.translate[0] + SkDoubleToMScalar(0.5)); nit: You might want to take a look at safe_integer_conversions.h here. Maybe you could use that rounding fn? (Here and elsewhere). https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:207: matrix.setScale(SkDoubleToMScalar(decomp.scale[0]), On 2013/10/23 14:57:19, danakj wrote: > This doesn't setIdentity like the other setFoo()s? It does. You could use the 'uninitialized' constructor for the matrix here.
https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:133: std::floor(decomp.translate[0] + SkDoubleToMScalar(0.5)); On 2013/10/23 19:03:13, Ian Vollick wrote: > nit: You might want to take a look at safe_integer_conversions.h here. Maybe you > could use that rounding fn? (Here and elsewhere). I don't need ints here though, only the rounding behaviour. Now this isn't implementing part of the css spec, but when I see rounding to integers, it's usually to nearest int, equal distance rounding towards positive infinity. ToRoundedInt rounds towards 0. I could extract it to a local round function. https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:207: matrix.setScale(SkDoubleToMScalar(decomp.scale[0]), On 2013/10/23 19:03:13, Ian Vollick wrote: > On 2013/10/23 14:57:19, danakj wrote: > > This doesn't setIdentity like the other setFoo()s? > > It does. You could use the 'uninitialized' constructor for the matrix here. Done. https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:240: Point3F point_original, point_transformed; On 2013/10/23 14:57:19, danakj wrote: > From style guide "In particular, initialization should be used instead of > declaration and assignment". > > Can this be instead: > > Point3F point_original(point); > Point3F point_transformed(point); > > This also gives us one statement per line which is nice. Done. https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util_un... File ui/gfx/transform_util_unittest.cc (right): https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util_un... ui/gfx/transform_util_unittest.cc:117: transform.Scale3d(5.0, 2.1, 1.0); On 2013/10/23 14:57:19, danakj wrote: > This function takes SkMScalars. You can use float literals for > SkFloat/DoubleToMScalar, but not double literals. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avallee@chromium.org/23444049/284001
https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc File ui/gfx/transform_util.cc (right): https://codereview.chromium.org/23444049/diff/184001/ui/gfx/transform_util.cc... ui/gfx/transform_util.cc:133: std::floor(decomp.translate[0] + SkDoubleToMScalar(0.5)); On 2013/10/23 19:44:15, avallee wrote: > On 2013/10/23 19:03:13, Ian Vollick wrote: > > nit: You might want to take a look at safe_integer_conversions.h here. Maybe > you > > could use that rounding fn? (Here and elsewhere). > > I don't need ints here though, only the rounding behaviour. > > Now this isn't implementing part of the css spec, but when I see rounding to > integers, it's usually to nearest int, equal distance rounding towards positive > infinity. ToRoundedInt rounds towards 0. When I read this I thought you were saying 1.5 rounded to 1.0, but I don't see that in the code. But -1.5 rounds to -2. Did you mean rounds away from 0? Should we fix ToRoundedInt? I wrote that without knowing about any such norms for negatives.
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... 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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avallee@chromium.org/23444049/494001
Message was sent while issue was closed.
Change committed as 230849 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
