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
