Hi Jim. If I haven't replied to a suggestion, that means I've implemented and I thought it was a good idea, so I don't have anything to say about it.
> 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. True, but I would like to preserve the naming differences. CURSLOPE makes it clear that the slope will change. > 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? Wow, I can't believe these slipped past me. What happened was that I used to initialize the type specific fields first, to avoid having 2 switch statements. However, that didn't work out (for reasons explained in the comment), so I needed 2 switches after all. I guess I just forgot to delete the init* calls in the first one. I'm pretty sure that the init* calls in the first switch can just be deleted. In fact, it might be a bug to leave them there, since init* calls the AFD iteration function. If we have 2 init calls, the AFD function will be called twice, so this is probably a bug. > line 37: If it can't be instantiated, why does it take arguments? Another mistake when cutting, pasting, and modifying old code. > 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? Should I get rid of these? I wanted to do it, but I wanted to wait until just before pushing because I was afraid I'd start needing them again at some point in the future. > 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. I looked around for a closed form solution but I only found variations of the one on wikipedia. I decided not to implement it because it looked like I was going to have to deal with complex numbers and I didn't want to do that. It also didn't seem like it would be a lot faster than Newton's method which actually does very few iterations (4-6 if I remember correctly). But I'll keep this in mind, and if I find a good implementation of a closed form formula, I'll use it. > line 566: shouldn't horizontal lines be ignored? they don't affect > rasterization. They don't, but it's important to always update the current position, otherwise, if we get something like: moveto(0,0),lineTo(100,0), lineTo(100,100), instead of recording a vertical line from 100,0 to 100,100 we would record a diagonal line from 0,0 to 100,100. The actual ignoring is done in the six lines following these two. The link is still http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/ I thoroughly tested it yet, but Java2DDemo looks good. Thanks, Denis. ----- "Jim Graham" <james.gra...@oracle.com> wrote: > 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 574: indentation? > > 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: > > 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
