On Fri, 5 Nov 2021 04:41:34 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 > > Addressing some of Laurent's code review recommendations/comments: > > 1. use the convention t for the parametric variable x(t),y(t) > 2. solve the quadratic equation using QuadCurve2d.solveQuadratic() or like > Helpers.quadraticRoots() > 3. always use braces for x = (a < b) ? ... > 4. always use double-precision constants in math or logical operations: (2 > * x => 2.0 * x) and (coefficients[3] != 0) => (coefficients[3] != 0.0) > > (There are two additional recommendations not in this commit that I'll ask > about shortly.) > > See https://github.com/openjdk/jdk/pull/6227#issuecomment-959757954 src/java.desktop/share/classes/java/awt/geom/Path2D.java line 2171: > 2169: definedParametricEquations = true; > 2170: > 2171: x_coeff[3] = -lastX + 3.0 * coords[0] - 3.0 * coords[2] > + coords[4]; To improve coefficient accuracy ~ few ulps, it should be written explicitely as differences (= vector notation). See Marlin DCurve formula: void set(final double x1, final double y1, final double x2, final double y2, final double x3, final double y3, final double x4, final double y4) { final double dx32 = 3.0 * (x3 - x2); final double dx21 = 3.0 * (x2 - x1); ax = (x4 - x1) - dx32; // A = P3 - P0 - 3 (P2 - P1) = (P3 - P0) + 3 (P1 - P2) bx = (dx32 - dx21); // B = 3 (P2 - P1) - 3(P1 - P0) = 3 (P2 + P0) - 6 P1 cx = dx21; // C = 3 (P1 - P0) dx = x1; // D = P0 dax = 3.0 * ax; dbx = 2.0 * bx; final double dy32 = 3.0 * (y3 - y2); final double dy21 = 3.0 * (y2 - y1); ay = (y4 - y1) - dy32; by = (dy32 - dy21); cy = dy21; dy = y1; day = 3.0 * ay; dby = 2.0 * by; } void set(final double x1, final double y1, final double x2, final double y2, final double x3, final double y3) { final double dx21 = (x2 - x1); ax = 0.0; // A = 0 bx = (x3 - x2) - dx21; // B = P3 - P0 - 2 P2 cx = 2.0 * dx21; // C = 2 (P2 - P1) dx = x1; // D = P1 dax = 0.0; dbx = 2.0 * bx; final double dy21 = (y2 - y1); ay = 0.0; by = (y3 - y2) - dy21; cy = 2.0 * dy21; dy = y1; day = 0.0; dby = 2.0 * by; } ------------- PR: https://git.openjdk.java.net/jdk/pull/6227
