From 1d8458a5888e1d030789d2854fc26150374571cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20H=C3=BCgel?= Date: Mon, 17 May 2021 22:10:25 +0100 Subject: [PATCH 1/2] Remove panic! instances --- rstar-benches/benches/benchmarks.rs | 4 ++-- rstar-demo/src/main.rs | 15 +++++++------ rstar/src/algorithm/iterators.rs | 2 +- rstar/src/algorithm/removal.rs | 4 ++-- rstar/src/algorithm/rstar.rs | 33 +++++++++++++++-------------- rstar/src/params.rs | 4 +++- rstar/src/rtree.rs | 13 +++++++----- 7 files changed, 42 insertions(+), 33 deletions(-) diff --git a/rstar-benches/benches/benchmarks.rs b/rstar-benches/benches/benchmarks.rs index 98f7021..848432f 100644 --- a/rstar-benches/benches/benchmarks.rs +++ b/rstar-benches/benches/benchmarks.rs @@ -54,7 +54,7 @@ fn bulk_load_comparison(c: &mut Criterion) { b.iter(move || { let mut rtree = rstar::RTree::new(); for point in &points { - rtree.insert(*point); + rtree.insert(*point).unwrap(); } }); }); @@ -67,7 +67,7 @@ fn tree_creation_quality(c: &mut Criterion) { let tree_bulk_loaded = RTree::<_, Params>::bulk_load_with_params(points.clone()); let mut tree_sequential = RTree::new(); for point in &points { - tree_sequential.insert(*point); + tree_sequential.insert(*point).unwrap(); } let query_points = create_random_points(100, SEED_2); diff --git a/rstar-demo/src/main.rs b/rstar-demo/src/main.rs index 06fd01f..68f8911 100644 --- a/rstar-demo/src/main.rs +++ b/rstar-demo/src/main.rs @@ -188,7 +188,7 @@ fn handle_input(window: &Window, scene: &mut Scene) -> Option { &Vector2::new(width as f32, height as f32), ); - scene.tree_2d.insert(unprojected.coords.into()); + scene.tree_2d.insert(unprojected.coords.into()).unwrap(); is_dirty = true; } WindowEvent::CursorPos(x, y, _) if scene.render_mode == RenderMode::TwoD => { @@ -201,12 +201,15 @@ fn handle_input(window: &Window, scene: &mut Scene) -> Option { if is_dirty { for point in points_to_add { if scene.render_mode == RenderMode::ThreeD { - scene.tree_3d.insert(point); + scene.tree_3d.insert(point).unwrap(); } else { - scene.tree_2d.insert([ - point[0] * window.width() as f32 * 0.5, - point[1] * window.height() as f32 * 0.5, - ]); + scene + .tree_2d + .insert([ + point[0] * window.width() as f32 * 0.5, + point[1] * window.height() as f32 * 0.5, + ]) + .unwrap(); } } create_render_data_from_scene(scene).into() diff --git a/rstar/src/algorithm/iterators.rs b/rstar/src/algorithm/iterators.rs index c664b90..4f84f95 100644 --- a/rstar/src/algorithm/iterators.rs +++ b/rstar/src/algorithm/iterators.rs @@ -240,7 +240,7 @@ mod test { let points = create_random_points(NUM_POINTS, SEED_1); let mut tree = RTree::new(); for p in &points { - tree.insert(*p); + tree.insert(*p).unwrap(); } let mut count = 0usize; for p in tree.iter() { diff --git a/rstar/src/algorithm/removal.rs b/rstar/src/algorithm/removal.rs index 3ec727e..531f677 100644 --- a/rstar/src/algorithm/removal.rs +++ b/rstar/src/algorithm/removal.rs @@ -76,7 +76,7 @@ mod test { let mut tree = RTree::bulk_load(points.clone()); for (point_to_remove, point_to_add) in points.iter().zip(later_insertions.iter()) { assert!(tree.remove_at_point(point_to_remove).is_some()); - tree.insert(*point_to_add); + tree.insert(*point_to_add).unwrap(); } assert_eq!(tree.size(), SIZE); assert!(points.iter().all(|p| !tree.contains(p))); @@ -98,7 +98,7 @@ mod test { initial_rectangles.iter().zip(new_rectangles.iter()) { assert!(tree.remove(rectangle_to_remove).is_some()); - tree.insert(*rectangle_to_add); + tree.insert(*rectangle_to_add).unwrap(); } assert_eq!(tree.size(), SIZE); assert!(initial_rectangles.iter().all(|p| !tree.contains(p))); diff --git a/rstar/src/algorithm/rstar.rs b/rstar/src/algorithm/rstar.rs index 0dfb35f..528203e 100644 --- a/rstar/src/algorithm/rstar.rs +++ b/rstar/src/algorithm/rstar.rs @@ -1,3 +1,5 @@ +use std::error; + use crate::envelope::Envelope; use crate::node::{envelope_for_children, ParentNode, RTreeNode}; use crate::object::RTreeObject; @@ -26,7 +28,7 @@ where } impl InsertionStrategy for RStarInsertionStrategy { - fn insert(tree: &mut RTree, t: T) + fn insert(tree: &mut RTree, t: T) -> Result<(), Box> where Params: RTreeParams, T: RTreeObject, @@ -42,15 +44,16 @@ impl InsertionStrategy for RStarInsertionStrategy { let mut target_height = 0; let mut insertion_stack = Vec::new(); match first { - InsertionResult::Split(node) => insertion_stack.push(PerformSplit(node)), - InsertionResult::Reinsert(nodes_to_reinsert, real_target_height) => { + Ok(InsertionResult::Split(node)) => insertion_stack.push(PerformSplit(node)), + Ok(InsertionResult::Reinsert(nodes_to_reinsert, real_target_height)) => { insertion_stack.extend(nodes_to_reinsert.into_iter().map(PerformReinsert)); target_height = real_target_height; } - InsertionResult::Complete => {} + Ok(InsertionResult::Complete) => {} + _ => (), }; - while let Some(next) = insertion_stack.pop() { + Ok(while let Some(next) = insertion_stack.pop() { match next { PerformSplit(node) => { // The root node was split, create a new root and increase height @@ -67,14 +70,12 @@ impl InsertionStrategy for RStarInsertionStrategy { let root = tree.root_mut(); match forced_insertion::(root, node_to_reinsert, target_height) { InsertionResult::Split(node) => insertion_stack.push(PerformSplit(node)), - InsertionResult::Reinsert(_, _) => { - panic!("Unexpected reinsert. This is a bug in rstar.") - } + InsertionResult::Reinsert(_, _) => (), InsertionResult::Complete => {} } } } - } + }) } } @@ -114,7 +115,7 @@ fn recursive_insert( node: &mut ParentNode, t: RTreeNode, current_height: usize, -) -> InsertionResult +) -> Result, Box> where T: RTreeObject, Params: RTreeParams, @@ -125,24 +126,24 @@ where if node.children.len() < expand_index { // Force insertion into this node node.children.push(t); - return resolve_overflow::<_, Params>(node, current_height); + return Ok(resolve_overflow::<_, Params>(node, current_height)); } let expand = if let RTreeNode::Parent(ref mut follow) = node.children[expand_index] { recursive_insert::<_, Params>(follow, t, current_height + 1) } else { - panic!("This is a bug in rstar.") + return Err("Something has gone badly wrong while attempting to insert a value".into()); }; match expand { - InsertionResult::Split(child) => { + Ok(InsertionResult::Split(child)) => { node.envelope.merge(&child.envelope()); node.children.push(child); - resolve_overflow::<_, Params>(node, current_height) + Ok(resolve_overflow::<_, Params>(node, current_height)) } - InsertionResult::Reinsert(a, b) => { + Ok(InsertionResult::Reinsert(a, b)) => { node.envelope = envelope_for_children(&node.children); - InsertionResult::Reinsert(a, b) + Ok(InsertionResult::Reinsert(a, b)) } other => other, } diff --git a/rstar/src/params.rs b/rstar/src/params.rs index b1c51eb..181856e 100644 --- a/rstar/src/params.rs +++ b/rstar/src/params.rs @@ -1,3 +1,5 @@ +use std::error; + use crate::algorithm::rstar::RStarInsertionStrategy; use crate::{Envelope, Point, RTree, RTreeObject}; @@ -79,7 +81,7 @@ impl RTreeParams for DefaultParams { /// This trait is not meant to be implemented by the user. pub trait InsertionStrategy { #[doc(hidden)] - fn insert(tree: &mut RTree, t: T) + fn insert(tree: &mut RTree, t: T) -> Result<(), Box> where Params: RTreeParams, T: RTreeObject; diff --git a/rstar/src/rtree.rs b/rstar/src/rtree.rs index bc988ce..32faa86 100644 --- a/rstar/src/rtree.rs +++ b/rstar/src/rtree.rs @@ -1,3 +1,5 @@ +use std::error; + use crate::algorithm::bulk_load; use crate::algorithm::intersection_iterator::IntersectionIterator; use crate::algorithm::iterators::*; @@ -742,9 +744,10 @@ where /// This method runs in `O(log(n))`. /// The [r-tree documentation](RTree) contains more information about /// r-tree performance. - pub fn insert(&mut self, t: T) { - Params::DefaultInsertionStrategy::insert(self, t); + pub fn insert(&mut self, t: T) -> Result<(), Box> { + Params::DefaultInsertionStrategy::insert(self, t)?; self.size += 1; + Ok(()) } } @@ -807,7 +810,7 @@ mod test { #[test] fn test_insert_single() { let mut tree: RTree<_> = RTree::new(); - tree.insert([0.02f32, 0.4f32]); + tree.insert([0.02f32, 0.4f32]).unwrap(); assert_eq!(tree.size(), 1); assert!(tree.contains(&[0.02, 0.4])); assert!(!tree.contains(&[0.3, 0.2])); @@ -819,7 +822,7 @@ mod test { let points = create_random_points(NUM_POINTS, SEED_1); let mut tree = RTree::new(); for p in &points { - tree.insert(*p); + tree.insert(*p).unwrap(); tree.root.sanity_check::(true); } assert_eq!(tree.size(), NUM_POINTS); @@ -920,7 +923,7 @@ mod test { for node in nodes { // Bulk loading will create nodes larger than Params::MAX_SIZE, // which is intentional and not harmful. - tree.insert(node); + tree.insert(node).unwrap(); tree.root().sanity_check::(false); } } From 939adf159970cf3ec50b3cdb711d38143afed230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20H=C3=BCgel?= Date: Mon, 17 May 2021 22:21:17 +0100 Subject: [PATCH 2/2] Actually return a reinsertion error when appropriate --- rstar/src/algorithm/rstar.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rstar/src/algorithm/rstar.rs b/rstar/src/algorithm/rstar.rs index 528203e..2e5e65e 100644 --- a/rstar/src/algorithm/rstar.rs +++ b/rstar/src/algorithm/rstar.rs @@ -70,7 +70,9 @@ impl InsertionStrategy for RStarInsertionStrategy { let root = tree.root_mut(); match forced_insertion::(root, node_to_reinsert, target_height) { InsertionResult::Split(node) => insertion_stack.push(PerformSplit(node)), - InsertionResult::Reinsert(_, _) => (), + InsertionResult::Reinsert(_, _) => { + Err("Unexpected reinsert. This is a bug in rstar.")? + } InsertionResult::Complete => {} } }