> That's great. I will be pushing today.
About that: you wrote the TransformingPathConsumer2D file,
so how should you be given credit? Should I put your name in
"Contributed-by"? Should I put an @author tag in the file?
Or does the "reviewed-by" suffice?
Regards,
Denis.
----- "Denis Lila" <dl...@redhat.com> wrote:
> Hi Jim.
>
> > How about this:
> >
> > (Math.abs(len-leftLen) < err*len)
> >
> > (noting that err*len can be calculated outside of the loop).
>
> This is what I use now.
>
> > 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.
>
> Right, of course. For some reason I only considered the test case I
> was
> using. Sorry for slip up.
>
> > There is only one question on the board from my perspective - the
> > question about dash length errors at the start of this message.
>
> I'm ok with the accuracy of dash length computations. My main
> concerns
> right now are dashing performance and AA rendering performance. The
> latter has improved, but not by as much as I was expecting. Also, it
> was
> bothering me that when I removed PiscesCache 1-2 weeks ago
> performance
> got worse.
> It would also be nice to find a method that is guaranteed to find
> all offset curve cusps.
>
> > Depending on how you feel about that I think we're ready to go
>
> That's great. I will be pushing today.
>
> > (and I have some ideas on further optimizations to consider if you
> are still
> > game after this goes in)...
>
> I'd love to hear what they are.
>
> Thank you for all your time,
> Denis.
>
> ----- "Jim Graham" <james.gra...@oracle.com> wrote:
>
> > 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.
> >
> > >> 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.
> >
> > > 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!
> >
> >
> > ...jim
