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:
Are you happy with the current variable names? 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?
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.
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).
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.
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?
line 374 - why is this moveto here? Doesn't this break the joined path
into 2 separate paths? Should this be a lineto? (Also, sx0==x0 and
sy0==y0 at this point).
line 389 - The test here is different from closePath. What if they were
both "prev == DRAWING_OP_TO"?
line 394 - or prev = CLOSE to match the initial state? (I guess it
shouldn't really matter unless an upstream feeder has a bug.)
line 486 - this leaves the current point in a different place than line
510, does that matter?
My head started spinning when evaluating the parallel curve methods so
I'll stop here for now...
...jim
