Improve solve_cubic accuracy when the cubic coefficient is negligible - #593
Open
signalwerk wants to merge 1 commit into
Open
Improve solve_cubic accuracy when the cubic coefficient is negligible#593signalwerk wants to merge 1 commit into
signalwerk wants to merge 1 commit into
Conversation
solve_cubic only degenerated to solve_quadratic when 1/c3 overflowed, i.e. when c3 was exactly zero or subnormal. When c3 is instead merely tiny relative to the other coefficients — e.g. cancellation noise of ~1e-14 against coefficients of order 1e2, as produced by an ordinary circle built from kappa cubics rotated 45° — the general branch scaled the other coefficients by 1/c3, amplifying their floating-point error enough that the returned roots could be far from solving the equation (a residual of 2.37 where ~0 is expected). BezPath::winding and Shape::contains could then misclassify points near such curves. Add a relative-magnitude degeneracy check before the scaling, mirroring the treatment solve_quadratic already documents for its nearly-linear case: if |c3| <= 1e-12 * max(|c0|, |c1|, |c2|), solve the quadratic and ignore the third root (of magnitude ~|c2/c3|). Assisted-by: Claude:claude-fable-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Shape::winding/containsmisclassify points when a cubic's leading coefficient is floating-point noiseFirst, a disclosure: I'm a consumer of kurbo (building boolean path operations on top of it) and don't claim to understand its internals. I noticed wrong classifications while testing boolean operations; this report, the analysis, and the suggested fix were crafted with AI assistance (Claude Fable 5). Please treat the fix and the accompanying PR as a suggestion only – feel free to disregard them and solve this however you see fit.
Reproduction
Fresh
cargo newwithkurbo = "0.13":Confirmed in 0.12.0, 0.13.1 and current
main; silently wrong (no panic). It is not one point: the whole bandx ∈ [329, 334]aty = 445.26is misclassified – the true boundary at that height is atx = 334.394(by bisection), 6 units away from where kurbo places it. The shape is borrowed from paper.js's boolean-operations test suite1 (full credit to the paper.js project!).Cause (as far as we understand it)
The rotation makes each segment's
y(t)nearly antisymmetric aboutt = 0.5, so its cubic coefficient mathematically cancels – but rounding leaves noise. For the deciding y-monotone piece,windingcallssolve_cubiconly falls back tosolve_quadraticwhen1/c3overflows; here1/c3 ≈ -1.8e13is finite, so the general branch scales the other coefficients by it and amplifies their rounding error. The returnedt = 0.5313…(the root in[0, 1], which winding trusts) leaves a polynomial residual of 2.37 – it is not a root. Treating the equation as the quadratic it effectively is givest = 0.4910141…, matching bisection.Suggested fix
Check
c3relative to the other coefficients before scaling – the counterpart of whatsolve_quadraticalready documents for its nearly-linear case:A PR with this change and a regression test is ready. Attached to this issue is
solve-cubic-report.html, a self-contained HTML report that shows the problem visually (before/after root tables, winding scans, SVG close-ups) and embeds the program that generates it, together with the steps to reproduce it.Possibly related: #411 (
intersect_linealso goes throughsolve_cubic), #531, #277.Full report in HTML
solve-cubic-report.html
Footnotes
paper.js test and the issue in paper.js ↩