Dear Jim,
Normally we'd isolate fixes for different bugs even if they are just
> preparatory formatting changes on comments.
>
I agree but we are short on time and I consider those syntax changes are
minor and without risk.
Here is an updated webrev:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170030.1/
> Some comments on the formatting changes:
>
> line 35 - you should also upper-case the E
>
Fixed
line 148 - I don't understand why this "float" was a formatting danger, but
> the one on line 328 isn't? Not a big issue, but curious since I think the
> edges array does contain floats in it, doesn't it?
>
It was an old comment that is no more true: the off-heap edges only contain
integer values via _unsafe.putInt(addr,...) in addLine()
>
> line 328 - (ints) => [ints] to match? (Also would match line 148)
>
Fixed for consistency.
>
> Comments on the bug fix:
>
> line 1401 - why does maxY want to be bounds+1?
> - later this is used to calculate buckets_maxY
>
Yes.
- do we want an extra Y bucket for some reason?
>
No more, apparently. It was certainly needed before due to the improper
handling of half-open intervals.
- and why only in case of spmaxY was clipped?
> - a comment about that would help
> Other than that one odd case of using "+1" there, that whole block of code
> is just:
> spmaxY = min(edgeMaxY, _boundsMaxY)
> which is much easier to read.
> Perhaps we need an extra bucket only when there were clipped edges below
> the clip?
>
I fixed all that conditions and tested again with Marlin (MapBench
regression tests) and it is now very obvious and simpler, thanks for the
tip !
>
> line 1456 - I can understand why we lose the +1, but not the min() with
> pmaxY?
>
Once again, it is no more needed as:
spmaxY <= (pmaxY << SUBPIXEL_LG_POSITIONS_Y)
ie spmaxY <= ((spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y <<
SUBPIXEL_LG_POSITIONS_Y) = ceil(spmaxY)
>
>
> NoAA comments are identical, though line numbers may be different...
>
I reported all changes in RendererNoAA.
Laurent
>
> On 12/9/16 8:46 AM, Laurent Bourgès wrote:
>
>> Please review this simple fix for MarlinFX:
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8170030
>> webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170030.0/
>>
>> PS: I will provide asap a Marlin2D patch incorporating changes made in
>> MarlinFX (including this one)
>>
>> Cheers,
>> Laurent
>>
>>
