Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150493002:
remove unnecessary SkScalar macros (Closed)
|
Created:
6 years, 10 months ago by reed1 Modified:
6 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionremove (unnecessary) SkScalarMul and SkScalarMulAdd macros from SkMatrix.cpp.
SkScalarMulDiv remains, but it can be removed later (though it is slightly clearer to use it at times).
BUG=skia:
Committed: http://code.google.com/p/skia/source/detail?r=13252
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : add SK_LEGACY_MATRIX_MATH_ORDER #Patch Set 8 : #
Total comments: 19
Patch Set 9 : review comments from #8 #Messages
Total messages: 12 (0 generated)
I propose landing this w/ the legacy flag ON, then landing it with the flag off and rebaselining skia if we want, but making that decision later.
https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (left): https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#o... src/core/SkMatrix.cpp:455: fMat[kMPersp2] = kMatrix22Elem; Is there no value in keeping a name on this to indicate it's special? (This is my one. There are many like it, but this one is mine.) https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:226: static inline SkScalar sdot3(SkScalar a, SkScalar b, SkScalar c, SkScalar d, Don't think calling this sdot would hurt if you're into that sort of thing. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:733: det = mat[SkMatrix::kMScaleX] * dcross(mat[SkMatrix::kMScaleY], mat[SkMatrix::kMPersp2], mat[SkMatrix::kMTransY], mat[SkMatrix::kMPersp1]) + These lines are pretty hard to parse (they were before too). Maybe drop the dcrosses to a new line each? https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:751: static SkScalar inline mul_diff_scale(double a, double b, double c, double d, mul_diff_scale might be better named dcross_scale? https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:836: inv->fMat[kMScaleX] = SkDoubleToScalar(scross(fMat[kMScaleY], fMat[kMPersp2], fMat[kMTransY], fMat[kMPersp1]) * scale); This part is also pretty hard to parse. could be an SkScalar scross_scale(SkScalar,SkScalar,SkScalar,SkScalar, double) would make it clearer? https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:850: inv->fMat[kMTransX] = SkDoubleToScalar(dcross(fMat[kMSkewX], fMat[kMTransY], fMat[kMScaleY], fMat[kMTransX]) * scale); Why switch away from mul_diff_scale here? https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:854: inv->fMat[kMTransY] = SkDoubleToScalar(dcross(fMat[kMSkewY], fMat[kMTransX], fMat[kMScaleX], fMat[kMTransY]) * scale); Same? https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1494: SkScalar a = sdot(m[SkMatrix::kMScaleX], m[SkMatrix::kMScaleX], m[SkMatrix::kMSkewY], m[SkMatrix::kMSkewY]); Clearer back on two lines each?
https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (left): https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#o... src/core/SkMatrix.cpp:455: fMat[kMPersp2] = kMatrix22Elem; On 2014/01/30 19:14:55, mtklein wrote: > Is there no value in keeping a name on this to indicate it's special? (This is > my one. There are many like it, but this one is mine.) The perspective column used to be special, when we supported fixed-point... it was stored in 2.30 (fract) instead of fixed (16.16). Since that backend is now completely gone, I don't think the column is special anymore. "And when everyones special... no one will be." -- Syndrome https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:226: static inline SkScalar sdot3(SkScalar a, SkScalar b, SkScalar c, SkScalar d, On 2014/01/30 19:14:55, mtklein wrote: > Don't think calling this sdot would hurt if you're into that sort of thing. Done. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:733: det = mat[SkMatrix::kMScaleX] * dcross(mat[SkMatrix::kMScaleY], mat[SkMatrix::kMPersp2], mat[SkMatrix::kMTransY], mat[SkMatrix::kMPersp1]) + On 2014/01/30 19:14:55, mtklein wrote: > These lines are pretty hard to parse (they were before too). Maybe drop the > dcrosses to a new line each? Done. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:751: static SkScalar inline mul_diff_scale(double a, double b, double c, double d, On 2014/01/30 19:14:55, mtklein wrote: > mul_diff_scale might be better named dcross_scale? That guy is now unused. deleting. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:850: inv->fMat[kMTransX] = SkDoubleToScalar(dcross(fMat[kMSkewX], fMat[kMTransY], fMat[kMScaleY], fMat[kMTransX]) * scale); On 2014/01/30 19:14:55, mtklein wrote: > Why switch away from mul_diff_scale here? Done. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:854: inv->fMat[kMTransY] = SkDoubleToScalar(dcross(fMat[kMSkewY], fMat[kMTransX], fMat[kMScaleX], fMat[kMTransY]) * scale); On 2014/01/30 19:14:55, mtklein wrote: > Same? Done. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1494: SkScalar a = sdot(m[SkMatrix::kMScaleX], m[SkMatrix::kMScaleX], m[SkMatrix::kMSkewY], m[SkMatrix::kMSkewY]); On 2014/01/30 19:14:55, mtklein wrote: > Clearer back on two lines each? Done.
lgtm
lgtm with nits that need not be addressed https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:320: this->setScale(SkScalarInvert(divx), SkScalarInvert(divy)); This makes me think that SkScalarInvert is special some of the time, on some platform, or may be in the future. Is that this case? Otherwise, this->setScale(1 / divx, 1 / divy); would be more readable for me. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:733: det = mat[SkMatrix::kMScaleX] * dcross(mat[SkMatrix::kMScaleY], mat[SkMatrix::kMPersp2], mat[SkMatrix::kMTransY], mat[SkMatrix::kMPersp1]) + Or, if this was a member of SkMatrix, it would be equally efficient as const float* m = fMat; det = m[kMScaleX] * dcross(m[kMScaleY], m[kMPersp2], m[kMTransY], m[kMPersp1]) + ... and fit in 100 chars.
https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:320: this->setScale(SkScalarInvert(divx), SkScalarInvert(divy)); On 2014/01/30 21:19:10, caryclark wrote: > This makes me think that SkScalarInvert is special some of the time, on some > platform, or may be in the future. Is that this case? > > Otherwise, > > this->setScale(1 / divx, 1 / divy); > > would be more readable for me. 1. I always imaging that we could somehow be faster if we knew that we were just inverting, and not a general divide... but that never seems to be the case. 2. In this case, 1 / divx will do the wrong thing, since divx is an int, not a scalar, and we will always get 0 or 1. SkScalarInvert performs the casts to ensure we treat the parameter as a float. Frankly, setIDiv was added specifically to help precision when we had a fixed-point backend (and inverting a fixed threw away too many bits). Since that is no longer a convert, I bet I can just remove this method. https://codereview.chromium.org/150493002/diff/140001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:733: det = mat[SkMatrix::kMScaleX] * dcross(mat[SkMatrix::kMScaleY], mat[SkMatrix::kMPersp2], mat[SkMatrix::kMTransY], mat[SkMatrix::kMPersp1]) + On 2014/01/30 21:19:10, caryclark wrote: > Or, if this was a member of SkMatrix, it would be equally efficient as > > const float* m = fMat; > det = m[kMScaleX] * dcross(m[kMScaleY], m[kMPersp2], m[kMTransY], > m[kMPersp1]) + > ... > > and fit in 100 chars. My rewrite address the 100 chars, but I agree that these could be namespaced into the class, so we could drop the prefix (and make it more readable). How about I do that next CL.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/reed@google.com/150493002/160001
Message was sent while issue was closed.
Change committed as 13252
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
On 2014/01/30 22:13:20, I haz the power (commit-bot) wrote: > CQ bit was unchecked on CL. Ignoring. FYI this 'CQ bit was unchecked on CL. Ignoring.' message is a known chromium infrastructure bug (https://code.google.com/p/chromium/issues/detail?id=339116).
Message was sent while issue was closed.
This CL broke some GM results; see https://code.google.com/p/skia/issues/detail?id=2113 ('SkMatrix change has modified perspective-related GM results on all 32-bit Win7 buildbots') |
