> 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
This is a second follow-up response to prrace's code review feedback about
method modifiers.
This commit more carefully preserves the getBounds2D() method modifiers for
all 3 classes: the Path2D.Double, the Path2D.Float, and the Path2D itself.
It is possible (if unlikely) that someone previously extended the Path2D
class and overrode getBounds2D(), because the Path2D#getBounds2D() method was
*not* final. So with this commit: any such existing code will not break.
Meanwhile the subclasses (Double and Float) preserve their previous modifiers
(final, synchronized).
This is in response to prrace's code review:
> 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.
-------------
Changes:
- all: https://git.openjdk.java.net/jdk/pull/6227/files
- new: https://git.openjdk.java.net/jdk/pull/6227/files/d79f067d..faeb2419
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6227&range=11
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6227&range=10-11
Stats: 24 lines in 1 file changed: 16 ins; 8 del; 0 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
