Merge branch 'fix-shared-path-bug' into v0.7

demos/arithmetic/src/main.rs

@@ -55,6 +55,44 @@ fn main() {
 
             let sub_path_test   = path_sub::<SimpleBezierPath>(&vec![path5.clone()], &vec![path4.clone()], 0.1);
 
+            // Chequerboard test
+            let mut chequerboard = vec![
+                BezierPathBuilder::<SimpleBezierPath>::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_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 {
+                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 {
+                        (x as f64)*2.0
+                    };
+                    let y = y as f64;
+
+                    let inner_square = BezierPathBuilder::<SimpleBezierPath>::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));
 
@@ -129,6 +167,31 @@ 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.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();
             });
         }
     });

src/bezier/path/graph_path/mod.rs

@@ -891,8 +891,9 @@ impl<Point: Coordinate+Coordinate2D, Label: Copy> GraphPath<Point, Label> {
     /// 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
     ///
-    #[inline]
-    fn find_loop(&self, connections: &Vec<SmallVec<[(usize, GraphEdgeRef); 4]>>, start_point_idx: usize, edge: usize) -> Option<Vec<(usize, GraphEdgeRef)>> {
+    /// 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)
+    ///
+    fn find_loop(&self, connections: &Vec<SmallVec<[(usize, GraphEdgeRef); 4]>>, start_point_idx: usize, edge: usize, used_edges: &Vec<u64>) -> Option<Vec<(usize, GraphEdgeRef)>> {
         // 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 +931,22 @@ impl<Point: Coordinate+Coordinate2D, Label: Copy> GraphPath<Point, Label> {
             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 = following_connections.len() > 1;
+
+            for (following_point_idx, following_edge) in following_connections.iter() {
                 // Don't follow visited edges
                 if visited_edges[following_edge.start_idx]&(1<<following_edge.edge_idx) != 0 {
                     continue;
                 }
 
+                // Also avoid edges used for previous shapes unless they are the only way to make progress
+                if avoid_already_used && used_edges[following_edge.start_idx]&(1<<following_edge.edge_idx) != 0 {
+                    continue;
+                }
+
                 // Update the previous point for this point
                 if previous_point[*following_point_idx].is_none() {
                     previous_point[*following_point_idx] = Some((next_point_idx, *following_edge));
@@ -1030,12 +1041,40 @@ impl<Point: Coordinate+Coordinate2D, Label: Copy> GraphPath<Point, Label> {
         // 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)
+        // 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::<Vec<_>>();
+        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).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)
+            } else if x_a < x_b {
+                Ordering::Less
+            } else {
+                Ordering::Greater
+            }
+        });
+
         // 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<<edge_ref.edge_idx) != 0 {
@@ -1047,7 +1086,16 @@ impl<Point: Coordinate+Coordinate2D, Label: Copy> GraphPath<Point, Label> {
                 included_edges[edge_ref.start_idx] |= 1<<edge_ref.edge_idx;
 
                 // Try to find a loop from this edge
-                if let Some(loop_edges) = self.find_loop(&connections, point_idx, edge_idx) {
+                let loop_edges = if let Some(loop_edges) = self.find_loop(&connections, point_idx, edge_idx, &included_edges) {
+                    // Loop was found without any re-used edges
+                    Some(loop_edges)
+                } else {
+                    // Loop was found with some re-used edges
+                    // TODO: this can produce bad path results when it occurs: see comment above
+                    self.find_loop(&connections, point_idx, edge_idx, &vec![0u64; self.num_points()])
+                };
+
+                if let Some(loop_edges) = loop_edges {
                     // Mark all the loop edges as visited
                     for (_, edge) in loop_edges.iter() {
                         included_edges[edge.start_idx] |= 1<<edge.edge_idx;

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::<Curve<Coord2>>() {
+    for curve in path.as_ref().unwrap()[0].to_curves::<Curve<Coord2>>() {
         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::<Curve<Coord2>>() {
+    for curve in path.unwrap()[1].to_curves::<Curve<Coord2>>() {
         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());
         }
     }
 }

tests/bezier/path/arithmetic_sub.rs

@@ -494,4 +494,119 @@ fn subtract_permutations_2() {
             }
         }
     }
-}
+}
+
+#[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::<SimpleBezierPath>(&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() != 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::<SimpleBezierPath>::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);
+                }
+
+                // 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(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());
+            }
+
+            // 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());
+            }
+
+            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]);
+        }
+    }
+}

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::<Vec<_>>();
-    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::<Vec<_>>();
-    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::<Vec<_>>();
-    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]