Hi Denis,
Lines 1094-1096, they could also be NaN if any of the numerators were
also zero and these tests might fail (but only for the case of all of
them being zero I guess, otherwise one of the other divisions would
result in infinity). Are accidental infinities (caused by overflow
rather than d==0.0) really a problem for the code to handle?
I don't have any recommendations on your comment about the code that
tests q for zero. You could probably check if 2*u and -u were distinct
as an alternate test, but that would cost you a cbrt() call. In
general, though, I'm guessing it's a rare case so saving the call to
cbrt() is probably not important. I will note, though, that if a number
is very close to 0, but not 0, then its cube root will be a larger
number than the original.
I just noticed that the code you are replacing actually used to refine
the roots so maybe you should do some of this magic. However, it only
bothered to refine the roots if there were 3 roots and they were near 0
or 1 (because that might cause the caller to reject the root if it fell
on the wrong side of 0 or 1). Either way, look and see if fixRoots()
and its friends are dead code now and/or if they should be replaced with
your refinement techniques below.
I tried to review the code for correctness of formulae, but since I
never really understood how the first version worked (I just copied it
from Numerical Recipes), all I could do was to compare to the old
version that it was similar to. Frustratingly, the variable names
conflicted and one of the values was calculated with reversed sign so it
ended up being more frustrating than educational. :-(
Since you apparently tested the new code extensively and got it from a
source that had some amount of editorial review - I'll trust that
process instead of my crossed eyes...
...jim
On 12/15/2010 7:13 AM, Denis Lila wrote:
Hi Jim.
Also, I wrote new hit testing code in jdk6 that used bezier recursion
to compute the values and it ran way faster than any root-finding methods
(mainly because all you have to worry about is subdividing enough that
the curve can be classified as above, below, or to the left or right
and you're done), so the containment methods could probably be fixed by
simply using the new code in sun.awt.geom.Curve and this method could
be updated with the new equations you found and left as an "approximate
method" like it always has been?
Well, I already have half the code I need to implement the ideas I
wrote, so... why not?
However, making solveCubic that good does not really seem to be a high
priority, so how about we quickly push just the new implementation I
found so we can fix most cases where an obvious root is missed, then
push this dashing/AA webrev (which will depend on the new solveCubic),
then I can fix the implementation of intersect using the recursive
subdivision you mentioned, and then I can take my time finishing
the implementation of the ideas from my last e-mail (these bugs/rfes
have waited 10+ years - they can wait 1-2 more months). Right now,
I would like to give priority to better pisces support of the new
parallelogram pipes and this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=662230
Here's the webrev for the new solveCubic implementation:
http://icedtea.classpath.org/~dlila/webrevs/cc2d/webrev/
Regards,
Denis.
