Kevin, Here are my comments below:
2018-06-16 1:47 GMT+02:00 Kevin Rushforth <[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 >> 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 >> > >
