From 22818037e609709f8fc382d9e8821755e8fe453d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 16:59:15 +0100 Subject: [PATCH 01/11] Consolidate duplicated code --- src/kernel/shapes/difference_2d.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index fc1113902..def5dbd03 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -16,8 +16,8 @@ impl ToShape for fj::Difference2d { let mut shape = Shape::new(); - let mut a = self.a.to_shape(tolerance, debug_info); - let mut b = self.b.to_shape(tolerance, debug_info); + let [mut a, mut b] = [&self.a, &self.b] + .map(|shape| shape.to_shape(tolerance, debug_info)); if a.cycles().all().count() == 1 && b.cycles().all().count() == 1 { let a = a.cycles().all().next().unwrap(); From 81a82e3b387311199389fc4f7527b7295eeb30b2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:02:25 +0100 Subject: [PATCH 02/11] Refactor --- src/kernel/shapes/difference_2d.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index def5dbd03..885de579e 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -19,7 +19,19 @@ impl ToShape for fj::Difference2d { let [mut a, mut b] = [&self.a, &self.b] .map(|shape| shape.to_shape(tolerance, debug_info)); - if a.cycles().all().count() == 1 && b.cycles().all().count() == 1 { + for shape in [&mut a, &mut b] { + if shape.cycles().all().count() != 1 { + // See issue: + // https://github.com/hannobraun/Fornjot/issues/95 + todo!( + "The 2-dimensional difference operation only supports one \ + cycle in each operand." + ); + } + } + + { + // Can't panic, as we just verified that both shapes have one cycle. let a = a.cycles().all().next().unwrap(); let b = b.cycles().all().next().unwrap(); @@ -29,13 +41,6 @@ impl ToShape for fj::Difference2d { shape.cycles().add(Cycle { edges: b.edges.clone(), }); - } else { - // See issue: - // https://github.com/hannobraun/Fornjot/issues/95 - todo!( - "The 2-dimensional difference operation only supports one \ - cycle in each operand." - ); } { From 2bd52a6028382ea5eb433ec6d85f00b0e1d5c050 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:03:41 +0100 Subject: [PATCH 03/11] Consolidate duplicated code --- src/kernel/shapes/difference_2d.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 885de579e..9fcd8c9a9 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -32,8 +32,8 @@ impl ToShape for fj::Difference2d { { // Can't panic, as we just verified that both shapes have one cycle. - let a = a.cycles().all().next().unwrap(); - let b = b.cycles().all().next().unwrap(); + let [a, b] = [&mut a, &mut b] + .map(|shape| shape.cycles().all().next().unwrap()); shape.cycles().add(Cycle { edges: a.edges.clone(), From c92195669ca38dd40ad1b53a1abc97e4e14885c1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:05:03 +0100 Subject: [PATCH 04/11] Consolidate duplicated code --- src/kernel/shapes/difference_2d.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 9fcd8c9a9..633bf80ac 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -32,15 +32,14 @@ impl ToShape for fj::Difference2d { { // Can't panic, as we just verified that both shapes have one cycle. - let [a, b] = [&mut a, &mut b] + let cycles = [&mut a, &mut b] .map(|shape| shape.cycles().all().next().unwrap()); - shape.cycles().add(Cycle { - edges: a.edges.clone(), - }); - shape.cycles().add(Cycle { - edges: b.edges.clone(), - }); + for cycle in cycles { + shape.cycles().add(Cycle { + edges: cycle.edges.clone(), + }); + } } { From 950fd90d98cc0cc2e3ca8f2c1c7310dcaedfc1a5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:05:41 +0100 Subject: [PATCH 05/11] Remove redundant block --- src/kernel/shapes/difference_2d.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 633bf80ac..32cfff804 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -30,16 +30,14 @@ impl ToShape for fj::Difference2d { } } - { - // Can't panic, as we just verified that both shapes have one cycle. - let cycles = [&mut a, &mut b] - .map(|shape| shape.cycles().all().next().unwrap()); + // Can't panic, as we just verified that both shapes have one cycle. + let cycles = + [&mut a, &mut b].map(|shape| shape.cycles().all().next().unwrap()); - for cycle in cycles { - shape.cycles().add(Cycle { - edges: cycle.edges.clone(), - }); - } + for cycle in cycles { + shape.cycles().add(Cycle { + edges: cycle.edges.clone(), + }); } { From 65cf917fe5a18c94cbc583509c988f3275696b01 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:07:55 +0100 Subject: [PATCH 06/11] Refactor --- src/kernel/shapes/difference_2d.rs | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 32cfff804..007c4bb37 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -28,6 +28,14 @@ impl ToShape for fj::Difference2d { cycle in each operand." ); } + if shape.faces().all().count() != 1 { + // See issue: + // https://github.com/hannobraun/Fornjot/issues/95 + todo!( + "The 2-dimensional difference operation only supports one \ + face in each operand." + ); + } } // Can't panic, as we just verified that both shapes have one cycle. @@ -41,22 +49,11 @@ impl ToShape for fj::Difference2d { } { - let (a, b) = if a.faces().all().count() == 1 - && b.faces().all().count() == 1 - { - // Can't panic. We just checked that length of `a` and `b` is 1. - ( - a.faces().all().next().unwrap(), - b.faces().all().next().unwrap(), - ) - } else { - // See issue: - // https://github.com/hannobraun/Fornjot/issues/95 - todo!( - "The 2-dimensional difference operation only supports one \ - face in each operand." - ); - }; + // Can't panic, as we just verified that both shapes have one face. + let (a, b) = ( + a.faces().all().next().unwrap(), + b.faces().all().next().unwrap(), + ); let (a, b, surface_a, surface_b) = match ((*a).clone(), (*b).clone()) { From 2036ef9040233f7652bab14a48fd103ccd3b8f85 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:09:23 +0100 Subject: [PATCH 07/11] Consolidate duplicated code --- src/kernel/shapes/difference_2d.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 007c4bb37..82a596254 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -50,10 +50,8 @@ impl ToShape for fj::Difference2d { { // Can't panic, as we just verified that both shapes have one face. - let (a, b) = ( - a.faces().all().next().unwrap(), - b.faces().all().next().unwrap(), - ); + let [a, b] = [&mut a, &mut b] + .map(|shape| shape.faces().all().next().unwrap()); let (a, b, surface_a, surface_b) = match ((*a).clone(), (*b).clone()) { From d321f1c1d1d62b71c2262b36a4a36a3300ee8970 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:11:04 +0100 Subject: [PATCH 08/11] Remove comment We're panicking everywhere in the kernel, if something isn't right. No need to call out this specific one. --- src/kernel/shapes/difference_2d.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 82a596254..6c8becc0b 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -74,8 +74,6 @@ impl ToShape for fj::Difference2d { assert!( surface_a == surface_b, - // Panicking is not great, but as long as we don't have a real - // error handling mechanism, it will do. "Trying to subtract sketches with different surfaces." ); let surface = surface_a; From de7d03efc467a6d4b36cd74ff2b27b13be44270f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:13:07 +0100 Subject: [PATCH 09/11] Rename variables to increase clarity --- src/kernel/shapes/difference_2d.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 6c8becc0b..7e52fcea0 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -50,11 +50,11 @@ impl ToShape for fj::Difference2d { { // Can't panic, as we just verified that both shapes have one face. - let [a, b] = [&mut a, &mut b] + let [face_a, face_b] = [&mut a, &mut b] .map(|shape| shape.faces().all().next().unwrap()); let (a, b, surface_a, surface_b) = - match ((*a).clone(), (*b).clone()) { + match ((*face_a).clone(), (*face_b).clone()) { ( Face::Face { cycles: a, From a0d6814ef825d372415fd256b8621ab16b1235ee Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:14:09 +0100 Subject: [PATCH 10/11] Rename variables to increase clarity --- src/kernel/shapes/difference_2d.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 7e52fcea0..61113f8d7 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -53,7 +53,7 @@ impl ToShape for fj::Difference2d { let [face_a, face_b] = [&mut a, &mut b] .map(|shape| shape.faces().all().next().unwrap()); - let (a, b, surface_a, surface_b) = + let (cycles_a, cycles_b, surface_a, surface_b) = match ((*face_a).clone(), (*face_b).clone()) { ( Face::Face { @@ -78,8 +78,8 @@ impl ToShape for fj::Difference2d { ); let surface = surface_a; - let mut cycles = a; - cycles.extend(b); + let mut cycles = cycles_a; + cycles.extend(cycles_b); shape.faces().add(Face::Face { cycles, surface }); }; From 386c10524c2389ead7f4fa08c3b60c179362a8ee Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 8 Mar 2022 17:15:08 +0100 Subject: [PATCH 11/11] Remove redundant block --- src/kernel/shapes/difference_2d.rs | 59 ++++++++++++++---------------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/src/kernel/shapes/difference_2d.rs b/src/kernel/shapes/difference_2d.rs index 61113f8d7..c6ea0cd69 100644 --- a/src/kernel/shapes/difference_2d.rs +++ b/src/kernel/shapes/difference_2d.rs @@ -48,41 +48,38 @@ impl ToShape for fj::Difference2d { }); } - { - // Can't panic, as we just verified that both shapes have one face. - let [face_a, face_b] = [&mut a, &mut b] - .map(|shape| shape.faces().all().next().unwrap()); + // Can't panic, as we just verified that both shapes have one face. + let [face_a, face_b] = + [&mut a, &mut b].map(|shape| shape.faces().all().next().unwrap()); - let (cycles_a, cycles_b, surface_a, surface_b) = - match ((*face_a).clone(), (*face_b).clone()) { - ( - Face::Face { - cycles: a, - surface: surface_a, - }, - Face::Face { - cycles: b, - surface: surface_b, - }, - ) => (a, b, surface_a, surface_b), - _ => { - // None of the 2D types still use triangle - // representation. - unreachable!() - } - }; + let (cycles_a, cycles_b, surface_a, surface_b) = + match ((*face_a).clone(), (*face_b).clone()) { + ( + Face::Face { + cycles: a, + surface: surface_a, + }, + Face::Face { + cycles: b, + surface: surface_b, + }, + ) => (a, b, surface_a, surface_b), + _ => { + // None of the 2D types still use triangle representation. + unreachable!() + } + }; - assert!( - surface_a == surface_b, - "Trying to subtract sketches with different surfaces." - ); - let surface = surface_a; + assert!( + surface_a == surface_b, + "Trying to subtract sketches with different surfaces." + ); + let surface = surface_a; - let mut cycles = cycles_a; - cycles.extend(cycles_b); + let mut cycles = cycles_a; + cycles.extend(cycles_b); - shape.faces().add(Face::Face { cycles, surface }); - }; + shape.faces().add(Face::Face { cycles, surface }); shape }