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

[Jim Graham]

Hi Denis,

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.

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?

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