Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

I confirm my comments can be summarized as a "+1" - Johan On Thu, Jul 5, 2018 at 3:24 PM Kevin Rushforth 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. >

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

> 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

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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 wrote: > Looks good. > > +1 -- note that needs a

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

Thanks Kevin for your approval. Phil or Sergey, could you make another review ? I would like to push this large patch in jfx 11. Regards, Laurent Le mar. 3 juil. 2018 à 18:03, Kevin Rushforth a écrit : > Looks good. > > +1 -- note that needs a second reviewer (doesn't need to be a capital-R

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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.

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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"

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

> 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

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

Hi Laurent, > 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

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

Kevin, Thanks for the review ! 2018-06-29 22:52 GMT+02:00 Kevin Rushforth : > I'm giving a +1 on the implementation changes. I scanned the webrev and > didn't see anything out of place. I compared the diffs of the FX Marlin > 0.9.2 with the Java2D 0.9.1 changeset, and there were a few more

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

I'm giving a +1 on the implementation changes. I scanned the webrev and didn't see anything out of place. I compared the diffs of the FX Marlin 0.9.2 with the Java2D 0.9.1 changeset, and there were a few more diffs than I might have expoected, but nothing jumped out of me as a problem. Also,

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

One more thing about the test. All of the OpenJFX unit tests should have GPL v2 + Classpath Exception (this differs from the JDK). -- Kevin On 6/29/2018 10:23 AM, Kevin Rushforth wrote: I'll plan to review the code today if possible. This will need one more reviewer, so maybe Phil can also

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

I'll plan to review the code today if possible. This will need one more reviewer, so maybe Phil can also review it, since he reviewed the Java2D patch? As for my comments on the test: Finally I think this test should be manually run only if Marlin renderer is modified. How to do that ?

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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

Re: [11] JDK-8204621: Upgrade MarlinFX to 0.9.2

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