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

[Denis Lila]

Hi Jim.

>>>  http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/

> lines 279 and 303 - maybe add Helpers.nearZero(v, nulp) for these?

I've introduced this method and am using it in 303. However, I think
line 279 is good as it is, since we're comparing Float.MIN_VALUE and a double.


> line 303 - could use either within or nearZero, same comment about the
> value being computed twice.  Also, aa+cc gets computed later, not sure
> if the compiler is smart enough to deal with that.

I think caching aa+cc would be a bit overkill. However, I've declared the
variables as final and hopefully that will help the compiler to not
recompute them.

> line 405 - I didn't understand why you use "2*err*(len+leftLen)" as
> the termination condition, why not just err?

Because the error is meant to be relative. What I use is supposed to be
equivalent to difference/AverageOfCorrectValueAndApproximation < err,
where, in our case, AverageOfCorrectValueAndApproximation=(len+leafLen)/2,
so that multiplication by 2 should have been a division.
Should I use the less fancy differece/CorrectValue < err and skip
a division by 2?

> lines 200,236 - for the most part, aren't these redundant?

I think so. When I first implemented the bezier round joins they were
necessary, but I'm not sure why. All I know is when I tested it without
them, joins weren't drawn properly. However, I can't reproduce the problem
now, so I've removed them.

> lines 98-101 - somewhere in the confusion moveToImpl became redundant
> with moveTo.

I thought I'd leave these in because they're interface methods,
and it's usually not a great idea to call these from private methods. I've
removed them anyway, because you said so and because I suppose
if ever we need to do something to the user input that shouldn't be done
to input coming from private methods in the class (such as the scaling
of the input coordinates done in Renderer) we can always easily
reintroduce them.

> lines 358-361 - lineToImpl redundant now?
> line 390 - should finish() really be done here?  That would mean that
> moveTo,close would try to emit something and close,close would as
> well. 
>   Is that right?

That's right. I don't think it's what should happen, but it's what closed
jre does, so I've left it in (with some changes to make it actually
replicate the behaviour of closed java, since it was buggy - the moveTo
was missing). I don't draw anything on the second close in the close,close
case, since that would look horrible with round joins and square caps.
However, the way path2D's are implemented this safeguard will not be
activated since a closePath() following another closePath() is ignored.

I also now initialize the *dxy *mxy variables in moveTo. This should be an
inconsequential change, but I've put it in just so the state of
Stroker after a moveTo is well defined.

> line 368 - does this cause a problem if t==1 because lastSegLen will
> now return the wrong value?  Maybe move this into an else clause on the 
> "t>=1" test?

It does cause a problem. I've fixed it by adding a variable that keeps track
of the length of the last returned segment. The way it was being done was
a dirty hack because it assumed that if the method didn't return in the loop,
done would remain false.

I made one more change in dasher: in somethingTo I removed the long comment
near the end, and I handle the case where phase == dash[idx] immediately.
I do this for consistency with lineTo. The only instances where this makes
a difference is when we have a path that starts with a dash, ends at exactly
the end of a dash, and has a closePath. So something like move(100,0),
line(0,0),line(0,50),line(100,50),line(100,0),close() with a dash pattern {60,60}.
In closed jdk (and also in openjdk with my patch), no join would be drawn
at 100,0 on this path with this dash pattern.
Whether that's correct behaviour is arguable, imho. On one hand, if we don't do it
, but I think it is because if
it isn't done certain dashed rectangles start looking weird at the top left.

Now, consider a path like move(100,0),line(0,0),curve(0,100,100,100,100,0),close()
with a dash pattern of {60,60}. The length of the curve in this is exactly 200 (I
have not proven this, but it seems clear since the more I increase precision, the
closer to 200 the computed length becomes. Also, if you render it with a dash pattern
of {10,10} in closed jdk, exactly 10 full dashes are drawn). So, this path has exactly the same
length as the above path. However, if this path is rendered with closed jdk a join
will be drawn at (100,0). This behaviour is inconsistent with the line only path
For this reason, I think this is a bug in closed jdk since dashing
should depend only on the path's length, not on the kind of segments in a path.

Handling the case where phase==dash[idx] immediately fixes this problem.

I also moved the second if statement in the loop of lineTo inside the first if
for symmetry with how this case is handled in somethingTo. The logic is the same.


I reran all the gfx tests. Nothing has changed.


Regards,
Denis.

----- "Jim Graham" <james.graham at oracle.com> wrote:

> Hi Denis,
> 
> This should be the last batch of comments, most of which may require a
> 
> 10 second answer.  Most of them could just be ignored as they are
> minor 
> optimizations.  There are only a couple where I think something is
> "off"...
> 
> PiscesRenderingEngine.java:
> 
> 
> line 325 - you don't need to clone at.  "outat = at" should be
> enough.
> 
> A note for next revision - you don't need to include the translation
> in 
> outat and inat, it would be enough to use just the abcd part of the 
> transform.  Right now, the TransformingPathConsumer implementations
> tend 
> to assume translation, but we could create variants for the 
> non-translating cases and maybe save some work.
> 
> Dasher.java:
> 
> lines 174-175 - lineToImpl now redundant too.
> 
> Stroker.java:
> 
> line 800 - technically, this could be [0] and [1] since all points on
> 
> the curve are the same.  ;-)
> 
> line 805,810 - abs()?
> 
> line 821,824 - you add 1 to nCurves just to subtract 1 later?  Maybe 
> rename it to nSplits and skip the +/-1?
> 
> 				...jim
> 
> On 10/21/2010 1:52 PM, Jim Graham wrote:
> > Hi Denis,
> >
> > I'll be focusing on this later today, just a last proofread.
> >
> > In the meantime, can you outline the tests that you ran?
> >
> > Also, have you used J2DBench at all? I know you ran some of your
> own
> > benchmarks, but didn't know if you were familiar with this tool:
> >
> > {OpenJDK}/src/share/demo/java2d/J2DBench/
> >
> > ...jim
> >
> > On 10/21/2010 12:27 PM, Denis Lila wrote:
> >> Good to push?
> >>
> >> http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/