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....@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....@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.rushfo...@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
>>> >
>>>
>>>
