Jim,
here is the latest webrev:
http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-3/
I tried to revert some syntax mistakes (indentation, moved constants +
cleanup) : code cleanup is still in progress;
so please don't have a frawny face while looking at the code!
I fixed also Ductus and Jules rendering engines (TileState added in
interface)
In this patch, I modified the rowAARLE[][] array to only contain a single
tile line [32][4096] (512K). Each line is quite big already because I want
to store there the alpha data instead of RLE values:
RLE values are very interesting for horizontal lines but for dashed lines
it becomes bigger than alpha data. For example a dashed line [2-1] will
produce many segments that will be encoding like [01][x2]...
I have few questions regarding renderer edge handling and float ceil calls:
private void addLine(float x1, float y1, float x2, float y2) {
...
// LBO: why ceil ie upper integer ?
final int firstCrossing = Math.max(ceil(y1), boundsMinY); // upper
integer
final int lastCrossing = Math.min(ceil(y2), boundsMaxY); // upper
integer
=> firstCrossing / lastCrossing should use lower and upper integer values
(rounding issues) ?
boolean endRendering() {
// TODO: perform shape clipping to avoid dealing with segments out
of bounding box
// Ensure shape edges are within bbox:
if (edgeMinX > edgeMaxX || edgeMaxX < 0f) {
return false; // undefined X bounds or negative Xmax
}
if (edgeMinY > edgeMaxY || edgeMaxY < 0f) {
return false; // undefined Y bounds or negative Ymax
}
// why use upper integer for edge min values ?
=> Here is the question: why use ceil instead of floor ?
final int eMinX = ceil(edgeMinX); // upper positive int
if (eMinX > boundsMaxX) {
return false; // Xmin > bbox maxX
}
final int eMinY = ceil(edgeMinY); // upper positive int
if (eMinY > boundsMaxY) {
return false; // Ymin > bbox maxY
}
int spminX = Math.max(eMinX, boundsMinX);
int spmaxX = Math.min(ceil(edgeMaxX), boundsMaxX); // upper
positive int
int spminY = Math.max(eMinY, boundsMinY);
int spmaxY = Math.min(ceil(edgeMaxY), boundsMaxY); // upper
positive int
int pminX = spminX >> SUBPIXEL_LG_POSITIONS_X;
int pmaxX = (spmaxX + SUBPIXEL_MASK_X) >> SUBPIXEL_LG_POSITIONS_X;
int pminY = spminY >> SUBPIXEL_LG_POSITIONS_Y;
int pmaxY = (spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y;
// store BBox to answer ptg.getBBox():
this.cache.init(pminX, pminY, pmaxX, pmaxY);
In PiscesCache:
void init(int minx, int miny, int maxx, int maxy) {
...
// LBO: why add +1 ??
bboxX1 = maxx /* + 1 */;
bboxY1 = maxy /* + 1 */;
=> piscesCache previously added +1 to maximum (upper loop condition y < y1)
but the pmaxX already uses ceil (+1) and (spmaxY + SUBPIXEL_MASK_Y) ensures
the last pixel is reached.
I fixed these limits to have correct rendering due to dirty rowAARLE reuse.
I think that the thread local's RendererContext is now small enough (< 1
Mb) to be used by web containers but if the number of threads is very high,
concurrent queue could help minimizing wasted memory.
Few comments:
2013/4/24 Jim Graham <james.gra...@oracle.com>
> On 4/24/13 1:59 AM, Laurent Bourgčs wrote:
>
>> Jim,
>>
>> First, here are both updated webrev and benchmark results:
>> - results: http://jmmc.fr/~bourgesl/**share/java2d-pisces/patch_opt_**
>> night.log<http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/patch_opt_night.log>
>> - webrev:
>> http://jmmc.fr/~bourgesl/**share/java2d-pisces/webrev-2/<http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/webrev-2/>
>>
>
> Some comments on the webrev:
>
> - This caching of pipeline data in SG2D is a new precedent and I'm wary of
> it for a couple of reasons:
> - It gets tricky to satisfy all pipelines using that kind of technique
> - It breaks encapsulation, but at least it is isolated to internal code
> - SG2D will need to deal with the new field in clone().
> - Rendering a hierarchy of Swing objects uses clone() a lot so caching
> storage in SG2D may not help much in that case and thread local may be more
> of a win
> - Thread local storage doesn't really have those issues
> - It's only used if that pipeline is used
> - No encapsulation issues
> - Don't need to modify clone() and it will work across clones
>
Ok nevermind, I thought first it was possible but I agree it is not a
proper solution.
> - Curve iterator uses auto-boxing, is that causing any churn?
>
No, integers are in the Integer cache.
> - RenderingEngine may want to provide "forwarding methods" for Ductus as a
> temporary solution until we fix Ductus to be aware of the new signatures
>
Done in the new patch.
> - new constants in Dasher were moved out of the class they are used
> from...?
>
To be done (TBD)
> - Why get rid of the final modifiers in Dasher? Was it not as effective
> as copying to local variables? Note that the manual copying to local
> variables is prone to maintenance issues if someone inserts a method call
> in a block where the fields are stale.
>
I have to fix it.
> - PRE.pathTo() could still be static, no? Also, it would be nice to make
> this change to the existing RE helper method directly (and possibly provide
> a backwards compatibility method with the old argument list that simply
> forwards with a "new float[6]").
>
Agreed: TBD
> - Perhaps it is time to get rid of the try/catch pairs in PTG.getAlpha()
> entirely?
>
Done.
> - When you have a field cached in a local variable and you compute a new
> value for it, then "field = var = ..." is probably better than "var = field
> = ..." since the latter implies that the answer gets stored in the field
> and then read back which is an extra memory operation. I noticed this in a
> couple of places in Renderer, but I know I saw the local variable caching
> in other files as well.
>
Fixed.
>
> - A lot of undoing of some very carefully planned indentation and code
> alignment issues. Auto-formatting is evil... ;)
>
Sorry, I may tune netbeans formatting settings.
> - A personal rant - I'm a big fan of the { on a line by itself if it
> follows an indented line-continued method declaration or control statement.
> It helps the eye quickly find the start of the body because it stands out.
> Your autoformatting got rid of a bunch of those and I made a frowny
> face... :( (It may not be true to the JDK code style guidelines, but we've
> been using that style throughout the 2D code for years...)
>
Sorry again; however I like autoformating to let me work more on the code
not on syntax / indentation.
>
> - We switched to a new "within" technique in the JavaFX version of Pisces
> based upon this paper:
>
> http://www.cygnus-software.**com/papers/comparingfloats/**
> comparingfloats.htm<http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm>
>
> Good idea, but I think there is many place where float <-> int conversion
/ tests should be improved ...
Do you have any faster implementation for Math.ceil/floor ? I now use
casting 1 + (int) / (int) but I know it is only correct for positive
numbers (> 0).
I have found few bugs:
- rendering a infinite line [vertical] draws nothing: I have to do a
diagnostic ...
- clipping issues:
for very large dashed rectangle, millions of segments are emited from
dasher (x1) to stroker (x4 segments) to renderer (x4 segments).
However, I would like to add a minimal clipping algorithm but the rendering
pipeline seems to be handling affine transforms between stages:
/*
* Pipeline seems to be:
* shape.getPathIterator
* -> inverseDeltaTransformConsumer
* -> Dasher
* -> Stroker
* -> deltaTransformConsumer OR transformConsumer
* -> pc2d = Renderer (bounding box)
*/
It is complicated to perform clipping in the Renderer and maybe to late as
a complete stroker's segment must be considered as a whole (4 segments
without caps ...).
Could you give me advices how to hack / add clipping algorithm in the
rendering pipeline (dasher / stroker / renderer) ?
Should I try to derive a bounding box for the dasher / stroker depending on
the Affine transforms ?
Laurent
