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

[Jim Graham]

Hi Denis,

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?

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.

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.

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

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

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?

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