Hi Jim,
Thanks for your comments, it helped refining the webrev.
Here are my answers:
2017-08-26 2:22 GMT+02:00 Jim Graham <james.gra...@oracle.com>:
> [D]Dasher.java - why the changes from (firstSegIdx > 0) to (firstSegIdx !=
> 0)?
>
As firstSegIdx is initialized to 0, I prefer testing (firstSegIdx != 0) as
it looks more obvious.
For me, (firstSegIdx > 0) indicates that the sign has a particular meaning
and that firstSegIdx may be negative.
> [D]Dasher.java - why is there a new goto_starting() which is only used
> from one place? To be able to add final to the parameters?
>
For inlining purposes, the JITWatch jarscan tool reported that this
(hotspot) method is larger than 325 byte codes so I wanted to split it in
smaller parts.
> [D]Dasher.java, line 268ish - why move the call to out.moveto() down a
> line?
>
To set the needsMoveTo flag just below the condition: if (needsMoveTo) {
needsMoveTo = false; ...}
> [D]Stroker.java, line 170ish - you added final to the float params, but
> not the double
>
Fixed. I also fixed all (D)PathConsumer2D methods overriden in Dasher,
Stroker & Renderer.
> [D]Stroker.java, line 196ish - why are ROUND treated differently. You
> have a question on that as well in a comment.
>
I found the answer: C = 4/3 * (SQRT(2) - 1) is used to compute the control
points (cubics) to approximate a circle. I fixed the constant estimation
according to the math formula.
> [D]Stroker.java, line 196ish - CAP_SQUARE would require more than lw/2
> padding. I think it needs lw*sqrt(2)/2 padding.
>
Exactly, good point !
I would think the padding would be (max of the CAP/JOIN values below):
> CAP_BUTT - lw/2
> CAP_ROUND - lw/2
> CAP_SQUARE - lw/2 * sqrt(2)
> JOIN_BEVEL - lw/2
> JOIN_ROUND - lw/2
> JOIN_MITER - max(lw/2, miter_limit * lw)
>
> In other words:
> - lw/2 by default
> - if CAP_SQUARE then multiply that by sqrt(2)
> - if JOIN_MITER then max it with limit
>
I agree your new rules.
I fixed the (D)Stroker init() methods according the latter rules and tested
again.
Probably I should write a new Clip test rendering Z shapes with all (cap /
join) combinations and their bounds aligned to the Stroker's outside clip
rules.
Here is an updated webrev (Marlin2D only):
http://cr.openjdk.java.net/~lbourges/marlin/marlin-080.1/
PS: I can send you an updated MarlinFX patch (when you need it).
Cheers,
Laurent
> I'm still looking through the management of the closed path detection
> coordinates, but I thought I'd get at least these questions out first
> before the weekend...
>
> ...jim
>
> On 8/25/17 1:09 PM, Laurent Bourgès wrote:
>
>> Hi,
>>
>> Please review Marlin/FX upgrades that provide an efficient path clipper in
>> Stroker (float / double variants) fixing the bug JDK-8184429
>> <https://bugs.openjdk.java.net/browse/JDK-8184429>
>>
>>
>> Marlin2D patch (jdk10):
>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-080.0/
>> MarlinFX patch (openjfx10):
>> http://cr.openjdk.java.net/~lbourges/marlinFX/marlin-080.0/
>>
>> Path Clipper in (D)Stroker:
>> - it uses outcode computation (cohen - sutherland) for segment edges (2
>> for
>> lines, 3 for quads, 4 for cubics)
>> - it opens the path when a segment is invisible ie special moveTo() and
>> ignore invisible joins; it does not compute any intersection (line /
>> curve), it just skips useless segment for better performance and accuracy
>> - the ClosedPathDetector is needed to infer if the path is closed or not
>> (call to closePath) to produce caps when appropriate. It reuses the former
>> PolyStack (moved into Helper classes) to store the forward segments
>> - the clip rectangle is adjusted either in Stroker but also in
>> Transformer2D to take into account the mitter limit, stroker width and
>> also
>> the Renderer offset
>>
>> That's why it can not be applied to closed paths (fill operations) as the
>> path must remain closed in such case (concave polygon).
>> This could be implemented later as it needs to insert corner points when
>> needed to avoid artefacts; so the algorithm seem more complicated to me.
>>
>> Marlin2D / FX Patches are slightly different to handle the Renderer
>> offsets.
>>
>> I tested these patches against my MapBench test with a small clip and
>> several affine transforms: it does not produce any artefact (0 pixel
>> difference between clip=true/false)
>>
>> PS: I also improved the accuracy of Renderer's AFD by using the kaham
>> compensated-sum approach (later patch)
>>
>> Cheers,
>> Laurent Bourgès
>>
>>
--
--
Laurent Bourgès
