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

[Denis Lila]

Hi Jim.

> I noticed that you went through all of the Pisces files and udpated
> their cast spacing. In general, we tend to leave style issues alone
> until we are actually doing something else with the code so I think it
> is a bit overkill. It might be best to just restrict this change to
> Stroker.java for now. (But, in general, I'm all for code cleanup. ;-)

Ok.

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

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.