[OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces

[Jim Graham]

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