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

[Denis Lila]

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

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

This way, that rare special case is made slower and everything
else is made a bit faster.

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

> About that particular line, though, if the lenSq is not finite then it
> is either infinity (which would fail the < test) or it is NaN (which
> would fail any test), so I'm not sure that the test adds anything. In
> other words, it can only return true for "< miterLimit" if it is
> finite.
> Am I figuring that correctly?

You are indeed. I inserted it because I was afraid of
lenSq == Float.NEGATIVE_INFINITY
but lenSq is a sum of squares. It can't be negative anything. Doh!

I've removed it.

Updated webrev:
http://icedtea.classpath.org/~dlila/webrevs/7036754/webrev/

Regards,
Denis.

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

> 
> On 4/20/2011 6:40 AM, Denis Lila wrote:
> >> Finally, line 754 - I can't figure out where leftoff[0,1,4,5] were
> >> set
> >> up? Is that a bug?
> >
> > Oops, yeah, it's a bug. Setting leftoff[0,1,4,5] used to be done
> > before
> > the if, but then I moved it after for some reason, and it broke it.
> > To fix it I just moved {right,left}Off[0,1,4,5] before the if. Now
> > the
> > stuff inside the if parallels the code outside very nicely.
> 

> 
> >> While you are fixing indents. Stroker.java line 252 got shifted by
> >> a
> >> character from its former alignment. And somehow line 701 got
> >> shifted
> >> by a character, but no change is showing up in the webrevs...?
> >
> > I think I fixed this too.
> >
> > FYI: I also inserted this check:&& !isNotFinite(lenSq)
> > in drawMiter to guard against computeIntersection returning
> > computing
> > NaN values.
> 

> 

> 
> ...jim
> 
> > Regards,
> > Denis.
> >
> > ----- Original Message -----
> >> Hi Denis,
> >>
> >>
> >
> >>
> >>
> >> Also, your suggestion below makes sense to me...
> >>
> >> ...jim
> >>
> >> On 4/19/11 11:11 AM, Denis Lila wrote:
> >>>
> >>> Jim, what do you think about completely removing this code:
> >>>
> >>>           final boolean p1eqp2 = within(x1,y1,x2,y2, 6 * ulp(y2));
> >>>           final boolean p2eqp3 = within(x2,y2,x3,y3, 6 * ulp(y3));
> >>>           if (p1eqp2 || p2eqp3) {
> >>>               getLineOffsets(x1, y1, x3, y3, leftOff, rightOff);
> >>>               return 4;
> >>>           }
> >>>
> >>>           // if p2-p1 and p3-p2 are parallel, that must mean this
> >>>           curve is a line
> >>>           float dotsq = (dx1 * dx3 + dy1 * dy3);
> >>>           dotsq = dotsq * dotsq;
> >>>           float l1sq = dx1 * dx1 + dy1 * dy1, l3sq = dx3 * dx3 +
> >>>           dy3
> >>>           * dy3;
> >>>           if (Helpers.within(dotsq, l1sq * l3sq, 4 * ulp(dotsq)))
> >>>           {
> >>>               getLineOffsets(x1, y1, x3, y3, leftOff, rightOff);
> >>>               return 4;
> >>>           }
> >>>
> >>> This sort of duplicates logic in the caller. I put it in anyway,
> >>> because I was afraid that the subdivision would introduce
> >>> degenerate
> >>> curves, despite the checks in somethingTo passing. However,
> >>> looking
> >>> at it again, I don't really think it's necessary. The parallel
> >>> check
> >>> in particular was introduced because I was afraid that if a
> >>> curve's
> >>> control vectors are close to parallel, computeIntersection would
> >>> return crazy, inaccurate values. But I tested that and it doesn't
> >>> seem to happen. The isNotFinite checks should be enough to deal
> >>> with quads that are actually lines.
> >>>
> >>> If we just replaced that whole chunk above with
> >>>       if ((x1 == x2&& y1 == y2) || (x2 == x3&& y2 == y3)) {
> >>>           getLineOffsets(x1, y1, x3, y3, leftOff, rightOff);
> >>>           return 4;
> >>>       }
> >>>
> >>> I think we could squeeze a fair bit of performance out of this.
> >>>
> >>> I've updated the webrev.