[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: 8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding 
From:       Jeremy <duke () openjdk ! java ! net>
Date:       2021-12-31 20:14:12
Message-ID: BTv7geAN9SJIFuALt1iiMUj8uCJNON_iiJPXA0jR8Cc=.126eaa56-ddd4-4101-8d26-991a2937d460 () github ! com
[Download RAW message or body]

On Thu, 16 Dec 2021 18:33:37 GMT, Jeremy <duke@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


[prev in list] [next in list] [prev in thread] [next in thread]