8176501: Method Shape.getBounds2D() incorrectly includes Bezier control points in bounding box #6227
Conversation
…ol points in bounding box 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. This also includes a new unit test (in Path2D/UnitTest.java) that fails without the changes in this commit.
|
👋 Welcome back mickleness! A progress list of the required criteria for merging this PR into |
|
@mickleness The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
I will have a look at your maths. |
Thanks. And this is my first attempt at an openjdk contribution, so please feel free to be patronizing if necessary. It's very likely I'll accidentally breach etiquette sooner or later. |
|
This issue relates to improving the bounding box of curves and general paths (Path2D). The former approach was based on the simple convex hull of curves (enclosing control points) but I agree it is sub-optimal for curves.
Your code converts any Cubic or Quad curve to a cubic equation (abcd coefficients) for both x / y equations, then you derive the dx/dy equations and finally find roots of the quadratic equations.
used in Helpers.findSubdivPoints(): I suggest your solution could be more direct and rename Curve.getPossibleExtremaInCubicEquation() to Curve.findExtrema():
Syntax / typos:
Hope it helps, |
mickleness
changed the title
…ol 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 openjdk#6227 (comment)
|
Thanks for your feedback. I just pushed a commit addressing 4 of those points (and I turned on actions). Can you elaborate on these recommendations: A. determine the derivatives da / db I'm not sure what you mean. Especially regarding the second point: if there are known problem areas I'd like to represent them with a unit test. (By which I mean: if I can understand what we're talking about I'll be sure it's covered in a unit test or write a new one as needed.) FWIW: this branch includes a new shape in the UnitTest.java class (an ellipse rotated 45 degrees) that should cover a novel degenerated cubic curve. In this case the coefficient for the t^3 term is practically zero. I can go into that more if you want, but I'm unclear if that's straying off-topic or not... |
|
First comments, I will review carefully the code changes later:
B. As you pointed out, degenerated cases are causing problems as computer precision is limited ~ 10^-15 x magnitude in double-precision ! If magnitude is 10^20, the ulp ~ 100 000 ! Solving roots and computing coeffs and points may give numerical errors ~ few ulps, so it may be necessary to refine roots... |
|
For math reference, this page explains how to use curve derivatives to find extrema: |
| returnValue = 0; | ||
| } | ||
|
|
||
| if (coefficients[3] > -.01 && coefficients[3] < .01 && coefficients[2] != 0.0) { |
There was a problem hiding this comment.
do not test coefficients[3] within 0.1 !
Always use QuadCurve2D.solveQuadratic() that handles the case coefficient(x^2) = 0.
Finally this method findExtrema() is only necessary if control points (x or y) are given to determine the dx , dy polynoms and return roots... I prefer moving this code directly in getBounds2D() to have a more efficient implementation (= 1 method with 1 loop) to avoid allocation of the array double[] eqn = new double[].
| double lastX = 0.0; | ||
| double lastY = 0.0; | ||
|
|
||
| pathIteratorLoop : while (!pi.isDone()) { |
There was a problem hiding this comment.
remove the label pathIteratorLoop (trivial)
There was a problem hiding this comment.
I prefer also for loops:
for (final PathIterator it = shape.getPathIterator(null); !it.isDone(); it.next()) {
int type = it.currentSegment(coords);
...
}
| public static Rectangle2D getBounds2D(PathIterator pi) { | ||
| // define x and y parametric coefficients where: | ||
| // x(t) = x_coeff[0] + x_coeff[1] * t + x_coeff[2] * t^2 + x_coeff[3] * t^3 | ||
| double[] x_coeff = new double[4]; |
There was a problem hiding this comment.
make arrays final to be obvious (dirty arrays)
| bottomY = (endY > bottomY) ? endY : bottomY; | ||
| } | ||
|
|
||
| // here's the slightly trickier part: examine quadratic and cubic |
There was a problem hiding this comment.
add a shortcut test for better readability:
if ((type == PathIterator.SEG_QUADTO) || (type == PathIterator.SEG_CUBICTO)) {
| } | ||
| } | ||
| } | ||
|
|
| if (type == PathIterator.SEG_QUADTO) { | ||
| definedParametricEquations = true; | ||
|
|
||
| x_coeff[3] = 0.0; |
There was a problem hiding this comment.
after computing coefficients (abcd), also compute (da db c) needed by root finding next
| // here's the slightly trickier part: examine quadratic and cubic | ||
| // segments for extrema where t is between (0, 1): | ||
|
|
||
| boolean definedParametricEquations; |
There was a problem hiding this comment.
useless with the shortcut test
| for(int i = 0; i < tExtremaCount; i++) { | ||
| double t = tExtrema[i]; | ||
| if (t > 0 && t < 1) { | ||
| double x = x_coeff[0] + t * (x_coeff[1] + t * (x_coeff[2] + t * x_coeff[3])); |
There was a problem hiding this comment.
using 3rd order polynom is only useful for cubic curves, for quads 2nd order is enough.
How to improve precision on (abcd) or (bcd) polynomial evaluation ?
There was a problem hiding this comment.
Ideally the compensated-horner scheme should be used to guarantee optimal precision (2x slower):
paper:
https://www-pequan.lip6.fr/~jmc/polycopies/Compensation-horner.pdf
See julia code:
https://discourse.julialang.org/t/more-accurate-evalpoly/45932/6
There was a problem hiding this comment.
I don't think I can implement that on my own. Even if I fully understood the julia code, I assume I'd want to use the Math.fma(..) method. That method uses BigDecimals, which I assume (?) we don't want to use in a loop like this for performance reasons.
If this level of high-precision is considered essential to resolving this bug: then personally I'm at an impasse and we should either close this PR or I'll need some external help.
Or possible compromises might include:
A. Add a method like this in Curve.java:
public double evaluateCubicPolynomial(double a, double b, double c, double d, double t) {
return d + t * (c + t * (b + t * a)));
}
And add a ticket that someone else can address separately to improve the accuracy of that method. (Also maybe other areas in the AWT codebase should use the same method? Like Order3#XforT?)
B. We can modify my new method signature from:
public static Rectangle2D getBounds2D(PathIterator pi)
to:
public static Rectangle2D getBounds2D(PathIterator pi, boolean highPrecision)
Where if highPrecision is true we use the new implementation which may suffer rounding errors and be a few ulps too small. And if it is false then we use the old implementation which suffers from the original bug. This has the benefit of definitely not breaking any existing code in existence. The downside is it puts the responsibility on developers to learn about and implement this new method. But at least it would exist.
Thoughts?
There was a problem hiding this comment.
I agree it is out of the scope of this issue to implement high-precision polynom evaluation (compensated horder scheme), that I or anybody else could implement it later.
To ensure bounds2D are enclosing the shape entirely, I propose to add a small margin (10 ulps or more) to ascertain precision is good enough.
To determine the precision level, using fuzzy testing is the good approach : generate lots ofrandom paths => check bounds2D accuracy in ulp units.
There was a problem hiding this comment.
@prrace @mrserb what do you think on this bug in terms of precision requirements?
- should bbox always include control points ?
- should ideal shape always fit in bbox ? So I propose to add a margin > numerical inaccuracies.
I can develop a numerical approach based on fuzzy testing:
- generate random curves (high magnitude changes on control points)
- use both this algorithm (double) and implement the same with BigDecimal
- estimate the extrema error = max distance(pts big decimal, pts double)
There was a problem hiding this comment.
You made an amazing job !
Your bigdecimal impl looks good.
I will play with your test experiment... asap.
I think numerical accuracies are related to the dynamic /magnitude of values: points.
The test should evaluate(max error) depending on the quick length(curve) ~ manhattan norm(curve arms), so small curves have less numerical error whereas huge curve (10^50 length) means points are wide spread and coeffs... solver...points are less accurate.
I will try making sampled control points vary in log scale: 10^-6 to 10^38 (float max) so curve length will vary in huge range and determine the histogram of max(error) / length ratio.
There was a problem hiding this comment.
see condition number = magnitude range on such plot:
https://github.com/JuliaMath/AccurateArithmetic.jl/blob/master/test/figs/sum_accuracy.svg
There was a problem hiding this comment.
I just pushed a commit ( 40bda06 ) that addresses the machine error problem to my satisfaction. It includes a unit test that failed before that commit and now passes.
At your convenience let me know your thoughts. (That commit continues to focus on very small doubles; it may (?) need help to address very large double errors.)
There was a problem hiding this comment.
That looks interesting as the new margin looks like ulp(x - bbox), however it may be not enough as the position error depends on curve polynom coefficients (magnitude).
I derived your (yesterday) test to play with the margin and try small to huge curves.
I found the condition number ~ sum (abs(coeffs)) so margin = math.ulp(cond). But sometimes the quad solver is giving t with few ulps error that cause high position shifts.
I will go on testing your last approach and share my experiments
There was a problem hiding this comment.
As this accuracy problem is a more general issue on quad / cubic curves, I created the Marlin-Math project to ascertain the math function accuracy.
See https://github.com/bourgesl/marlin-math/blob/main/src/main/java/test/FindExtremaAccuracyTest.java
I will propose a new upper margin value that quarantees the edge condition.
| int tExtremaCount = Curve.findExtrema(x_coeff, tExtrema); | ||
| for(int i = 0; i < tExtremaCount; i++) { | ||
| double t = tExtrema[i]; | ||
| if (t > 0 && t < 1) { |
There was a problem hiding this comment.
use if (t > 0.0 && t < 1.0) {
| } else if (type == PathIterator.SEG_CUBICTO) { | ||
| definedParametricEquations = true; | ||
|
|
||
| x_coeff[3] = -lastX + 3.0 * coords[0] - 3.0 * coords[2] + coords[4]; |
There was a problem hiding this comment.
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;
}
| endX = coords[4]; | ||
| endY = coords[5]; | ||
| break; | ||
| default: |
There was a problem hiding this comment.
add explicitely the SEG_CLOSE case (skip = continue) before the default case (impossible case)
|
Finally I inspected OpenJFX code and Shape has such algorithm: it looks very close to your approach, but I am not sure about precision or accuracy. |
…ol points in bounding box
Addressing more of Laurent's code review recommendations/comments:
1. solve the quadratic equation using QuadCurve2d.solveQuadratic() or like Helpers.quadraticRoots()
(I was pleasantly surprised to see QuadCurve2D.solveQuadratic(..) does well for the unit test where the t^2 coefficient approaches zero. We still get an extra root, but it's greater than 10^13, so it is ignored by our (0,1) bounds check later.)
2. determine the derivatives da / db
We now define x_deriv_coeff and y_deriv_coeff.
3. remove the label pathIteratorLoop
4. use `for (final PathIterator it = shape.getPathIterator(null); !it.isDone(); it.next()) {`
(The initial statement is empty in this case because PathIterator is an argument.)
5. make arrays final to be obvious
6. add a shortcut test for better readability / close the shortcut test
7. after computing coefficients (abcd), also compute (da db c) needed by root finding next
8. useless with the shortcut test (re "definedParametricEquations" boolean)
9. use if (t > 0.0 && t < 1.0)
(Sorry, that got lost in the prev refactor.)
10. add explicitely the SEG_CLOSE case (skip = continue) before the default case
This commit does not address comments about accuracy/precision. I'll explore those separately later.
|
Looks going in good shape ! I noticed javafx uses an optimization to only process quad / cubic curves if control points are out of the current bbox, it can reduce a lot the extra overhead to find extrema... if useless. |
…ol points in bounding box Addressing code review recommendation to copy javafx's optimization: we can avoid looking at the derivative and polynomial if the control points are inside our existing bounds. This involved refactoring the method getBounds2D(PathIterator) more generally. It should now be a little more efficient (and perhaps a little less readable). Since we don't have concrete performance or readability goals this is a subjective trade-off that's hard to assess.
…ol points in bounding box Addressing code review recommendation to calculate polynomial coefficients using differences / vector notation. openjdk#6227 (comment) 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.
|
@prrace thanks for your feedback. The first several points you raised were bugs/problems that I believe I have addressed just now. Specifically:
Regarding bigger issues, you wrote:
I would argue the crux of the Shape#getBounds2D() contract is this phrase: "the Shape lies entirely within the indicated Rectangle2D". And by "Shape" the original authors probably mean "the defining outline" (a phrase referenced in the second paragraph of the javadoc). If instead the word "Shape" is interpreted as "all the points in the path, including control points" then (I think?) that means the Area class suddenly violates the contract. (Because it has always returned a very tight bounding box that (I think?) disregards control points.) And if we "fix" the Area class so its getBounds2D() method can intentionally return more empty space: that seems antithetical to the intent of the original ticket under consideration. So as of this writing: What do you think? I'm happy to proceed with a CSR if you think this requires further consideration, I just wanted to double-check. |
|
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.) |
|
@prrace could you say on CSR or not ? |
|
I still recommend a CSR since
Actually regarding (1) there's also a real change there too since now The Float subclass will return a Rectangle2D.Double .. I am not sure that we should actually do that. |
|
@prrace how to proceed ? Could you manage the CSR in JBS ? I never had to do it. |
|
@mickleness This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
I don't suppose any new client-lib members would be willing to set up a CSR for this PR? No worries if not. I just wanted to double-check before this PR gets auto-closed. (I'm under the impression I can't initiate a CSR and there's nothing else I can do here; if I'm wrong please let me know.) |
|
This work is great for jdk19, 25 years after java 1.2 providing the java2d API, to get more precise curve bounding boxes (closer to extrema), let's write the CSR altogether, @prrace |
|
So .. @mickleness please email me directly about any help needed with the CSR. Give me the text you propose |
|
|
@mickleness This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2101 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
I am not an openjdk reviewer but committer, especially in java2d & graphics as the marlin renderer author. |
|
I can sponsor the PR but not pprove it formally. |
|
This is waiting for @mickleness to type "/integrate", so I can follow it up with "/sponsor" |
|
/integrate |
|
Sorry, it's been a hectic month over here. |
|
@mickleness |
|
/sponsor |
|
Going to push as commit 8a16842.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@prrace @mickleness Pushed as commit 8a16842. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Congratulations! |
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.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6227/head:pull/6227$ git checkout pull/6227Update a local copy of the PR:
$ git checkout pull/6227$ git pull https://git.openjdk.java.net/jdk pull/6227/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6227View PR using the GUI difftool:
$ git pr show -t 6227Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6227.diff