Skip to content

Commit

Permalink
Fixes #113
Browse files Browse the repository at this point in the history 
`ConflictRegionEnd::NewVertex` was renamed to `ConflictRegionEnd::ConstraintEdgeSplit`
  • Loading branch information
@Stoeoef
Stoeoef committed Sep 9, 2024
1 parent bddafbb commit 87700f1
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 35 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [2.12.1] - 2024-09-09

### Fix

- Fixes #113. This could lead to rare crashes when calling `ConstrainedDelaunayTriangulation::add_constraint_and_split`

## [2.12.0] - 2024-08-13

### Fix
Expand Down Expand Up @@ -469,6 +475,8 @@ A lot has changed for the 1.0. release, only larger changes are shown.

Initial commit

[2.12.1]: https://github.com/Stoeoef/spade/compare/v2.12.0...v2.12.1

[2.12.0]: https://github.com/Stoeoef/spade/compare/v2.11.0...v2.12.0

[2.11.0]: https://github.com/Stoeoef/spade/compare/v2.10.0...v2.11.0
Expand Down
140 changes: 105 additions & 35 deletions src/cdt.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,10 +842,13 @@ where
for region in initial_conflict_regions {
let group_end_vertex = match region.group_end {
Existing(v) => v,
ConflictRegionEnd::NewVertex(new_vertex, edge) => {
let new_handle = self
.insert(new_vertex)
.expect("Failed to insert vertex as expected. This is a bug in spade.");
ConflictRegionEnd::ConstraintEdgeSplit(new_vertex, edge) => {
let new_handle = match new_vertex {
Ok(new_vertex) => self
.insert(new_vertex)
.expect("Failed to insert vertex as expected. This is a bug in spade."),
Err(handle) => handle,
};

let [old_from, old_to] = self.directed_edge(edge).vertices().map(|v| v.fix());
// The conflict edge can prevent the forced insertion to the split vertex.
Expand Down Expand Up @@ -905,32 +908,43 @@ where
Intersection::EdgeIntersection(edge) => {
if !edge.is_constraint_edge() {
current_group.push(edge.fix());
} else {
match conflict_resolver(edge) {
ConflictResolution::Cancel => {
return (Vec::new(), true);
}
ConflictResolution::Split(new_vertex) => {
let position = new_vertex.position();
let (group_end, is_valid) =
self.verify_split_position(edge, position);

// A region is considered to be intact if the split vertex lies
// within the region and not outside or on its border.
all_regions_intact &= is_valid;

let conflict_edges = core::mem::take(&mut current_group);

let group_end = group_end.unwrap_or(ConflictRegionEnd::NewVertex(
new_vertex,
edge.fix(),
));

conflict_groups.push(InitialConflictRegion {
conflict_edges,
group_end,
});
}
continue;
}

// The new constraint intersects an existing constraint edge. Start conflict
// resolution.
match conflict_resolver(edge) {
ConflictResolution::Cancel => {
return (Vec::new(), true);
}
ConflictResolution::Split(new_vertex) => {
let position = new_vertex.position();
let (overlap_vertex, is_valid) =
self.verify_split_position(edge, position);

// A region is considered to be intact if the split vertex lies
// within the region and not outside or on its border.
all_regions_intact &= is_valid;

let conflict_edges = core::mem::take(&mut current_group);

// overlap_vertex.is_some() indicates that the split position
// overlaps an existing vertex. This can happen due to rounding
// errors and needs some special handling
ignore_next_vertex = overlap_vertex.is_some();

let group_end_vertex =
overlap_vertex.map(|h| Err(h)).unwrap_or(Ok(new_vertex));

let group_end = ConflictRegionEnd::ConstraintEdgeSplit(
group_end_vertex,
edge.fix(),
);

conflict_groups.push(InitialConflictRegion {
conflict_edges,
group_end,
});
}
}
}
Expand Down Expand Up @@ -966,7 +980,7 @@ where
&self,
conflict_edge: DirectedEdgeHandle<V, DE, CdtEdge<UE>, F>,
split_position: Point2<<V as HasPosition>::Scalar>,
) -> (Option<ConflictRegionEnd<V>>, bool) {
) -> (Option<FixedVertexHandle>, bool) {
// Not every split vertex may lead to a conflict region that will properly contain the
// split vertex. This can happen as not all split positions can be represented precisely.
//
Expand All @@ -989,7 +1003,7 @@ where
let is_valid = conflict_edge.is_part_of_convex_hull();
(None, is_valid)
}
PositionInTriangulation::OnVertex(v) => (Some(Existing(v)), false),
PositionInTriangulation::OnVertex(v) => (Some(v), false),
PositionInTriangulation::NoTriangulation => unreachable!(),
}
}
Expand All @@ -1010,7 +1024,12 @@ where
let mut last_edge = None;
let target_vertex = match group_end {
Existing(v) => v,
ConflictRegionEnd::NewVertex(v, conflict_edge) => {
ConflictRegionEnd::ConstraintEdgeSplit(v, conflict_edge) => {
let v = v.expect(
"Expected a new vertex for insertion. \
An existing vertex should be handled by the fallback routine. \
This is a bug in spade.",
);
let (new_vertex, [e0, e1]) = self.insert_on_edge(conflict_edge, v);
let e1_handle = self.directed_edge(e1);
// edge_in / edge_out refer to the edge going into / out of the newly split off
Expand Down Expand Up @@ -1169,8 +1188,12 @@ enum ConflictRegionEnd<V> {
/// However, it makes sense to handle this specially to prevent having to look up the overlapped
/// edge later.
EdgeOverlap(FixedDirectedEdgeHandle),
/// The conflict region ends in a vertex that does not exist yet.
NewVertex(V, FixedDirectedEdgeHandle),
/// The conflict region ends in a vertex that splits an existing constraint edge. Usually, this
/// vertex is constructed anew and given by the `Ok` case.
/// In rare cases, the split vertex may be an existing vertex that does not lie exactly on the
/// line due to rounding issues. This is indicated by the `Err` case. The constraint edge that
/// should be split is the second field.
ConstraintEdgeSplit(Result<V, FixedVertexHandle>, FixedDirectedEdgeHandle),
}

/// Represents a conflict region that does not yet fully exist as a vertex may be missing. This can
Expand Down Expand Up @@ -2109,6 +2132,53 @@ mod test {
Ok(())
}

#[test]
fn edge_intersection_precision_test_3() -> Result<(), InsertionError> {
let edges = [
[
Point2 {
x: -11.673287,
y: -28.37192,
},
Point2 {
x: -16.214716,
y: -43.81278,
},
],
[
Point2 {
x: 7.4022045,
y: -51.355137,
},
Point2 {
x: -13.92232,
y: -36.01863,
},
],
];

// `f32` is important. This makes the intersection of the two edges coincide with an
// existing vertex, triggering an edge case.
let mut cdt: ConstrainedDelaunayTriangulation<Point2<f32>> =
ConstrainedDelaunayTriangulation::new();
let mut returned_constraint_edge_counts = Vec::new();
for edge in edges {
let point_a = cdt.insert(edge[0])?;
let point_b = cdt.insert(edge[1])?;
returned_constraint_edge_counts
.push(cdt.add_constraint_and_split(point_a, point_b, |v| v).len());
cdt.cdt_sanity_check();
}

// Usually, 4 constraints should be present. However, due to the overlap of the intersection
// point, the second call to `add_constraint_and_split` does not add 2 constraint edges.
// See issue #113 for more information
assert_eq!(cdt.num_constraints, 3);
assert_eq!(returned_constraint_edge_counts, vec![1, 1]);

Ok(())
}

fn check_returned_edges(
cdt: &mut ConstrainedDelaunayTriangulation<Point2<f64>>,
edges: &[FixedDirectedEdgeHandle],
Expand Down

0 comments on commit 87700f1

Please sign in to comment.