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

[Jim Graham]

Hi Denis,

Looks good except for a typo in a comment on computeIntersection().  You 
have duplicated the entire phrase "in m[off] ... in m[off] ..." twice.

I don't need to see a webrev of fixing the comment.  It looks good to go...

			...jim

On 4/27/2011 7:57 AM, Denis Lila wrote:
>> 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...
>
> Sure (I only posted the one with the changes because I thought
> you wanted to see what, exactly, I had done).
> The webrev is updated. I included the regression test.
> I also removed a comment from computeIntersection. It didn't
> belong there, since that was a general intersection computing
> routine, and didn't necessarily have anything to do with miters.
>
>
> Regards,
> Denis.
>
> ----- Original Message -----
>
>>
>> ...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