-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve robustness of curve fitting #269
Conversation
Make quartic solver report failure when there are no real solutions, rather than panicking. Make the ITP solver report its bracket to handle cases where there are discontinuities better.
This commit moves the distance estimation into its own struct, but doesn't change the behavior.
Measure whether curve is "spicy," and use arc length measure if so.
Give up more quickly when accuracy is exceeded, which improves performance.
Doing so avoids bumps but still allows us to exactly fit cubic. Also fixes potential angle wraparound issues.
I have a test case. The code below can be copy-pasted into src/offset.rs. I tested on HEAD of robust_fit and got a hang, I think during the cubic solver. #[cfg(test)]
mod tests {
use super::CubicOffset;
use crate::{fit_to_bezpath_opt, CubicBez, PathEl};
// This test tries combinations of parameters that have caused problems in the past.
#[test]
fn pathological_curves() {
let curve = CubicBez {
p0: (-1236.3746269978635, 152.17981429574826).into(),
p1: (-1175.18662093517, 108.04721798590596).into(),
p2: (-1152.142883879584, 105.76260301083356).into(),
p3: (-1151.842639804639, 105.73040758939104).into(),
};
let offset = 3603.7267536453924;
let accuracy = 0.1;
let offset_path = CubicOffset::new(curve, offset);
let path = fit_to_bezpath_opt(&offset_path, accuracy);
assert!(matches!(path.iter().next(), Some(PathEl::MoveTo(_))));
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome, I didn't realise that you also implemented a simplifier over BezPaths as well as improving the robustness of the original algorithm.
I didn't understand everything in the method, so I'm taking an 'empirical' approach to review - I've used the code in this branch and thrown some wierd curves at it, and it performs better than what's in master
in all cases.
The main improvement is to try to fit a line when the chord is very short. Very short segments can occur when the cusp finding reports imprecise results, and also the main cubic fit logic is not robust when the chord is short. I believe the simple subdivision case is now fairly robust in that it won't keep recursing. This is in spite of not having an explicit bound on recursion depth; the theory is that each subdivision reduces the parameter space in half, and at some point you'll reach the point where the chord is shorter than the given tolerance. This is true even for an adversarial break_cusp, as it's bypassed in the short chord case. The logic for the optimized case is not as careful, in particular break_cusp is not bypassed. I'm curious whether there are still failure cases. This is likely not the end of the numerical robustness work. In particular, break_cusp can be better tuned to not over-report cusps, and the tangent detection can be improved for cusp and near-cusp source curves. But hopefully this commit gets us to where we at least return a valid fit. Also adds a test lightly adapted from #269. Progress toward #269 and #279
This PR contains a number of improvements to the robustness and accuracy of curve fitting. A milestone is that seems to handle the test case of #268 fairly well. It also implements one aspect of #267 (handling the case where the quartic has no real roots), but does not implement the higher level driver proposed in that issue. Thus, it does its best to fit corners with smooth curves.
A list of the changes in somewhat more detail:
fit_to_bezpath_opt
logic has been changed to make accuracy bounds tighter for the curve fitting steps; those are allowed to fail, and logic can deal with that. Previously it set a HUGE accuracy bound with the expectation that some solution would always be found. This should improve both robustness and performance (relativeThe new method is based on arc length parametrized sampling of the approximation curve (the sample points are sampled evenly in parameter space in the source curve, which might possibly be improved, but otherwise would require solving inverse arc length problems there). While this error metric is more robust, avoiding the loops, it is also dramatically slower. Thus, this PR applies a hybrid approach, first estimating whether the source curve is "spicy" by determining whether the tangent of the delta between tangent vectors between samples exceeds 0.1 (the
SPICY_THRESH
constant in the code). If so, the arc length method is used (after first running the ray cast method).Experimentation is encouraged, as the goal is truly robust curve fitting. The techniques deserve a proper writeup. One thing of note is that arc length is computed (as an integral) as another application of the
ParamCurveFit
trait; no changes were needed to the trait itself. It's also worth noting that errors in computing arc length make the error metric more conservative, so extreme care is not needed there.