Hi Laurent,
I'm now looking at your webrev. There are a number of things that I
think we should look at once it goes in, and I'll mention those in
subsequent emails, but I want to just deal with basic issues that we
should deal with before the first commit here...
AAShapePipe - the static println in TileState should probably be removed
(for one thing, it will now be run regardless of whether or not Marlin
is selected).
Is there anything about the patch to AAShapePipe that is specific to
Marlin? Perhaps it should have its own bugid?
Even though doCleanDirty is turned off, Dasher.java line 153 looks like
it may overflow if you've ever had to widen the firstSegs array with it
turned on.
Stroker.java, 312 - indent looks off.
Use of "fluent API". In general I think that is a nice way to do thing
in a builder-style API where a whole chain of related methods all return
"this" to facilitate the chaining, but identifying isolated methods that
happen to often be used just prior to a return of the object they were
executing on seems to me to be more of a break in object encapsulation.
It adds a burden to the called method to deal with a return value so
that we can eliminate a line of code in the caller. But, in reality the
caller still has to return something - it just returns a different value
(the value saved in a local variable instead of the value returned by
the called method) so you are actually adding code to the interface to
make things appear simpler. This isn't a veto of those cases here, but
I wanted to point out that their value in isolated cases is questionable
unlike the "chained setter methods" case for which they were initially
targeted.
TransformingPathConsumer2D (and any other new code) - All methods should
have a blank line between them. This notably applies to the Path2D
wrapper. Also, a common style point throughout 2D is that when there is
a continuation line in an argument list declaration, we place the "{"
brace on a new line all by itself so that the eye has a good landmark
for where the body of the method begins. (See how the old "curveTo()"
methods differ from the new one created in Path2DWrapper.)
TransformingPathConsumer2D - the if/else style being replaced here was
actually intentional. The lack of a trailing "return" statement ensures
that all cases were specifically handled above in either an if, or its
associated else block, and that none of the cases ended up falling
through to a subsequent "catch-all" operation by accident. Nothing is
broken here, I just wanted to point out the "whys" of the original code
style. In the new code, there are still some bare "} else {" lines left
which raises the question - what was the specific style goal in whether
an "} else {" was eliminated or left intact? The prior style goal was
"no fallthrough cases anywhere" - the new style seems to be "usually
fallthrough, but sometimes explicit alternate return values in an else
clause".
-------- Here are some "things to consider later" ---------
Is there ever a reason to use doCleanDirty? It might matter for arrays
of objects to null out old references so as to not interfere with GC,
but don't most growable arrays get filled in as they are used so old
data isn't really processed? I notice that it is turned off right now
so perhaps that's an argument in favor of my comment, but it seems to me
that having to clean out a primitive array would be an odd rare
exception and so simply adding in a clear where it is needed in the
unlikely case that it is needed would be fine (and get rid of a lot of
cruft in the code).
I just realized the main case where we want to deal with cleaning out
dirty arrays and it would be the crossings accumulation arrays. It
seems to me that those are special cases and so we should concentrate
the cleaning code on those explicitly and not have a global "lets clean
all arrays everywhere" setting.
Isn't declaring flatLeafCoefficients as final enough to not need to
cache it's reference in a local variable? Also, I would think you would
gain more by simply deleting the array and storing the coefficients in
fields instead? An array index lookup should be no simpler than a local
field access.
I was also skeptical of these Iterators that Denis created and thought
that it may be better to just manually iterate the subdivisions instead
of creating (or even keeping) an object around to hide the simple job of
iterating a curve. I now wonder if inlining the actions of the
BreakPoint iterator might not improve performance for complex paths and
leave less state that we need to manage.
FastMath - In general, if we use "(int) ceil()" a lot, we might want to
consider offsetting the math so that all values are negative and then
use a cast to (int). In particular, if crossings are going to use
"ceil(v - 0.5)" which is the proper way to do half-open intervals with
centers included on top and left sides, then simply shifting everything
once so that the entire range of interesting X,Y are negative would mean
that all crossings calculations could be done by casting to integer,
especially if we also offset them all by 0.5 as well.
FastMath - if we can prove that no values <0 are interesting (as in, the
only interesting property about them is that they are less than 0), then
we can just use a cast to integer. This is particular true if we clamp
a value to >= 0 after we convert it to an integer - in that case we
might as well have used an int cast rather than a floor() computation.
Eventually we will need to make a decision on all LBO and TODO comments.
Either make a call and change the code and delete the comment, or
submit the issue as a new bugid and reference the bugid rather than TODO
or LBO.
Those were just a few thoughts/questions that came to mind as I was
reviewing it, but I didn't want to get bogged down too much in the
specifics that we can tweak later until we can get an implementation
published for people to test on so I tried to back off and keep my
review at a higher level. I may have more comments as I continue to
look through the code...
...jim
