Hi Denis,
On 4/20/2011 2:06 PM, Denis Lila wrote:
One thing I just looked at and I'm wondering about is why there are
linetos at line 850 and 865. Shouldn't those points already be
interpolated either by the end of the previous segment or the end of
the
one that was just added? Does this add a lot of extra dirt into the
outlines?
I'm not completely sure. Maybe to plug holes between curves after
subdivision? I will think about this more and give you a better
answer later. We should be very careful if changing this though. I vaguely
remember inserting them when I originally wrote this code to solve
some nasty bugs.
Yes, it would be good to keep this in mind. Should I file a bug/rfe on it?
I didn't see where you had the "x1 == x2&& y1 == y2..." block that
you
mentioned below. Did you decide against that one as well? I'm not sure
it will end up causing non-finite values in the computeIntersection
methods because computeOffset protects against divide by zero and
returns dummy values which may actually induce us to find finite
intersection points.
Yes, I decided against it. I think it was redundant because if
"(x1 == x2&& y1 == y2 ||..." is true then computeIntersection
is guaranteed to compute NaNs, for both the right path and the
left path. In that case we call getLineOffsets, which is exactly
what we would do in the "(x1 == x2&& y1 == y2 ||..." case.
Ah, I see it now. Either dx1,dy1 are both zero or dx3,dy3 are both zero
in which case computeOffsets returns 0 offsets for one of the endpoints
and computeIntersection is called to find the intersection of a line and
a pair of identical points and that identical pair ends up generating a
den==0 which causes Inf or NaN. Cool. Is that worth a comment somewhere?
Also, the comment in computeIntersection claims that it can't get
infinities because drawMiter already eliminated them, but the quad code
doesn't eliminate them so that is worth a mention.
This way, that rare special case is made slower and everything
else is made a bit faster.
Perfect!
Now that you have a case where you are testing "!isNot", it points out
that maybe the method should be renamed "isFinite" and have its return
value inverted so that we don't get a bunch of double negatives in the
code (my head is reeling a little from reviewing that line ;-). (OK,
so I'm about to point out that this new addition isn't needed anyway, but
in case we find a need to apply ! to the isNot method in the future,
it will look annoying when that happens.)
Eh, I was hoping you wouldn't mind :-)
I did it this way to minimize the total number of "!"s. With isNotFinite
there are no "!" at all. With isFinite there must be at least 2.
!isFinite() is shorter than isNonFinite. I find them about equal to
read, but if we ever end up with another "!isNonFinite" my head might
explode and I would not enjoy that. ;)
I just noticed that you made this change and I was going to suggest the
version that you just wrote so I'm doing the happy dance (not like
that's a goal here, but thanks ;-) Just in case I sent an internal
message to one of our IEEE FP experts to verify that the range test is
complete and performant.
You could actually avoid all of the "!" by swapping the code bodies
around and using if/else:
if (both finite) {
quad1 code
} else {
computeIntersection2
if (both finite) {
quad2 code
} else {
line code
}
}
Updated webrev:
http://icedtea.classpath.org/~dlila/webrevs/7036754/webrev/
Looks correct, but what do you think about the suggestions I've made above?
...jim
