On Thu, 16 Dec 2021 18:33:37 GMT, Jeremy <d...@openjdk.java.net> wrote:
>> 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.
Philip requested a CSR, but (as I understand it) I'm not authorized to initiate
a CSR.
If Philip still thinks a CSR is appropriate (see previous comment): can someone
on this mailing list help us initiate that?
And/or: if this PR doesn't make it out of review, it might be nice to at least
add some comments to the openjdk bug detailing this solution for future
reference. (That is: if we can't update Path2D.java, then we can give a
work-around in the openjdk bug so developers can copy and paste it as needed.)
-------------
PR: https://git.openjdk.java.net/jdk/pull/6227
