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

[Denis Lila]

> Keep in mind that there is a deadline coming up for JDK7 where making
> bug fixes will get a lot more bureaucratic so we should probably get
> this fix in before the paperwork sets in.
> 
> It sounds like this isn't worth pursuing now for many reasons so I
> think
> we should back off on it, especially if it has created new problem
> cases
> and caused Stroker to run slower.

Makes sense. Ok.
Though, making Stroker slower may be worth it if paths are simplified
and future stages are made faster.
 
> I looked at the webrev at the bottom of this email and it didn't seem
> to have any of the fixes you were talking about. Did I not get a link to
> an updated webrev or did you back out all the work you did?

Argh, sorry, I forgot to copy it into the server.
It's updated now.

After I back out the latest changes, should I push it? I'll include
the regression test you gave me.

Regards,
Denis.

----- Original Message -----
> Hi Denis,
> 

> 
> ...jim
> 
> On 4/26/2011 1:01 PM, Denis Lila wrote:
> >>> 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