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

[Jim Graham]

Round 3...

On 10/6/2010 1:36 PM, Denis Lila wrote:
>
> webrev:
> http://icedtea.classpath.org/~dlila/webrevs/noflatten/webrev/

I'm going to set the rest of Stroker.java aside for a moment and focus 
on other areas where I have some better knowledge...

Renderer.java:

lines 83, 91, 99: can't these be folded into the prior loops?  You can 
update their Y while searching for the [eqc]hi value.

lines 178,192: indent to the preceding "("?  (Also, I'm a big fan of 
moving the "{" to a line by itself if an conditional or argument list 
was wrapped to more than 1 line - the 2D team tends to use that style 
everywhere in the 2D code...)

line 193: add fieldForCmp here instead of every time in the loop?  (The 
compiler will probably/hopefully do that anyway)

line 238: If X0,Y0,XL,COUNT were bumped up by 1 then you could just 
reuse "SLOPE" from the linear indices - just a thought.

lines 521,527,533: Why are these executed twice?  You call these methods 
again after the "initialize common fields" code.  That seems like double 
the work just to maybe save 4 lines of shared code?  Maybe put the 4 
lines of shared code in a helper function that all of the init() methods 
call?

line 574: indentation?

line 566: shouldn't horizontal lines be ignored?  they don't affect 
rasterization.

line 612: delete?  Or will this be making a comeback sometime?

lines 624,626: indentation?

lines 724,725: doesn't the assert false omit these?  I usually throw an 
InternalError in cases like this with a description of what went wrong.

I've read up through the use of the cache in emitRow().  I'll continue 
with reviewing the cache in the next set, meanwhile I also took a look 
at the helper classes...

Helpers.java:

line 37: If it can't be instantiated, why does it take arguments?

getTransformedPoints isn't used?
getUntransformedPoints isn't used?
fillWithIndxes(float) isn't used?
evalQuad isn't used?  (Though it does provide symmetry with evalCubic 
which is used)
getFlatness* aren't used?
ptSegDistSq isn't used?

line 105: There is a closed form solution to cubic roots.  I 
unfortunately used a bad version in CubicCurve2D.solveCubic and I don't 
remember if I ever went back and fixed it (it may even have been 
"Cardano's method" for all I know).  There are versions out there which 
do work better.  The problem with the one in CC2D was that I copied it 
out of Numerical Recipes in C and apparently the author somehow assumed 
that all cubics would have 1 or 3 roots, but a cubic of the form 
(x-a)(x-a)(x-b) has 2 roots.  D'oh!  While I did find other 
implementations out there on the net, in the end fixing the closed form 
solution seemed wrought with issues since many of the tests would use 
radically different approaches depending on tiny changes in one of the 
intermediate results and so I worried about FP error even in doubles 
possibly skewing the results.  I think you should leave your code in 
there, but I wanted to fill you in on other possibities.

BezCurve.java:

Didn't you get a complaint that this class is defined in a file of the 
wrong name?  Maybe the compiler doesn't complain because the class isn't 
public, but one of the names should change to match.

line 59: I'd throw an internal error and the compiler would be appeased.

line 35: If you make this a "create" factory then it can leverage the 
code in the existing constructors - just a thought.

I'll stop here and hit send - not much left after this round...

		...jim