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

 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control 
points in bounding box
   
   This is broadly addresses the code review feedback: "I see many duplicated 
lines". This commit does NOT include "the previous unified solution" that is 
also mentioned... so we'll try this on and see how we feel about it.
   
   https://github.com/openjdk/jdk/pull/6227#issuecomment-964020449
 - 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control 
points in bounding box
   
   Applying Laurent's implementation to address machine error.
   
   See https://github.com/openjdk/jdk/pull/6227#discussion_r749672090

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6227/files
  - new: https://git.openjdk.java.net/jdk/pull/6227/files/40bda064..0e2b5293

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

  Stats: 316 lines in 2 files changed: 142 ins; 158 del; 16 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

Reply via email to