RFR: 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box [v3]
Jeremy
duke at openjdk.java.net
Fri Nov 5 19:51:18 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 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
More information about the client-libs-dev
mailing list