Handle special angles with care in SkMatrix44

cos and sin aren't quite precise when their results are close to zero. Since this is the common case, I've added special logic to return exactly zero when appropriate.

[vollick, 2011-06-24 18:01:35]

+TomH

[reed1, 2011-06-24 18:19:32]

I've added a (disabled) version of your new test in rev. 1708 1. What is the performance hit for your change? fmod() can't be free. 2. Is it equivalent to perform the Almost test on the output of sin/cos as on the input? 3. Should/could we have variants of these calls that do or do not perform these extra tests (assuming there is some cost) 4. Is it perhaps equivalent (or better) to perform the Almost check inside rectStaysRect?

[TomH, 2011-06-24 18:23:22]

http://codereview.appspot.com/4631068/diff/1004/include/utils/SkMatrix44.h File include/utils/SkMatrix44.h (right): http://codereview.appspot.com/4631068/diff/1004/include/utils/SkMatrix44.h#ne... include/utils/SkMatrix44.h:159: void setRotateDegreesAbout(SkMScalar x, SkMScalar y, SkMScalar z, I'm not sure why you're removing the radians versions? Is there a clear reason not to leave them in? (Perhaps merging implementations by converting rads to degs?) http://codereview.appspot.com/4631068/diff/1004/include/utils/SkMatrix44.h#ne... include/utils/SkMatrix44.h:166: SkMScalar degrees); I originally wrote this in the CPP file, but it actually needs to be here: document the nonlinearity around 0/90/... here! http://codereview.appspot.com/4631068/diff/1004/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): http://codereview.appspot.com/4631068/diff/1004/src/utils/SkMatrix44.cpp#newc... src/utils/SkMatrix44.cpp:22: namespace { We should probably change the Skia style guide; IIRC it says prefer static, but anonymous namespaces are usually considered the "right thing" in C++. Mike? http://codereview.appspot.com/4631068/diff/1004/src/utils/SkMatrix44.cpp#newc... src/utils/SkMatrix44.cpp:39: if (ApproximatelyEqual(fmod(degrees + 90.0, 180.0), 0.0)) { I was going to worry about performance of fmod(), but this isn't being called in any inner loops, is it. http://codereview.appspot.com/4631068/diff/1004/src/utils/SkMatrix44.cpp#newc... src/utils/SkMatrix44.cpp:46: if (ApproximatelyEqual(fmod(degrees, 180.0), 0.0)) { I can see the value to you of the use case, and people doing animations aren't likely to need to have an incremental rotation smaller than your tolerance value. It still raises my hackles, and I'd at least like to see a comment in the header about these clamps.

[TomH, 2011-06-24 18:25:21]

It'd be easier for me to evaluate these kinds of questions if we had a couple of concrete use cases for this. Who's calling it? Why? What are they working with? e.g. in the unit tests when we look at rectStaysRect, we build the transform matrices directly. Is this a reasonable option for the caller? Or are they hooked up to a dial in a GUI that feeds them a float?

[vollick, 2011-06-24 19:40:01]

> 4. Is it perhaps equivalent (or better) to perform the Almost check inside rectStaysRect? Good point. I think that modifying rectStaysRect is a much better approach. I will abandon this change for now. > I've added a (disabled) version of your new test in rev. 1708 > > 1. What is the performance hit for your change? fmod() can't be free. > 2. Is it equivalent to perform the Almost test on the output of sin/cos as on > the input? I think it is equivalen. I now clamp outputs that are very close to zero to zero. > 3. Should/could we have variants of these calls that do or do not perform these > extra tests (assuming there is some cost) I think that the cost is negligible now

[vollick, 2011-06-24 20:15:06]

On 2011/06/24 18:25:21, TomH wrote: > It'd be easier for me to evaluate these kinds of questions if we had a couple of > concrete use cases for this. Who's calling it? Why? What are they working with? > > e.g. in the unit tests when we look at rectStaysRect, we build the transform > matrices directly. Is this a reasonable option for the caller? Or are they > hooked up to a dial in a GUI that feeds them a float? Hi Tom, I'm working with some code that transforms rects, and it's currently sensitive to floating point errors. I think that addressing this sensitivity (as with rectStaysRect) is the right approach, not massaging the matrix.

[TomH, 2012-02-29 15:11:43]

On 2011/06/24 19:40:01, vollick wrote: > > 4. Is it perhaps equivalent (or better) to perform the Almost check inside > rectStaysRect? > Good point. I think that modifying rectStaysRect is a much better approach. I > will abandon this change for now. If this change is abandoned, can you please close the issue?