Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Johan Vos]

I confirm my comments can be summarized as a "+1"

- Johan

On Thu, Jul 5, 2018 at 3:24 PM Kevin Rushforth 
wrote:

>
> > 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  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  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 <
>>> kevin.rushfo...@oracle.com> 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/
 >> 
 >>
 >> PS: I am not really satisfied by adding such noise in build.gradle,
 >> but it can be improved later ...
 >>
 >> Laurent
 >


>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

> 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 > 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 mailto:johan@gluonhq.com>> 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
mailto:kevin.rushfo...@oracle.com>> 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/


>>

>>
>> PS: I am not really satisfied by adding such noise in
build.gradle,
>> but it can be improved later ...
>>
>> Laurent
>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

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  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  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 <
>> kevin.rushfo...@oracle.com> 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/
>>> >> 
>>> >>
>>> >> PS: I am not really satisfied by adding such noise in build.gradle,
>>> >> but it can be improved later ...
>>> >>
>>> >> Laurent
>>> >
>>>
>>>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Johan Vos]

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  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 
> 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/
>> >> 
>> >>
>> >> PS: I am not really satisfied by adding such noise in build.gradle,
>> >> but it can be improved later ...
>> >>
>> >> Laurent
>> >
>>
>>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Johan Vos]

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 
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/
> >> 
> >>
> >> PS: I am not really satisfied by adding such noise in build.gradle,
> >> but it can be improved later ...
> >>
> >> Laurent
> >
>
>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

Thanks Kevin for your approval.

Phil or Sergey, could you make another review ?

I would like to push this large patch in jfx 11.

Regards,
Laurent

Le mar. 3 juil. 2018 à 18:03, Kevin Rushforth 
a écrit :

> 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/
> >> 
> >>
> >> PS: I am not really satisfied by adding such noise in build.gradle,
> >> but it can be improved later ...
> >>
> >> Laurent
> >
>
>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

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/ 



PS: I am not really satisfied by adding such noise in build.gradle, 
but it can be improved later ...


Laurent

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

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/ 



PS: I am not really satisfied by adding such noise in build.gradle, 
but it can be improved later ...


Laurent

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

>

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/ 



PS: I am not really satisfied by adding such noise in build.gradle, 
but it can be improved later ...


Laurent

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

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/

PS: I am not really satisfied by adding such noise in build.gradle, but it
can be improved later ...

Laurent

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

Hi Laurent,

> 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")

        }

-- Kevin


On 7/3/2018 2:13 AM, Laurent Bourgès wrote:


Kevin,

Thanks for the review !

2018-06-29 22:52 GMT+02:00 Kevin Rushforth >:


I'm giving a +1 on the implementation changes. I scanned the
webrev and didn't see anything out of place. I compared the diffs
of the FX Marlin 0.9.2 with the Java2D 0.9.1 changeset, and there
were a few more diffs than I might have expoected, but nothing
jumped out of me as a problem. Also, I've tested it pretty well on
all three platforms.

The overall +1 is pending the fixes needed for the test: at least
the copyright header and shortening or disabling the test.


Here is the updated webrev fixing the test:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.1/ 



ClipShapeTest incremental diff:
--- /tmp/meld-tmpyWO0FS
+++ 
/home/bourgesl/libs/marlin/branches/marlin-fx-openjdk/src/test/java/test/manual/marlin/ClipShapeTest.java

@@ -4,7 +4,9 @@
  *
  * This code is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation.
+ * published by the Free Software Foundation. Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
  *
  * This code is distributed in the hope that it will be useful, but 
WITHOUT

  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
@@ -219,12 +221,10 @@

 // cubic min/max error:
System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
- System.setProperty("prism.marlin.cubic_inc_d1", "1e-4"); // or 
disabled ~ 1e-6

+ System.setProperty("prism.marlin.cubic_inc_d1", "1e-4");

 // quad max error:
System.setProperty("prism.marlin.quad_dec_d2", "5e-4");
-
- System.setProperty("javafx.animation.fullspeed", "true"); // full speed
 }

 // Application class. An instance is created and initialized 
before running

@@ -273,7 +273,7 @@
 }

 private static void resetOptions() {
-    NUM_TESTS = 5000;
+    NUM_TESTS = Integer.getInteger("ClipShapeTest.numTests", 100);

 // shape settings:
 SHAPE_MODE = ShapeMode.NINE_LINE_POLYS;

Changes:
- fixed license (Classpath exception)
- removed "javafx.animation.fullspeed"in the test setup
- use 100 tests by default to shorten the test duration (but I kept 
the high timeout values if the following parameter is increased):


I added the system property "ClipShapeTest.numTests" but it requires a 
build.gradle change to pass the parameter:
  
+ // Marlin ClipShapeTest
+ systemProperty "ClipShapeTest.numTests", 
project.getProperty("ClipShapeTest.numTests")

+
If it is not recommended to add such specific parameters into the 
build.gradle file, what do you recommend ? (manual edit ?)


Best regards,
Laurent




On 6/29/2018 11:25 AM, Kevin Rushforth wrote:

One more thing about the test. All of the OpenJFX unit tests
should have GPL v2 + Classpath Exception (this differs from
the JDK).

-- Kevin


On 6/29/2018 10:23 AM, Kevin Rushforth wrote:


I'll plan to review the code today if possible. This will
need one more reviewer, so maybe Phil can also review it,
since he reviewed the Java2D patch?

As for my comments on the test:

Finally I think this test should be manually run only
if Marlin renderer is modified.
How to do that ? use @Ignore or specific tags ?


As a slight variation of this: How about running a small
number (say, 200 or 250) by default, but adding a flag to
run more? Alternatively, you could use a flag to enable
it, but since you would need to do something extra
(provide a flag or modify the test) to run it, we might as
well get at least some testing all the time. Unless you
really think there is no value in doing this.

I deliberately set all these Marlin clip (runtime +
always subdivider) / curve quality settings (quads /
cubics thresholds) to be sure of t

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

Kevin,

Thanks for the review !

2018-06-29 22:52 GMT+02:00 Kevin Rushforth :

> I'm giving a +1 on the implementation changes. I scanned the webrev and
> didn't see anything out of place. I compared the diffs of the FX Marlin
> 0.9.2 with the Java2D 0.9.1 changeset, and there were a few more diffs than
> I might have expoected, but nothing jumped out of me as a problem. Also,
> I've tested it pretty well on all three platforms.
>
> The overall +1 is pending the fixes needed for the test: at least the
> copyright header and shortening or disabling the test.
>

Here is the updated webrev fixing the test:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.1/

ClipShapeTest incremental diff:
--- /tmp/meld-tmpyWO0FS
+++
/home/bourgesl/libs/marlin/branches/marlin-fx-openjdk/src/test/java/test/manual/marlin/ClipShapeTest.java
@@ -4,7 +4,9 @@
  *
  * This code is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation.
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
  *
  * This code is distributed in the hope that it will be useful, but WITHOUT
  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
@@ -219,12 +221,10 @@

 // cubic min/max error:
 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
-System.setProperty("prism.marlin.cubic_inc_d1", "1e-4"); // or
disabled ~ 1e-6
+System.setProperty("prism.marlin.cubic_inc_d1", "1e-4");

 // quad max error:
 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");
-
-System.setProperty("javafx.animation.fullspeed", "true"); // full
speed
 }

 // Application class. An instance is created and initialized before
running
@@ -273,7 +273,7 @@
 }

 private static void resetOptions() {
-NUM_TESTS = 5000;
+NUM_TESTS = Integer.getInteger("ClipShapeTest.numTests", 100);

 // shape settings:
 SHAPE_MODE = ShapeMode.NINE_LINE_POLYS;

Changes:
- fixed license (Classpath exception)
- removed "javafx.animation.fullspeed" in the test setup
- use 100 tests by default to shorten the test duration (but I kept the
high timeout values if the following parameter is increased):

I added the system property "ClipShapeTest.numTests" but it requires a
build.gradle change to pass the parameter:

 +// Marlin ClipShapeTest+
systemProperty "ClipShapeTest.numTests",
project.getProperty("ClipShapeTest.numTests")+

If it is not recommended to add such specific parameters into the
build.gradle file, what do you recommend ? (manual edit ?)

Best regards,
Laurent



>
> On 6/29/2018 11:25 AM, Kevin Rushforth wrote:
>
>> One more thing about the test. All of the OpenJFX unit tests should have
>> GPL v2 + Classpath Exception (this differs from the JDK).
>>
>> -- Kevin
>>
>>
>> On 6/29/2018 10:23 AM, Kevin Rushforth wrote:
>>
>>>
>>> I'll plan to review the code today if possible. This will need one more
>>> reviewer, so maybe Phil can also review it, since he reviewed the Java2D
>>> patch?
>>>
>>> As for my comments on the test:
>>>
>>> Finally I think this test should be manually run only if Marlin renderer
 is modified.
 How to do that ? use @Ignore or specific tags ?

>>>
>>> As a slight variation of this: How about running a small number (say,
>>> 200 or 250) by default, but adding a flag to run more? Alternatively, you
>>> could use a flag to enable it, but since you would need to do something
>>> extra (provide a flag or modify the test) to run it, we might as well get
>>> at least some testing all the time. Unless you really think there is no
>>> value in doing this.
>>>
>>> I deliberately set all these Marlin clip (runtime + always subdivider) /
 curve quality settings (quads / cubics thresholds) to be sure of the
 concrete Marlin setup as quality thresholds are sensitive to such values.

>>>
>>> As a best practice, tests should generally be run using the same
>>> settings as are used in production. Other than to verify how it behaves
>>> when you change these settings, I don't see the value in testing the system
>>> running in a mode that no application will ever see. I may be missing some
>>> point here.
>>>
>>> -- Kevin
>>>
>>>
>>> On 6/25/2018 9:01 AM, Laurent Bourgès wrote:
>>>
 Kevin,

 Here are my comments below:

 2018-06-16 1:47 GMT+02:00 Kevin Rushforth >>> >:

 I tested this on all three platforms and the updated rasterizer
 looks good.

 I spot checked the code changes, but didn't get time to do a
 complete review yet. I was mostly looking for diffs between the
 Java2D version which was already reviewed, and this one.

 I do have

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

I'm giving a +1 on the implementation changes. I scanned the webrev and 
didn't see anything out of place. I compared the diffs of the FX Marlin 
0.9.2 with the Java2D 0.9.1 changeset, and there were a few more diffs 
than I might have expoected, but nothing jumped out of me as a problem. 
Also, I've tested it pretty well on all three platforms.


The overall +1 is pending the fixes needed for the test: at least the 
copyright header and shortening or disabling the test.


-- Kevin


On 6/29/2018 11:25 AM, Kevin Rushforth wrote:
One more thing about the test. All of the OpenJFX unit tests should 
have GPL v2 + Classpath Exception (this differs from the JDK).


-- Kevin


On 6/29/2018 10:23 AM, Kevin Rushforth wrote:


I'll plan to review the code today if possible. This will need one 
more reviewer, so maybe Phil can also review it, since he reviewed 
the Java2D patch?


As for my comments on the test:

Finally I think this test should be manually run only if Marlin 
renderer is modified.

How to do that ? use @Ignore or specific tags ?


As a slight variation of this: How about running a small number (say, 
200 or 250) by default, but adding a flag to run more? Alternatively, 
you could use a flag to enable it, but since you would need to do 
something extra (provide a flag or modify the test) to run it, we 
might as well get at least some testing all the time. Unless you 
really think there is no value in doing this.


I deliberately set all these Marlin clip (runtime + always 
subdivider) / curve quality settings (quads / cubics thresholds) to 
be sure of the concrete Marlin setup as quality thresholds are 
sensitive to such values.


As a best practice, tests should generally be run using the same 
settings as are used in production. Other than to verify how it 
behaves when you change these settings, I don't see the value in 
testing the system running in a mode that no application will ever 
see. I may be missing some point here.


-- Kevin


On 6/25/2018 9:01 AM, Laurent Bourgès wrote:

Kevin,

Here are my comments below:

2018-06-16 1:47 GMT+02:00 Kevin Rushforth 
mailto:kevin.rushfo...@oracle.com>>:


    I tested this on all three platforms and the updated rasterizer
    looks good.

    I spot checked the code changes, but didn't get time to do a
    complete review yet. I was mostly looking for diffs between the
    Java2D version which was already reviewed, and this one.

    I do have a couple comments on the new ClipShapeTest (which looks
    like a nice accuracy test, btw).

    1. The test runs for way too long (about 20x too long) to include
    in our normal test runs. By default the entire class file (all
    three tests) needs to take < 5 minutes and 2 minutes would be
    better. I measured the time on 4 machines that I have and found
    that if you cut the number of iterations down from 5000 to 250 it
    will be just about the right run time. Then you can set the
    timeout to 120 seconds (the slowest test on the slowest of my
    machines took about 48 seconds, so a 2 minute timeout should be
    plenty).


I agree this test is very long but it is the only mean I found to 
test all possible stroke combinations and test enough shapes (5000) 
to detect bugs.
I wondered if using mask directly (via ShapeUtils.getMaskData()) 
would become faster but it will never run below the 2 minutes 
threshold in total.


Finally I think this test should be manually run only if Marlin 
renderer is modified.

How to do that ? use @Ignore or specific tags ?


    2. Can you explain the reason for setting the following?

     206 // disable static clipping setting:
     207 System.setProperty("prism.marlin.clip", "false");
     208
    System.setProperty("prism.marlin.clip.runtime.enable", "true");
     209
     210 // enable subdivider:
     211 System.setProperty("prism.marlin.clip.subdivider",
    "true");
     212
     213 // disable min length check: always subdivide curves
    at clip edges
     214 System.setProperty("prism.marlin.clip.subdivider.minLength",
    "-1");
     215
     216 // If any curve, increase curve accuracy:
     217 // curve length max error:
     218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
     219
     220 // cubic min/max error:
     221 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
     222 System.setProperty("prism.marlin.cubic_inc_d1",
    "1e-4"); // or disabled ~ 1e-6
     223
     224 // quad max error:
     225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");

    It seems better to test with the default parameters (i.e., it
    makes a better regression test that way).


I deliberately set all these Marlin clip (runtime + always 
subdivider) / curve quality settings (quads / cubics thresholds) to 
be sure of the concrete Marlin setup as quality thresholds are 
sensitive to such values.


The ClipShapeTest is dedicated to test the clipper (+ subdivider) 
part of th

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

One more thing about the test. All of the OpenJFX unit tests should have 
GPL v2 + Classpath Exception (this differs from the JDK).


-- Kevin


On 6/29/2018 10:23 AM, Kevin Rushforth wrote:


I'll plan to review the code today if possible. This will need one 
more reviewer, so maybe Phil can also review it, since he reviewed the 
Java2D patch?


As for my comments on the test:

Finally I think this test should be manually run only if Marlin 
renderer is modified.

How to do that ? use @Ignore or specific tags ?


As a slight variation of this: How about running a small number (say, 
200 or 250) by default, but adding a flag to run more? Alternatively, 
you could use a flag to enable it, but since you would need to do 
something extra (provide a flag or modify the test) to run it, we 
might as well get at least some testing all the time. Unless you 
really think there is no value in doing this.


I deliberately set all these Marlin clip (runtime + always 
subdivider) / curve quality settings (quads / cubics thresholds) to 
be sure of the concrete Marlin setup as quality thresholds are 
sensitive to such values.


As a best practice, tests should generally be run using the same 
settings as are used in production. Other than to verify how it 
behaves when you change these settings, I don't see the value in 
testing the system running in a mode that no application will ever 
see. I may be missing some point here.


-- Kevin


On 6/25/2018 9:01 AM, Laurent Bourgès wrote:

Kevin,

Here are my comments below:

2018-06-16 1:47 GMT+02:00 Kevin Rushforth >:


    I tested this on all three platforms and the updated rasterizer
    looks good.

    I spot checked the code changes, but didn't get time to do a
    complete review yet. I was mostly looking for diffs between the
    Java2D version which was already reviewed, and this one.

    I do have a couple comments on the new ClipShapeTest (which looks
    like a nice accuracy test, btw).

    1. The test runs for way too long (about 20x too long) to include
    in our normal test runs. By default the entire class file (all
    three tests) needs to take < 5 minutes and 2 minutes would be
    better. I measured the time on 4 machines that I have and found
    that if you cut the number of iterations down from 5000 to 250 it
    will be just about the right run time. Then you can set the
    timeout to 120 seconds (the slowest test on the slowest of my
    machines took about 48 seconds, so a 2 minute timeout should be
    plenty).


I agree this test is very long but it is the only mean I found to 
test all possible stroke combinations and test enough shapes (5000) 
to detect bugs.
I wondered if using mask directly (via ShapeUtils.getMaskData()) 
would become faster but it will never run below the 2 minutes 
threshold in total.


Finally I think this test should be manually run only if Marlin 
renderer is modified.

How to do that ? use @Ignore or specific tags ?


    2. Can you explain the reason for setting the following?

     206 // disable static clipping setting:
     207 System.setProperty("prism.marlin.clip", "false");
     208
    System.setProperty("prism.marlin.clip.runtime.enable", "true");
     209
     210 // enable subdivider:
     211 System.setProperty("prism.marlin.clip.subdivider",
    "true");
     212
     213 // disable min length check: always subdivide curves
    at clip edges
     214 System.setProperty("prism.marlin.clip.subdivider.minLength",
    "-1");
     215
     216 // If any curve, increase curve accuracy:
     217 // curve length max error:
     218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
     219
     220 // cubic min/max error:
     221 System.setProperty("prism.marlin.cubic_dec_d2", 
"1e-3");

     222 System.setProperty("prism.marlin.cubic_inc_d1",
    "1e-4"); // or disabled ~ 1e-6
     223
     224 // quad max error:
     225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");

    It seems better to test with the default parameters (i.e., it
    makes a better regression test that way).


I deliberately set all these Marlin clip (runtime + always 
subdivider) / curve quality settings (quads / cubics thresholds) to 
be sure of the concrete Marlin setup as quality thresholds are 
sensitive to such values.


The ClipShapeTest is dedicated to test the clipper (+ subdivider) 
part of the Marlin renderer.




    3. Related to that, I think you should eliminate the following (I
    don't recommend running functional tests with this set), although
    since you don't do any animation, it probably doesn't matter.

     227 System.setProperty("javafx.animation.fullspeed",
    "true"); // full speed


I will remove it and see if the overall test is not slower.
Is Platform.runLater() impacted by such setting (latency of FX thread 
-> Prism rendering thread ?) ?


Laurent



    On 6/8/2018 7

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

I'll plan to review the code today if possible. This will need one more 
reviewer, so maybe Phil can also review it, since he reviewed the Java2D 
patch?


As for my comments on the test:

Finally I think this test should be manually run only if Marlin 
renderer is modified.

How to do that ? use @Ignore or specific tags ?


As a slight variation of this: How about running a small number (say, 
200 or 250) by default, but adding a flag to run more? Alternatively, 
you could use a flag to enable it, but since you would need to do 
something extra (provide a flag or modify the test) to run it, we might 
as well get at least some testing all the time. Unless you really think 
there is no value in doing this.


I deliberately set all these Marlin clip (runtime + always subdivider) 
/ curve quality settings (quads / cubics thresholds) to be sure of the 
concrete Marlin setup as quality thresholds are sensitive to such values.


As a best practice, tests should generally be run using the same 
settings as are used in production. Other than to verify how it behaves 
when you change these settings, I don't see the value in testing the 
system running in a mode that no application will ever see. I may be 
missing some point here.


-- Kevin


On 6/25/2018 9:01 AM, Laurent Bourgès wrote:

Kevin,

Here are my comments below:

2018-06-16 1:47 GMT+02:00 Kevin Rushforth >:


I tested this on all three platforms and the updated rasterizer
looks good.

I spot checked the code changes, but didn't get time to do a
complete review yet. I was mostly looking for diffs between the
Java2D version which was already reviewed, and this one.

I do have a couple comments on the new ClipShapeTest (which looks
like a nice accuracy test, btw).

1. The test runs for way too long (about 20x too long) to include
in our normal test runs. By default the entire class file (all
three tests) needs to take < 5 minutes and 2 minutes would be
better. I measured the time on 4 machines that I have and found
that if you cut the number of iterations down from 5000 to 250 it
will be just about the right run time. Then you can set the
timeout to 120 seconds (the slowest test on the slowest of my
machines took about 48 seconds, so a 2 minute timeout should be
plenty).


I agree this test is very long but it is the only mean I found to test 
all possible stroke combinations and test enough shapes (5000) to 
detect bugs.
I wondered if using mask directly (via ShapeUtils.getMaskData()) would 
become faster but it will never run below the 2 minutes threshold in 
total.


Finally I think this test should be manually run only if Marlin 
renderer is modified.

How to do that ? use @Ignore or specific tags ?


2. Can you explain the reason for setting the following?

 206 // disable static clipping setting:
 207 System.setProperty("prism.marlin.clip", "false");
 208
System.setProperty("prism.marlin.clip.runtime.enable", "true");
 209
 210 // enable subdivider:
 211 System.setProperty("prism.marlin.clip.subdivider",
"true");
 212
 213 // disable min length check: always subdivide curves
at clip edges
 214 System.setProperty("prism.marlin.clip.subdivider.minLength",
"-1");
 215
 216 // If any curve, increase curve accuracy:
 217 // curve length max error:
 218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
 219
 220 // cubic min/max error:
 221 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
 222 System.setProperty("prism.marlin.cubic_inc_d1",
"1e-4"); // or disabled ~ 1e-6
 223
 224 // quad max error:
 225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");

It seems better to test with the default parameters (i.e., it
makes a better regression test that way).


I deliberately set all these Marlin clip (runtime + always subdivider) 
/ curve quality settings (quads / cubics thresholds) to be sure of the 
concrete Marlin setup as quality thresholds are sensitive to such values.


The ClipShapeTest is dedicated to test the clipper (+ subdivider) part 
of the Marlin renderer.




3. Related to that, I think you should eliminate the following (I
don't recommend running functional tests with this set), although
since you don't do any animation, it probably doesn't matter.

 227 System.setProperty("javafx.animation.fullspeed",
"true"); // full speed


I will remove it and see if the overall test is not slower.
Is Platform.runLater() impacted by such setting (latency of FX thread 
-> Prism rendering thread ?) ?


Laurent



On 6/8/2018 7:28 AM, Laurent Bourgès wrote:

Hi,

Please review this large patch to upgrade MarlinFX to 0.9.2 in
OpenJFX11:
JBS: https://bugs.o

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

Kevin,

Here are my comments below:

2018-06-16 1:47 GMT+02:00 Kevin Rushforth :

> I tested this on all three platforms and the updated rasterizer looks good.
>
> I spot checked the code changes, but didn't get time to do a complete
> review yet. I was mostly looking for diffs between the Java2D version which
> was already reviewed, and this one.
>
> I do have a couple comments on the new ClipShapeTest (which looks like a
> nice accuracy test, btw).
>
> 1. The test runs for way too long (about 20x too long) to include in our
> normal test runs. By default the entire class file (all three tests) needs
> to take < 5 minutes and 2 minutes would be better. I measured the time on 4
> machines that I have and found that if you cut the number of iterations
> down from 5000 to 250 it will be just about the right run time. Then you
> can set the timeout to 120 seconds (the slowest test on the slowest of my
> machines took about 48 seconds, so a 2 minute timeout should be plenty).
>

I agree this test is very long but it is the only mean I found to test all
possible stroke combinations and test enough shapes (5000) to detect bugs.
I wondered if using mask directly (via ShapeUtils.getMaskData()) would
become faster but it will never run below the 2 minutes threshold in total.

Finally I think this test should be manually run only if Marlin renderer is
modified.
How to do that ? use @Ignore or specific tags ?


> 2. Can you explain the reason for setting the following?
>
>  206 // disable static clipping setting:
>  207 System.setProperty("prism.marlin.clip", "false");
>  208 System.setProperty("prism.marlin.clip.runtime.enable",
> "true");
>  209
>  210 // enable subdivider:
>  211 System.setProperty("prism.marlin.clip.subdivider", "true");
>  212
>  213 // disable min length check: always subdivide curves at clip
> edges
>  214 System.setProperty("prism.marlin.clip.subdivider.minLength", "-1");
>  215
>  216 // If any curve, increase curve accuracy:
>  217 // curve length max error:
>  218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
>  219
>  220 // cubic min/max error:
>  221 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
>  222 System.setProperty("prism.marlin.cubic_inc_d1", "1e-4"); //
> or disabled ~ 1e-6
>  223
>  224 // quad max error:
>  225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");
>
> It seems better to test with the default parameters (i.e., it makes a
> better regression test that way).
>

I deliberately set all these Marlin clip (runtime + always subdivider) /
curve quality settings (quads / cubics thresholds) to be sure of the
concrete Marlin setup as quality thresholds are sensitive to such values.

The ClipShapeTest is dedicated to test the clipper (+ subdivider) part of
the Marlin renderer.


>
>
> 3. Related to that, I think you should eliminate the following (I don't
> recommend running functional tests with this set), although since you don't
> do any animation, it probably doesn't matter.
>
>  227 System.setProperty("javafx.animation.fullspeed", "true"); //
> full speed
>

I will remove it and see if the overall test is not slower.
Is Platform.runLater() impacted by such setting (latency of FX thread ->
Prism rendering thread ?) ?

Laurent



>
> On 6/8/2018 7:28 AM, Laurent Bourgès wrote:
>
>> Hi,
>>
>> Please review this large patch to upgrade MarlinFX to 0.9.2 in OpenJFX11:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8198885
>> webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.0/ <
>> http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/>
>> PR: https://github.com/javafxports/openjdk-jfx/pull/96 (CI OK)
>>
>> This patch is almost identical to Marlin(2D) patch, see:
>> https://bugs.openjdk.java.net/browse/JDK-8198885
>>
>> I added the ClipShapeTest (ported to jfx) that compares shape clipping
>> (within threshold) and it works (within large timeouts):
>> gradle -PFULL_TEST=true :system:test --tests
>> test.com.sun.marlin.ClipShapeTest
>>
>> Regards,
>> Laurent
>>
>
>

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Kevin Rushforth]

I tested this on all three platforms and the updated rasterizer looks good.

I spot checked the code changes, but didn't get time to do a complete 
review yet. I was mostly looking for diffs between the Java2D version 
which was already reviewed, and this one.


I do have a couple comments on the new ClipShapeTest (which looks like a 
nice accuracy test, btw).


1. The test runs for way too long (about 20x too long) to include in our 
normal test runs. By default the entire class file (all three tests) 
needs to take < 5 minutes and 2 minutes would be better. I measured the 
time on 4 machines that I have and found that if you cut the number of 
iterations down from 5000 to 250 it will be just about the right run 
time. Then you can set the timeout to 120 seconds (the slowest test on 
the slowest of my machines took about 48 seconds, so a 2 minute timeout 
should be plenty).



2. Can you explain the reason for setting the following?

 206 // disable static clipping setting:
 207 System.setProperty("prism.marlin.clip", "false");
 208 System.setProperty("prism.marlin.clip.runtime.enable", 
"true");

 209
 210 // enable subdivider:
 211 System.setProperty("prism.marlin.clip.subdivider", "true");
 212
 213 // disable min length check: always subdivide curves at 
clip edges

 214 System.setProperty("prism.marlin.clip.subdivider.minLength", "-1");
 215
 216 // If any curve, increase curve accuracy:
 217 // curve length max error:
 218 System.setProperty("prism.marlin.curve_len_err", "1e-4");
 219
 220 // cubic min/max error:
 221 System.setProperty("prism.marlin.cubic_dec_d2", "1e-3");
 222 System.setProperty("prism.marlin.cubic_inc_d1", "1e-4"); 
// or disabled ~ 1e-6

 223
 224 // quad max error:
 225 System.setProperty("prism.marlin.quad_dec_d2", "5e-4");

It seems better to test with the default parameters (i.e., it makes a 
better regression test that way).



3. Related to that, I think you should eliminate the following (I don't 
recommend running functional tests with this set), although since you 
don't do any animation, it probably doesn't matter.


 227 System.setProperty("javafx.animation.fullspeed", "true"); 
// full speed



-- Kevin


On 6/8/2018 7:28 AM, Laurent Bourgès wrote:

Hi,

Please review this large patch to upgrade MarlinFX to 0.9.2 in OpenJFX11:
JBS: https://bugs.openjdk.java.net/browse/JDK-8198885
webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.0/ 


PR: https://github.com/javafxports/openjdk-jfx/pull/96 (CI OK)

This patch is almost identical to Marlin(2D) patch, see:
https://bugs.openjdk.java.net/browse/JDK-8198885

I added the ClipShapeTest (ported to jfx) that compares shape clipping 
(within threshold) and it works (within large timeouts):
gradle -PFULL_TEST=true :system:test --tests 
test.com.sun.marlin.ClipShapeTest


Regards,
Laurent

[11] JDK-8204621: Upgrade MarlinFX to 0.9.2

[Laurent Bourgès]

Hi,

Please review this large patch to upgrade MarlinFX to 0.9.2 in OpenJFX11:
JBS: https://bugs.openjdk.java.net/browse/JDK-8198885
webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-092.0/
PR: https://github.com/javafxports/openjdk-jfx/pull/96 (CI OK)

This patch is almost identical to Marlin(2D) patch, see:
https://bugs.openjdk.java.net/browse/JDK-8198885

I added the ClipShapeTest (ported to jfx) that compares shape clipping
(within threshold) and it works (within large timeouts):
gradle -PFULL_TEST=true :system:test --tests
test.com.sun.marlin.ClipShapeTest

Regards,
Laurent