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

[Denis Lila]

> Why don't you back them out and put up a webrev so I can proof it
> before
> you push. I'll get to it today...

Sure (I only posted the one with the changes because I thought
you wanted to see what, exactly, I had done).
The webrev is updated. I included the regression test.
I also removed a comment from computeIntersection. It didn't
belong there, since that was a general intersection computing
routine, and didn't necessarily have anything to do with miters.


Regards,
Denis.

----- Original Message -----

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