> Kevin & Johan, do you consider the last webrev is approved by 2
reviewers ?
I took Johan's comment to be a "+1" from him. He can correct us if that
wasn't his intent.
-- Kevin
On 7/5/2018 6:07 AM, Laurent Bourgès wrote:
Johan,
I agree Marlin 0.9.2 provides new clipping algorithms (curve
subdivision & clipping in the dasher stage) so it involves lots of
maths and tricks ...
I implemented the automated ClipShapeTest that compares rendering with
clipping enabled vs disabled for all possible combinations:
- stroked & dashed paths with all sort of joins / caps
- filled paths with all filling rules (NZ /EO)
with
lines/quads/curves ...
This intensive test proves new algorithms do not introduce visual
regressions except that clipped stroked curves are slightly different
due to the curve offsetting algorithm. Such problem has no exact
solution and the approximated offsetted curve causes the minor visual
differences along the stroked curve.
Finally MarlinFX 0.9.2 is giving same clipping accuracy than Marlin
0.9.1 integrated in jdk11.
Performance is mainly improved for huge dashed paths.
Kevin & Johan, do you consider the last webrev is approved by 2
reviewers ?
Laurent
Le jeu. 5 juil. 2018 à 11:46, Johan Vos <[email protected]
<mailto:[email protected]>> a écrit :
I had a slightly deeper look and had a few comments, but it's hard
to check the math behind it and evaluating performance without
doing real tests.
However, I feel confident we can merge it. That will also allow
eager developers to use it and provide feedback.
- Johan
On Wed, Jul 4, 2018 at 9:55 AM Johan Vos <[email protected]
<mailto:[email protected]>> wrote:
I looked at it, mainly at the differences between the Java2D
patch and the JavaFX patch, and it looks ok to me.
I'll try to test it on linux and/or mac later today and have a
deeper look.
- Johan
On Tue, Jul 3, 2018 at 6:14 PM Kevin Rushforth
<[email protected]
<mailto:[email protected]>> wrote:
Looks good.
+1 -- note that needs a second reviewer (doesn't need to
be a capital-R
Reviewer).
-- Kevin
On 7/3/2018 8:56 AM, Kevin Rushforth wrote:
>> PS: I am not really satisfied by adding such noise in
build.gradle,
>> but it can be improved later ...
>
> Agreed. This can be a follow-on issue. I'll finish my
review shortly.
>
> -- Kevin
>
>
> On 7/3/2018 8:45 AM, Laurent Bourgès wrote:
>> Kevin,
>>
>> > I added the system property
"ClipShapeTest.numTests" but it
>> requires a
>> build.gradle change to pass the parameter:
>>
>> Yes, something like this is what I had in mind. As
long as we
>> don't add too many of these, it is OK with me. Note
that as coded,
>> the build will fail if you don't define
ClipShapeTest.numTests, so
>> you will need to check for that. I note also that
you used tabs in
>> build.gradle (so please change them to spaces). I
recommend the
>> following logic:
>>
>> if
(rootProject.hasProperty("ClipShapeTest.numTests")) {
>> systemProperty "ClipShapeTest.numTests",
>> rootProject.getProperty("ClipShapeTest.numTests")
>> }
>>
>>
>> I adopted your proposal and updated the webrev:
>>
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.2/
<http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.2/>
>>
<http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.2/>
>>
>> PS: I am not really satisfied by adding such noise in
build.gradle,
>> but it can be improved later ...
>>
>> Laurent
>
