From f7f2e8ea4962a0722a11a912002946b9e5b8f08d Mon Sep 17 00:00:00 2001 From: Mikolaj Wielgus Date: Fri, 30 Aug 2024 16:32:37 +0200 Subject: [PATCH] autorouter: if band removal fails, propagate error instead of panicking --- src/autorouter/autorouter.rs | 22 ++++++++++++++++++---- src/autorouter/invoker.rs | 16 +++++++++++----- src/bin/topola-egui/app.rs | 4 +--- src/drawing/drawing.rs | 24 +++++++++++++----------- src/layout/layout.rs | 8 ++++---- src/router/draw.rs | 12 ++++++------ src/router/router.rs | 12 +++++++----- 7 files changed, 60 insertions(+), 38 deletions(-) diff --git a/src/autorouter/autorouter.rs b/src/autorouter/autorouter.rs index b736ada..2bcfee0 100644 --- a/src/autorouter/autorouter.rs +++ b/src/autorouter/autorouter.rs @@ -4,7 +4,11 @@ use thiserror::Error; use crate::{ board::{mesadata::AccessMesadata, Board}, - drawing::{dot::FixedDotIndex, Infringement}, + drawing::{ + band::{BandTermsegIndex, BandUid}, + dot::FixedDotIndex, + Infringement, + }, layout::via::ViaWeight, router::{navmesh::NavmeshError, RouterError}, triangulation::GetTrianvertexNodeIndex, @@ -30,6 +34,8 @@ pub enum AutorouterError { Router(#[from] RouterError), #[error("could not place via")] CouldNotPlaceVia(#[from] Infringement), + #[error("could not remove band")] + CouldNotRemoveBand(BandTermsegIndex), #[error("need exactly two ratlines")] NeedExactlyTwoRatlines, } @@ -56,11 +62,14 @@ impl Autorouter { Autoroute::new(self, ratlines) } - pub fn undo_autoroute(&mut self, selection: &PinSelection) { + pub fn undo_autoroute(&mut self, selection: &PinSelection) -> Result<(), AutorouterError> { self.undo_autoroute_ratlines(self.selected_ratlines(selection)) } - pub(super) fn undo_autoroute_ratlines(&mut self, ratlines: Vec>) { + pub(super) fn undo_autoroute_ratlines( + &mut self, + ratlines: Vec>, + ) -> Result<(), AutorouterError> { for ratline in ratlines.iter() { let band = self .ratsnest @@ -69,8 +78,13 @@ impl Autorouter { .unwrap() .band_termseg .unwrap(); - self.board.layout_mut().remove_band(band); + self.board + .layout_mut() + .remove_band(band) + .map_err(|_| AutorouterError::CouldNotRemoveBand(band))?; } + + Ok(()) } pub fn place_via(&self, weight: ViaWeight) -> Result { diff --git a/src/autorouter/invoker.rs b/src/autorouter/invoker.rs index 30b7834..20e3d1a 100644 --- a/src/autorouter/invoker.rs +++ b/src/autorouter/invoker.rs @@ -289,11 +289,17 @@ impl Invoker { let command = self.history.last_done()?; match command { - Command::Autoroute(ref selection) => self.autorouter.undo_autoroute(selection), - Command::PlaceVia(weight) => self.autorouter.undo_place_via(*weight), - Command::RemoveBands(ref selection) => self.autorouter.undo_remove_bands(selection), - Command::CompareDetours(..) => (), - Command::MeasureLength(..) => (), + Command::Autoroute(ref selection) => { + self.autorouter.undo_autoroute(selection); + } + Command::PlaceVia(weight) => { + self.autorouter.undo_place_via(*weight); + } + Command::RemoveBands(ref selection) => { + self.autorouter.undo_remove_bands(selection); + } + Command::CompareDetours(..) => {} + Command::MeasureLength(..) => {} } Ok::<(), InvokerError>(self.history.undo()?) diff --git a/src/bin/topola-egui/app.rs b/src/bin/topola-egui/app.rs index 6756b99..1b340c5 100644 --- a/src/bin/topola-egui/app.rs +++ b/src/bin/topola-egui/app.rs @@ -22,7 +22,7 @@ use topola::{ graph::{MakePrimitive, PrimitiveIndex}, primitive::MakePrimitiveShape, rules::AccessRules, - Drawing, Infringement, LayoutException, + Drawing, DrawingException, Infringement, }, geometry::{ compound::ManageCompounds, @@ -152,8 +152,6 @@ impl App { invoker.replay(serde_json::from_reader(bufread).unwrap()) } - // TODO: Make Save History work too. - if let Some(ref mut execute) = self.maybe_execute { let status = match execute.step(invoker) { Ok(status) => status, diff --git a/src/drawing/drawing.rs b/src/drawing/drawing.rs index f2317a9..c1bdbc1 100644 --- a/src/drawing/drawing.rs +++ b/src/drawing/drawing.rs @@ -42,7 +42,7 @@ use super::head::{Head, HeadRef}; #[enum_dispatch] #[derive(Error, Debug, Clone, Copy)] -pub enum LayoutException { +pub enum DrawingException { #[error(transparent)] NoTangents(#[from] NoTangents), #[error(transparent)] @@ -91,7 +91,7 @@ impl Drawing { } } - pub fn remove_band(&mut self, band: BandTermsegIndex) { + pub fn remove_band(&mut self, band: BandTermsegIndex) -> Result<(), DrawingException> { match band { BandTermsegIndex::Straight(seg) => { self.geometry_with_rtree.remove_seg(seg.into()); @@ -148,10 +148,12 @@ impl Drawing { } for outer in outers { - self.update_this_and_outward_bows(outer).unwrap(); // Must never fail. + self.update_this_and_outward_bows(outer)?; } } } + + Ok(()) } #[debug_ensures(ret.is_ok() -> self.geometry_with_rtree.graph().node_count() == old(self.geometry_with_rtree.graph().node_count() + 1))] @@ -272,7 +274,7 @@ impl Drawing { around: GearIndex, weight: LooseBendWeight, infringables: Option<&[PrimitiveIndex]>, - ) -> Result { + ) -> Result { // It makes no sense to wrap something around or under one of its connectables. // if let Some(net) = weight.maybe_net { @@ -399,7 +401,7 @@ impl Drawing { seg_weight: SeqLooseSegWeight, bend_weight: LooseBendWeight, cw: bool, - ) -> Result { + ) -> Result { let maybe_next_gear = around.ref_(self).next_gear(); let cane = self.add_cane_with_infringables( from, @@ -430,7 +432,7 @@ impl Drawing { return Err(collision.into()); } - Ok::(cane) + Ok::(cane) } #[debug_ensures(self.geometry_with_rtree.graph().node_count() == old(self.geometry_with_rtree.graph().node_count()))] @@ -438,7 +440,7 @@ impl Drawing { fn update_this_and_outward_bows( &mut self, around: LooseBendIndex, - ) -> Result<(), LayoutException> { + ) -> Result<(), DrawingException> { // FIXME: Fail gracefully on infringement. let mut maybe_rail = Some(around); @@ -536,7 +538,7 @@ impl Drawing { maybe_rail = self.primitive(rail).outer(); } - Ok::<(), LayoutException>(()) + Ok::<(), DrawingException>(()) } #[debug_ensures(ret.is_ok() -> self.geometry_with_rtree.graph().node_count() == old(self.geometry_with_rtree.graph().node_count() + 4))] @@ -551,7 +553,7 @@ impl Drawing { seg_weight: SeqLooseSegWeight, bend_weight: LooseBendWeight, cw: bool, - ) -> Result { + ) -> Result { self.add_cane_with_infringables( from, around, @@ -576,7 +578,7 @@ impl Drawing { bend_weight: LooseBendWeight, cw: bool, infringables: Option<&[PrimitiveIndex]>, - ) -> Result { + ) -> Result { let seg_to = self.add_dot_with_infringables(dot_weight, infringables)?; let seg = self .add_seg_with_infringables(from, seg_to.into(), seg_weight, infringables) @@ -604,7 +606,7 @@ impl Drawing { err })?; - Ok::(Cane { + Ok::(Cane { seg, dot: seg_to, bend, diff --git a/src/layout/layout.rs b/src/layout/layout.rs index b76b063..8520770 100644 --- a/src/layout/layout.rs +++ b/src/layout/layout.rs @@ -17,7 +17,7 @@ use crate::{ FixedSegIndex, FixedSegWeight, LoneLooseSegIndex, LoneLooseSegWeight, SeqLooseSegIndex, SeqLooseSegWeight, }, - Drawing, Infringement, LayoutException, + Drawing, DrawingException, Infringement, }, geometry::{compound::ManageCompounds, shape::MeasureLength, GenericNode}, graph::{GenericIndex, GetPetgraphIndex}, @@ -54,7 +54,7 @@ impl Layout { seg_weight: SeqLooseSegWeight, bend_weight: LooseBendWeight, cw: bool, - ) -> Result { + ) -> Result { self.drawing .insert_cane(from, around, dot_weight, seg_weight, bend_weight, cw) } @@ -204,8 +204,8 @@ impl Layout { ) } - pub fn remove_band(&mut self, band: BandTermsegIndex) { - self.drawing.remove_band(band); + pub fn remove_band(&mut self, band: BandTermsegIndex) -> Result<(), DrawingException> { + self.drawing.remove_band(band) } pub fn polys( diff --git a/src/router/draw.rs b/src/router/draw.rs index c0a8cbc..8e39ed0 100644 --- a/src/router/draw.rs +++ b/src/router/draw.rs @@ -14,7 +14,7 @@ use crate::{ primitive::GetOtherJoint, rules::AccessRules, seg::{LoneLooseSegWeight, SeqLooseSegWeight}, - Infringement, LayoutException, + DrawingException, Infringement, }, layout::Layout, math::{Circle, NoTangents}, @@ -26,9 +26,9 @@ pub enum DrawException { NoTangents(#[from] NoTangents), // TODO add real error messages + these should eventually use Display #[error("cannot finish in {0:?}")] - CannotFinishIn(FixedDotIndex, #[source] LayoutException), + CannotFinishIn(FixedDotIndex, #[source] DrawingException), #[error("cannot wrap around {0:?}")] - CannotWrapAround(GearIndex, #[source] LayoutException), + CannotWrapAround(GearIndex, #[source] DrawingException), } pub struct Draw<'a, R: AccessRules> { @@ -158,7 +158,7 @@ impl<'a, R: AccessRules> Draw<'a, R> { cw: bool, width: f64, offset: f64, - ) -> Result { + ) -> Result { let head = self.extend_head(head, from)?; self.cane(head, around, to, cw, width, offset) } @@ -183,7 +183,7 @@ impl<'a, R: AccessRules> Draw<'a, R> { cw: bool, width: f64, offset: f64, - ) -> Result { + ) -> Result { let layer = head.face().primitive(self.layout.drawing()).layer(); let maybe_net = head.face().primitive(self.layout.drawing()).maybe_net(); let cane = self.layout.insert_cane( @@ -210,7 +210,7 @@ impl<'a, R: AccessRules> Draw<'a, R> { }, cw, )?; - Ok::(CaneHead { + Ok::(CaneHead { face: self .layout .drawing() diff --git a/src/router/router.rs b/src/router/router.rs index 1d25cd6..ce0ba0a 100644 --- a/src/router/router.rs +++ b/src/router/router.rs @@ -12,7 +12,7 @@ use crate::{ head::GetFace, primitive::MakePrimitiveShape, rules::AccessRules, - Collision, Infringement, LayoutException, + Collision, DrawingException, Infringement, }, geometry::{ primitive::{AccessPrimitiveShape, PrimitiveShape}, @@ -140,12 +140,14 @@ impl<'a, R: AccessRules> AstarStrategy }; let (ghost, obstacle) = match layout_err { - LayoutException::NoTangents(..) => return None, - LayoutException::Infringement(Infringement(ghost, obstacle)) => { + DrawingException::NoTangents(..) => return None, + DrawingException::Infringement(Infringement(ghost, obstacle)) => { (ghost, obstacle) } - LayoutException::Collision(Collision(ghost, obstacle)) => (ghost, obstacle), - LayoutException::AlreadyConnected(..) => return None, + DrawingException::Collision(Collision(ghost, obstacle)) => { + (ghost, obstacle) + } + DrawingException::AlreadyConnected(..) => return None, }; self.probe_ghosts = vec![ghost];