[OpenJDK 2D-Dev] RFC: Fix for 7036754

[Denis Lila]

> > 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