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 >> >>
