Re: [OpenJDK 2D-Dev] Various fixes to pisces stroke widening code
Hi Denis, float operations are pretty fast and they are usually done in a separate part of the processor so the compiler can schedule a lot of bookkeeping instructions to run in parallel with waiting for the results of the FP instruction. In the end, they often end up being free if there are enough bookkeeping instructions (branches, fetching data, etc.) in amongst the data. Given how many alternate instructions you are pointing out for the fixed point code it would be very likely that this would be a win. The main reason that Pisces is implemented heavily in fixed point is that it was originally written for the mobile platform where there are no FP instructions. We don't have to worry about that on the desktop (any more). I strongly support you converting things to fp if you want to do it... ...jim On 7/12/2010 8:05 AM, Denis Lila wrote: Hello. Is it ok if I do this? I already started working on it on Friday. I think I should be done by tomorrow. But yes, I agree that we should convert to floating point. As for performance, there's also the fact that right now we're trading one double multiplication for 2 casts to long, 1 long multiplication, 1 bit shift of a long, and 1 cast back to an int. I don't know much about how these are done in hardware, but it doesn't seem like they'd be faster than the double multiplication. As for large coordinates, there's a bug report about it (one not reported by me :) ) here: https://bugzilla.redhat.com/show_bug.cgi?id=597227 I submitted a matching bug report on bugs.sun.com (ID 6967436), but I can't find it when I search for it. Denis. - Jim Grahamjames.gra...@oracle.com wrote: Sigh... Pisces could really stand to upgrade to floats/doubles everywhere, for several reasons: - FPU ops are likely as fast if not faster on modern hardware due to parallel execution of FP instructions alongside regular instructions. - Don't have to worry about getting coordinates larger than 32K (I don't think this is well tested, but I imagine that Pisces will not deal with it very gracefully). - Lots of code is greatly simplified not having to deal with the semantics of how to do fixed point multiplies, divides, and conversions. I meant to do this during the original integration, but I wanted to get the task done as quickly as possible so that we could have an open source alternative to the closed Ductus code so I left that task for a later update. But, now that a lot of work is being done on the code to fix it up, it might be better to do the float conversion now so that the maintenance is easier and before we end up creating a lot of new fixed point code. My plate is full right now, but hopefully I can interest someone else in doing a cleanup cycle? (Donning my Tom Sawyer hat... ;-) ...jim
Re: [OpenJDK 2D-Dev] Various fixes to pisces stroke widening code
Hello. And, I just finished it. At least it compiled successfully. I'm sure there are a few runtime bugs. I'll try to have a webrev out by today. Regards, Denis. - Jim Graham james.gra...@oracle.com wrote: Hi Denis, float operations are pretty fast and they are usually done in a separate part of the processor so the compiler can schedule a lot of bookkeeping instructions to run in parallel with waiting for the results of the FP instruction. In the end, they often end up being free if there are enough bookkeeping instructions (branches, fetching data, etc.) in amongst the data. Given how many alternate instructions you are pointing out for the fixed point code it would be very likely that this would be a win. The main reason that Pisces is implemented heavily in fixed point is that it was originally written for the mobile platform where there are no FP instructions. We don't have to worry about that on the desktop (any more). I strongly support you converting things to fp if you want to do it... ...jim On 7/12/2010 8:05 AM, Denis Lila wrote: Hello. Is it ok if I do this? I already started working on it on Friday. I think I should be done by tomorrow. But yes, I agree that we should convert to floating point. As for performance, there's also the fact that right now we're trading one double multiplication for 2 casts to long, 1 long multiplication, 1 bit shift of a long, and 1 cast back to an int. I don't know much about how these are done in hardware, but it doesn't seem like they'd be faster than the double multiplication. As for large coordinates, there's a bug report about it (one not reported by me :) ) here: https://bugzilla.redhat.com/show_bug.cgi?id=597227 I submitted a matching bug report on bugs.sun.com (ID 6967436), but I can't find it when I search for it. Denis. - Jim Grahamjames.gra...@oracle.com wrote: Sigh... Pisces could really stand to upgrade to floats/doubles everywhere, for several reasons: - FPU ops are likely as fast if not faster on modern hardware due to parallel execution of FP instructions alongside regular instructions. - Don't have to worry about getting coordinates larger than 32K (I don't think this is well tested, but I imagine that Pisces will not deal with it very gracefully). - Lots of code is greatly simplified not having to deal with the semantics of how to do fixed point multiplies, divides, and conversions. I meant to do this during the original integration, but I wanted to get the task done as quickly as possible so that we could have an open source alternative to the closed Ductus code so I left that task for a later update. But, now that a lot of work is being done on the code to fix it up, it might be better to do the float conversion now so that the maintenance is easier and before we end up creating a lot of new fixed point code. My plate is full right now, but hopefully I can interest someone else in doing a cleanup cycle? (Donning my Tom Sawyer hat... ;-) ...jim
Re: [OpenJDK 2D-Dev] Fix for drawing round endcaps on scaled lines.
Hi Denis, In looking through this code, I can see where it emits the proper join for ccw turns, but it looks like it just emits a round join for cw turns. Is that true? Doesn't this mean that a cw path will always have round joins? Or am I missing something? My expectation was that the code would emit the proper joins for both ccw and cw turns, but it would either place it in the outgoing or the return path depending on whether the turn was ccw, no? I don't see how that happens (but I haven't actually tested the code)... ...jim Denis Lila wrote: Hello Jim. I'm terribly sorry about that. I fixed it. Now the only highlighted lines in the webrev should be lines I actually changed. Please take another look at it. (link is still the same: http://icedtea.classpath.org/~dlila/webrevs/bezierRoundJoins/webrev/) Thanks, Denis. PS: I added a new method: emitReverse, that goes through the reverse list and emits it. It's used in 2 methods which used to just do it themselves. - Jim Graham james.gra...@oracle.com wrote: Hi Denis, You moved some code around without modifying it. This makes it hard to see what changed and what didn't and verify that the changes are accurate. Also, it adds diffs to the file that are unrelated to the fixing of the bug. Was there a practical reason for why you moved the code around? In particular I refer to: - the setup code that deals with if (joinStyle == JOIN_MITER) - the setOutput method - which not only moved, but lost its javadocs - computeMiter() and drawMiter() All of that code appears to be unchanged at first glance, but it is hard to tell from the webrevs. Also, a couple of stylistic issues: - you changed the declarations of isCCW which moved its arguments over, but the continuation lines aren't indented to match - The two flavors of emitCurveTo() should probably be next to each other (i.e. not have emitLineTo() between them or fall between the two flavors of emitLineTo()). In general I think the changes are OK, but I'm still reviewing them and the above issues sprung to mind on a first pass (and/or they are complicating the contextual review) so I thought I'd mention them earlier than later... ...jim Denis Lila wrote: Hello. I just noticed that approximation of circle arcs by bezier curves had already been implemented in ArcIterator by Jim Graham. It computes the same control points as my solution, but it does so more straightforwardly, without any rotation, so it is faster and clearer. I have updated my solution to include this. The link remains the same. Thanks, Denis. - Denis Lila dl...@redhat.com wrote: Hello. I think I got this working. The webrev is at: http://icedtea.classpath.org/~dlila/webrevs/bezierRoundJoins/webrev/ (NOTE: this is not a final version. I have included 2 versions of 2 methods. Only one set should be kept. See below for more.) My Changes: --- 1. I've made LineSink into an interface, rather than an abstract class, because all of its methods were abstract, so it makes more sense this way. 2. I've introduced a new interface that extends LineSink called PathSink, which allows the curveTo method, so there have been no changes to Stroker's public interface. When someone wants to create a Stroker with a PathSink output, it simply passes its constructor a PathSink, so the only changes outside of Stroker are in PiscesRenderingEngine, where the methods that handle Path2D and PathConsumer2D objects create nameless PathSinks instead of nameless LineSinks. 3. In Stroker: I've introduced a method called drawBezRoundJoin, analogous to computeRoundJoin. In drawRoundJoin I detect whether the output is a PathSink. If it is, I call drawBezRoundJoin, otherwise everything proceeds as it used to. drawBezRoundJoin uses computeBezierPoints to compute the control points. computeBezierPoints computes the control points for an arc of t radians, starting at angle a, with radius r by computing the control points of an arc of radius 1 of t radians that starts at angle -t/2. This is done by solving the equations resulting from the constraints that (P3-P2) and (P1-P0) must be parallel to the arc's tangents at P3 and P0 respectively, and that B(1/2)=(1,0). Then the points are scaled by r, and rotated counter clockwise by a+t/2. Then drawBezRoundJoin emits the curve. All this is done in a loop which is used to break up large arcs into more than one bezier curve. Through the iterations, the computed control points don't change - the only thing that changes is how they're rotated. So a good alternative approach would be to do the rotation outside of computeBezierPoints, and call computeBezierPoints once outside of the loop, so that the control points aren't recomputed unnecessarily. I have included code for this in the methods computeBezierPoints2 and drawBezRoundJoin2. This is my favoured approach, since it is
Re: [OpenJDK 2D-Dev] Fix for drawing round endcaps on scaled lines.
Sorry, ignore this message. I was misinterpreting the field internalJoins to mean joins on the inside of the path (!ccw). It actually means implicit joins in the middle of a flattened curve and I can see why those are always done round, so the code makes sense now (naming of the variable notwithstanding)... ...jim Jim Graham wrote: Hi Denis, In looking through this code, I can see where it emits the proper join for ccw turns, but it looks like it just emits a round join for cw turns. Is that true? Doesn't this mean that a cw path will always have round joins? Or am I missing something? My expectation was that the code would emit the proper joins for both ccw and cw turns, but it would either place it in the outgoing or the return path depending on whether the turn was ccw, no? I don't see how that happens (but I haven't actually tested the code)... ...jim Denis Lila wrote: Hello Jim. I'm terribly sorry about that. I fixed it. Now the only highlighted lines in the webrev should be lines I actually changed. Please take another look at it. (link is still the same: http://icedtea.classpath.org/~dlila/webrevs/bezierRoundJoins/webrev/) Thanks, Denis. PS: I added a new method: emitReverse, that goes through the reverse list and emits it. It's used in 2 methods which used to just do it themselves. - Jim Graham james.gra...@oracle.com wrote: Hi Denis, You moved some code around without modifying it. This makes it hard to see what changed and what didn't and verify that the changes are accurate. Also, it adds diffs to the file that are unrelated to the fixing of the bug. Was there a practical reason for why you moved the code around? In particular I refer to: - the setup code that deals with if (joinStyle == JOIN_MITER) - the setOutput method - which not only moved, but lost its javadocs - computeMiter() and drawMiter() All of that code appears to be unchanged at first glance, but it is hard to tell from the webrevs. Also, a couple of stylistic issues: - you changed the declarations of isCCW which moved its arguments over, but the continuation lines aren't indented to match - The two flavors of emitCurveTo() should probably be next to each other (i.e. not have emitLineTo() between them or fall between the two flavors of emitLineTo()). In general I think the changes are OK, but I'm still reviewing them and the above issues sprung to mind on a first pass (and/or they are complicating the contextual review) so I thought I'd mention them earlier than later... ...jim Denis Lila wrote: Hello. I just noticed that approximation of circle arcs by bezier curves had already been implemented in ArcIterator by Jim Graham. It computes the same control points as my solution, but it does so more straightforwardly, without any rotation, so it is faster and clearer. I have updated my solution to include this. The link remains the same. Thanks, Denis. - Denis Lila dl...@redhat.com wrote: Hello. I think I got this working. The webrev is at: http://icedtea.classpath.org/~dlila/webrevs/bezierRoundJoins/webrev/ (NOTE: this is not a final version. I have included 2 versions of 2 methods. Only one set should be kept. See below for more.) My Changes: --- 1. I've made LineSink into an interface, rather than an abstract class, because all of its methods were abstract, so it makes more sense this way. 2. I've introduced a new interface that extends LineSink called PathSink, which allows the curveTo method, so there have been no changes to Stroker's public interface. When someone wants to create a Stroker with a PathSink output, it simply passes its constructor a PathSink, so the only changes outside of Stroker are in PiscesRenderingEngine, where the methods that handle Path2D and PathConsumer2D objects create nameless PathSinks instead of nameless LineSinks. 3. In Stroker: I've introduced a method called drawBezRoundJoin, analogous to computeRoundJoin. In drawRoundJoin I detect whether the output is a PathSink. If it is, I call drawBezRoundJoin, otherwise everything proceeds as it used to. drawBezRoundJoin uses computeBezierPoints to compute the control points. computeBezierPoints computes the control points for an arc of t radians, starting at angle a, with radius r by computing the control points of an arc of radius 1 of t radians that starts at angle -t/2. This is done by solving the equations resulting from the constraints that (P3-P2) and (P1-P0) must be parallel to the arc's tangents at P3 and P0 respectively, and that B(1/2)=(1,0). Then the points are scaled by r, and rotated counter clockwise by a+t/2. Then drawBezRoundJoin emits the curve. All this is done in a loop which is used to break up large arcs into more than one bezier curve. Through the iterations, the computed control points don't change - the only thing that changes is how they're rotated. So a
