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

[Denis Lila]

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.graham at 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