On Tue, 9 Nov 2021 10:03:14 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 code review recommendation to calculate polynomial coefficients > using differences / vector notation. > > https://github.com/openjdk/jdk/pull/6227#discussion_r743534560 > > I generally understand the intention of this change, but I don't know > exactly how to test/evaluate it. > > The unit tests still pass. I ran some sample calculations involving a > 100x100 Ellipse2D as it was rotated, and the two getBounds2D(..) > implementations (before and after this commit) only differed by a few ulps > (usually around 10^-15), as expected. Great ! I see many duplicated lines, probably it is time to add 2 small methods to processCubic() and processQuad() that can be called on x and y equations => only 2 arrays needed for coeff and deriv_coeff. Maybe I prefer the previous unified solution relying on QuadCurve2D.solveQuadratic() that handles both case. ------------- PR: https://git.openjdk.java.net/jdk/pull/6227
