> 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 more of Laurent's code review recommendations/comments:
1. solve the quadratic equation using QuadCurve2d.solveQuadratic() or like
Helpers.quadraticRoots()
(I was pleasantly surprised to see QuadCurve2D.solveQuadratic(..) does well
for the unit test where the t^2 coefficient approaches zero. We still get an
extra root, but it's greater than 10^13, so it is ignored by our (0,1) bounds
check later.)
2. determine the derivatives da / db
We now define x_deriv_coeff and y_deriv_coeff.
3. remove the label pathIteratorLoop
4. use `for (final PathIterator it = shape.getPathIterator(null);
!it.isDone(); it.next()) {`
(The initial statement is empty in this case because PathIterator is an
argument.)
5. make arrays final to be obvious
6. add a shortcut test for better readability / close the shortcut test
7. after computing coefficients (abcd), also compute (da db c) needed by root
finding next
8. useless with the shortcut test (re "definedParametricEquations" boolean)
9. use if (t > 0.0 && t < 1.0)
(Sorry, that got lost in the prev refactor.)
10. add explicitely the SEG_CLOSE case (skip = continue) before the default
case
This commit does not address comments about accuracy/precision. I'll explore
those separately later.
-------------
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6227/files
- new: https://git.openjdk.java.net/jdk/pull/6227/files/5a0e2bd0..410cd6ce
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6227&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6227&range=01-02
Stats: 121 lines in 2 files changed: 9 ins; 70 del; 42 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
