Fix panic when fitting a sliver via fit_to_bezpath_opt - #603
Open
mlwilkerson wants to merge 4 commits into
Open
Conversation
`fit_to_cubic` short-circuits to a straight-line fit whenever the chord between the range endpoints is no longer than `accuracy`, on the assumption that a short chord implies a short, simple piece of curve. That assumption fails when the source doubles back on itself: a nearly-collinear sliver can travel several units out and back while its endpoints stay close together. The line fit then fails and the function returns `None` without ever trying the cubic solver, which is what made `fit_to_bezpath_opt` panic on such paths. Fall through to the general cubic fit when the line fit fails. Zero-length chords still bail out early, as the cubic fit is scaled by the chord. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The final pass of `fit_to_bezpath_opt_inner` unwrapped `fit_to_cubic`, on the assumption that the segment search had already proven every piece fittable. The search runs at a tightened accuracy while the final fit runs at the requested one, and the error metric is not monotonic in accuracy, so the final fit can fail even though the search succeeded. `fit_to_cubic` is documented to return `None` when it cannot meet the accuracy, so unwrapping it was a latent panic regardless of the geometry that triggers it. Report the midpoint of the failing piece upward instead, as is already done for a cusp, so the range is retried in smaller pieces. When the piece is too small to subdivide any further, fall back to a line, mirroring the escape hatch in `fit_to_bezpath_rec`; this bounds the retries and guarantees termination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #601
This was heavily assisted by Claude. Needs capable human review to confirm or correct it.
Fix panic in
fit_to_bezpath_opton near-degenerate pathsRepro
Root cause
fit_to_bezpath_optworks in two passes. The first searches for split points andthe number of segments needed, at a tightened accuracy
x. The second re-walksthe range and builds each cubic, at the requested accuracy — and ended in
fit_to_cubic(source, t0..t1, accuracy).unwrap(), on the assumption that thesearch pass had already proven each piece fittable.
That assumption does not hold, because the two passes can take different code
paths. For the repro, the first piece runs from
(1.0, 0.7)to(2.56, 1.22)—endpoints 1.64 apart, but the source doesn't run between them, it travels out
toward
(4.4, 2.1)and doubles back. A hairpin.x ≈ 1.218. Chord 1.64 > 1.218, so it ran the generalcubic solver, which found a curve matching to within 0.036 units — but with a
control arm ~17 units long. The long-control-arm penalty (which exists to avoid
bumps) pushed the score just barely past tolerance. Rejected, by a hair.
fit_to_cubictook its short-chord shortcut: "a chord this short means a tinypiece of curve, just try a line." The piece isn't tiny, the line is off by more
than 2, and the function returned
Nonewithout ever reaching the cubic solver.So loosening the accuracy made fitting harder, by flipping the code onto a branch
that only considers straight lines. The error metric is not monotonic in accuracy,
and the
.unwrap()didn't survive that.Changes
1.
fit_to_cubic: don't give up when a short chord hides a long curve.The short-chord shortcut now falls through to the general cubic fit when the line
fit fails, instead of returning
Noneimmediately. A short chord doesn't imply ashort curve when the source doubles back on itself. Zero-length chords still bail
out early, since the general fit is scaled by the chord and can't do anything with
a degenerate one. This alone fixes the repro: at accuracy 2.0 the cubic solver's
candidate scores 1.485 against a budget of 4.0 and is accepted.
2.
fit_to_bezpath_opt_inner: remove the.unwrap().fit_to_cubicis documented to returnNonewhen it can't meet the requestedaccuracy, so unwrapping it is a latent panic independent of which geometry
triggers it. On
None, the midpoint of the failing piece is now reported upwardas if it were a cusp, so the driver subdivides and retries — the same mechanism
already used for cusps. If the piece is too small to subdivide further, it falls
back to emitting a line, mirroring the existing escape hatch in
fit_to_bezpath_rec. That bounds the retries and guarantees termination.Change 1 removes the cause for this input; change 2 removes the crash class.
Risk
Change 1 touches shared code — stroke expansion and offsetting lean on
fit_to_cubicheavily. It only affects ranges that previously returnedNone(callers already had to handle that), so it can turn a former "give up and
subdivide" into a successful single-cubic fit. That's a quality improvement, but
it is an output change for such paths, and worth a careful look.
Testing
simplify::tests::simplify_sliveradded alongside the existingsimplify_lines_corner; it fails with the original panic before these changes.--all-targets, andcargo fmt --checkare cleanat each commit;
--no-default-features --features libmstill builds.final fit to return
None: no hang, and all 188 tests still pass on the linefallback path.