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

[Jim Graham]

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