Kevin, Thanks for the review !
2018-06-29 22:52 GMT+02:00 Kevin Rushforth <[email protected]>: > 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 <[email protected] >>>> <mailto:[email protected]>>: >>>> >>>> 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 >>>> <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/> >>>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/ >>>> <http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-092.0/>> >>>> PR: https://github.com/javafxports/openjdk-jfx/pull/96 >>>> <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 >>>> <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 >>>> >>>> >>>> >>>> >>> >> > -- -- Laurent Bourgès
