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

[Jim Graham]

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

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?

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.

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

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

> 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