From b74bcbb8ce7f9c913cda316a7449b08560048943 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Fri, 24 Feb 2023 21:19:56 +0000 Subject: [PATCH 01/18] Add a test that checks for an issue when there's a hole where all the edges are external and surrounded by other paths --- tests/bezier/path/arithmetic_sub.rs | 34 ++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/bezier/path/arithmetic_sub.rs b/tests/bezier/path/arithmetic_sub.rs index 3372c53a..cc7265ca 100644 --- a/tests/bezier/path/arithmetic_sub.rs +++ b/tests/bezier/path/arithmetic_sub.rs @@ -494,4 +494,36 @@ fn subtract_permutations_2() { } } } -} \ No newline at end of file +} + +#[test] +fn subtract_center_overlapping() { + // Plus sign + let plus = vec![Coord2(0.0, 10.0), Coord2(0.0, 20.0), Coord2(10.0, 20.0), Coord2(10.0, 30.0), Coord2(20.0, 30.0), Coord2(20.0, 20.0), + Coord2(30.0, 20.0), Coord2(30.0, 10.0), Coord2(20.0, 10.0), Coord2(20.0, 0.0), Coord2(10.0, 0.0), Coord2(10.0, 10.0)]; + + // Remove the exact center using subtract + let center = vec![Coord2(10.0, 10.0), Coord2(10.0, 20.0), Coord2(20.0, 20.0), Coord2(20.0, 10.0)]; + + for forward_1 in [true, false] { + for forward_2 in [true, false] { + for pos1 in 0..plus.len() { + let plus = path_permutation(plus.clone(), pos1, forward_1); + + for pos2 in 0..center.len() { + let center = path_permutation(center.clone(), pos2, forward_2); + + println!(); + println!("=== {} {} {} {}", pos1, pos2, forward_1, forward_2); + let sub_path = path_sub::(&vec![plus.clone()], &vec![center.clone()], 0.1); + println!(" Num paths in result: {}", sub_path.len()); + + // Result should be either 2 paths (ie, the plus with the center removed) or 4 paths (four squares around the center) + // 5 is the wrong answer (all 5 squares make a valid loop but we should never detect the center section as a separate path along with the + // 4 other sections) + assert!(sub_path.len() == 2 || sub_path.len() == 4); + } + } + } + } +} From 27a86eb9492969c4563dcf20291b2f716cb75efe Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 11:05:43 +0000 Subject: [PATCH 02/18] Add a test for the even more complicated case of subtracting squares to make a chequerboard --- tests/bezier/path/arithmetic_sub.rs | 47 +++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/bezier/path/arithmetic_sub.rs b/tests/bezier/path/arithmetic_sub.rs index cc7265ca..bde3398d 100644 --- a/tests/bezier/path/arithmetic_sub.rs +++ b/tests/bezier/path/arithmetic_sub.rs @@ -521,9 +521,56 @@ fn subtract_center_overlapping() { // Result should be either 2 paths (ie, the plus with the center removed) or 4 paths (four squares around the center) // 5 is the wrong answer (all 5 squares make a valid loop but we should never detect the center section as a separate path along with the // 4 other sections) + assert!(sub_path.len() != 5, "Should not generate center as a separate path"); assert!(sub_path.len() == 2 || sub_path.len() == 4); } } } } } + +#[test] +fn subtract_chequerboard() { + // This subtracts alternating squares from a 'main' square to make a chequerboard + // It's a more involved version of the '+' test above as each square after the first row will + // share edges with other squares so it's easy for the path finding algorithm to get confused and + // turn a 'hole' into a shape. + // + // The condition here isn't perfect, it's possible for the result to be 'bad' but the overall number + // of shapes in the result to be correct + + // Outer square + let square = vec![Coord2(0.0, 0.0), Coord2(10.0, 0.0), Coord2(10.0, 10.0), Coord2(0.0, 10.0)]; + + for forward in [true, false] { + for pos in 0..square.len() { + println!("{:?} {:?}", forward, pos); + + let mut chequerboard = vec![path_permutation(square.clone(), pos, forward)]; + + // Subtract every other square + for y in 0..10 { + for x in 0..5 { + let x = if y%2 == 0 { + (x as f64)*2.0 + 1.0 + } else { + (x as f64)*2.0 + }; + let y = y as f64; + + let inner_square = BezierPathBuilder::::start(Coord2(x, y)) + .line_to(Coord2(x+1.0, y)) + .line_to(Coord2(x+1.0, y+1.0)) + .line_to(Coord2(x, y+1.0)) + .line_to(Coord2(x, y)) + .build(); + + chequerboard = path_sub(&chequerboard, &vec![inner_square], 0.01); + } + } + + println!("{:?}", chequerboard.len()); + assert!(chequerboard.len() == 50); + } + } +} From 739721569bdd8843e5da287ff9ad8f8b5f6af40b Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 13:25:21 +0000 Subject: [PATCH 03/18] One approach to fixing this issue is not to follow edges that are already taken by another shape unless there's no alternative (For our plus shape, this means we sometimes generate the 4-path version or the 2-path version, and it's not clear if the harder chequerboard issue is fixed) --- src/bezier/path/graph_path/mod.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/bezier/path/graph_path/mod.rs b/src/bezier/path/graph_path/mod.rs index 25a34b75..c5a98845 100644 --- a/src/bezier/path/graph_path/mod.rs +++ b/src/bezier/path/graph_path/mod.rs @@ -891,8 +891,10 @@ impl GraphPath { /// Given the index of a starting edge in a connections list, attempts to find the shortest loop of edges that returns /// back to the edge's start point /// + /// If there's a choice, this will not follow any previously used edge (but will follow them if that's the way to make progress with a loop of edges) + /// #[inline] - fn find_loop(&self, connections: &Vec>, start_point_idx: usize, edge: usize) -> Option> { + fn find_loop(&self, connections: &Vec>, start_point_idx: usize, edge: usize, used_edges: &Vec) -> Option> { // The algorithm here is a slight modification of Dijkstra's algorithm, we start knowing the path has to contain a particular edge let mut previous_point = vec![None; connections.len()]; @@ -930,12 +932,31 @@ impl GraphPath { visited_edges[edge_ref.start_idx] |= 1<<(edge_ref.edge_idx); // Visit all the points reachable from this edge, ignoring any edges that we've already visited - for (following_point_idx, following_edge) in connections[next_point_idx].iter() { + let following_connections = &connections[next_point_idx]; + + // Check the 'already used' list only if there are no alternative edges from this point + let avoid_already_used = if following_connections.len() > 1 { + // Use the 'used' array to exclude edges unless it would exclude all edges + following_connections.iter() + .any(|(_following_point_idx, following_edge)| { + visited_edges[following_edge.start_idx]&(1< GraphPath { included_edges[edge_ref.start_idx] |= 1< Date: Sat, 25 Feb 2023 13:38:28 +0000 Subject: [PATCH 04/18] Test the chequerboard by ray-casting --- tests/bezier/path/arithmetic_sub.rs | 39 +++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/tests/bezier/path/arithmetic_sub.rs b/tests/bezier/path/arithmetic_sub.rs index bde3398d..7306b539 100644 --- a/tests/bezier/path/arithmetic_sub.rs +++ b/tests/bezier/path/arithmetic_sub.rs @@ -567,10 +567,45 @@ fn subtract_chequerboard() { chequerboard = path_sub(&chequerboard, &vec![inner_square], 0.01); } + + // Always should be 10 collisions horizontally, and 2 on the following line. Vertical collisions should go up as we add new lines + let y = y as f64; + let ray = (Coord2(0.0, y+0.5), Coord2(1.0, y+0.5)); + let collisions_1 = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))).ray_collisions(&ray); + let ray = (Coord2(0.0, y+1.5), Coord2(1.0, y+1.5)); + let collisions_2 = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))).ray_collisions(&ray); + let ray = (Coord2(0.5, 0.0), Coord2(0.5, 1.0)); + let collisions_3 = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))).ray_collisions(&ray); + println!("{} - {} {} {}", y, collisions_1.len(), collisions_2.len(), collisions_3.len()); + } + + // Should produce a fixed number of collisions per row/column. Turn into a graph path and fire rays at it to see how it looks. + let chequerboard = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))); + let mut row_collisions = vec![]; + let mut col_collisions = vec![]; + + for y in 0..10 { + let y = y as f64; + let ray = (Coord2(0.0, y+0.5), Coord2(1.0, y+0.5)); + let collisions = chequerboard.ray_collisions(&ray); + + row_collisions.push(collisions.len()); } - println!("{:?}", chequerboard.len()); - assert!(chequerboard.len() == 50); + for x in 0..10 { + let x = x as f64; + let ray = (Coord2(x+0.5, 0.0), Coord2(x+0.5, 1.0)); + let collisions = chequerboard.ray_collisions(&ray); + + col_collisions.push(collisions.len()); + } + + println!("{:?}", row_collisions); + println!("{:?}", col_collisions); + + // All rows/columns should have 10 collisions on them + assert!(row_collisions == vec![10, 10, 10, 10, 10, 10, 10, 10, 10, 10]); + assert!(col_collisions == vec![10, 10, 10, 10, 10, 10, 10, 10, 10, 10]); } } } From 44f7a9258cad2b558a097be5b8b018b2a3ecba3b Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 13:44:23 +0000 Subject: [PATCH 05/18] For the vertical ray, can move it left/right to check for issues --- tests/bezier/path/arithmetic_sub.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/bezier/path/arithmetic_sub.rs b/tests/bezier/path/arithmetic_sub.rs index 7306b539..a822cd69 100644 --- a/tests/bezier/path/arithmetic_sub.rs +++ b/tests/bezier/path/arithmetic_sub.rs @@ -569,12 +569,13 @@ fn subtract_chequerboard() { } // Always should be 10 collisions horizontally, and 2 on the following line. Vertical collisions should go up as we add new lines + let x = if y%2 == 0 { 0.5 } else { 1.5 }; let y = y as f64; let ray = (Coord2(0.0, y+0.5), Coord2(1.0, y+0.5)); let collisions_1 = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))).ray_collisions(&ray); let ray = (Coord2(0.0, y+1.5), Coord2(1.0, y+1.5)); let collisions_2 = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))).ray_collisions(&ray); - let ray = (Coord2(0.5, 0.0), Coord2(0.5, 1.0)); + let ray = (Coord2(x, 0.0), Coord2(x, 1.0)); let collisions_3 = GraphPath::from_merged_paths(chequerboard.iter().map(|path| (path, PathLabel(0)))).ray_collisions(&ray); println!("{} - {} {} {}", y, collisions_1.len(), collisions_2.len(), collisions_3.len()); } From 75cfcd79c634cdb7601bc2bb99e78deb8e8ad736 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 15:17:16 +0000 Subject: [PATCH 06/18] Add the chequerboard test to the arithmetic demo so it's possible to see it break down --- demos/arithmetic/src/main.rs | 60 ++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 6 deletions(-) diff --git a/demos/arithmetic/src/main.rs b/demos/arithmetic/src/main.rs index 1811772e..c1434c56 100644 --- a/demos/arithmetic/src/main.rs +++ b/demos/arithmetic/src/main.rs @@ -50,10 +50,44 @@ fn main() { let path5 = path_permutation(vec![Coord2(64.0, 263.0), Coord2(877.0, 263.0), Coord2(877.0, 168.0), Coord2(64.0, 168.0)], 0, false); // Add and subtract them to generate the final path - let path = path_add::<_, _, SimpleBezierPath>(&vec![path1.clone()], &vec![path2.clone()], 0.1); - let path = path_sub::<_, _, SimpleBezierPath>(&path, &vec![path3.clone()], 0.1); - - let sub_path_test = path_sub::<_, _, SimpleBezierPath>(&vec![path5.clone()], &vec![path4.clone()], 0.1); + let path = path_add::(&vec![path1.clone()], &vec![path2.clone()], 0.1); + let path = path_sub::(&path, &vec![path3.clone()], 0.1); + + let sub_path_test = path_sub::(&vec![path5.clone()], &vec![path4.clone()], 0.1); + + // Chequerboard test + let mut chequerboard = vec![ + BezierPathBuilder::::start(Coord2(0.0, 0.0)) + .line_to(Coord2(10.0, 0.0)) + .line_to(Coord2(10.0, 10.0)) + .line_to(Coord2(0.0, 10.0)) + .line_to(Coord2(0.0, 0.0)) + .build() + ]; + + // Subtract every other square + let num_rows = since_start / 1_000_000_000.0; + let num_rows = (num_rows as u64) % 10; + + for y in 0..num_rows { + for x in 0..5 { + let x = if y%2 == 0 { + (x as f64)*2.0 + 1.0 + } else { + (x as f64)*2.0 + }; + let y = y as f64; + + let inner_square = BezierPathBuilder::::start(Coord2(x, y)) + .line_to(Coord2(x+1.0, y)) + .line_to(Coord2(x+1.0, y+1.0)) + .line_to(Coord2(x, y+1.0)) + .line_to(Coord2(x, y)) + .build(); + + chequerboard = path_sub(&chequerboard, &vec![inner_square], 0.01); + } + } canvas.draw(|gc| { gc.clear_canvas(Color::Rgba(1.0, 1.0, 1.0, 1.0)); @@ -105,10 +139,10 @@ fn main() { // Create the graph path from the source side let mut merged_path = GraphPath::new(); - merged_path = merged_path.merge(GraphPath::from_merged_paths(vec![path5].iter().map(|path| (path, PathLabel(0, PathDirection::from(path)))))); + merged_path = merged_path.merge(GraphPath::from_merged_paths(vec![path5].iter().map(|path| (path, PathLabel(0))))); // Collide with the target side to generate a full path - merged_path = merged_path.collide(GraphPath::from_merged_paths(vec![path4].iter().map(|path| (path, PathLabel(1, PathDirection::from(path))))), 0.1); + merged_path = merged_path.collide(GraphPath::from_merged_paths(vec![path4].iter().map(|path| (path, PathLabel(1)))), 0.1); merged_path.round(0.1); // Set the exterior edges using the 'subtract' algorithm @@ -129,6 +163,20 @@ fn main() { gc.stroke(); } + + // Draw the chequerboard + gc.push_state(); + + gc.transform(Transform2D::translate(100.0, 600.0) * Transform2D::scale(20.0, 20.0)); + + gc.fill_color(Color::Rgba(0.0, 0.4, 0.6, 1.0)); + gc.new_path(); + for p in chequerboard.iter() { + gc.bezier_path(p); + } + gc.fill(); + + gc.pop_state(); }); } }); From 3a6cf2301b5f6625bde7d0a8c5612688e884ef35 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 15:24:10 +0000 Subject: [PATCH 07/18] Draw some coloured borders to indicate where the shapes are --- demos/arithmetic/src/main.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/demos/arithmetic/src/main.rs b/demos/arithmetic/src/main.rs index c1434c56..39dba25b 100644 --- a/demos/arithmetic/src/main.rs +++ b/demos/arithmetic/src/main.rs @@ -176,6 +176,17 @@ fn main() { } gc.fill(); + gc.line_width_pixels(2.0); + let mut h = 0.0; + for p in chequerboard.iter() { + gc.stroke_color(Color::Hsluv(h % 360.0, 75.0, 70.0, 1.0)); + h += 43.0; + + gc.new_path(); + gc.bezier_path(p); + gc.stroke(); + } + gc.pop_state(); }); } From bf8fa0cce5e90b36f79bc3706098e5b0529c977e Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 15:24:28 +0000 Subject: [PATCH 08/18] Change rate at which the rows are displayed for the chequerboard pattern --- demos/arithmetic/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/demos/arithmetic/src/main.rs b/demos/arithmetic/src/main.rs index 39dba25b..90bc8f6f 100644 --- a/demos/arithmetic/src/main.rs +++ b/demos/arithmetic/src/main.rs @@ -66,8 +66,8 @@ fn main() { ]; // Subtract every other square - let num_rows = since_start / 1_000_000_000.0; - let num_rows = (num_rows as u64) % 10; + let num_rows = since_start / 250_000_000.0; + let num_rows = (num_rows as u64) % 11; for y in 0..num_rows { for x in 0..5 { From c76de9914dce55da69d7a6c150eae0e3d5581830 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 15:26:27 +0000 Subject: [PATCH 09/18] Expand the squares that are being removed so that the algorithm works --- demos/arithmetic/src/main.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/demos/arithmetic/src/main.rs b/demos/arithmetic/src/main.rs index 90bc8f6f..4c5c53d1 100644 --- a/demos/arithmetic/src/main.rs +++ b/demos/arithmetic/src/main.rs @@ -79,9 +79,9 @@ fn main() { let y = y as f64; let inner_square = BezierPathBuilder::::start(Coord2(x, y)) - .line_to(Coord2(x+1.0, y)) - .line_to(Coord2(x+1.0, y+1.0)) - .line_to(Coord2(x, y+1.0)) + .line_to(Coord2(x+1.1, y)) + .line_to(Coord2(x+1.1, y+1.1)) + .line_to(Coord2(x, y+1.1)) .line_to(Coord2(x, y)) .build(); From 24e657bba20d5d8cd7bb2f07870611369ca7849d Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 15:28:54 +0000 Subject: [PATCH 10/18] ... and break it again --- demos/arithmetic/src/main.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/demos/arithmetic/src/main.rs b/demos/arithmetic/src/main.rs index 4c5c53d1..90bc8f6f 100644 --- a/demos/arithmetic/src/main.rs +++ b/demos/arithmetic/src/main.rs @@ -79,9 +79,9 @@ fn main() { let y = y as f64; let inner_square = BezierPathBuilder::::start(Coord2(x, y)) - .line_to(Coord2(x+1.1, y)) - .line_to(Coord2(x+1.1, y+1.1)) - .line_to(Coord2(x, y+1.1)) + .line_to(Coord2(x+1.0, y)) + .line_to(Coord2(x+1.0, y+1.0)) + .line_to(Coord2(x, y+1.0)) .line_to(Coord2(x, y)) .build(); From c2ebc527166af58f6b6f68ddfad2de7f43d24145 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sat, 25 Feb 2023 17:27:28 +0000 Subject: [PATCH 11/18] Show all the subtractions, so it's possible to see where the chequerboard fails --- demos/arithmetic/src/main.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/demos/arithmetic/src/main.rs b/demos/arithmetic/src/main.rs index 90bc8f6f..424dc72a 100644 --- a/demos/arithmetic/src/main.rs +++ b/demos/arithmetic/src/main.rs @@ -66,11 +66,15 @@ fn main() { ]; // Subtract every other square - let num_rows = since_start / 250_000_000.0; - let num_rows = (num_rows as u64) % 11; + let num_cols = since_start / 250_000_000.0; + let num_rows = num_cols/5.0; + let num_rows = (num_rows as u64) % 10 + 1; + let num_cols = (num_cols as u64) % 5 + 1; for y in 0..num_rows { - for x in 0..5 { + let num_cols = if y == (num_rows-1) { num_cols } else { 5 }; + + for x in 0..num_cols { let x = if y%2 == 0 { (x as f64)*2.0 + 1.0 } else { From 230a04dd9b00354bdc8dca9d8906574eedb094cc Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sun, 26 Feb 2023 09:42:39 +0000 Subject: [PATCH 12/18] Sweep from left to right when searching for edges (This is based on the idea that if we sweep for edges from the outside we'll see fewer cases where the lines start to overlap. This nearly works for the chequerboard but is still producing errors eventually) --- src/bezier/path/graph_path/mod.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/bezier/path/graph_path/mod.rs b/src/bezier/path/graph_path/mod.rs index c5a98845..cc18b1a6 100644 --- a/src/bezier/path/graph_path/mod.rs +++ b/src/bezier/path/graph_path/mod.rs @@ -1051,12 +1051,32 @@ impl GraphPath { // Get the graph of exterior connections for the graph let connections = self.all_exterior_connections(); + // Order points by x then y index (ie, generate paths by sweeping from left to right) + let mut points = (0..self.points.len()).into_iter().collect::>(); + points.sort_by(|point_a, point_b| { + use std::cmp::{Ordering}; + + let x_a = self.points[*point_a].position.x(); + let x_b = self.points[*point_b].position.x(); + + if x_a < x_b { + Ordering::Less + } else if x_a > x_b { + Ordering::Greater + } else { + let y_a = self.points[*point_a].position.y(); + let y_b = self.points[*point_b].position.y(); + + y_a.partial_cmp(&y_b).unwrap_or(Ordering::Equal) + } + }); + // Store a list of edges that have been visited or are already in a path (these are flags: up to 32 edges per point are allowed by this algorithm) // Even a complex path very rarely has more than 2 edges per point let mut included_edges = vec![0u64; self.num_points()]; // Each connection describes the exterior edges for a point - for (point_idx, edge_list) in connections.iter().enumerate() { + for (point_idx, edge_list) in points.into_iter().map(|point_idx| (point_idx, &connections[point_idx])) { for (edge_idx, (_end_point_idx, edge_ref)) in edge_list.iter().enumerate() { // Ignore visited/included edges if included_edges[edge_ref.start_idx]&(1< Date: Sun, 26 Feb 2023 09:55:39 +0000 Subject: [PATCH 13/18] Use y-ordering if the x coordinates are close rather than exact, reverse the ordering too Hmm, there's still more work to be done on this case really, but this does fix the chequerboard test. --- src/bezier/path/graph_path/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/bezier/path/graph_path/mod.rs b/src/bezier/path/graph_path/mod.rs index cc18b1a6..5f09e404 100644 --- a/src/bezier/path/graph_path/mod.rs +++ b/src/bezier/path/graph_path/mod.rs @@ -1059,15 +1059,15 @@ impl GraphPath { let x_a = self.points[*point_a].position.x(); let x_b = self.points[*point_b].position.x(); - if x_a < x_b { - Ordering::Less - } else if x_a > x_b { - Ordering::Greater - } else { + if (x_a - x_b).abs() < 0.01 { let y_a = self.points[*point_a].position.y(); let y_b = self.points[*point_b].position.y(); - y_a.partial_cmp(&y_b).unwrap_or(Ordering::Equal) + y_b.partial_cmp(&y_a).unwrap_or(Ordering::Equal) + } else if x_a < x_b { + Ordering::Less + } else { + Ordering::Greater } }); From dd0f26bae253666b5c27fbf3e11f6c4ad9ef760d Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sun, 26 Feb 2023 09:57:22 +0000 Subject: [PATCH 14/18] Order of the outer and inner curves will be swapped with the new algorithm, so fix this test --- tests/bezier/algorithms/fill_concave.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/bezier/algorithms/fill_concave.rs b/tests/bezier/algorithms/fill_concave.rs index 40952dc1..29d920d1 100644 --- a/tests/bezier/algorithms/fill_concave.rs +++ b/tests/bezier/algorithms/fill_concave.rs @@ -210,21 +210,21 @@ fn fill_doughnut_with_extra_holes() { assert!(path.as_ref().unwrap().len() != 1); assert!(path.as_ref().unwrap().len() == 2); - for curve in path.as_ref().unwrap()[1].to_curves::>() { + for curve in path.as_ref().unwrap()[0].to_curves::>() { for t in 0..100 { let t = (t as f64)/100.0; let distance = circle_center.distance_to(&curve.point_at_pos(t)); - assert!((distance-outer_radius).abs() < 5.0); + assert!((distance-outer_radius).abs() < 5.0, "Outer curve had distance {:?}", (distance-outer_radius).abs()); } } - for curve in path.unwrap()[0].to_curves::>() { + for curve in path.unwrap()[1].to_curves::>() { for t in 0..100 { let t = (t as f64)/100.0; let distance = circle_center.distance_to(&curve.point_at_pos(t)); - assert!((distance-inner_radius).abs() < 2.0); + assert!((distance-inner_radius).abs() < 2.0, "Inner curve had distance {:?}", (distance-inner_radius).abs()); } } } From b6f9e158841eb6b110e4aa33b0bfa096c4319cdd Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sun, 26 Feb 2023 10:20:12 +0000 Subject: [PATCH 15/18] Update tests to use has_end_points_in_order so they don't fail when we change how the exterior path is generated --- tests/bezier/path/graph_path.rs | 47 +++++++++++++++------------------ 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/tests/bezier/path/graph_path.rs b/tests/bezier/path/graph_path.rs index 0fe18d23..d47e2abe 100644 --- a/tests/bezier/path/graph_path.rs +++ b/tests/bezier/path/graph_path.rs @@ -1,3 +1,5 @@ +use super::checks::*; + use flo_curves::*; use flo_curves::arc::*; use flo_curves::bezier::path::*; @@ -895,15 +897,12 @@ fn get_path_from_exterior_lines() { println!("{:?}", rectangle2); assert!(rectangle2.len() == 1); - assert!(rectangle2[0].start_point() == Coord2(1.0, 1.0)); - - let points = rectangle2[0].points().collect::>(); - assert!(points.len() == 4); - - assert!(points[2].2 == Coord2(1.0, 5.0)); - assert!(points[1].2 == Coord2(5.0, 5.0)); - assert!(points[0].2 == Coord2(5.0, 1.0)); - assert!(points[3].2 == Coord2(1.0, 1.0)); + assert!(path_has_end_points_in_order(rectangle2[0].clone(), vec![ + Coord2(1.0, 1.0), + Coord2(1.0, 5.0), + Coord2(5.0, 5.0), + Coord2(5.0, 1.0), + ], 0.001)); } #[test] @@ -938,24 +937,20 @@ fn get_path_from_exterior_lines_multiple_paths() { println!("{:?}", rectangle3); assert!(rectangle3.len() == 2); - assert!(rectangle3[0].start_point() == Coord2(1.0, 1.0)); - assert!(rectangle3[1].start_point() == Coord2(11.0, 1.0)); - - let points = rectangle3[0].points().collect::>(); - assert!(points.len() == 4); - - assert!(points[2].2 == Coord2(1.0, 5.0)); - assert!(points[1].2 == Coord2(5.0, 5.0)); - assert!(points[0].2 == Coord2(5.0, 1.0)); - assert!(points[3].2 == Coord2(1.0, 1.0)); - - let points = rectangle3[1].points().collect::>(); - assert!(points.len() == 4); - assert!(points[2].2 == Coord2(11.0, 5.0)); - assert!(points[1].2 == Coord2(15.0, 5.0)); - assert!(points[0].2 == Coord2(15.0, 1.0)); - assert!(points[3].2 == Coord2(11.0, 1.0)); + assert!(path_has_end_points_in_order(rectangle3[0].clone(), vec![ + Coord2(1.0, 1.0), + Coord2(1.0, 5.0), + Coord2(5.0, 5.0), + Coord2(5.0, 1.0), + ], 0.001)); + + assert!(path_has_end_points_in_order(rectangle3[1].clone(), vec![ + Coord2(11.0, 5.0), + Coord2(15.0, 5.0), + Coord2(15.0, 1.0), + Coord2(11.0, 1.0), + ], 0.001)); } #[test] From 0dac7753a544a31bc583770283c6eaab5890f889 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sun, 26 Feb 2023 22:40:01 +0000 Subject: [PATCH 16/18] Add some comments about how the ordering works --- src/bezier/path/graph_path/mod.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/bezier/path/graph_path/mod.rs b/src/bezier/path/graph_path/mod.rs index 5f09e404..fdc50af6 100644 --- a/src/bezier/path/graph_path/mod.rs +++ b/src/bezier/path/graph_path/mod.rs @@ -937,6 +937,7 @@ impl GraphPath { // Check the 'already used' list only if there are no alternative edges from this point let avoid_already_used = if following_connections.len() > 1 { // Use the 'used' array to exclude edges unless it would exclude all edges + // TODO: only do this if no loop is found that does not re-use edges (this is slightly too eager to re-use an edge) following_connections.iter() .any(|(_following_point_idx, following_edge)| { visited_edges[following_edge.start_idx]&(1< GraphPath { let connections = self.all_exterior_connections(); // Order points by x then y index (ie, generate paths by sweeping from left to right) + // This is to try to ensure that paths are matched from outside in: when there are paths that share vertices, just finding loops + // is insufficient to always build a valid result (it's possible to generate paths that share all of their edges, which will fill + // or clear path sections incorrectly) + // + // We try to avoid re-using edges but will re-use an edge to generate a loop if that's the only way, which can cause this same + // problem to show up. Ordering like this reduces the incidence of this issue by making it so we find paths by working inwards + // instead of randomly (though a most of the time this issue does not occur, so this is wasted effort, though having outer paths + // come before inner paths is a side-benefit) let mut points = (0..self.points.len()).into_iter().collect::>(); points.sort_by(|point_a, point_b| { use std::cmp::{Ordering}; From 0d3bf125fdef1c3f570cbc02a8d8f3f947f22125 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sun, 26 Feb 2023 22:49:42 +0000 Subject: [PATCH 17/18] Remove the fine-tuning on the 'y compare' routine and just always avoid re-using edges (This leaves the tests passing but can result in unused edges in the right circumstances) --- src/bezier/path/graph_path/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bezier/path/graph_path/mod.rs b/src/bezier/path/graph_path/mod.rs index fdc50af6..c022f17b 100644 --- a/src/bezier/path/graph_path/mod.rs +++ b/src/bezier/path/graph_path/mod.rs @@ -936,6 +936,8 @@ impl GraphPath { // Check the 'already used' list only if there are no alternative edges from this point let avoid_already_used = if following_connections.len() > 1 { + true + /* // Use the 'used' array to exclude edges unless it would exclude all edges // TODO: only do this if no loop is found that does not re-use edges (this is slightly too eager to re-use an edge) following_connections.iter() @@ -943,6 +945,7 @@ impl GraphPath { visited_edges[following_edge.start_idx]&(1< GraphPath { let y_a = self.points[*point_a].position.y(); let y_b = self.points[*point_b].position.y(); - y_b.partial_cmp(&y_a).unwrap_or(Ordering::Equal) + y_a.partial_cmp(&y_b).unwrap_or(Ordering::Equal) } else if x_a < x_b { Ordering::Less } else { From 6cd33b43c8777fb1ba49e60228f923b3ffeb2ef2 Mon Sep 17 00:00:00 2001 From: Andrew Hunter Date: Sun, 26 Feb 2023 23:02:03 +0000 Subject: [PATCH 18/18] Change the calls to find_loop so that it tries to find a loop without re-using edges before it allows edge re-use. --- src/bezier/path/graph_path/mod.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/bezier/path/graph_path/mod.rs b/src/bezier/path/graph_path/mod.rs index c022f17b..1b1cda0f 100644 --- a/src/bezier/path/graph_path/mod.rs +++ b/src/bezier/path/graph_path/mod.rs @@ -893,7 +893,6 @@ impl GraphPath { /// /// If there's a choice, this will not follow any previously used edge (but will follow them if that's the way to make progress with a loop of edges) /// - #[inline] fn find_loop(&self, connections: &Vec>, start_point_idx: usize, edge: usize, used_edges: &Vec) -> Option> { // The algorithm here is a slight modification of Dijkstra's algorithm, we start knowing the path has to contain a particular edge let mut previous_point = vec![None; connections.len()]; @@ -935,20 +934,7 @@ impl GraphPath { let following_connections = &connections[next_point_idx]; // Check the 'already used' list only if there are no alternative edges from this point - let avoid_already_used = if following_connections.len() > 1 { - true - /* - // Use the 'used' array to exclude edges unless it would exclude all edges - // TODO: only do this if no loop is found that does not re-use edges (this is slightly too eager to re-use an edge) - following_connections.iter() - .any(|(_following_point_idx, following_edge)| { - visited_edges[following_edge.start_idx]&(1< 1; for (following_point_idx, following_edge) in following_connections.iter() { // Don't follow visited edges @@ -1100,7 +1086,16 @@ impl GraphPath { included_edges[edge_ref.start_idx] |= 1<