RFR: 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box [v10]

[Phil Race]

On Fri, 19 Nov 2021 19:09:54 GMT, Jeremy <duke at openjdk.java.net> wrote:

>> This removes code that relied on consulting the Bezier control points to calculate the Rectangle2D bounding box. Instead it's pretty straight-forward to convert the Bezier control points into the x & y parametric equations. At their most complex these equations are cubic polynomials, so calculating their extrema is just a matter of applying the quadratic formula to calculate their extrema. (Or in path segments that are quadratic/linear/constant: we do even less work.)
>> 
>> The bug writeup indicated they wanted Path2D#getBounds2D() to be more accurate/concise. They didn't explicitly say they wanted CubicCurve2D and QuadCurve2D to become more accurate too. But a preexisting unit test failed when Path2D#getBounds2D() was updated and those other classes weren't. At this point I considered either:
>> A. Updating CubicCurve2D and QuadCurve2D to use the new more accurate getBounds2D() or
>> B. Updating the unit test to forgive the discrepancy.
>> 
>> I chose A. Which might technically be seen as scope creep, but it feels like a more holistic/better approach.
>> 
>> Other shapes in java.awt.geom should not require updating, because they already identify concise bounds.
>> 
>> This also includes a new unit test (in Path2D/UnitTest.java) that fails without the changes in this commit.
>
> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6227