Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Denis,
This should be the last batch of comments, most of which may require a
10 second answer. Most of them could just be ignored as they are minor
optimizations. There are only a couple where I think something is "off"...
PiscesRenderingEngine.java:
lines 279 and 303 - maybe add Helpers.nearZero(v, nulp) for these? For
one thing, you compute the same values twice - hopefully the compiler
will notice that.
line 303 - could use either within or nearZero, same comment about the
value being computed twice. Also, aa+cc gets computed later, not sure
if the compiler is smart enough to deal with that.
line 325 - you don't need to clone at. "outat = at" should be enough.
A note for next revision - you don't need to include the translation in
outat and inat, it would be enough to use just the abcd part of the
transform. Right now, the TransformingPathConsumer implementations tend
to assume translation, but we could create variants for the
non-translating cases and maybe save some work.
Dasher.java:
lines 98-101 - somewhere in the confusion moveToImpl became redundant
with moveTo.
lines 174-175 - lineToImpl now redundant too.
line 368 - does this cause a problem if t==1 because lastSegLen will now
return the wrong value? Maybe move this into an else clause on the
"t>=1" test?
line 405 - I didn't understand why you use "2*err*(len+leftLen)" as the
termination condition, why not just err?
Stroker.java:
lines 200,236 - for the most part, aren't these redundant?
lines 358-361 - lineToImpl redundant now?
line 390 - should finish() really be done here? That would mean that
moveTo,close would try to emit something and close,close would as well.
Is that right?
line 800 - technically, this could be [0] and [1] since all points on
the curve are the same. ;-)
line 805,810 - abs()?
line 821,824 - you add 1 to nCurves just to subtract 1 later? Maybe
rename it to nSplits and skip the +/-1?
...jim
On 10/21/2010 1:52 PM, Jim Graham wrote:
Hi Denis,
I'll be focusing on this later today, just a last proofread.
In the meantime, can you outline the tests that you ran?
Also, have you used J2DBench at all? I know you ran some of your own
benchmarks, but didn't know if you were familiar with this tool:
{OpenJDK}/src/share/demo/java2d/J2DBench/
...jim
On 10/21/2010 12:27 PM, Denis Lila wrote:
Good to push?
http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
This comment may make more sense if I explain that the condition for when finish() is executed in closePath is the reverse of the condition in move and done. I agree that it should return if prev!=OP_TO, but I'm not so sure about doing a finish(). Does closed JDK emit something on moveto,close? (Or close,close?) ...jim On 10/21/2010 6:10 PM, Jim Graham wrote: Hi Denis, I saw something in the latest webrev that reminded me of an earlier comment. On 10/18/2010 2:21 PM, Denis Lila wrote: line 389 - The test here is different from closePath. What if they were both "prev == DRAWING_OP_TO"? I am now using prev!=DRAWING_OP_TO (not ==, since it is supposed to execute finish() if no nonzero length lines have been fed to Stroker yet). In fact I have removed the "started" variable since it's equivalent to prev==DRAWING_OP_TO. It looks like closePath still uses a different test than moveTo and pathDone. They all test for DRAWING_OP_TO, but closepath uses != whereas the others use ==. Is that right? ...jim
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Denis, I saw something in the latest webrev that reminded me of an earlier comment. On 10/18/2010 2:21 PM, Denis Lila wrote: line 389 - The test here is different from closePath. What if they were both "prev == DRAWING_OP_TO"? I am now using prev!=DRAWING_OP_TO (not ==, since it is supposed to execute finish() if no nonzero length lines have been fed to Stroker yet). In fact I have removed the "started" variable since it's equivalent to prev==DRAWING_OP_TO. It looks like closePath still uses a different test than moveTo and pathDone. They all test for DRAWING_OP_TO, but closepath uses != whereas the others use ==. Is that right? ...jim
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Jim.
> In the meantime, can you outline the tests that you ran?
I ran Java2D without any problems. There's also been an
icedtea bug http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=450
related to a look and feel (tinylaf:
http://www.muntjak.de/hans/java/tinylaf/tinylaf-1_4_0.zip
to run a demo, download, extract and run tinycp.jar) that wasn't being
painted properly. Now it looks good (I think there's no LCD text antialiasing,
but
that's a different story).
I also wrote rendered at least 5 random cubic curves. I didn't
inspect these too closely - this test was just to find any obvious
errors. There weren't any.
Most importantly, I also ran this test suite:
http://icedtea.classpath.org/hg/gfx-test/
It generates a lot of images using a reference java implementation and
an implementation to be tested. I used closed source java as a reference,
and I ran it using a fresh checkout of the 2d branch of openjdk7:
http://icedtea.classpath.org/~dlila/prevPiscesSungfx/results/
and a fresh checkout of the 2d branch with my patch applied:
http://icedtea.classpath.org/~dlila/latestPiscesSungfx/
As you can see, the number of images that are the same as the reference
implementation has increased subtantially. It has also increased
consistenly across all categories.
I've gone through most of the images, and the ones that are different
aren't dramatically different. The differences are barely noticeable.
The improvement is even greater when compared to openjdk6 which produces
some pretty scary test results, especially in the scaled rectangles
category (but I haven't uploaded these tests so you'll just have to trust
me on this ;-) )
NOTE: the webrev has changed slightly since the last e-mail I sent. The
gfx test suite revealed a bug in the drawing of dashed rectangles, so
to fix it I have changed a line in Dasher.closePath from "if(needsMoveTo) {"
to "if(needsMoveTo || !dashOn) {"
> Also, have you used J2DBench at all? I know you ran some of your own
> benchmarks, but didn't know if you were familiar with this tool:
>
> {OpenJDK}/src/share/demo/java2d/J2DBench/
I was going to run these today, but fixing the dashing bug above and
rerunning the tests took a while and it's already 8:30 pm here and I have to
go home. I'll run them tomorrow morning.
Regards,
Denis.
- "Jim Graham" wrote:
> Hi Denis,
>
> I'll be focusing on this later today, just a last proofread.
>
>
> ...jim
>
> On 10/21/2010 12:27 PM, Denis Lila wrote:
> > Good to push?
> >
> > http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Denis,
I'll be focusing on this later today, just a last proofread.
In the meantime, can you outline the tests that you ran?
Also, have you used J2DBench at all? I know you ran some of your own
benchmarks, but didn't know if you were familiar with this tool:
{OpenJDK}/src/share/demo/java2d/J2DBench/
...jim
On 10/21/2010 12:27 PM, Denis Lila wrote:
Good to push?
http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Jim. > When one talks about curves and being parallel, my mind > tends to think of the tangents of the curves being parallel and > tangents are directed by the first derivative. That's what I was trying to express. The tangents of curves A and B at t are parallel if and only if there's some c such that A'(t) = c*B'(t), which is what I defined || to mean, although perhaps I didn't make this clear enough. > Also, if you are going to use your definition of "vector" then > parallel is an odd term to use I agree. I was never comfortable with using "parallel" but I couldn't find a better word. How about "aligned"? Anyway, I've changed the comments to something that hopefully will avoid this sort of confusion in the future. Other than this, I think I've followed all your other suggestions (putting some of the degenerate curve handling in separate functions, fixing the style issues, removing unused methods, etc). I also fixed a bug in Renderer.edgeSetCurY where I was using edges[idx+MINY] instead of edges[idx+CURY] (which produced visible artifacts on nearly horizontal lines). I am also now using your TransformingPathConsumer2D class, which simplified things quite a bit where ever it was used. Good to push? http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/ Regards, Denis. - "Jim Graham" wrote: > OK, I can see how your terminology works now, but it seems odd to me. > I > never consider re-expressing the coordinates on a curve as a vector > and > basing geometric properties on those constructed vectors. I either > consider the points on the curve, or its tangent or its normal - none > of > which is the value you are expressing. You are, essentially, > operating > on tangent vectors, but you aren't calling them that, you are calling > > them something like "the vector of the derivative" which is a relative > > (direction only) version of the tangent vector (which has location and > > direction). > for values that originate from the same point > (points considered as a vector are taken to originate from 0,0) - > really > you want those "vectors" to be collinear, not (just) parallel. > > So, either || means "the coordinates of the curves expressed as > vectors > are collinear" or it means "the curves (i.e. the tangents of the curve > > at the indicated point) are parallel". Saying "vector I() is parallel > > to vector B()" didn't really have meaning to me based on the above > biases. > > So, I get your comment now and all of the math makes sense, but the > terminology seemed foreign to me... > > ...jim > > On 10/20/10 10:48 AM, Denis Lila wrote: > >> Also, how is A(t) and B(t) are parallel not the same as "the curves > A > >> and B are parallel at t"? > > > > Well, suppose A and B are lines with endpoints (0,0), (2,0) for A > > and (0,1),(2,1) for B. Obviously, for all t, A and B are parallel at > t. > > However let t = 0.5. Then A(t) = (1,0) and B(t) = (1, 1). The > vectors > > (1,0) and (1,1) are not parallel, so saying A(t) || B(t) is the > same > > as saying that there exists c such that (1,0) = c*(1,1), which isn't > true. > > > > However, A'(t)=(2,0) and B'(t)=(2,0), and the vectors > > (2,0) and (2,0) are parallel. > > > > Does this make more sense? > > > > Regards, > > Denis. > > > > - "Jim Graham" wrote: > > > >> On 10/20/10 7:54 AM, Denis Lila wrote: > In #2, you have a bunch of "I'() || B'()" which I read as "the > >> slope > of the derivative (i.e. acceleration) is equal", don't you > really > >> mean > "I() || B()" which would mean the original curves should be > >> parallel? > Otherwise you could say "I'() == B'()", but I think you want to > >> show > parallels because that shows how you can use the dxy1,dxy4 > values > >> as > the parallel equivalents. > >>> > >>> Not really. I've updated the comment explaining what || does, and > >>> it should be clearer now. Basically, A(t) || B(t) means that > >> vectors > >>> A(t) and B(t) are parallel (i.e. A(t) = c*B(t), for some nonzero > >> t), > >>> not that curves A and B are parallel at t. > >> > >> I'm not sure we are on the same page here. > >> > >> I'() is usually the symbol indicating the "derivative" of I(). My > >> issue > >> is not with the || operator, but with the fact that you are > applying > >> it > >> to the I'() instead of I(). > >> > >> Also, A(t) = c*B(t) is always true for all A and B and all t if > you > >> take > >> a sample in isolation. Parallel means something like "A(t) = > c*B(t) > >> with the same value of c for some interval around t", not that the > >> values at t can be expressed as a multiple. > >> > >> Again, I'() || B'() says to me that the derivative curves are > >> parallel, > >> not that the original curves are parallel... > >> > >>...jim
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hello Jim. > So either it is always on the right side, always on the wrong side > (i.e. just reverse the rotation in the math), or always on the right/wrong > side depending on the CWness of the join angle - which would be > reflected in rev... No? That's a good point. I've changed the test to simply if(rev) because the normal we compute is on the left of the original vector (omx,omy)-(mx,my). So, if the arc was counter clockwise no adjustment was needed. Regards, Denis. - "Jim Graham" wrote: > Right, but it seemed to me that if omxy was the "from" vector and mxy > > was the "to" vector, that the computed mmxy should always be > predictably > on the same side of it, no? If it was on the wrong side then it > wouldn't be a random occurence, it must be related to the input data. > > ...jim > > On 10/20/10 10:29 AM, Denis Lila wrote: > >> Cool, but above I was also asking the same question about line > 231, > >> and you provided a lot of information about line 231 (and a test to > verify > >> it), but didn't answer if the test in line 231 also tracks rev the > >> same way...? > > > > Oh, no, line 231 isn't mean to be related to rev at all. It just > checks > > to see on which side of the (omx,omy),(mx,my) chord the computed > (mmx, mmy) > > is. > > > > Regards, > > Denis. > > > > - "Jim Graham" wrote: > > > >> Hi Denis, > >> > >> One clarification: > >> > >> On 10/20/10 7:11 AM, Denis Lila wrote: > When would the isCW test trigger? Does it track "rev"? What > >> happens > at 180 degrees (is that test reliable for the randomization that > >> might > happen when omxy are directly opposite mxy)? > >>> > >>> isCw is used for computing the arc bisector by testing whether > the > >> computed > >>> point is on the side it should be (and multiplying by -1 if not), > it > >> is used > >>> to compute the sign of cv in drawBezApproxForArc, and for > computing > >> rev. > >>> > The only reason I ask is > because I think the sign of mmxy is probably controllable by > understanding the input conditions, but this test should be safe > (modulo if it really works at 180 degrees). If it has failure > >> modes at 180 > degrees then reworking the math to produce the right sign in the > >> first > place may be more robust for that case. A test for this is to > >> render > "(0,0) -> (100,0) -> (0,0)" with round caps and then rotate > it > >> through > 360 degrees and see if the round caps invert at various angles. > >>> > >>> I already did that. I drew 100 lines like the one you describe. I > >> attached > >>> the results. It never fails. It is still possible that there > could > >> be some > >>> case where it fails, but this does prove that such a case would > be > >> very rare. > >>> > Also, line 256 - does that track "rev"? > >>> > >>> It does. I changed the test to if(rev). > >> > >>...jim
