[OpenJDK 2D-Dev] Fwd: Various fixes to pisces stroke widening code

Denis Lila dlila at redhat.com
Tue Aug 10 13:42:29 UTC 2010


Hi Jim.

> What was the problem - I might have a guess as to the cause if I saw a 
> picture.  You should file a bug against it to make sure it doesn't fall 
> through the cracks.
    I can confirm that none of my changes introduced the bug. I just tested it
with a truly fresh build of openjdk7. I've attached 3 screenshots with 
results. 

> But, these 3 are really getting down to the nitty gritty.  I'd check it 
> in before I drive you crazy... ;-)
    Good idea :)
    I'll keep the 3 comments in mind for future work. I actually already
implemented something like your third suggestion, but it was along with
some other changes and I introduced a bug, so I discarded it because I
didn't want to spend too much time debugging.
    Speaking of which, wouldn't it be a good idea to clip lines (perhaps
earlier than Renderer, so that Stroker, Dasher, and Renderer can benefit
from it)? We could only do this for lines that are out of bounds vertically,
otherwise anti aliasing would break, but for lines that are out of bounds
horizontally we could collapse their x coordinate to boundsMinX-1 or boundsMaxX+1
(or bounds +/- lineWidth, if we're doing it before Renderer.java).
That would at least reduce their length, and I can't see how it might break
anti aliasing.
    Am I missing anything? Or would this simply not be worth the added processing?

Thanks,
Denis.



----- "Jim Graham" <james.graham at oracle.com> wrote:


> 
> But, the webrev you sent looks good to go.
> 
> If you want some more optimization comments, I'll always have more
> (I'm 
> evil that way)...  ;-)
> 
> - Testing for y0==y1 in lineTo is redundant.  addEdge already ignores
> 
> the line with a looser test.  It does take more processing to reject
> the 
> edge in that case, though, but that test in lineTo is saving less and
> 
> less work with every revision.
> 
> - pix_sxy0 aren't really needed.  close() could simply:
> 	addEdge(x0, y0, sx0, sy0);
> 	this.x0 = sx0;
> 	this.y0 = sy0;
> and bypass what little processing is left in lineTo.
> 
> - addEdge rejects horizontal edges.  It could also reject any of the 
> following classes of edges:
> 	- completely above clip
> 	- completely below clip
> 	- completely to the right of clip
> since those edges will never contribute to the coverage.  The
> algorithm 
> should skip the above and below edges reasonably quickly, but this
> would 
> save storage for them.  The edges to the right would have to be
> updated 
> every scanline and waste storage, but that isn't a huge hit.  This
> only 
> helps for corner-case huge paths which aren't common.  At least you 
> aren't iterating over miles and miles of irrelevant geometry which
> would 
> be an important performance hit.
> 

> 
> 		...jim
> 
> Denis Lila wrote:
> > ----- Forwarded Message -----
> > From: "Denis Lila" <dlila at redhat.com>
> > To: "Jim Graham" <james.graham at oracle.com>
> > Sent: Monday, August 9, 2010 4:58:10 PM GMT -05:00 US/Canada
> Eastern
> > Subject: Re: [OpenJDK 2D-Dev] Various fixes to pisces stroke
> widening code
> > 
> > Hi Jim.
> > 
> > Good idea. I've implemented it. I also noticed the quicksort 
> > method wasn't very friendly to 0 length arrays. I fixed that.
> > I ran Java2Demo and a few of my tests, and everything looks good.
> > http://icedtea.classpath.org/~dlila/webrevs/fpBetterAAv2/webrev/
> > 
> > Everything, except the joins demo, that is. The interior of the
> > star's right arm (our left) and left leg is not drawn exactly like
> > in proprietary java or openjdk6. I am not sure if this is a bug
> > since I don't know exactly how the results of a
> bs.createStrokedShape
> > are supposed to look. However, I am almost certain that this
> > isn't my fault, since I can reproduce the behaviour with an only 
> > slightly changed version of openjdk7 with completely different
> > changes than what's in this patch. I'll run it next morning with
> > a fresh build of openjdk7 to be completely sure.
> > 
> > Can I push it?
> > 
> > Thanks,
> > Denis.
> > 
> > ----- "Jim Graham" <james.graham at oracle.com> wrote:
> > 
> >> To the end of getting rid of "flips" - all they are used for is to
> 
> >> resize the crossings array, right?  This is not a huge array that
> >> costs 
> >> a lot to resize - why not simply use a default array of, say, 30 
> >> elements and then resize it if we ever have more crossings than
> that?
> >>
> >> Only very complicated paths would have more than 30 crossings to
> >> track. 
> >>   The check for array length is only needed once per scanline since
> we
> >>
> >> know how many active edges are on each scan line (hi-lo) and you
> can 
> >> only have 1 crossing per active edge so with one test per scanline
> we
> >>
> >> can keep the crossings array within range...
> >>
> >> 			...jim
> >>
> >> Jim Graham wrote:
> >>> Hi Denis,
> >>>
> >>> Well, I guess it's a good thing that Java2Demo had a path like
> that
> >> in 
> >>> it - not a very common case, so it's good we found it!
> >>>
> >>> The fix looks fine.  It still seems like there is way more logic
> >> there 
> >>> than is needed - hopefully if we can get rid of flips at some
> point,
> >>> much of it will go away.
> >>>
> >>> Fixes look good to go to me...
> >>>
> >>>             ...jim
> >>>
> >>> On 8/5/2010 3:58 PM, Denis Lila wrote:
> >>>> Hi Jim.
> >>>>
> >>>> I didn't know about Java2Demo. If I did I would have run it
> >> sooner.
> >>>> But I ran it a few hours ago, and everything looked fine
> >> (surprisingly
> >>>> high fps too) until I got to the append test.
> >>>>
> >>>> Apparently I introduced a bug when solving the "2 consecutive
> >> moveTos 
> >>>> bug".
> >>>> Basically, when there's a close() after a horizontal lineTo(),
> the
> >> lineTo
> >>>> in close() won't be executed because it's inside the if 
> >>>> (firstOrientation != 0)
> >>>> test. So instead of going back to the starting point, close will
> >> stay 
> >>>> where
> >>>> it is, which will draw a triangle above the rectangle.
> >>>>
> >>>> I fixed this by introducing a variable that keeps track of the
> last
> >>>> method
> >>>> called (lineTo, moveTo, or close), and instead of checking for 
> >>>> firstOrientation != 0
> >>>> in close(), I check for (last == LINE_TO).
> >>>>
> >>>> webrev (hopefully final):
> >>>> http://icedtea.classpath.org/~dlila/webrevs/fpBetterAAv2/webrev/
> >>>>
> >>>> I'm sorry about this. I wish I had known about Java2Demo sooner.
> >>>>
> >>>> Thanks,
> >>>> Denis.
> >>>>
> >>>> ----- "Jim Graham"<james.graham at oracle.com>  wrote:
> >>>>
> >>>>> Hi Denis,
> >>>>>
> >>>>> That's great!  I just did a last minute double-check of your
> last
> >>>>> (final) webrevs to be sure.
> >>>>>
> >>>>> Have you tested Java2Demo with these changes?  I'd also run any
> >>>>> regression tests you can find with the changes.  If there are
> no
> >>>>> problems there, then you are good to go to push it...
> >>>>>
> >>>>>             ...jim
> >>>>>
> >>>>> On 8/5/2010 8:08 AM, Denis Lila wrote:
> >>>>>> Hello.
> >>>>>>
> >>>>>>> Are you a registered OpenJDK developer?
> >>>>>> I am now.
> >>>>>> Can I go ahead and push it?
> >>>>>>
> >>>>>> Regards,
> >>>>>> Denis.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: triangleOpenJDK7.png
Type: image/png
Size: 7052 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20100810/01a787be/triangleOpenJDK7.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: triangleGood.png
Type: image/png
Size: 13970 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20100810/01a787be/triangleGood.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Java2DStarOpenJDK7.png
Type: image/png
Size: 28534 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20100810/01a787be/Java2DStarOpenJDK7.png>


More information about the 2d-dev mailing list