Hi Phil,
Thank you a lot for your review and your time on this long & complex review.
2018-03-26 22:50 GMT+02:00 Philip Race <philip.r...@oracle.com>:
> Hi Laurent,
>
> It took me I at least 5 tries to get all the way through this.
>
>
I agree it was a complex patch (clipping) with many other small
improvements, sorry.
> I don't have any substantial comments, just a few clean up questions.
>
>
> What is this in Curve.java + DCurve.java ?
>
> Even if derivatives are totally useless for lines, I removed the condition
to reset these values anyway.
> + if (false) {+ dax = 0.0d;+ day = 0.0d;+
> dbx = 0.0d;+ dby = 0.0d;+ }
>
> In Renderer.java, should the commented line be removed ?
>
> + && ((Math.abs(ddx) + Math.abs(ddy) * _SCALE_DY) <=
> _INC_BND+// && (Math.abs(ddx + dddx) + Math.abs(ddy +
> dddy) * _SCALE_DY) <= _INC_BND
>
> A similar pattern occurs a little later in the same file.
>
> +// || (Math.abs(ddx + dddx) + Math.abs(ddy + dddy) *
> _SCALE_DY) >= _DEC_BND
>
> Fixed (removed lines)
> + static final float LEN_TH =
> MarlinProperties.getSubdividerMinLength();
>
> You really meant to name it LEN_TH and not LENGTH ?
>
> That was deliberate, I wanted to shorten LENGTH_THRESHOLD => LEN_TH, not
LENGTH.
There are a few TODO's I see but they seem to be more about later
clean up or optimisation
> so are probably all OK.
>
> I added few new TODO that I hope to fix later (nothing critical).
> So I am OK to push, and if you clean up any of the above first I don't need
> to see a new webrev.
>
> Thank you again,
Laurent
>
> On 3/23/18, 2:03 PM, Laurent Bourgès wrote:
>
> Hi,
>
> Sorry to insist but I would like to get feedback on this Marlin patch soon
> before going forward on tile-size tuning in java2d accelerated pipelines.
>
> Laurent
>
> 2018-03-21 22:56 GMT+01:00 Laurent Bourgès <bourges.laur...@gmail.com>:
>
>> Hi,
>>
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-091.1/
>>
>> Changes in MarlinProperties only:
>> - getTileSize_Log2() & getTileWidth_Log2(); 32x32 tiles ie default = 5
>> (log2)
>>
>> I hope it is good for now as tile settings are the same as in jdk9/10.
>>
>> Regards,
>> Laurent
>>
>>
>> 2018-03-21 21:44 GMT+01:00 Laurent Bourgès <bourges.laur...@gmail.com>:
>>
>>> Sergey,
>>>
>>> Le mer. 14 mars 2018 à 17:14, Sergey Bylokhov <
>>> sergey.bylok...@oracle.com> a écrit :
>>>
>>>> On 13/03/2018 17:04, Sergey Bylokhov wrote:
>>>>
>>>>> I have started to look to this review, will run some closed tests and
>>>>> send a feedback soon.
>>>>>
>>>>
>>>> No issues found so far, +1.
>>>
>>>
>>> Thanks for your vote.
>>> I need another approval I suppose ?
>>>
>>> I will prepare another review asap reverting only tile size changes as
>>> using large tiles has performance drop on d3d & ogl that needs more work.
>>> It can be done later in follow-up issues.
>>>
>>> Phil do you agree the proposed plan ?
>>>
>>> Laurent
>>>
>>
>>
>
