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

[Jim Graham]

Why don't you back them out and put up a webrev so I can proof it before 
you push.  I'll get to it today...

		...jim

On 4/26/2011 2:11 PM, Denis Lila wrote:
>> 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.
>
> Makes sense. Ok.
> Though, making Stroker slower may be worth it if paths are simplified
> and future stages are made faster.
>
>> 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?
>
> Argh, sorry, I forgot to copy it into the server.
> It's updated now.
>
> After I back out the latest changes, should I push it? I'll include
> the regression test you gave me.
>
> Regards,
> Denis.
>
> ----- Original Message -----
>> Hi Denis,
>>
>
>>
>> ...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