On 10/22/2010 12:22 PM, Denis Lila wrote:
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?
If it is relative, then shouldn't it be relative to the desired length?
Why does the computed approximation factor in to the size of the error
you are looking for? If you use the average then the amount of error
that you will "accept" will be larger if your estimate is wronger. I
don't think the "wrongness" of your approximation should have any effect
on your error. How about this:
(Math.abs(len-leftLen) < err*len)
(noting that err*len can be calculated outside of the loop).
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.
I actually thought about the interface concept after I sent that and was
at peace with them, but I'm also at peace with them being gone. ;-)
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.
Note that a custom shape can send segments in any order that it wants so
close,close can happen from a custom shape even if Path2D won't do it.
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.
New code looks good (even if we think it's a silly side effect of closed
JDK to have to implement).
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.
Woohoo! I was actually bothered by the side-effecting in the old code -
this is a better approach.
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.
I think it probably should not have a join since we don't join two
segments if one of the "OFF" lengths is 0. We should probably only save
the first dash if it starts in the middle of a dash.
Perhaps the closed JDK is wrong here.
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.
Note that curve length is imperfect so their behavior may differ because
the above path had exact lengths that didn't rely on fp calculations
with errors to get it right, but the curved case had a lot of
computations. Even if they believe the last dash had .000000001 left on
it, they would close, even though they believed the length of the curve
to be "about 200".
Handling the case where phase==dash[idx] immediately fixes this problem.
Good point.
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.
Looks good. I was going to say that, but figured it was too "nitty" to
even mention (perhaps you're getting the hint that I'm a bit OCD about
code... ;-)
I reran all the gfx tests. Nothing has changed.
Woohoo!
There is only one question on the board from my perspective - the
question about dash length errors at the start of this message.
Depending on how you feel about that I think we're ready to go (and I
have some ideas on further optimizations to consider if you are still
game after this goes in)...
...jim
