> Are you happy with the current variable names? Not really. The names you suggested are much better. I'm using them now. As for making a vector class, I think we should push this and then decide. It's absence has already done most of the damage it could possibly do, so it's not an urgent matter. And besides, pushing a good version of this first will make it easier to determine the performance impact of the vector class.
> line 208 - isn't this just "side = false" since side is either 0 or
> 1?
> Also, side is only ever 1 for an end cap in which case we need exactly
> 2 90 degree beziers which are very simple to compute and could be hard
> coded. Was there a reason not to just have a special "roundCap"
> function which would be 2 hardcoded and fast emitCurveTo calls? The
> code would be something like:
> curveTo(/*x+mx, y+my,*/
> x+mx-C*my, y+my+C*mx,
> x-my+C*mx, y+mx+C*my,
> x-my, y+mx);
> curveTo(/*x-my, y+mx,*/
> x-my-C*mx, y+mx-C*my,
> x-mx-C*my, y-my+C*mx,
> x-mx, y-my);
> where C = 0.5522847498307933;
> // Computed btan constant for 90 degree arcs
>
> (rest of drawRoundJoin method - it may take some doing, but eventually
> this method should simplify based on: there will only ever be 1 or 2
> curves, Math.sin(Math.atan2()) cancels out as does
> Math.cos(Math.atan2()) though to do so requires Math.hypot() which is
> a simpler calculation than any of the transcendentals. So, if there was
> an easy test for "acute/obtuse angle" and a way to find the center of
> an angle (both I'm sure we could find on the net), then we could
> eliminate the transcendentals from this method).
I introduced a drawRoundCap method. This eliminated the side argument from
the round join drawing, which made it easier to eliminate the trig function
calls. I did this by using dot products to compute cosines (which was possible
because now Stroker takes only untransformed paths, and all lineWidths are the
same), and I used the double angle identities to compute any sines.
I came up with my own ways of detecting acute/obtuse angles and finding the
centres
of angles ("my own" meaning I didn't look at any websites), and they consist of:
1. if (omx * mx + omy * my) >= 0 then the angle is acute (line 200).
2. I explain this in a comment in the file (line 208).
> I would combine the emit*To methods into just the one version that
> takes a boolean. The number of times you call them without the boolean are
> few and far between and the versions that don't take the boolean are
> so few lines of code that inlining them into the boolean versions of the
> methods will still make for nice and tight code.
I was tempted to do this. I didn't because the boolean versions will need
absolute coordinates, while the non boolean ones require relative ones. So
if the non boolean versions need to be called and all we have are the boolean
ones, 2 dummy arguments need to be supplied. However, I've looked at the code
more closesly, and it turns out that we only use the non boolean versions
from inside the boolean versions, so I've followed your suggestion (except
on emitLineTo, since the non boolean version of that is used quite a bit).
> line 374 - why is this moveto here? Doesn't this break the joined
> path into 2 separate paths? Should this be a lineto?
It does break it into 2 separate paths, but that's the correct behaviour
in this case. Mathematically speaking, the 2 offset curves are spearate
curves (despite any intersections). This changes when we use caps, but
when using closePath(), caps aren't drawn so we <i>should</i> have 2 separate
paths. This is also the behaviour of oracle's closed source java (which
can be seen in one of the Java2Ddemo demos - the one that draws the offset
curves of a star with a vertical slider controlling the line width).
> (Also, sx0==x0 and sy0==y0 at this point).
I am now using s*0 instead of *0, since the expressions involve sdx and sdy,
so it's a bit clearer.
> 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.
> line 337 - shouldn't this just return? I don't think that empty lines
> should modify the path at all. If this is a case of "moveto(x,y);
> lineto(x,y)" then the finish() code should deal with the "path that
> never went anywhere - i.e. drawing a dot", shouldn't it? The only
> problem is that moveTo never set up omx,omy so finish will likely draw
> something random. Perhaps if moveto (and closepath) simply set up
> omx,omy to w,0 (or 0,-w if you prefer) then finish would do a
> reasonable thing for empty paths?
The reason I made it the way it is is to match what proprietary java
does. If one tries to draw a path like moveTo(0,0);lineTo(100,-100);
lineTo(100,-100);lineTo(0,-200); instead of ignoring the second lineTo(100,-100)
it will instead behave as if it were something like
lineTo(100.00001,-100.00001),
and it will draw the join. Of course, just because proprietary java does it, it
doesn't mean it's the right thing to do. So, should I still make it ignore
segments
of 0 length?
> line 486 - this leaves the current point in a different place than
> line 510, does that matter?
No. 486 sets the first point of the consumer we're feeding to the first
point of one of the offset curves. Line 510 makes sure the inner offsets of
non parallel, consecutive segments join together nicely. 486 and 510 aren't
really closely related. I don't even think 510 is necessary to generate
correct fill volumes, but it makes outlines look nicer. I've attached 2
screenshots of the star Java2DDemo that show what 510 does.
> line 283 - doesn't this simplify to?:
> t = x10p*(y0-y0p) - y10p*(x0-x0p)
> (source: http://local.wasp.uwa.edu.au/~pbourke/geometry/lineline2d/)
> then calculating:
> t = (...)/den;
> can amortize the dividend from the following 2 calculations.
I am using this t calculation now. I don't see how what I had simplified
into this though. This is makes me think we were using a wrong equation, which
is
puzzling since I didn't notice any problems with drawing miters or quadratic
beziers.
Well, maybe I just made some mistake in trying to show they're equivalent. It
doesn't
matter.
> My head started spinning when evaluating the parallel curve methods so
> I'll stop here for now...
Sorry about that. Is there anything I can do to make it easier?
Thanks,
Denis.
----- "Jim Graham" <james.gra...@oracle.com> wrote:
> Round 2
>
> On 10/13/2010 3:40 PM, Jim Graham wrote:
> > HI Denis,
> >
> > I'm just now getting down to the nitty gritty of your webrevs
> (sigh).
> >
> > On 10/6/2010 1:36 PM, Denis Lila wrote:
> >>
> >> webrev:
> >> http://icedtea.classpath.org/~dlila/webrevs/noflatten/webrev/
>
> Stroker.java:
>
> You're doing the bulk
> of
> the work now so if they make sense to you now then it might be best to
>
> leave them alone, but they give me headaches trying to figure them
> out.
> I think you are right that it might help to create some "vector"
> helper classes. I eventually got used to the naming by the time I was
>
> done with the file, but yikes - this will hurt the next guy that comes
>
> along to maintain the code.
> The sx0,sy0,sdx,sdy variables are (reasonably) well named.
> The x0,y0,pdx,pdy variables aren't consistent. Perhaps
> cx0,cy0,cdx,cdy
> for "current" would make more sense?
> The mx0,my0,omx,omy variables are even further from the prior naming
> conventions, what about smx,smy,cmx,cmy?
>
> line 394 - or prev = CLOSE to match the initial state? (I guess it
> shouldn't really matter unless an upstream feeder has a bug.)
>
> ...jim
<<attachment: with510.png>>
<<attachment: without510.png>>
