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 gave what you said some thought, and simply removing the
emitLineTo's
didn't work. That's because we were using them to get joins right
almost by accident. The whole thing was pretty ugly. I tried
fixing
this to do things in a more principled way. What I did was: for
the
path we draw directly (i.e. that we don't put in the polyStack)
I never emit the starting point, and for polynomials on the
reverse
path I never emit the last point. Getting these starting and
ending
points right is now the responsibility of drawJoin (and finish(),
when we're starting to emit the reverse path).
Of course, this has the benefit of not introducing any of the
junk lines you pointed out (and their equivalents in lineTo).
While this worked in most cases, it broke a few special cases:
quads like 0,0,100,100,50,50 and
cubics like 0,0,100,100,0,100,100,0
Like I said above, with my "fix" we rely on drawJoin to get the
starting and ending points of the curves right. However we don't
call drawJoin in between emitting the offsets of a curve's
subdivisions.
Most of the time this isn't a problem because the tangent at the
end
of a subdivision has the same direction as the tangent at the
beginning of the next subdivision so we don't need any of
drawJoin's
fancy logic. However, that is not true in those special cases, so
that's why my "fix" broke them.
We could bypass this pretty easily by just calling drawJoin in
between
subdivisions, and just making it do nothing if the end and start
tangents are parallel. That would have the possibly negative
effect
of drawing joins in the middle of the curve in those special cases
(which we can bypass by pretending the join is JOIN_BEVEL when
calling
it in between subdivisions of the same curve).
So, to summarize, if we do all this, it will clean up the output
paths
and the code would be less hacky, but it will add to the execution
time
in Stroker, and it does sound like quite a bit of work*, so I'm
not
sure
it's worth it. What do you think?
* Though about half this work is done and it's in the webrev
(which,
btw
means that the current webrev is broken, because I'm not handling
those
special cases. It can easily be changed back though). All that
would
be
left to do is to insert the drawJoin calls in between subdivisions
of curves.
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?
Definitely. Three months from now we wouldn't want to end up with
a situation like the above paragraph ;-)
!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 ;-)
:-D
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.
I wrote a microbenchmark, and it ran about 30-40% faster on my
machine
(and I'm pretty sure I avoided all the microbenchmark pitfalls).
As for completeness - it handles cases where x is NaN or an
infinity, and
all other x values are obvious, I think.
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
}
}
True. I think I'll stick with what I have though (less work this
way,
and we don't really gain anything by changing it).
Regards,
Denis.
----- Original Message -----
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 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.
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.
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
