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

Jeremy duke at openjdk.java.net
Thu Dec 16 11:25:44 UTC 2021


> 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 five additional commits since the last revision:

 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box
   
   Fixing compilation error now that Path2D#getBounds is not public
 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box
   
   Fixing unit test failures introduced with b3e84a5e4281c2bb9be1c7e1d751ef3593cc387c .
   
   (The likely explanation for why this went unaddressed previously is: I probably didn't correctly execute unit tests after this change. A less likely explanation might be: the Path2D/UnitTest class uses random numbers each session, so it's possible results passed in that session and failed in others.)
 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box
   
   Adding missing @param tag.
   
   This is in response to prrace's code review feedback:
   
   > 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
   
   https://github.com/openjdk/jdk/pull/6227#issuecomment-995305817
 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box
   
   Make the new getBounds2D(PathIterator) method package private.
   
   This is in response to prrace's code review feedback:
   
   > 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.
   
   https://github.com/openjdk/jdk/pull/6227#issuecomment-995305817
   
   Personally (having written custom java.awt.Shape classes) I think this method would be of general use (so it could be public), but if that's the minority opinion: I'm happy to convert it to package private for now. If we ever want to discuss making it public then that could be a separate PR/conversation.
 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box
   
   Restore the "final" and "synchronized" modifiers for this method. I don't think these were removed on purpose; the intent behind this PR is not related to the modifiers of the method.
   
   This is in response to prrace's code review comment:
   
   > 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.
   
   https://github.com/openjdk/jdk/pull/6227#issuecomment-995305817

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6227/files
  - new: https://git.openjdk.java.net/jdk/pull/6227/files/76805330..d79f067d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6227&range=10
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6227&range=09-10

  Stats: 9 lines in 3 files changed: 5 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6227.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6227/head:pull/6227

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



More information about the client-libs-dev mailing list