forked from 1b-agent/opencadstudio-upstream
Guard constraint solving and DWG export against degenerate geometry
Retaining a line's length added a P2PDistance constraint, whose gradient divides by the current distance. For a zero-length line the solver got a NaN Jacobian and its SVD fallback never returned, freezing the editor (e.g. ECONSTRAINT on a line drawn by clicking one point twice). Hold coincident endpoints together with per-axis differences instead. Control-point splines read without knots are given a degree their control points can carry (at most n - 1) and a clamped knot vector when a drawing loads. Generating knots for fewer points than the degree underflowed during DWG write: a panic in debug builds, an unbounded allocation in release, and reachable from autosave. A lone control point with no knots or fit points is purged as corrupt. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
1 parent
a3d65e2fc8
commit
1ac9309f85
3 files changed
+110
-6
No files matched your search
@@ -2346,3 +2346,22 @@ mod region_tests {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod degenerate_constraint_tests {
|
||||
use crate::app::OpenCADStudio;
|
||||
|
||||
/// Retaining a zero-length line's size used to hand the solver a NaN
|
||||
/// Jacobian, and its SVD fallback never returned.
|
||||
#[test]
|
||||
fn equal_constraint_with_a_zero_length_line_returns() {
|
||||
let mut app = OpenCADStudio::new_for_test();
|
||||
app.automation_op(r#"{"op":"new"}"#);
|
||||
app.automation_op(r#"{"op":"run","cmd":"LINE 0,0 0,0 "}"#);
|
||||
app.automation_op(r#"{"op":"run","cmd":"LINE 5,5 20,7 "}"#);
|
||||
app.automation_op(r#"{"op":"select","type":"Line"}"#);
|
||||
app.automation_op(r#"{"op":"run","cmd":"ECONSTRAINT"}"#);
|
||||
let lines = app.automation_op(r#"{"op":"query","type":"Line","detail":"geometry"}"#);
|
||||
assert_eq!(lines["ok"], true, "{lines}");
|
||||
}
|
||||
}
|
||||
@@ -2146,8 +2146,12 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool {
|
||||
&& (s.knots.iter().any(|k| !k.is_finite())
|
||||
|| s.knots.windows(2).any(|w| w[1] < w[0])
|
||||
|| (!compact_periodic_knots && s.knots.len() != n + deg + 1));
|
||||
// A single control point with no knots and no fit points cannot
|
||||
// form a curve, and no knot vector exists for it.
|
||||
let lone_point = n == 1 && s.knots.is_empty() && s.fit_points.is_empty();
|
||||
n >= MAX_VERTS
|
||||
|| degree_bad
|
||||
|| lone_point
|
||||
|| s.control_points.iter().any(|p| !finite_vec3(p))
|
||||
|| knots_bad
|
||||
}
|
||||
@@ -2155,8 +2159,32 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
/// Give control-point splines stored without knots a degree their control
|
||||
/// points can carry, and the matching clamped knot vector.
|
||||
///
|
||||
/// A degree-p curve needs at least p + 1 control points. Knot generation
|
||||
/// (here, and in the DWG writer for knot-less splines) subtracts in `usize`
|
||||
/// and underflows when there are fewer; in release builds the wrapped count
|
||||
/// drives an unbounded allocation. Lowering the degree keeps the geometry the
|
||||
/// points describe: two points are a straight segment at any degree.
|
||||
fn normalize_knotless_splines(doc: &mut CadDocument) {
|
||||
for entity in doc.entities_mut() {
|
||||
let EntityType::Spline(spline) = entity else {
|
||||
continue;
|
||||
};
|
||||
let n = spline.control_points.len();
|
||||
if !spline.knots.is_empty() || n < 2 || spline.degree < 1 {
|
||||
continue;
|
||||
}
|
||||
let degree = (spline.degree as usize).min(n - 1);
|
||||
spline.degree = degree as i32;
|
||||
spline.knots = acadrust::entities::Spline::generate_clamped_knots(degree, n);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn purge_corrupt_entities(doc: &mut CadDocument) -> usize {
|
||||
use crate::par::prelude::*;
|
||||
normalize_knotless_splines(doc);
|
||||
// Detection is pure and read-only; the per-vertex finite/extent checks on
|
||||
// large polylines dominate, so fan the scan out across cores. Gather
|
||||
// entity references in one pass, test in parallel, then remove serially
|
||||
@@ -2363,6 +2391,42 @@ mod corrupt_guard_tests {
|
||||
use acadrust::entities::{Arc, Circle, EntityType, Spline};
|
||||
use acadrust::types::Vector3;
|
||||
|
||||
fn knotless_spline(degree: i32, points: usize) -> Spline {
|
||||
let mut spline = Spline::new();
|
||||
spline.degree = degree;
|
||||
spline.control_points = (0..points)
|
||||
.map(|index| Vector3::new(index as f64, index as f64, 0.0))
|
||||
.collect();
|
||||
spline
|
||||
}
|
||||
|
||||
// Degree 3 over two control points with no knot vector: generating knots
|
||||
// for it underflows, and writing it to DWG used to panic (debug) or
|
||||
// allocate without bound (release).
|
||||
#[test]
|
||||
fn knotless_spline_with_too_few_points_is_lowered_and_saves_as_dwg() {
|
||||
let mut doc = CadDocument::new();
|
||||
doc.add_entity(EntityType::Spline(knotless_spline(3, 2)));
|
||||
doc.add_entity(EntityType::Spline(knotless_spline(3, 1)));
|
||||
assert_eq!(purge_corrupt_entities(&mut doc), 1, "the lone point is dropped");
|
||||
let spline = doc
|
||||
.entities()
|
||||
.find_map(|entity| match entity {
|
||||
EntityType::Spline(spline) => Some(spline.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.expect("the two-point spline is kept");
|
||||
assert_eq!(spline.degree, 1);
|
||||
assert_eq!(spline.knots, vec![0.0, 0.0, 1.0, 1.0]);
|
||||
|
||||
let path = std::env::temp_dir().join(format!(
|
||||
"ocs_knotless_spline_{}.dwg",
|
||||
std::process::id()
|
||||
));
|
||||
save_as_version(&doc, &path, acadrust::DxfVersion::AC1032).expect("save");
|
||||
let _ = std::fs::remove_file(&path);
|
||||
}
|
||||
|
||||
// Small but finite arcs are valid records. Kernel tessellation is bounded,
|
||||
// so opening must retain them instead of treating their size as corruption.
|
||||
#[test]
|
||||
|
||||
@@ -1830,6 +1830,29 @@ fn retained_radius(entity: &EntityType) -> Option<f64> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Hold the distance between two points at `length`.
|
||||
///
|
||||
/// `P2PDistance` divides by the current distance, so a zero-length segment
|
||||
/// gives the solver a NaN Jacobian. Keep coincident endpoints together with
|
||||
/// per-axis differences instead.
|
||||
fn retain_segment_length(
|
||||
sys: &mut System,
|
||||
a: GPoint,
|
||||
b: GPoint,
|
||||
current_length: f64,
|
||||
length: f64,
|
||||
) {
|
||||
if current_length > f64::EPSILON && length > f64::EPSILON {
|
||||
let target = sys.add_param(length, true);
|
||||
sys.add_constraint(Rc::new(P2PDistance::new(a, b, target)));
|
||||
return;
|
||||
}
|
||||
let dx = sys.add_param(0.0, true);
|
||||
let dy = sys.add_param(0.0, true);
|
||||
sys.add_constraint(Rc::new(Difference::new(a.x, b.x, dx)));
|
||||
sys.add_constraint(Rc::new(Difference::new(a.y, b.y, dy)));
|
||||
}
|
||||
|
||||
/// Visit vertices and segments in stored drawing order, including the closing edge last.
|
||||
fn polyline_ref_order(
|
||||
document: &acadrust::CadDocument,
|
||||
@@ -2139,8 +2162,7 @@ fn solve_scope(
|
||||
.get(handle)
|
||||
.and_then(|entity| retained_line_length(entity, None))
|
||||
.unwrap_or(current_length);
|
||||
let target = sys.add_param(length, true);
|
||||
sys.add_constraint(Rc::new(P2PDistance::new(line.p1, line.p2, target)));
|
||||
retain_segment_length(&mut sys, line.p1, line.p2, current_length, length);
|
||||
}
|
||||
EntityGeom::Polyline {
|
||||
points,
|
||||
@@ -2169,8 +2191,7 @@ fn solve_scope(
|
||||
.get(handle)
|
||||
.and_then(|entity| retained_line_length(entity, Some(index)))
|
||||
.unwrap_or(current_length);
|
||||
let target = sys.add_param(length, true);
|
||||
sys.add_constraint(Rc::new(P2PDistance::new(a, b, target)));
|
||||
retain_segment_length(&mut sys, a, b, current_length, length);
|
||||
}
|
||||
}
|
||||
EntityGeom::Circle(circle) => {
|
||||
@@ -2346,8 +2367,8 @@ fn solve_scope(
|
||||
})
|
||||
});
|
||||
if directional && !retain_size {
|
||||
let length = sys.add_param((x2 - x1).hypot(y2 - y1), true);
|
||||
sys.add_constraint(Rc::new(P2PDistance::new(line.p1, line.p2, length)));
|
||||
let length = (x2 - x1).hypot(y2 - y1);
|
||||
retain_segment_length(&mut sys, line.p1, line.p2, length, length);
|
||||
} else if !directional {
|
||||
let dx = sys.add_param(x2 - x1, true);
|
||||
let dy = sys.add_param(y2 - y1, true);
|
||||
|
||||
Reference in new issue
Block a user