Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
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
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
On 10/19/2010 10:38 AM, Denis Lila wrote: 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. That's mostly true too for me, but there are a couple that I might go back to - I'll let you know when I think I've reached a 100% coverage (getting close). 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. OK, use your best judgment. If they are small and they add to symmetry of services (like evalQuad) or they might be used later, then it isn't a big deal, but dead code in private APIs shouldn't be just left laying around if we can help it. ...jim
Re: [OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces
Hi Denis, On 10/19/2010 10:40 AM, Denis Lila wrote: ROCsq - I looked at the wikipedia article and it wasn't clear how it directly related to your function since the article is dealing with the curvature of a function graphed against its own t, but you are dealing with 2 parametric equations combined and graphed against each other. I think I'm going to have to just trust you on this one for now. :-( http://en.wikipedia.org/wiki/Radius_of_curvature_%28applications%29 Did you look at the above wikipedia article? When researching I came across 2 of them, and one of them only mentions natural parameterizations, but the above has the general equation for a R-Rn function, then below that they have the special case where n=2, x(t)=t, and y(t)=f(t). I used the first equation on that page. Actually, I wrote a simple program to make sure the radius of curvature function was correct. I have attached it. It's not a proof, but I think it is convincing. Just hold down the mouse button and move it horizontally. This will change the t value on the curve and the circle drawn will have radius equal to Math.sqrt(ROCsq). You can also change the control points of the curve. There's a bug where when you run it the window is really tiny, so you have to manually resize it and maximize it. I actually did read that article, but I wasn't seeing the fact that it was a multiple parametric equation and that the || were distance calculations rather than simply absolute values. Now I see it. Plugging those concepts in to the first equation the mapping is very direct. One thing that confused me when I was proof-reading it was that the numerator seemed to be dx2dy2 squared when it should be cubed. Then I spotted the final *dx2dy2 term at the end which makes it cubed. I wasn't sure why you isolated that term out there instead of just grouping it with the rest of the numerator - is there a danger of overflow if you multiply it before you do the division? If so, then that is fine since it doesn't actually affect the number of fp ops so it should be the same performance. lines 621-626 and 643-646 - I'm not sure what the derivation of these lines are. I tried to do my own equations, but I seem to be heading in a different direction than you used and it didn't seem like I was going to converge. What equations did you set up to solve to get these calculations? From what I can see we have: - new p1,p4 are calculated - new p(0.5) is calculated - the corresponding dx,dy at t=0,0.5,1 (gives slopes) - slopes at t=0, 0.5 and 1 should be the same for all curves and what you are trying to compute are two scaling constants c1 and c2 that represent how much to scale the dx1,dy1 and dx4,dy4 values to get a curve that interpolates both position and slope at t=0.5. A comment might help here... :-( I see how (dxm,dym) was confusing. The only reason for computing the slope at t=0.5 is to get the point of the offset curve at t=0.5. We don't make the computed curve and the input curve have equal slopes at t=0.5 because this would give us an overdetermined system. What we're trying to do in this function is to approximate an ideal offset curve (call it I) of the input curve B using a bezier curve Bp. The constraints I use to get the equations are: It does help *a lot*, though, so thank you for writing it up. I would move it closer to the code in question since the function has such a long preamble that separates the comment from the code that implements it (also method comments are usually reserved for API documentation purposes). lines 544,559 - I'd remove the line numbers from the comment. They are already wrong and they won't survive any more edits any better. ;-) In #2, you have a bunch of I'() || B'() which I read as the slope of the derivative (i.e. acceleration) is equal, don't you really mean I() || B() which would mean the original curves should be parallel? Otherwise you could say I'() == B'(), but I think you want to show parallels because that shows how you can use the dxy1,dxy4 values as the parallel equivalents. I would rename det43 to invdet43 to indicate that it is the inverse of the determinant. I kept looking at it and thinking he has the determinant in the wrong side until I noticed that it was in the denominator of det43 (which is hard to read in parenthesized C-math). One side note. At first glance I would have thought that the final equations would have subtracted the c2*dxy4 terms rather than adding them (since dxy4 represent p4-p3, not p3-p4 and so the linear interpolation equation looks backwards), but that isn't true because you did all of your math looking to find the c2 that belongs in this equation (as backwards as it seems) and so you got that answer. Interestingly if you look at the effect on the results if you calculate the dxy4 terms the other way around, they are simply negated and the impact would be that c1 would be unaffected (both num and
