Jim,<br><br>here is the latest webrev:<br><a href="http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-3/">http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-3/</a><br><br>I tried to revert some syntax mistakes (indentation, moved constants + cleanup) : code cleanup is still in progress;<br>
so please don't have a frawny face while looking at the code!<br><br>I fixed also Ductus and Jules rendering engines (TileState added in interface)<br><br>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: <br>
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]...<br><br>I have few questions regarding renderer edge handling and float ceil calls:<br>
    private void addLine(float x1, float y1, float x2, float y2) {<br>...<br>        // LBO: why ceil ie upper integer ?<br>        final int firstCrossing = Math.max(ceil(y1), boundsMinY); // upper integer<br>        final int lastCrossing  = Math.min(ceil(y2), boundsMaxY); // upper integer<br>
<br>=> firstCrossing / lastCrossing should use lower and upper integer values (rounding issues) ?<br><br>    boolean endRendering() {<br>        // TODO: perform shape clipping to avoid dealing with segments out of bounding box <br>
        <br>        // Ensure shape edges are within bbox:<br>        if (edgeMinX > edgeMaxX || edgeMaxX < 0f) {<br>            return false; // undefined X bounds or negative Xmax<br>        }<br>        if (edgeMinY > edgeMaxY || edgeMaxY < 0f) {<br>
            return false; // undefined Y bounds or negative Ymax<br>        }<br><br>        // why use upper integer for edge min values ?<br>=> Here is the question: why use ceil instead of floor ?<br>        <br>        final int eMinX = ceil(edgeMinX); // upper positive int<br>
        if (eMinX > boundsMaxX) {<br>            return false; // Xmin > bbox maxX<br>        }<br><br>        final int eMinY = ceil(edgeMinY); // upper positive int<br>        if (eMinY > boundsMaxY) {<br>            return false; // Ymin > bbox maxY<br>
        }<br><br>        int spminX = Math.max(eMinX, boundsMinX);<br>        int spmaxX = Math.min(ceil(edgeMaxX), boundsMaxX); // upper positive int<br>        int spminY = Math.max(eMinY, boundsMinY);<br>        int spmaxY = Math.min(ceil(edgeMaxY), boundsMaxY); // upper positive int<br>
<br>        int pminX = spminX >> SUBPIXEL_LG_POSITIONS_X;<br>        int pmaxX = (spmaxX + SUBPIXEL_MASK_X) >> SUBPIXEL_LG_POSITIONS_X;<br>        int pminY = spminY >> SUBPIXEL_LG_POSITIONS_Y;<br>        int pmaxY = (spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y;<br>
<br>        // store BBox to answer ptg.getBBox():<br>        this.cache.init(pminX, pminY, pmaxX, pmaxY);<br><br>In PiscesCache:<br>    void init(int minx, int miny, int maxx, int maxy) {<br>...<br>        // LBO: why add +1 ??<br>
        bboxX1 = maxx /* + 1 */;<br>        bboxY1 = maxy /* + 1 */;<br><br>=> piscesCache previously added +1 to maximum (upper loop condition y < y1)<br>but the pmaxX already uses ceil (+1) and (spmaxY + SUBPIXEL_MASK_Y) ensures the last pixel is reached.<br>
<br>I fixed these limits to have correct rendering due to dirty rowAARLE reuse.<br><br>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. <br>
<br>Few comments:<br><br>2013/4/24 Jim Graham <span dir="ltr"><<a href="mailto:james.graham@oracle.com" target="_blank">james.graham@oracle.com</a>></span><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">On 4/24/13 1:59 AM, Laurent Bourgčs wrote:<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Jim,<br>
<br>
First, here are both updated webrev and benchmark results:<br>
- results: <a href="http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/patch_opt_night.log" target="_blank">http://jmmc.fr/~bourgesl/<u></u>share/java2d-pisces/patch_opt_<u></u>night.log</a><br>
- webrev: <a href="http://jmmc.fr/%7Ebourgesl/share/java2d-pisces/webrev-2/" target="_blank">http://jmmc.fr/~bourgesl/<u></u>share/java2d-pisces/webrev-2/</a><br>
</blockquote>
<br></div>
Some comments on the webrev:<br>
<br>
- This caching of pipeline data in SG2D is a new precedent and I'm wary of it for a couple of reasons:<br>
    - It gets tricky to satisfy all pipelines using that kind of technique<br>
    - It breaks encapsulation, but at least it is isolated to internal code<br>
    - SG2D will need to deal with the new field in clone().<br>
    - 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<br>
- Thread local storage doesn't really have those issues<br>
    - It's only used if that pipeline is used<br>
    - No encapsulation issues<br>
    - Don't need to modify clone() and it will work across clones<br></blockquote><div><br>Ok nevermind, I thought first it was possible but I agree it is not a proper solution.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


- Curve iterator uses auto-boxing, is that causing any churn?<br></blockquote><div>No, integers are in the Integer cache.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

- RenderingEngine may want to provide "forwarding methods" for Ductus as a temporary solution until we fix Ductus to be aware of the new signatures<br></blockquote><div>Done in the new patch.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

- new constants in Dasher were moved out of the class they are used from...?<br></blockquote><div>To be done (TBD)<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

- 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.<br>
</blockquote><div>I have to fix it.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- 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]").<br>
</blockquote><div>Agreed: TBD<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- Perhaps it is time to get rid of the try/catch pairs in PTG.getAlpha() entirely?<br></blockquote><div>Done.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

- 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.<br>
</blockquote><div>Fixed. <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- A lot of undoing of some very carefully planned indentation and code alignment issues.  Auto-formatting is evil... ;)<br></blockquote><div>Sorry, I may tune netbeans formatting settings.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

- 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...)<br>
</blockquote><div>Sorry again; however I like autoformating to let me work more on the code not on syntax / indentation. <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<br>
- We switched to a new "within" technique in the JavaFX version of Pisces based upon this paper:<br>
<br>
<a href="http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm" target="_blank">http://www.cygnus-software.<u></u>com/papers/comparingfloats/<u></u>comparingfloats.htm</a><br>
<br></blockquote><div>Good idea, but I think there is many place where float <-> int conversion / tests should be improved ...<br> <br>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).<br>
</div></div><br>I have found few bugs:<br>- rendering a infinite line [vertical] draws nothing: I have to do a diagnostic ...<br>- clipping issues: <br>for very large dashed rectangle, millions of segments are emited from dasher (x1) to stroker (x4 segments) to renderer (x4 segments).<br>
<br>However, I would like to add a minimal clipping algorithm but the rendering pipeline seems to be handling affine transforms between stages:<br>        /*<br>         * Pipeline seems to be:<br>         *    shape.getPathIterator <br>
         * -> inverseDeltaTransformConsumer <br>         * -> Dasher <br>         * -> Stroker <br>         * -> deltaTransformConsumer OR transformConsumer<br>         * -> pc2d = Renderer (bounding box)<br>
         */<br><br>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 ...).<br><br>Could you give me advices how to hack / add clipping algorithm in the rendering pipeline (dasher / stroker / renderer) ?<br>
<br>Should I try to derive a bounding box for the dasher / stroker depending on the Affine transforms ?<br><br>Laurent<br>