From 32fe55f70ac4f7fa315b84c4932f1f8deaf01638 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 16:24:59 +0100 Subject: [PATCH 1/8] Simplify test names --- src/kernel/approximation.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/kernel/approximation.rs b/src/kernel/approximation.rs index 7cb2938b3..06a5e938b 100644 --- a/src/kernel/approximation.rs +++ b/src/kernel/approximation.rs @@ -224,7 +224,7 @@ mod tests { use super::Approximation; #[test] - fn test_for_edge() { + fn for_edge() { let tolerance = Scalar::ONE; let a = Point::from([1., 2., 3.]); @@ -264,7 +264,7 @@ mod tests { } #[test] - fn test_for_cycle() { + fn for_cycle() { let tolerance = Scalar::ONE; let a = Point::from([1., 2., 3.]); @@ -302,7 +302,7 @@ mod tests { } #[test] - fn test_for_edges() { + fn for_edges() { let tolerance = Scalar::ONE; let a = Point::from([1., 2., 3.]); @@ -351,7 +351,7 @@ mod tests { } #[test] - fn test_validate() { + fn validate() { let a = Point::from([0., 1., 2.]); let b = Point::from([1., 2., 3.]); let c = Point::from([3., 5., 8.]); From 0092a71cfa55073715d3d242eabb9eabde950363 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 16:43:06 +0100 Subject: [PATCH 2/8] Add dependency on map-macro I'm going to use it in unit tests. --- Cargo.lock | 7 +++++++ Cargo.toml | 1 + 2 files changed, 8 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 617f6f90e..8cdebd1b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -545,6 +545,7 @@ dependencies = [ "fj", "futures", "libloading", + "map-macro", "nalgebra", "notify", "num-traits", @@ -991,6 +992,12 @@ dependencies = [ "libc", ] +[[package]] +name = "map-macro" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b3dec3b229449b1a54bd96dc32108086263d6830624e576dc0e6c80e619a0130" + [[package]] name = "matrixmultiply" version = "0.3.2" diff --git a/Cargo.toml b/Cargo.toml index 0d65e1729..440826aba 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ bytemuck = "1.7.3" decorum = "0.3.1" futures = "0.3.21" libloading = "0.7.2" +map-macro = "0.2.0" nalgebra = "0.30.0" notify = "5.0.0-pre.13" num-traits = "0.2.14" From bbfb88e1fdb677cbc56b34c9d56c05eb3f19ffdb Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 16:47:20 +0100 Subject: [PATCH 3/8] Prevent duplicate points in `Approximation` This is simpler and less error-prone that what was there previously. --- src/kernel/approximation.rs | 83 +++++++++---------------------------- 1 file changed, 20 insertions(+), 63 deletions(-) diff --git a/src/kernel/approximation.rs b/src/kernel/approximation.rs index 06a5e938b..d4e50d364 100644 --- a/src/kernel/approximation.rs +++ b/src/kernel/approximation.rs @@ -1,4 +1,4 @@ -use std::{cmp::Ordering, collections::HashSet}; +use std::collections::HashSet; use crate::math::{Point, Scalar, Segment}; @@ -11,7 +11,7 @@ pub struct Approximation { /// /// These could be actual vertices from the model, points that approximate /// an edge, or points that approximate a face. - pub points: Vec>, + pub points: HashSet>, /// Segments that approximate edges /// @@ -65,7 +65,10 @@ impl Approximation { segments.push(Segment::from([p0, p1])); } - Self { points, segments } + Self { + points: points.into_iter().collect(), + segments, + } } /// Compute an approximation for a cycle @@ -73,7 +76,7 @@ impl Approximation { /// `tolerance` defines how far the approximation is allowed to deviate from /// the actual cycle. pub fn for_cycle(cycle: &Cycle, tolerance: Scalar) -> Self { - let mut points = Vec::new(); + let mut points = HashSet::new(); let mut segments = Vec::new(); for edge in &cycle.edges { @@ -83,32 +86,6 @@ impl Approximation { segments.extend(approx.segments); } - // As this is a cycle, neighboring edges are going to share vertices. - // Let's remove all those duplicates. - points.sort_by(|a, b| { - if a.x < b.x { - return Ordering::Less; - } - if a.x > b.x { - return Ordering::Greater; - } - if a.y < b.y { - return Ordering::Less; - } - if a.y > b.y { - return Ordering::Greater; - } - if a.z < b.z { - return Ordering::Less; - } - if a.z > b.z { - return Ordering::Greater; - } - - Ordering::Equal - }); - points.dedup(); - Self { points, segments } } @@ -117,7 +94,7 @@ impl Approximation { /// `tolerance` defines how far the approximation is allowed to deviate from /// the actual edges. pub fn for_edges(edges: &Edges, tolerance: Scalar) -> Self { - let mut points = Vec::new(); + let mut points = HashSet::new(); let mut segments = Vec::new(); for cycle in &edges.cycles { @@ -135,21 +112,10 @@ impl Approximation { /// Returns an `Err(ValidationError)`, if the validation is not valid. See /// [`ValidationError`] for the ways that the approximation can be invalid. pub fn validate(&self) -> Result<(), ValidationError> { - let mut duplicate_points = Vec::new(); let mut duplicate_segments = Vec::new(); let mut invalid_segments = Vec::new(); let mut segments_with_invalid_points = Vec::new(); - // Verify that there are no duplicate points - let mut points = HashSet::new(); - for &point in &self.points { - if points.contains(&point) { - duplicate_points.push(point); - } - - points.insert(point); - } - let mut segments = HashSet::new(); for &segment in &self.segments { let [a, b] = segment.points(); @@ -173,13 +139,11 @@ impl Approximation { } } - if !(duplicate_points.is_empty() - && duplicate_segments.is_empty() + if !(duplicate_segments.is_empty() && invalid_segments.is_empty() && segments_with_invalid_points.is_empty()) { return Err(ValidationError { - duplicate_points, duplicate_segments, invalid_segments, segments_with_invalid_points, @@ -193,9 +157,6 @@ impl Approximation { /// Error returned by [`Approximation::validate`] #[derive(Debug)] pub struct ValidationError { - /// Points that are duplicated - pub duplicate_points: Vec>, - /// Segments that are duplicated pub duplicate_segments: Vec>, @@ -210,6 +171,8 @@ pub struct ValidationError { mod tests { use std::cell::RefCell; + use map_macro::set; + use crate::{ kernel::{ geometry::Curve, @@ -244,7 +207,7 @@ mod tests { assert_eq!( Approximation::for_edge(&edge_regular, tolerance), Approximation { - points: vec![a, b, c, d], + points: set![a, b, c, d], segments: vec![ Segment::from([a, b]), Segment::from([b, c]), @@ -257,7 +220,7 @@ mod tests { assert_eq!( Approximation::for_edge(&edge_self_connected, tolerance), Approximation { - points: vec![b, c], + points: set![b, c], segments: vec![Segment::from([b, c]), Segment::from([c, b])], } ); @@ -291,7 +254,7 @@ mod tests { assert_eq!( Approximation::for_cycle(&cycle, tolerance), Approximation { - points: vec![a, b, c], + points: set![a, b, c], segments: vec![ Segment::from([a, b]), Segment::from([b, c]), @@ -339,7 +302,7 @@ mod tests { assert_eq!( Approximation::for_edges(&edges, tolerance), Approximation { - points: vec![a, b, c, d], + points: set![a, b, c, d], segments: vec![ Segment::from([a, b]), Segment::from([b, a]), @@ -357,37 +320,31 @@ mod tests { let c = Point::from([3., 5., 8.]); let valid = Approximation { - points: vec![a, b, c], + points: set![a, b, c], segments: vec![Segment::from([a, b])], }; assert!(valid.validate().is_ok()); - let duplicate_points = Approximation { - points: vec![a, b, c, b], - segments: vec![Segment::from([a, b])], - }; - assert!(duplicate_points.validate().is_err()); - let duplicate_segments = Approximation { - points: vec![a, b, c], + points: set![a, b, c], segments: vec![Segment::from([a, b]), Segment::from([a, b])], }; assert!(duplicate_segments.validate().is_err()); let duplicate_segments_inverted = Approximation { - points: vec![a, b, c], + points: set![a, b, c], segments: vec![Segment::from([a, b]), Segment::from([b, a])], }; assert!(duplicate_segments_inverted.validate().is_err()); let invalid_segment = Approximation { - points: vec![a, b, c], + points: set![a, b, c], segments: vec![Segment::from([a, a])], }; assert!(invalid_segment.validate().is_err()); let segment_with_invalid_point = Approximation { - points: vec![a, c], + points: set![a, c], segments: vec![Segment::from([a, b])], }; assert!(segment_with_invalid_point.validate().is_err()); From 8da1ad6a99a7817c6e1a5bb7026d9e42c2c0c401 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 16:57:15 +0100 Subject: [PATCH 4/8] Prevent duplicate segments in `Approximation` This is simpler and less error-prone than what was there previously. --- src/kernel/approximation.rs | 53 +++++++++---------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/src/kernel/approximation.rs b/src/kernel/approximation.rs index d4e50d364..88f0c9494 100644 --- a/src/kernel/approximation.rs +++ b/src/kernel/approximation.rs @@ -21,7 +21,7 @@ pub struct Approximation { /// All the points of these segments will also be available in the `points` /// field of this struct. This can be verified by calling /// [`Approximation::validate`]. - pub segments: Vec>, + pub segments: HashSet>, } impl Approximation { @@ -57,12 +57,12 @@ impl Approximation { } } - let mut segments = Vec::new(); + let mut segments = HashSet::new(); for segment in segment_points.windows(2) { let p0 = segment[0]; let p1 = segment[1]; - segments.push(Segment::from([p0, p1])); + segments.insert(Segment::from([p0, p1])); } Self { @@ -77,7 +77,7 @@ impl Approximation { /// the actual cycle. pub fn for_cycle(cycle: &Cycle, tolerance: Scalar) -> Self { let mut points = HashSet::new(); - let mut segments = Vec::new(); + let mut segments = HashSet::new(); for edge in &cycle.edges { let approx = Self::for_edge(edge, tolerance); @@ -95,7 +95,7 @@ impl Approximation { /// the actual edges. pub fn for_edges(edges: &Edges, tolerance: Scalar) -> Self { let mut points = HashSet::new(); - let mut segments = Vec::new(); + let mut segments = HashSet::new(); for cycle in &edges.cycles { let approx = Self::for_cycle(cycle, tolerance); @@ -112,21 +112,11 @@ impl Approximation { /// Returns an `Err(ValidationError)`, if the validation is not valid. See /// [`ValidationError`] for the ways that the approximation can be invalid. pub fn validate(&self) -> Result<(), ValidationError> { - let mut duplicate_segments = Vec::new(); let mut invalid_segments = Vec::new(); let mut segments_with_invalid_points = Vec::new(); - let mut segments = HashSet::new(); for &segment in &self.segments { let [a, b] = segment.points(); - // Verify that there are no duplicate segments - let ab = [a, b]; - let ba = [b, a]; - if segments.contains(&ab) { - duplicate_segments.push(segment); - } - segments.insert(ab); - segments.insert(ba); // Verify that segments are actually segments if a == b { @@ -139,12 +129,10 @@ impl Approximation { } } - if !(duplicate_segments.is_empty() - && invalid_segments.is_empty() + if !(invalid_segments.is_empty() && segments_with_invalid_points.is_empty()) { return Err(ValidationError { - duplicate_segments, invalid_segments, segments_with_invalid_points, }); @@ -157,9 +145,6 @@ impl Approximation { /// Error returned by [`Approximation::validate`] #[derive(Debug)] pub struct ValidationError { - /// Segments that are duplicated - pub duplicate_segments: Vec>, - /// Segments that have two equal points pub invalid_segments: Vec>, @@ -208,7 +193,7 @@ mod tests { Approximation::for_edge(&edge_regular, tolerance), Approximation { points: set![a, b, c, d], - segments: vec![ + segments: set![ Segment::from([a, b]), Segment::from([b, c]), Segment::from([c, d]), @@ -221,7 +206,7 @@ mod tests { Approximation::for_edge(&edge_self_connected, tolerance), Approximation { points: set![b, c], - segments: vec![Segment::from([b, c]), Segment::from([c, b])], + segments: set![Segment::from([b, c]), Segment::from([c, b])], } ); } @@ -255,7 +240,7 @@ mod tests { Approximation::for_cycle(&cycle, tolerance), Approximation { points: set![a, b, c], - segments: vec![ + segments: set![ Segment::from([a, b]), Segment::from([b, c]), Segment::from([c, a]), @@ -303,7 +288,7 @@ mod tests { Approximation::for_edges(&edges, tolerance), Approximation { points: set![a, b, c, d], - segments: vec![ + segments: set![ Segment::from([a, b]), Segment::from([b, a]), Segment::from([c, d]), @@ -321,31 +306,19 @@ mod tests { let valid = Approximation { points: set![a, b, c], - segments: vec![Segment::from([a, b])], + segments: set![Segment::from([a, b])], }; assert!(valid.validate().is_ok()); - let duplicate_segments = Approximation { - points: set![a, b, c], - segments: vec![Segment::from([a, b]), Segment::from([a, b])], - }; - assert!(duplicate_segments.validate().is_err()); - - let duplicate_segments_inverted = Approximation { - points: set![a, b, c], - segments: vec![Segment::from([a, b]), Segment::from([b, a])], - }; - assert!(duplicate_segments_inverted.validate().is_err()); - let invalid_segment = Approximation { points: set![a, b, c], - segments: vec![Segment::from([a, a])], + segments: set![Segment::from([a, a])], }; assert!(invalid_segment.validate().is_err()); let segment_with_invalid_point = Approximation { points: set![a, c], - segments: vec![Segment::from([a, b])], + segments: set![Segment::from([a, b])], }; assert!(segment_with_invalid_point.validate().is_err()); } From c8e5559479b2685d134a4a42046cdeb8c5147a06 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 17:02:10 +0100 Subject: [PATCH 5/8] Refactor --- src/math/segment.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/math/segment.rs b/src/math/segment.rs index 9ecd7735f..92576e7b8 100644 --- a/src/math/segment.rs +++ b/src/math/segment.rs @@ -32,10 +32,8 @@ impl Segment<3> { impl From<[Point; 2]> for Segment { fn from(points: [Point; 2]) -> Self { - Self { - a: points[0], - b: points[1], - } + let [a, b] = points; + Self { a, b } } } From b6cd43b216a58a92dcb91820e32263d25b773e1a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 17:20:12 +0100 Subject: [PATCH 6/8] Validate segments on constructions This addresses part of #240. This required a modification to the approximation test suite, which had some redundant checking code. I will follow this up with a cleanup in the next commit, to remove the redundancy. --- src/kernel/approximation.rs | 6 ------ src/math/segment.rs | 10 +++++++++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/kernel/approximation.rs b/src/kernel/approximation.rs index 88f0c9494..56fd99dcf 100644 --- a/src/kernel/approximation.rs +++ b/src/kernel/approximation.rs @@ -310,12 +310,6 @@ mod tests { }; assert!(valid.validate().is_ok()); - let invalid_segment = Approximation { - points: set![a, b, c], - segments: set![Segment::from([a, a])], - }; - assert!(invalid_segment.validate().is_err()); - let segment_with_invalid_point = Approximation { points: set![a, c], segments: set![Segment::from([a, b])], diff --git a/src/math/segment.rs b/src/math/segment.rs index 92576e7b8..fd89b6ab7 100644 --- a/src/math/segment.rs +++ b/src/math/segment.rs @@ -33,7 +33,15 @@ impl Segment<3> { impl From<[Point; 2]> for Segment { fn from(points: [Point; 2]) -> Self { let [a, b] = points; - Self { a, b } + + if a == b { + panic!("Invalid segment; both points are identical {a:?}"); + } + + Self { + a: points[0], + b: points[1], + } } } From b023c94cc4ea0d2d994611a60852423a9575dc19 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 17:23:44 +0100 Subject: [PATCH 7/8] Remove redundant code --- src/kernel/approximation.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/kernel/approximation.rs b/src/kernel/approximation.rs index 56fd99dcf..e13b3e748 100644 --- a/src/kernel/approximation.rs +++ b/src/kernel/approximation.rs @@ -112,28 +112,19 @@ impl Approximation { /// Returns an `Err(ValidationError)`, if the validation is not valid. See /// [`ValidationError`] for the ways that the approximation can be invalid. pub fn validate(&self) -> Result<(), ValidationError> { - let mut invalid_segments = Vec::new(); let mut segments_with_invalid_points = Vec::new(); for &segment in &self.segments { let [a, b] = segment.points(); - // Verify that segments are actually segments - if a == b { - invalid_segments.push(segment); - } - // Verify that segments refer to valid points if !(self.points.contains(&a) && self.points.contains(&b)) { segments_with_invalid_points.push(segment); } } - if !(invalid_segments.is_empty() - && segments_with_invalid_points.is_empty()) - { + if !(segments_with_invalid_points.is_empty()) { return Err(ValidationError { - invalid_segments, segments_with_invalid_points, }); } @@ -145,9 +136,6 @@ impl Approximation { /// Error returned by [`Approximation::validate`] #[derive(Debug)] pub struct ValidationError { - /// Segments that have two equal points - pub invalid_segments: Vec>, - /// Segments that do not refer to points from the approximation pub segments_with_invalid_points: Vec>, } From cd48a7ab2c77cd020e19bcd08eb0cb85721436de Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 23 Feb 2022 17:25:49 +0100 Subject: [PATCH 8/8] Remove `Approximation::validate` It doesn't check very much any more, after the latest round of cleanups. What it does check, should be well-covered by the existing unit tests, so there's not much point in keeping it around. --- src/kernel/approximation.rs | 51 ------------------------------------ src/kernel/topology/faces.rs | 1 - 2 files changed, 52 deletions(-) diff --git a/src/kernel/approximation.rs b/src/kernel/approximation.rs index e13b3e748..e65014b16 100644 --- a/src/kernel/approximation.rs +++ b/src/kernel/approximation.rs @@ -106,38 +106,6 @@ impl Approximation { Self { points, segments } } - - /// Validate the approximation - /// - /// Returns an `Err(ValidationError)`, if the validation is not valid. See - /// [`ValidationError`] for the ways that the approximation can be invalid. - pub fn validate(&self) -> Result<(), ValidationError> { - let mut segments_with_invalid_points = Vec::new(); - - for &segment in &self.segments { - let [a, b] = segment.points(); - - // Verify that segments refer to valid points - if !(self.points.contains(&a) && self.points.contains(&b)) { - segments_with_invalid_points.push(segment); - } - } - - if !(segments_with_invalid_points.is_empty()) { - return Err(ValidationError { - segments_with_invalid_points, - }); - } - - Ok(()) - } -} - -/// Error returned by [`Approximation::validate`] -#[derive(Debug)] -pub struct ValidationError { - /// Segments that do not refer to points from the approximation - pub segments_with_invalid_points: Vec>, } #[cfg(test)] @@ -285,23 +253,4 @@ mod tests { } ); } - - #[test] - fn validate() { - let a = Point::from([0., 1., 2.]); - let b = Point::from([1., 2., 3.]); - let c = Point::from([3., 5., 8.]); - - let valid = Approximation { - points: set![a, b, c], - segments: set![Segment::from([a, b])], - }; - assert!(valid.validate().is_ok()); - - let segment_with_invalid_point = Approximation { - points: set![a, c], - segments: set![Segment::from([a, b])], - }; - assert!(segment_with_invalid_point.validate().is_err()); - } } diff --git a/src/kernel/topology/faces.rs b/src/kernel/topology/faces.rs index 5e2e59ac2..c18e98b6b 100644 --- a/src/kernel/topology/faces.rs +++ b/src/kernel/topology/faces.rs @@ -104,7 +104,6 @@ impl Face { match self { Self::Face { edges, surface } => { let approx = Approximation::for_edges(edges, tolerance); - approx.validate().expect("Invalid approximation"); let points: Vec<_> = approx .points