Re: [OpenJDK 2D-Dev] Fwd: Various fixes to pisces stroke widening code
Hello Jim. > WRT the difference in the strokepath - as long as it fills correctly > then I don't think it is out of spec as we only specify that > BasicStroke returns a path that fills the appropriate pixels. Is > their fill area correct? Ah. That's why I said I wasn't sure this was a bug. If that's what the spec says, then it's not a bug, because the fill areas are the same. > But, it would be a nice thing to have some consistency, and it begs > the question - is there some error that this alternate stroking technique > might introduce that we just haven't found yet? For instance, is it > robust with respect to winding rules if you overlay 2 such stroked > objects? Also, it looks like a simple error in choosing which pairs > of points to connect when rounding a bend (probably back to the initial > moveto?). I was thinking the "problem" was that instead of going to the position of the last moveTo, we were going to the position of the last moveTo + Offset. That would explain the difference in the top left, but not the bottom right. I will have to look at Stroker some more. Regards, Denis. - "Jim Graham" wrote: > Hi Denis, > > Clipping geometry outside of a clip is a topic for another discussion. > > One issue to keep in mind is that floating point clipping equations > can > sometimes return numbers that are N.4 or N.5 or N.50001 and > that > subtle difference may not matter for AA, but it can cause a pixel to > appear or disappear under non-AA rendering which would be bad if it > happens when the scene is already rendered and then you have to update > a > small clipped part of it for some repaint() call and the rendering > turns > out different during that update due to the clipping math... 8-| So, > I > typically only cull/clip items when I am down to the bottom level and > I > can directly know how the equations are going to relate to the final > numbers that are used to choose pixels. > > ...jim > > On 8/10/2010 6:42 AM, Denis Lila wrote: > > 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.
Re: [OpenJDK 2D-Dev] Fwd: Various fixes to pisces stroke widening code
Hi Denis, WRT the difference in the strokepath - as long as it fills correctly then I don't think it is out of spec as we only specify that BasicStroke returns a path that fills the appropriate pixels. Is their fill area correct? But, it would be a nice thing to have some consistency, and it begs the question - is there some error that this alternate stroking technique might introduce that we just haven't found yet? For instance, is it robust with respect to winding rules if you overlay 2 such stroked objects? Also, it looks like a simple error in choosing which pairs of points to connect when rounding a bend (probably back to the initial moveto?). Clipping geometry outside of a clip is a topic for another discussion. One issue to keep in mind is that floating point clipping equations can sometimes return numbers that are N.4 or N.5 or N.50001 and that subtle difference may not matter for AA, but it can cause a pixel to appear or disappear under non-AA rendering which would be bad if it happens when the scene is already rendered and then you have to update a small clipped part of it for some repaint() call and the rendering turns out different during that update due to the clipping math... 8-| So, I typically only cull/clip items when I am down to the bottom level and I can directly know how the equations are going to relate to the final numbers that are used to choose pixels. ...jim On 8/10/2010 6:42 AM, Denis Lila wrote: 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.
Re: [OpenJDK 2D-Dev] Fwd: Various fixes to pisces stroke widening code
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. 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. But, these 3 are really getting down to the nitty gritty. I'd check it in before I drive you crazy... ;-) ...jim Denis Lila wrote: - Forwarded Message - From: "Denis Lila" To: "Jim Graham" 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" 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" 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 t
[OpenJDK 2D-Dev] Fwd: Various fixes to pisces stroke widening code
- Forwarded Message - From: "Denis Lila" To: "Jim Graham" 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" 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" 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.
