[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 <kevin.rushforth at oracle.com>
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 <johan.vos at gluonhq.com> 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 <johan.vos at 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 <
>>> kevin.rushforth at 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/
>>>> >> <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
>>>> >
>>>>
>>>>
>