I confirm my comments can be summarized as a "+1" - Johan
On Thu, Jul 5, 2018 at 3:24 PM Kevin Rushforth <[email protected]> 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 <[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]> 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]> 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/> >>>> >> >>>> >> PS: I am not really satisfied by adding such noise in build.gradle, >>>> >> but it can be improved later ... >>>> >> >>>> >> Laurent >>>> > >>>> >>>> >
