On Wed, 15 Dec 2021 23:49:24 GMT, Phil Race <p...@openjdk.org> wrote:
>> Jeremy has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control >> points in bounding box >> >> Addressing code review feedback: "Use BigDecomal.setScale(40) to ascertain >> precision." >> >> The geom.* unit tests passed before & after this change. >> >> https://github.com/openjdk/jdk/pull/6227#discussion_r752908494 > > There seems to be a number of things to discuss aside from the maths > > - You are changing the signature of public API > src/java.desktop/share/classes/java/awt/geom/Path2D.java > public final synchronized Rectangle2D getBounds2D() => public Rectangle2D > getBounds2D() { > > So no longer final, and no longer synchronized. > This means a CSR is required and we need to think about it .. the intention > was that the subclass not over-ride. > And why remove synchronized ? I am fairly sure it was there to make sure no > one was mutating the Path whilst > bounds are being calculated. > And you are using getPathIterator(AffineTransform) and the docs for that say > it isn't thread safe. > So I think this implementation needs to be thought about very carefully. > > - You are adding a new public static method > > public static Rectangle2D getBounds2D(final PathIterator pi); > > Is this really necessary ? It is just for the benefit of the public API > caller so can be package private. > > - For me it doesn't build because of a doclint error > > src/java.desktop/share/classes/java/awt/geom/Path2D.java:2102: warning: no > @param for pi > public static Rectangle2D getBounds2D(final PathIterator pi) { > ^ > error: warnings found and -Werror specified > > Fixing that I find that the updated UnitTest.java fail with a build with > these changes :- > > % java UnitTest > Exception in thread "main" java.lang.RuntimeException: the shape must not > intersect anything above its bounds > at UnitTest.testGetBounds2D(UnitTest.java:1396) > at UnitTest.testBounds(UnitTest.java:1302) > at UnitTest.test(UnitTest.java:1175) > at UnitTest.main(UnitTest.java:1435) > > Then we have some even more difficult questions. > The control points may not be within the bounds of the curve which is of > course the premise of the bug report. > The specification (javadoc) isn't very forthcoming on whether it is allowed > to not include them .. ie is it expected they should be ? > And it appears we have JCK tests which expect they ARE included. So as it is > this change would not pass the existing TCK/JCK. > Which means we'd have to be 100% sure about this change in behaviour and be > able to justify it. > FWIW unless the spec explicitly says they are, in some place I've overlooked, > then I'd say its a quality-of-implementation thing as to how tight the bounds > are, and the new behaviour is allowed by spec. > Applications however might still find this a behaviourally incompatible > change .. and could potentially see undesirable consequences - > even as other applications see better consequences. > So perhaps new API that defines tighter bounds might be the way to address > this .. but I am far from being sure about that. @prrace thanks for your feedback. The first several points you raised were bugs/problems that I believe I have addressed just now. Specifically: 1. The method signatures were not meant to change, so the method modifiers are restored. 2. I see no reason why the new Path2D#getBounds2D() can't be package private for now. So that's updated. (If we ever want to change that: that can be a separate ticket/PR.) 3. The missing @param tag is fixed 4. The unit test failure you observed is fixed. (That's actually a new unit test. Although your point stands that existing tests had to be modified to adjust for this new behavior.) Regarding bigger issues, you wrote: > The specification (javadoc) isn't very forthcoming on whether [the bounding > box] is allowed to not include > [control points] .. ie is it expected they should be ? I would argue the crux of the Shape#getBounds2D() contract is this phrase: "the Shape lies entirely within the indicated Rectangle2D". And by "Shape" the original authors probably mean "the defining outline" (a phrase referenced in the second paragraph of the javadoc). If instead the word "Shape" is interpreted as "all the points in the path, including control points" then (I think?) that means the Area class suddenly violates the contract. (Because it has always returned a very tight bounding box that (I think?) disregards control points.) And if we "fix" the Area class so its getBounds2D() method can intentionally return more empty space: that seems antithetical to the intent of the original ticket under consideration. So as of this writing: A. There should be no changed (or new) public method signatures in this PR. B. IMO there is no need to review an interface change for the Shape#getBounds2D() method -- or the Path2D#getBounds2D() method. What do you think? I'm happy to proceed with a CSR if you think this requires further consideration, I just wanted to double-check. ------------- PR: https://git.openjdk.java.net/jdk/pull/6227
