Kevin,
Here are my comments below:
2018-06-16 1:47 GMT+02:00 Kevin Rushforth <kevin.rushfo...@oracle.com
<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 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