HI Denis,
I'm just now getting down to the nitty gritty of your webrevs (sigh).
On 10/6/2010 1:36 PM, Denis Lila wrote:
webrev:
http://icedtea.classpath.org/~dlila/webrevs/noflatten/webrev/
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
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:
lines 110,111 - shouldn't you check if there are any "first segments"
before writing the extra move?
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:
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
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)
line 235 - is there a reason to output a curve of 0 length? lines of 0
length are omitted...
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?
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.
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.
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
