Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Denis, I think the first version is a better choice for now since you said that the performance difference isn't noticeable. I think the lower level flattening might look a little different if we ever decide to upgrade the pipeline to deal with curves. In particular, you are still flattening above the dashing/stroking code and I think the flattening should be done below that code (i.e. in Renderer). So, I'd go with the first one with the following comments: - You indent by 8 spaces in a few places. Is that a tabs vs. spaces issue? We try to stick to 4 space indentations with no tabs for consistentcy. - I'd make the internal error message a little less personal. ;-) "normalization not needed in OFF mode" or something. - lines 362,363 - you don't need to set cur_adjust variables here, they are already being set below. Other than that, it looks good to go... ...jim Denis Lila wrote: Hi Jim. So, I have the nicer webrevs. FlatteningIterator version: http://icedtea.classpath.org/~dlila/webrevs/fpWithStrokeControl/webrev/ Pisces flattening version: http://icedtea.classpath.org/~dlila/webrevs/fpWithSCandPiscesFlattening/webrev/ I dealt with the issue of handling OFF by just not accepting it as an input. After all, a normalizing iterator only needs to be created, and is only created if the normalization mode is not OFF. Thanks, Denis. - "Jim Graham" wrote: Hi Denis, I'll wait for some clean webrevs once you get the float stuff in for a final review. I did take a really quick look and thought that a better way to handle "OFF" would be to set rval to -1 and then check "rval < 0" as the (quicker) test for OFF in the currentSegment() method. Does that make sense? In any case, let's wait for cleaner webrevs to go further on this (hopefully in a day or so?)... ...jim On 8/5/2010 8:06 AM, Denis Lila wrote: Hi Jim. I made all the suggested changes. Links: http://icedtea.classpath.org/~dlila/webrevs/fpWithStrokeControl/webrev/ http://icedtea.classpath.org/~dlila/webrevs/fpWithSCandPiscesFlattening/webrev/ Thanks, Denis. - "Jim Graham" wrote: Hi Denis, First, comments on the high level normalizer (Normalizing iterator): - If there is no normalization going on, I would use the Shape's own flattening (i.e. getPathIterator(at, flat)). The reason being that some shapes may know how to flatten themselves better, or faster, than a Flattening Iterator. In particular, rectangles and polygons would simply ignore the argument and save themselves the cost of wrapping with an extra iterator. This would probably only be a big issue for very long Polygons. - Line 331 - the initializations to NaN aren't necessary as far as I can tell...? - Rather than saving "mode" in the normalizing iterator, how about saving 2 constants: (0.0, 0.5) for AA and (0.25, 0.25) for non-AA and then simply add those constants in rather than having to have the conditional with the 2 different equations? ...jim
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Jim. So, I have the nicer webrevs. FlatteningIterator version: http://icedtea.classpath.org/~dlila/webrevs/fpWithStrokeControl/webrev/ Pisces flattening version: http://icedtea.classpath.org/~dlila/webrevs/fpWithSCandPiscesFlattening/webrev/ I dealt with the issue of handling OFF by just not accepting it as an input. After all, a normalizing iterator only needs to be created, and is only created if the normalization mode is not OFF. Thanks, Denis. - "Jim Graham" wrote: > Hi Denis, > > I'll wait for some clean webrevs once you get the float stuff in for a > > final review. I did take a really quick look and thought that a > better > way to handle "OFF" would be to set rval to -1 and then check "rval < > 0" > as the (quicker) test for OFF in the currentSegment() method. Does > that > make sense? > > In any case, let's wait for cleaner webrevs to go further on this > (hopefully in a day or so?)... > > ...jim > > On 8/5/2010 8:06 AM, Denis Lila wrote: > > Hi Jim. > > > > I made all the suggested changes. > > Links: > > > http://icedtea.classpath.org/~dlila/webrevs/fpWithStrokeControl/webrev/ > > > http://icedtea.classpath.org/~dlila/webrevs/fpWithSCandPiscesFlattening/webrev/ > > > > Thanks, > > Denis. > > > > - "Jim Graham" wrote: > > > >> Hi Denis, > >> > >> First, comments on the high level normalizer (Normalizing > iterator): > >> > >> - If there is no normalization going on, I would use the Shape's > own > >> flattening (i.e. getPathIterator(at, flat)). The reason being > that > >> some > >> shapes may know how to flatten themselves better, or faster, than > a > >> Flattening Iterator. In particular, rectangles and polygons would > >> simply ignore the argument and save themselves the cost of > wrapping > >> with > >> an extra iterator. This would probably only be a big issue for > very > >> long Polygons. > >> > >> - Line 331 - the initializations to NaN aren't necessary as far as > I > >> can > >> tell...? > >> > >> - Rather than saving "mode" in the normalizing iterator, how about > >> saving 2 constants: (0.0, 0.5) for AA and (0.25, 0.25) for non-AA > and > >> > >> then simply add those constants in rather than having to have the > >> conditional with the 2 different equations? > >> > >>...jim
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.
[OpenJDK 2D-Dev] hg: jdk7/2d/jdk: 6967436: lines longer than 2^15 can fill window.; ...
Changeset: d47bd9d94ba4 Author:dlila Date: 2010-08-10 13:19 -0400 URL: http://hg.openjdk.java.net/jdk7/2d/jdk/rev/d47bd9d94ba4 6967436: lines longer than 2^15 can fill window. 6967433: dashed lines broken when using scaling transforms. Summary: converted pisces to floating point. Also, using better AA algorithm Reviewed-by: flar ! src/share/classes/sun/java2d/pisces/Dasher.java ! src/share/classes/sun/java2d/pisces/LineSink.java - src/share/classes/sun/java2d/pisces/PiscesMath.java ! src/share/classes/sun/java2d/pisces/PiscesRenderingEngine.java ! src/share/classes/sun/java2d/pisces/Renderer.java ! src/share/classes/sun/java2d/pisces/Stroker.java - src/share/classes/sun/java2d/pisces/Transform4.java
