From 9e0bdb5bc7eb224cd5ef175f92bc1c8b4a128764 Mon Sep 17 00:00:00 2001 From: Alain Emilia Anna Zscheile Date: Wed, 11 Dec 2024 14:13:21 +0000 Subject: [PATCH] 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 Co-committed-by: Alain Emilia Anna Zscheile --- Cargo.toml | 5 + crates/specctra-core/Cargo.toml | 2 +- crates/specctra-core/src/math.rs | 15 +- crates/specctra-core/src/structure.rs | 17 ++ crates/topola-egui/src/painter.rs | 28 +-- src/autorouter/invoker.rs | 2 +- src/board/mod.rs | 9 +- src/drawing/drawing.rs | 12 +- src/geometry/geometry.rs | 6 +- src/geometry/primitive.rs | 10 ++ src/geometry/recording_with_rtree.rs | 235 +++++++++++--------------- src/interactor/activity.rs | 2 +- src/specctra/design.rs | 60 +++---- src/triangulation.rs | 10 +- 14 files changed, 191 insertions(+), 222 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0bbf235..53c3d2b 100644 --- a/Cargo.toml +++ b/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 diff --git a/crates/specctra-core/Cargo.toml b/crates/specctra-core/Cargo.toml index 161f05e..5f7cd92 100644 --- a/crates/specctra-core/Cargo.toml +++ b/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 diff --git a/crates/specctra-core/src/math.rs b/crates/specctra-core/src/math.rs index e098d2b..0b02743 100644 --- a/crates/specctra-core/src/math.rs +++ b/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; diff --git a/crates/specctra-core/src/structure.rs b/crates/specctra-core/src/structure.rs index 385065e..aaa8f9a 100644 --- a/crates/specctra-core/src/structure.rs +++ b/crates/specctra-core/src/structure.rs @@ -392,6 +392,23 @@ pub struct Point { pub y: f64, } +impl From for Point { + #[inline(always)] + fn from(z: geo_types::Point) -> Self { + Point { x: z.0.x, y: z.0.y } + } +} + +impl From 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 ReadDsn for Vec { fn read_dsn(tokenizer: &mut ListTokenizer) -> Result { diff --git a/crates/topola-egui/src/painter.rs b/crates/topola-egui/src/painter.rs index 87f06f9..4d4368f 100644 --- a/crates/topola-egui/src/painter.rs +++ b/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) => { - let circle = bend.circle(); - - let angle_from = bend.start_angle(); - let angle_step = bend.spanned_angle() / 100.0; - - let mut points: Vec = vec![]; - - 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), - ) - } + PrimitiveShape::Bend(bend) => egui::Shape::line( + bend.render_discretization(101) + .map(|point| { + self.transform + .mul_pos([point.0.x as f32, -point.0.y as f32].into()) + }) + .collect(), + egui::Stroke::new(bend.width as f32 * self.transform.scaling, color), + ), }; self.ui.painter().add(epaint_shape); diff --git a/src/autorouter/invoker.rs b/src/autorouter/invoker.rs index 55d7654..1a218a5 100644 --- a/src/autorouter/invoker.rs +++ b/src/autorouter/invoker.rs @@ -202,6 +202,6 @@ impl Invoker { self.execute(entry.command().clone()); } - self.history.set_undone(undone.into_iter()); + self.history.set_undone(undone); } } diff --git a/src/board/mod.rs b/src/board/mod.rs index d6da0e9..665b837 100644 --- a/src/board/mod.rs +++ b/src/board/mod.rs @@ -239,15 +239,10 @@ impl Board { /// Finds a band between two pin names. pub fn band_between_pins(&self, pinname1: &str, pinname2: &str) -> Option { - if let Some(band) = self - .band_bandname + self.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())) - { - Some(*band) - } else { - None - } + .copied() } /// Returns the mesadata associated with the layout's drawing rules. diff --git a/src/drawing/drawing.rs b/src/drawing/drawing.rs index 96bbe54..97b5ff0 100644 --- a/src/drawing/drawing.rs +++ b/src/drawing/drawing.rs @@ -495,10 +495,9 @@ impl Drawing { 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 Drawing { 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 Drawing { 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 { diff --git a/src/geometry/geometry.rs b/src/geometry/geometry.rs index 95af85e..1876fc3 100644 --- a/src/geometry/geometry.rs +++ b/src/geometry/geometry.rs @@ -197,10 +197,8 @@ impl< } pub(super) fn add_compound_at_index(&mut self, compound: GenericIndex, weight: CW) { - self.graph.update_node( - compound.petgraph_index(), - GenericNode::Compound(weight.into()), - ); + self.graph + .update_node(compound.petgraph_index(), GenericNode::Compound(weight)); } fn init_bend_joints_and_core>( diff --git a/src/geometry/primitive.rs b/src/geometry/primitive.rs index 2c3f324..b04be08 100644 --- a/src/geometry/primitive.rs +++ b/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 + '_ { + 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 { diff --git a/src/geometry/recording_with_rtree.rs b/src/geometry/recording_with_rtree.rs index b950220..78e9263 100644 --- a/src/geometry/recording_with_rtree.rs +++ b/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, compound: GenericIndex, ) { - let old_members = self - .geometry_with_rtree - .geometry() - .compound_members(compound) - .collect(); - let old_weight = self - .geometry_with_rtree - .geometry() - .compound_weight(compound); + let geometry = self.geometry_with_rtree.geometry(); + let old_members = geometry.compound_members(compound).collect(); + let old_weight = geometry.compound_weight(compound); - let new_members = self - .geometry_with_rtree - .geometry() - .compound_members(compound) - .collect(); - let new_weight = self - .geometry_with_rtree - .geometry() - .compound_weight(compound); + // TODO ??? - if let Some(value) = recorder.compounds.get_mut(&compound) { - value.1 = Some((new_members, new_weight)); - } else { - recorder.compounds.insert( - compound, - ( - Some((old_members, old_weight)), - Some((new_members, new_weight)), - ), - ); - } + let geometry = self.geometry_with_rtree.geometry(); + let new_members = geometry.compound_members(compound).collect(); + let new_weight = geometry.compound_weight(compound); + + recorder + .compounds + .entry(compound) + .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)?; - - if let Some((None, Some(..))) = recorder.dots.get(&dot) { - recorder.dots.remove(&dot); - } else { - recorder.dots.insert(dot, (Some(weight), None)); - }; - + edit_remove_from_map(&mut recorder.dots, dot, weight); Ok(()) } @@ -199,15 +178,11 @@ impl< recorder: &mut GeometryEdit, seg: SI, ) { - let weight = self.geometry_with_rtree.geometry().seg_weight(seg); - let joints = self.geometry_with_rtree.geometry().seg_joints(seg); + let geometry = self.geometry_with_rtree.geometry(); + let weight = geometry.seg_weight(seg); + let joints = geometry.seg_joints(seg); self.geometry_with_rtree.remove_seg(seg); - - if let Some((None, Some(..))) = recorder.segs.get(&seg) { - recorder.segs.remove(&seg); - } else { - recorder.segs.insert(seg, (Some((joints, weight)), None)); - } + edit_remove_from_map(&mut recorder.segs, seg, (joints, weight)); } pub fn remove_bend( @@ -215,18 +190,16 @@ impl< recorder: &mut GeometryEdit, bend: BI, ) { - let weight = self.geometry_with_rtree.geometry().bend_weight(bend); - let joints = self.geometry_with_rtree.geometry().bend_joints(bend); - let core = self.geometry_with_rtree.geometry().core(bend); + let geometry = self.geometry_with_rtree.geometry(); + let weight = geometry.bend_weight(bend); + let joints = geometry.bend_joints(bend); + let core = geometry.core(bend); self.geometry_with_rtree.remove_bend(bend); - - if let Some((None, Some(..))) = recorder.bends.get(&bend) { - recorder.bends.remove(&bend); - } else { - recorder - .bends - .insert(bend, (Some(((joints.0, joints.1, core), weight)), None)); - } + edit_remove_from_map( + &mut recorder.bends, + bend, + ((joints.0, joints.1, core), weight), + ); } pub fn remove_compound( @@ -234,24 +207,11 @@ impl< recorder: &mut GeometryEdit, compound: GenericIndex, ) { - let weight = self - .geometry_with_rtree - .geometry() - .compound_weight(compound); - let members = self - .geometry_with_rtree - .geometry() - .compound_members(compound) - .collect(); + let geometry = self.geometry_with_rtree.geometry(); + let weight = geometry.compound_weight(compound); + let members = geometry.compound_members(compound).collect(); self.geometry_with_rtree.remove_compound(compound); - - if let Some((None, Some(..))) = recorder.compounds.get(&compound) { - recorder.compounds.remove(&compound); - } else { - recorder - .compounds - .insert(compound, (Some((members, weight)), None)); - } + edit_remove_from_map(&mut recorder.compounds, compound, (members, weight)); } 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))); - } + recorder + .dots + .entry(dot) + .or_insert((Some(old_weight), None)) + .1 = Some(new_weight); + } + + fn modify_bend( + &mut self, + recorder: &mut GeometryEdit, + bend: BI, + f: F, + ) where + F: FnOnce(&mut GeometryWithRtree, 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); - let old_core = self.geometry_with_rtree.geometry().core(bend); - 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)), - ), - ); - } + self.modify_bend(recorder, bend, |geometry_with_rtree, bend| { + geometry_with_rtree.shift_bend(bend, offset) + }); } pub fn flip_bend( @@ -305,25 +277,9 @@ impl< recorder: &mut GeometryEdit, bend: BI, ) { - let old_joints = self.geometry_with_rtree.geometry().bend_joints(bend); - let old_core = self.geometry_with_rtree.geometry().core(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)), - ), - ); - } + self.modify_bend(recorder, bend, |geometry_with_rtree, bend| { + geometry_with_rtree.flip_bend(bend) + }); } pub fn reattach_bend( @@ -332,26 +288,9 @@ impl< bend: BI, maybe_new_inner: Option, ) { - let old_joints = self.geometry_with_rtree.geometry().bend_joints(bend); - let old_core = self.geometry_with_rtree.geometry().core(bend); - 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)), - ), - ); - } + self.modify_bend(recorder, bend, |geometry_with_rtree, bend| { + geometry_with_rtree.reattach_bend(bend, maybe_new_inner) + }); } pub fn compound_weight(&self, compound: GenericIndex) -> CW { @@ -382,6 +321,28 @@ impl< } } +fn edit_remove_from_map( + map: &mut std::collections::HashMap, Option)>, + 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 + TryInto + TryInto + Retag + Copy, DW: AccessDotWeight + GetLayer, diff --git a/src/interactor/activity.rs b/src/interactor/activity.rs index a29008c..63b5176 100644 --- a/src/interactor/activity.rs +++ b/src/interactor/activity.rs @@ -137,7 +137,7 @@ impl Step, String> for ActivityStepper ) -> Result, ActivityError> { let status = self.activity.step(context)?; self.maybe_status = Some(status.clone()); - Ok(status.into()) + Ok(status) } } diff --git a/src/specctra/design.rs b/src/specctra/design.rs index fc4cf5e..b899aef 100644 --- a/src/specctra/design.rs +++ b/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; - - let circle = bend.circle(); - let angle_from = bend.start_angle(); - 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 + bend.render_discretization(segment_count + 1) + .map(Into::into) + .collect() } // Intentionally skipped for now. @@ -137,26 +126,23 @@ impl SpecctraDesign { }, }; - if let Some(net) = net_outs.get_mut(&net) { - net.wire.push(wire); - } else { - net_outs.insert( - net, - structure::NetOut { - name: mesadata - .net_netname(net) - .ok_or_else(|| { - std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!("tried to reference invalid net ID {}", net), - ) - })? - .to_owned(), - wire: vec![wire], - via: Vec::new(), - }, - ); - } + let net_out = match net_outs.entry(net) { + HashMapEntry::Occupied(occ) => occ.into_mut(), + HashMapEntry::Vacant(vac) => vac.insert(structure::NetOut { + name: mesadata + .net_netname(net) + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!("tried to reference invalid net ID {}", net), + ) + })? + .to_owned(), + 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 diff --git a/src/triangulation.rs b/src/triangulation.rs index 39e6776..3160caf 100644 --- a/src/triangulation.rs +++ b/src/triangulation.rs @@ -17,7 +17,7 @@ pub struct Triangulation< EW: Copy + Default, > { triangulation: DelaunayTriangulation, - trianvertex_to_handle: Vec>, + trianvertex_to_handle: Box<[Option]>, index_marker: PhantomData, } @@ -28,13 +28,11 @@ impl< > Triangulation { pub fn new(node_bound: usize) -> Self { - let mut this = Self { + Self { triangulation: 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> {