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

[Denis Lila]

Hi Jim.

> I'm just now getting down to the nitty gritty of your webrevs (sigh).

Thanks. I hope it's not too bad.

> PiscesRenderingEngine.java:
> line 278 - the det calculation is missing "b".
> 
> line 296 - is there an "epsilon" that we can use?  "==" with floating
> point often fails with calculations.
> 
> line 308 - miterlimit is a ratio of lengths and should not need to be
> scaled.
> 
> line 332 - I think you can use a null transform for the same result.
> 
> line 338 - null here too

I fixed these.

> lines 110,111 - shouldn't you check if there are any "first segments"
> before writing the extra move?

I suppose I should. I didn't because I don't think it affects what is
drawn. It doesn't, does it?

> lines 150-152 - starting should be left true until you reach the end
> of the dash, no?  Otherwise you only hold back the starting segments up 
> until the first "piece" of a curve.  Everything should be held back 
> until you reach an "off" piece.  I don't think the logic for these 
> variables is right yet.  Here is what I see:

That's right, it should stay true until the end of the first dash is reached.
I'm pretty sure that's what it does. This is what fullCurve is used for. It
is a hint from the caller that indicates whether the input curve has been
subdivided. If it hasn't, that means the first dash isn't over yet so starting
will remain true (lines 150-152). Admittedly, this is a bit clunky so I've
implemented your solution.

> line 228 - Lazy allocate lc?  Polygons, rectangles, and lines won't
> need it to be dashed (though dashing is already expensive enough it might
> not be noticeable, still waste is waste and there is only one piece of
> code that uses lc so it is easy to protect with a lazy allocation)

Done.

> line 257 - shouldn't the left and right indices always be at 0 and 
> type-curCurveoff?  It looks like after the first time through this
> loop you are storing the right half on top of the left half (see line 262)?
> That would output odd values if any curve gets subdivided more than 
> once, though, right?

What is happening is that at any time there is a "current curve" - the one to
be emitted in goTo, and the rest of the curve. The current curve gets bounced
around between 0 and type-curCurveoff. So, suppose type==8 and the curve needs
to be subdivided twice. Then on the second iteration, the subdivision will put the
left half at curCurvepts, which is 8 and the right half will go to 0. Then
goTo will be called with an offset of curCurveoff+2, which is 10, which is correct
since that's where the current curve starts in relative coordinates.

For some reason, when writing this, I was under the impression that this would
let us move fewer coordinates around in the coordinate array, but of course,
the subdivision routine will need to write to 2*type array elements anyway. So, I
changed this to the more straightforward implementation (I also changed
BezCurve.breakPtsAtTs, since it was doing the same thing).

> line 347,352 - it might be cleaner to have the calling function keep 
> track of how far into the curve you are and communicate this to the 
> method so it doesn't have to clobber its data based on an assumption
> of how the calling function is structured.  But, this arrangement works 
> fine for the current purposes and you do have a "TODO" comment in
> there about this.

> line 289 - LenComputer always allocates maxcurves segements which is 
> 8*1024 words (32K) + object overhead * 1024 + 2 more arrays of 1025 
> words.  That's a lot of memory for the worst case scenario.  It might
> be nice to come back to this and have it be more dynamic (and it is more
> of a push to have the "lc" variable be lazily allocated above).  Also, if
> you are clever enough, you never need storage for more than about 10 
> (maybe 11) curves even if you produce 1024 t's and len's - due to the
> recursive nature of the subdivision that leaves one half dormant while
> the other half is explored.

I turned LengthComputer into an iterator. I think it's much cleaner now.
There's no longer any of that "scale every t in the array so that they become
valid parameters of the right subdivided curve".
It also uses less memory - just limit+1 curves. I guess I am clever enough ;)
(though unfortunately not clever enough to have thought of the idea myself).

I found a problem with Dashing though. Curves like moveTo(0,0);
curveTo(498,498,499,499,500,500); are not handled well at all.
http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/ is the link
with the new webrev. I have fixed this problem by doing binary search on
the results of the flattening. I really don't like this solution because
it does *a lot* more subdivisions than just flattening.

Regards,
Denis.

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

> HI Denis,
> 
> On 10/6/2010 1:36 PM, Denis Lila wrote:
> >
> > webrev:
> > http://icedtea.classpath.org/~dlila/webrevs/noflatten/webrev/
> 
> TransformingPolyIOHelper should be in its own file - we consider more
> than one class per file to be bad form these days, especially if the 
> class is used outside of that file.
> 
> I'm a little troubled by how the PolyIOHelper fits into the design. 
> It's odd to talk to the same object for both input and output.  I have
> some ideas there, but I think I'll leave it for a followon email and
> effort.
> 
> Dasher.java:
> 

> 
>     boolean needsMoveto;
> 
> in moveTo and pathDone:
>      if (firstSegBuf is not empty) {
>          output moveto(sx,sy)
>          output firstSegs
>      }
>      needsMoveto = true;  // not needed in pathDone
> 
> in goTo() {
>      if (ON) {
>          if (starting) {
>              store it in firstSegBuf
>          } else {
>              if (needsMoveto) {
>                  output moveto(x0,y0);
>                  needsMoveto = false;
>              }
>              send it to output
>          }
>      } else {
>          starting = false;
>          needsMoveto = true;
>          // nothing goes to output
>      }
> }
> 
> and in ClosePath:
>      lineToImpl(sx, sy, LINE);
>      if (firstSegBuf is not empty) {
>          if (!ON) {  // Or "if (needsMoveto)"
>              output moveTo(sx, sy)
>          }
>          output firstSegs
>      }
> 
> I don't see a need for firstDashOn or fullCurve
> 
> 
> Stroker.java:
> 
> line 129 - proof that miterLimit does not need to be scaled... ;-)
> 
> I'm going to send this buffer of comments off now and continue on with
> 
> Stroker.java in a future email...
> 
> 			...jim