> 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
