refactor: various further refactorings (#128)

These optimize out unnecessary code duplication, reserve vector capacity beforehand by leveraging `Iterator`s and avoid unnecessary double-lookups into HashMaps.

Reviewed-on: https://codeberg.org/topola/topola/pulls/128
Co-authored-by: Alain Emilia Anna Zscheile <fogti+devel@ytrizja.de>
Co-committed-by: Alain Emilia Anna Zscheile <fogti+devel@ytrizja.de>

Cargo.toml

@@ -11,6 +11,11 @@ serde_json = "1.0"
 spade = "2.12"
 thiserror = "2.0"
 
+[workspace.dependencies.geo-types]
+version = "0.7"
+default-features = false
+features = ["serde"]
+
 [workspace.dependencies.geo]
 version = "0.29"
 default-features = false

crates/specctra-core/Cargo.toml

@@ -5,7 +5,7 @@ edition = "2021"
 
 [dependencies]
 bimap.workspace = true
-geo.workspace = true
+geo-types.workspace = true
 serde.workspace = true
 specctra_derive.path = "../specctra_derive"
 thiserror.workspace = true

crates/specctra-core/src/math.rs

@@ -1,5 +1,5 @@
 use core::ops::Sub;
-use geo::geometry::Point;
+use geo_types::geometry::Point;
 use serde::{Deserialize, Serialize};
 
 #[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]
@@ -14,6 +14,19 @@ pub struct PointWithRotation {
     pub rot: f64,
 }
 
+impl Circle {
+    /// Calculate the point that lies on the circle at angle `phi`,
+    /// relative to coordinate axes.
+    ///
+    /// `phi` is the angle given in radians starting at `(r, 0)`.
+    pub fn position_at_angle(&self, phi: f64) -> Point {
+        geo_types::point! {
+            x: self.pos.0.x + self.r * phi.cos(),
+            y: self.pos.0.y + self.r * phi.sin()
+        }
+    }
+}
+
 impl Sub for Circle {
     type Output = Self;
 

crates/specctra-core/src/structure.rs

@@ -392,6 +392,23 @@ pub struct Point {
     pub y: f64,
 }
 
+impl From<geo_types::Point> for Point {
+    #[inline(always)]
+    fn from(z: geo_types::Point) -> Self {
+        Point { x: z.0.x, y: z.0.y }
+    }
+}
+
+impl From<Point> for geo_types::Point {
+    #[inline(always)]
+    fn from(z: Point) -> Self {
+        geo_types::point! {
+            x: z.x,
+            y: z.y
+        }
+    }
+}
+
 // Custom impl for the case described above
 impl<R: std::io::BufRead> ReadDsn<R> for Vec<Point> {
     fn read_dsn(tokenizer: &mut ListTokenizer<R>) -> Result<Self, ParseErrorContext> {

crates/topola-egui/src/painter.rs

@@ -36,25 +36,15 @@ impl<'a> Painter<'a> {
                 ],
                 egui::Stroke::new(seg.width as f32 * self.transform.scaling, color),
             ),
-            PrimitiveShape::Bend(bend) => {
+            PrimitiveShape::Bend(bend) => egui::Shape::line(
-                let circle = bend.circle();
+                bend.render_discretization(101)
-
+                    .map(|point| {
-                let angle_from = bend.start_angle();
+                        self.transform
-                let angle_step = bend.spanned_angle() / 100.0;
+                            .mul_pos([point.0.x as f32, -point.0.y as f32].into())
-
+                    })
-                let mut points: Vec<egui::Pos2> = vec![];
+                    .collect(),
-
-                for i in 0..=100 {
-                    let x = circle.pos.x() + circle.r * (angle_from + i as f64 * angle_step).cos();
-                    let y = circle.pos.y() + circle.r * (angle_from + i as f64 * angle_step).sin();
-                    points.push(self.transform.mul_pos([x as f32, -y as f32].into()));
-                }
-
-                egui::Shape::line(
-                    points,
                 egui::Stroke::new(bend.width as f32 * self.transform.scaling, color),
-                )
+            ),
-            }
         };
 
         self.ui.painter().add(epaint_shape);

src/autorouter/invoker.rs

@@ -202,6 +202,6 @@ impl<M: AccessMesadata> Invoker<M> {
             self.execute(entry.command().clone());
         }
 
-        self.history.set_undone(undone.into_iter());
+        self.history.set_undone(undone);
     }
 }

src/board/mod.rs

@@ -239,15 +239,10 @@ impl<M: AccessMesadata> Board<M> {
 
     /// Finds a band between two pin names.
     pub fn band_between_pins(&self, pinname1: &str, pinname2: &str) -> Option<BandUid> {
-        if let Some(band) = self
+        self.band_bandname
-            .band_bandname
             // note: it doesn't matter in what order pinnames are given, the constructor sorts them
             .get_by_right(&BandName::new(pinname1.to_string(), pinname2.to_string()))
-        {
+            .copied()
-            Some(*band)
-        } else {
-            None
-        }
     }
 
     /// Returns the mesadata associated with the layout's drawing rules.

src/drawing/drawing.rs

@@ -495,10 +495,9 @@ impl<CW: Copy, R: AccessRules> Drawing<CW, R> {
 
         if let Some(outer) = self.primitive(cane.bend).outer() {
             self.update_this_and_outward_bows(recorder, outer)
-                .map_err(|err| {
+                .inspect_err(|_| {
                     let joint = self.primitive(cane.bend).other_joint(cane.dot);
                     self.remove_cane(recorder, &cane, joint);
-                    err
                 })?;
         }
 
@@ -669,20 +668,18 @@ impl<CW: Copy, R: AccessRules> Drawing<CW, R> {
         let seg_to = self.add_dot_with_infringables(recorder, dot_weight, infringables)?;
         let seg = self
             .add_seg_with_infringables(recorder, from, seg_to.into(), seg_weight, infringables)
-            .map_err(|err| {
+            .inspect_err(|_| {
                 self.recording_geometry_with_rtree
                     .remove_dot(recorder, seg_to.into());
-                err
             })?;
 
         let to = self
             .add_dot_with_infringables(recorder, dot_weight, infringables)
-            .map_err(|err| {
+            .inspect_err(|_| {
                 self.recording_geometry_with_rtree
                     .remove_seg(recorder, seg.into());
                 self.recording_geometry_with_rtree
                     .remove_dot(recorder, seg_to.into());
-                err
             })?;
 
         let (bend_from, bend_to) = if cw { (to, seg_to) } else { (seg_to, to) };
@@ -696,14 +693,13 @@ impl<CW: Copy, R: AccessRules> Drawing<CW, R> {
                 bend_weight,
                 infringables,
             )
-            .map_err(|err| {
+            .inspect_err(|_| {
                 self.recording_geometry_with_rtree
                     .remove_dot(recorder, to.into());
                 self.recording_geometry_with_rtree
                     .remove_seg(recorder, seg.into());
                 self.recording_geometry_with_rtree
                     .remove_dot(recorder, seg_to.into());
-                err
             })?;
 
         Ok(Cane {

src/geometry/geometry.rs

@@ -197,10 +197,8 @@ impl<
     }
 
     pub(super) fn add_compound_at_index(&mut self, compound: GenericIndex<CW>, weight: CW) {
-        self.graph.update_node(
+        self.graph
-            compound.petgraph_index(),
+            .update_node(compound.petgraph_index(), GenericNode::Compound(weight));
-            GenericNode::Compound(weight.into()),
-        );
     }
 
     fn init_bend_joints_and_core<W: AccessBendWeight<PW>>(

src/geometry/primitive.rs

@@ -296,6 +296,16 @@ impl BendShape {
             angle
         }
     }
+
+    /// Render this bend as a list of points on its circle.
+    pub fn render_discretization(&self, point_count: usize) -> impl Iterator<Item = Point> + '_ {
+        let circle = self.circle();
+        let angle_from = self.start_angle();
+        // we need to use one less than the whole point count
+        // because we need to also emit the end-point
+        let angle_step = self.spanned_angle() / ((point_count - 1) as f64);
+        (0..point_count).map(move |i| circle.position_at_angle(angle_from + i as f64 * angle_step))
+    }
 }
 
 impl MeasureLength for BendShape {

src/geometry/recording_with_rtree.rs

@@ -1,3 +1,4 @@
+use std::collections::hash_map::Entry as HashMapEntry;
 use std::hash::Hash;
 
 use geo::Point;
@@ -144,37 +145,21 @@ impl<
         primitive: GenericIndex<W>,
         compound: GenericIndex<CW>,
     ) {
-        let old_members = self
+        let geometry = self.geometry_with_rtree.geometry();
-            .geometry_with_rtree
+        let old_members = geometry.compound_members(compound).collect();
-            .geometry()
+        let old_weight = geometry.compound_weight(compound);
-            .compound_members(compound)
-            .collect();
-        let old_weight = self
-            .geometry_with_rtree
-            .geometry()
-            .compound_weight(compound);
 
-        let new_members = self
+        // TODO ???
-            .geometry_with_rtree
-            .geometry()
-            .compound_members(compound)
-            .collect();
-        let new_weight = self
-            .geometry_with_rtree
-            .geometry()
-            .compound_weight(compound);
 
-        if let Some(value) = recorder.compounds.get_mut(&compound) {
+        let geometry = self.geometry_with_rtree.geometry();
-            value.1 = Some((new_members, new_weight));
+        let new_members = geometry.compound_members(compound).collect();
-        } else {
+        let new_weight = geometry.compound_weight(compound);
-            recorder.compounds.insert(
+
-                compound,
+        recorder
-                (
+            .compounds
-                    Some((old_members, old_weight)),
+            .entry(compound)
-                    Some((new_members, new_weight)),
+            .or_insert((Some((old_members, old_weight)), None))
-                ),
+            .1 = Some((new_members, new_weight));
-            );
-        }
     }
 
     pub fn remove_dot(
@@ -184,13 +169,7 @@ impl<
     ) -> Result<(), ()> {
         let weight = self.geometry_with_rtree.geometry().dot_weight(dot);
         self.geometry_with_rtree.remove_dot(dot)?;
-
+        edit_remove_from_map(&mut recorder.dots, dot, weight);
-        if let Some((None, Some(..))) = recorder.dots.get(&dot) {
-            recorder.dots.remove(&dot);
-        } else {
-            recorder.dots.insert(dot, (Some(weight), None));
-        };
-
         Ok(())
     }
 
@@ -199,15 +178,11 @@ impl<
         recorder: &mut GeometryEdit<PW, DW, SW, BW, CW, PI, DI, SI, BI>,
         seg: SI,
     ) {
-        let weight = self.geometry_with_rtree.geometry().seg_weight(seg);
+        let geometry = self.geometry_with_rtree.geometry();
-        let joints = self.geometry_with_rtree.geometry().seg_joints(seg);
+        let weight = geometry.seg_weight(seg);
+        let joints = geometry.seg_joints(seg);
         self.geometry_with_rtree.remove_seg(seg);
-
+        edit_remove_from_map(&mut recorder.segs, seg, (joints, weight));
-        if let Some((None, Some(..))) = recorder.segs.get(&seg) {
-            recorder.segs.remove(&seg);
-        } else {
-            recorder.segs.insert(seg, (Some((joints, weight)), None));
-        }
     }
 
     pub fn remove_bend(
@@ -215,18 +190,16 @@ impl<
         recorder: &mut GeometryEdit<PW, DW, SW, BW, CW, PI, DI, SI, BI>,
         bend: BI,
     ) {
-        let weight = self.geometry_with_rtree.geometry().bend_weight(bend);
+        let geometry = self.geometry_with_rtree.geometry();
-        let joints = self.geometry_with_rtree.geometry().bend_joints(bend);
+        let weight = geometry.bend_weight(bend);
-        let core = self.geometry_with_rtree.geometry().core(bend);
+        let joints = geometry.bend_joints(bend);
+        let core = geometry.core(bend);
         self.geometry_with_rtree.remove_bend(bend);
-
+        edit_remove_from_map(
-        if let Some((None, Some(..))) = recorder.bends.get(&bend) {
+            &mut recorder.bends,
-            recorder.bends.remove(&bend);
+            bend,
-        } else {
+            ((joints.0, joints.1, core), weight),
-            recorder
+        );
-                .bends
-                .insert(bend, (Some(((joints.0, joints.1, core), weight)), None));
-        }
     }
 
     pub fn remove_compound(
@@ -234,24 +207,11 @@ impl<
         recorder: &mut GeometryEdit<PW, DW, SW, BW, CW, PI, DI, SI, BI>,
         compound: GenericIndex<CW>,
     ) {
-        let weight = self
+        let geometry = self.geometry_with_rtree.geometry();
-            .geometry_with_rtree
+        let weight = geometry.compound_weight(compound);
-            .geometry()
+        let members = geometry.compound_members(compound).collect();
-            .compound_weight(compound);
-        let members = self
-            .geometry_with_rtree
-            .geometry()
-            .compound_members(compound)
-            .collect();
         self.geometry_with_rtree.remove_compound(compound);
-
+        edit_remove_from_map(&mut recorder.compounds, compound, (members, weight));
-        if let Some((None, Some(..))) = recorder.compounds.get(&compound) {
-            recorder.compounds.remove(&compound);
-        } else {
-            recorder
-                .compounds
-                .insert(compound, (Some((members, weight)), None));
-        }
     }
 
     pub fn move_dot(
@@ -264,13 +224,41 @@ impl<
         self.geometry_with_rtree.move_dot(dot, to);
         let new_weight = self.geometry_with_rtree.geometry().dot_weight(dot);
 
-        if let Some(value) = recorder.dots.get_mut(&dot) {
-            value.1 = Some(new_weight);
-        } else {
         recorder
             .dots
-                .insert(dot, (Some(old_weight), Some(new_weight)));
+            .entry(dot)
+            .or_insert((Some(old_weight), None))
+            .1 = Some(new_weight);
     }
+
+    fn modify_bend<F>(
+        &mut self,
+        recorder: &mut GeometryEdit<PW, DW, SW, BW, CW, PI, DI, SI, BI>,
+        bend: BI,
+        f: F,
+    ) where
+        F: FnOnce(&mut GeometryWithRtree<PW, DW, SW, BW, CW, PI, DI, SI, BI>, BI),
+    {
+        let geometry = self.geometry_with_rtree.geometry();
+        let old_joints = geometry.bend_joints(bend);
+        let old_core = geometry.core(bend);
+        let old_weight = geometry.bend_weight(bend);
+
+        f(&mut self.geometry_with_rtree, bend);
+
+        let geometry = self.geometry_with_rtree.geometry();
+        let new_joints = geometry.bend_joints(bend);
+        let new_core = geometry.core(bend);
+        let new_weight = geometry.bend_weight(bend);
+
+        recorder
+            .bends
+            .entry(bend)
+            .or_insert((
+                Some(((old_joints.0, old_joints.1, old_core), old_weight)),
+                None,
+            ))
+            .1 = Some(((new_joints.0, new_joints.1, new_core), new_weight));
     }
 
     pub fn shift_bend(
@@ -279,25 +267,9 @@ impl<
         bend: BI,
         offset: f64,
     ) {
-        let old_joints = self.geometry_with_rtree.geometry().bend_joints(bend);
+        self.modify_bend(recorder, bend, |geometry_with_rtree, bend| {
-        let old_core = self.geometry_with_rtree.geometry().core(bend);
+            geometry_with_rtree.shift_bend(bend, offset)
-        let old_weight = self.geometry_with_rtree.geometry().bend_weight(bend);
+        });
-        self.geometry_with_rtree.shift_bend(bend, offset);
-        let new_joints = self.geometry_with_rtree.geometry().bend_joints(bend);
-        let new_core = self.geometry_with_rtree.geometry().core(bend);
-        let new_weight = self.geometry_with_rtree.geometry().bend_weight(bend);
-
-        if let Some(value) = recorder.bends.get_mut(&bend) {
-            value.1 = Some(((new_joints.0, new_joints.1, new_core), new_weight));
-        } else {
-            recorder.bends.insert(
-                bend,
-                (
-                    Some(((old_joints.0, old_joints.1, old_core), old_weight)),
-                    Some(((new_joints.0, new_joints.1, new_core), new_weight)),
-                ),
-            );
-        }
     }
 
     pub fn flip_bend(
@@ -305,25 +277,9 @@ impl<
         recorder: &mut GeometryEdit<PW, DW, SW, BW, CW, PI, DI, SI, BI>,
         bend: BI,
     ) {
-        let old_joints = self.geometry_with_rtree.geometry().bend_joints(bend);
+        self.modify_bend(recorder, bend, |geometry_with_rtree, bend| {
-        let old_core = self.geometry_with_rtree.geometry().core(bend);
+            geometry_with_rtree.flip_bend(bend)
-        let old_weight = self.geometry_with_rtree.geometry().bend_weight(bend);
+        });
-        self.geometry_with_rtree.flip_bend(bend);
-        let new_joints = self.geometry_with_rtree.geometry().bend_joints(bend);
-        let new_core = self.geometry_with_rtree.geometry().core(bend);
-        let new_weight = self.geometry_with_rtree.geometry().bend_weight(bend);
-
-        if let Some(value) = recorder.bends.get_mut(&bend) {
-            value.1 = Some(((new_joints.0, new_joints.1, new_core), new_weight));
-        } else {
-            recorder.bends.insert(
-                bend,
-                (
-                    Some(((old_joints.0, old_joints.1, old_core), old_weight)),
-                    Some(((new_joints.0, new_joints.1, new_core), new_weight)),
-                ),
-            );
-        }
     }
 
     pub fn reattach_bend(
@@ -332,26 +288,9 @@ impl<
         bend: BI,
         maybe_new_inner: Option<BI>,
     ) {
-        let old_joints = self.geometry_with_rtree.geometry().bend_joints(bend);
+        self.modify_bend(recorder, bend, |geometry_with_rtree, bend| {
-        let old_core = self.geometry_with_rtree.geometry().core(bend);
+            geometry_with_rtree.reattach_bend(bend, maybe_new_inner)
-        let old_weight = self.geometry_with_rtree.geometry().bend_weight(bend);
+        });
-        self.geometry_with_rtree
-            .reattach_bend(bend, maybe_new_inner);
-        let new_joints = self.geometry_with_rtree.geometry().bend_joints(bend);
-        let new_core = self.geometry_with_rtree.geometry().core(bend);
-        let new_weight = self.geometry_with_rtree.geometry().bend_weight(bend);
-
-        if let Some(value) = recorder.bends.get_mut(&bend) {
-            value.1 = Some(((new_joints.0, new_joints.1, new_core), new_weight));
-        } else {
-            recorder.bends.insert(
-                bend,
-                (
-                    Some(((old_joints.0, old_joints.1, old_core), old_weight)),
-                    Some(((new_joints.0, new_joints.1, new_core), new_weight)),
-                ),
-            );
-        }
     }
 
     pub fn compound_weight(&self, compound: GenericIndex<CW>) -> CW {
@@ -382,6 +321,28 @@ impl<
     }
 }
 
+fn edit_remove_from_map<I, T>(
+    map: &mut std::collections::HashMap<I, (Option<T>, Option<T>)>,
+    index: I,
+    data: T,
+) where
+    I: core::cmp::Eq + Hash,
+{
+    let to_be_inserted = (Some(data), None);
+    match map.entry(index) {
+        HashMapEntry::Occupied(mut occ) => {
+            if let (None, Some(_)) = occ.get() {
+                occ.remove();
+            } else {
+                *occ.get_mut() = to_be_inserted;
+            }
+        }
+        HashMapEntry::Vacant(vac) => {
+            vac.insert(to_be_inserted);
+        }
+    }
+}
+
 impl<
         PW: GetWidth + GetLayer + TryInto<DW> + TryInto<SW> + TryInto<BW> + Retag<PI> + Copy,
         DW: AccessDotWeight<PW> + GetLayer,

src/interactor/activity.rs

@@ -137,7 +137,7 @@ impl<M: AccessMesadata> Step<ActivityContext<'_, M>, String> for ActivityStepper
     ) -> Result<ControlFlow<String>, ActivityError> {
         let status = self.activity.step(context)?;
         self.maybe_status = Some(status.clone());
-        Ok(status.into())
+        Ok(status)
     }
 }
 

src/specctra/design.rs

@@ -3,7 +3,7 @@
 //! exporting the session file
 
 use geo::{point, Point, Rotate};
-use std::collections::HashMap;
+use std::collections::{hash_map::Entry as HashMapEntry, HashMap};
 
 use crate::{
     board::{mesadata::AccessMesadata, Board},
@@ -94,20 +94,9 @@ impl SpecctraDesign {
                         // line segments.
                         // TODO: make this configurable? pick a smarter value?
                         let segment_count: usize = 100;
-
+                        bend.render_discretization(segment_count + 1)
-                        let circle = bend.circle();
+                            .map(Into::into)
-                        let angle_from = bend.start_angle();
+                            .collect()
-                        let angle_step = bend.spanned_angle() / segment_count as f64;
-
-                        let mut points = Vec::new();
-                        for i in 0..=segment_count {
-                            let x = circle.pos.x()
-                                + circle.r * (angle_from + i as f64 * angle_step).cos();
-                            let y = circle.pos.y()
-                                + circle.r * (angle_from + i as f64 * angle_step).sin();
-                            points.push(structure::Point { x, y });
-                        }
-                        points
                     }
 
                     // Intentionally skipped for now.
@@ -137,12 +126,9 @@ impl SpecctraDesign {
                     },
                 };
 
-                if let Some(net) = net_outs.get_mut(&net) {
+                let net_out = match net_outs.entry(net) {
-                    net.wire.push(wire);
+                    HashMapEntry::Occupied(occ) => occ.into_mut(),
-                } else {
+                    HashMapEntry::Vacant(vac) => vac.insert(structure::NetOut {
-                    net_outs.insert(
-                        net,
-                        structure::NetOut {
                         name: mesadata
                             .net_netname(net)
                             .ok_or_else(|| {
@@ -152,11 +138,11 @@ impl SpecctraDesign {
                                 )
                             })?
                             .to_owned(),
-                            wire: vec![wire],
+                        wire: Vec::new(),
                         via: Vec::new(),
-                        },
+                    }),
-                    );
+                };
-                }
+                net_out.wire.push(wire);
             }
         }
 
@@ -210,10 +196,10 @@ impl SpecctraDesign {
                     .netname_net(&net_pin_assignments.name)
                     .unwrap();
 
-                net_pin_assignments.pins.as_ref().and_then(|pins| {
+                net_pin_assignments.pins.as_ref().map(|pins| {
                     // take the list of pins
                     // and for each pin output (pin name, net id)
-                    Some(pins.names.iter().map(move |pinname| (pinname.clone(), net)))
+                    pins.names.iter().map(move |pinname| (pinname.clone(), net))
                 })
             })
             // flatten the nested iters into a single stream of tuples

src/triangulation.rs

@@ -17,7 +17,7 @@ pub struct Triangulation<
     EW: Copy + Default,
 > {
     triangulation: DelaunayTriangulation<VW, EW>,
-    trianvertex_to_handle: Vec<Option<FixedVertexHandle>>,
+    trianvertex_to_handle: Box<[Option<FixedVertexHandle>]>,
     index_marker: PhantomData<I>,
 }
 
@@ -28,13 +28,11 @@ impl<
     > Triangulation<I, VW, EW>
 {
     pub fn new(node_bound: usize) -> Self {
-        let mut this = Self {
+        Self {
             triangulation: <DelaunayTriangulation<VW, EW> as spade::Triangulation>::new(),
-            trianvertex_to_handle: Vec::new(),
+            trianvertex_to_handle: vec![None; node_bound].into_boxed_slice(),
             index_marker: PhantomData,
-        };
+        }
-        this.trianvertex_to_handle.resize(node_bound, None);
-        this
     }
 
     pub fn add_vertex(&mut self, weight: VW) -> Result<(), InsertionError> {