From 97e9b44032f6605d9d8e18b4e0445327c19c7daf Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Tue, 20 Oct 2020 10:51:43 -0700 Subject: [PATCH 1/8] is_closed for multiline string --- geo-types/src/multi_line_string.rs | 35 ++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/geo-types/src/multi_line_string.rs b/geo-types/src/multi_line_string.rs index 0243ef777..fc44f2f34 100644 --- a/geo-types/src/multi_line_string.rs +++ b/geo-types/src/multi_line_string.rs @@ -34,6 +34,41 @@ pub struct MultiLineString(pub Vec>) where T: CoordinateType; +impl MultiLineString { + /// True if the MultiLineString is non-empty and each of its LineStrings is closed - that is, + /// that the first and last coordinates of each line string are the same. + /// + /// # Examples + /// + /// ``` + /// use geo_types::{MultiLineString, LineString, line_string}; + /// + /// let open_line_string: LineString = line_string![(x: 0., y: 0.), (x: 5., y: 0.)]; + /// assert!(!MultiLineString(vec![open_line_string.clone()]).is_closed()); + /// + /// let closed_line_string: LineString = line_string![(x: 0., y: 0.), (x: 5., y: 0.), (x: 0., y: 0.)]; + /// assert!(MultiLineString(vec![closed_line_string.clone()]).is_closed()); + /// + /// // MultiLineString is not closed if *any* of it's LineStrings are not closed + /// assert!(!MultiLineString(vec![open_line_string, closed_line_string]).is_closed()); + /// + /// // An empty MultiLineString is not closed + /// assert!(!MultiLineString::(vec![]).is_closed()); + /// + /// // Because an empty LineString is not closed, a MultiLineString containing an empty + /// // LineString cannot be closed. + /// let empty_line_string: LineString = line_string![]; + /// assert!(!MultiLineString(vec![empty_line_string]).is_closed()); + /// ``` + pub fn is_closed(&self) -> bool { + if self.0.is_empty() { + return false; + } + + self.0.iter().all(LineString::is_closed) + } +} + impl>> From for MultiLineString { fn from(ls: ILS) -> Self { MultiLineString(vec![ls.into()]) From 253cbd0fb9674ab356fc3f58f724251dc195d370 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Tue, 20 Oct 2020 12:17:56 -0700 Subject: [PATCH 2/8] fix is_closed for empty LineString --- geo-types/src/line_string.rs | 14 ++++++++++---- geo/src/algorithm/contains/polygon.rs | 6 ++++++ geo/src/utils.rs | 2 +- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/geo-types/src/line_string.rs b/geo-types/src/line_string.rs index 746eb86de..8f7aeef12 100644 --- a/geo-types/src/line_string.rs +++ b/geo-types/src/line_string.rs @@ -178,12 +178,14 @@ impl LineString { }) } - /// Close the `LineString`. Specifically, if the `LineString` has is at least one coordinate, - /// and the value of the first coordinate does not equal the value of the last coordinate, then - /// a new coordinate is added to the end with the value of the first coordinate. + /// Close the `LineString`. Specifically, if the `LineString` has at least one coordinate, and + /// the value of the first coordinate does not equal the value of the last coordinate, then a + /// new coordinate is added to the end with the value of the first coordinate. pub fn close(&mut self) { if !self.is_closed() { - self.0.push(self.0[0]); + if self.0.len() > 0 { + self.0.push(self.0[0]); + } } } @@ -216,6 +218,10 @@ impl LineString { /// assert!(line_string.is_closed()); /// ``` pub fn is_closed(&self) -> bool { + if self.0.is_empty() { + return false; + } + self.0.first() == self.0.last() } } diff --git a/geo/src/algorithm/contains/polygon.rs b/geo/src/algorithm/contains/polygon.rs index b0abd6ab6..db14b403c 100644 --- a/geo/src/algorithm/contains/polygon.rs +++ b/geo/src/algorithm/contains/polygon.rs @@ -12,6 +12,12 @@ where T: HasKernel, { fn contains(&self, coord: &Coordinate) -> bool { + // Officially, we require the exterior to be a valid ring, but this isn't curently enforced + // by the Polygon contructor. We bail out here rather than trigger later debug asserts. + if self.exterior().0.is_empty() { + return false; + } + match utils::coord_pos_relative_to_ring(*coord, &self.exterior()) { utils::CoordPos::OnBoundary | utils::CoordPos::Outside => false, _ => self.interiors().iter().all(|ls| { diff --git a/geo/src/utils.rs b/geo/src/utils.rs index 62bca87e4..0b78245ca 100644 --- a/geo/src/utils.rs +++ b/geo/src/utils.rs @@ -98,7 +98,7 @@ where // // See: https://en.wikipedia.org/wiki/Point_in_polygon - assert!(linestring.is_closed()); + debug_assert!(linestring.is_closed()); // LineString without points if linestring.0.is_empty() { From e7dfb7fe9b3b76e0bc34789c296dd22a7f0391ac Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Tue, 20 Oct 2020 13:03:58 -0700 Subject: [PATCH 3/8] update changelog --- geo-types/CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/geo-types/CHANGES.md b/geo-types/CHANGES.md index 9e423f780..40a2870ea 100644 --- a/geo-types/CHANGES.md +++ b/geo-types/CHANGES.md @@ -3,6 +3,8 @@ ## 0.6.2 * `Rect::new` automatically determines min/max points +* Add `MultiLineString::is_closed` method +* Fixed `LineString::is_closed` to return false when empty ## 0.6.1 From a36b7dba181019b6ac40bdfbd194a6d4992f9e3b Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 21 Oct 2020 09:44:16 -0700 Subject: [PATCH 4/8] partial revert of 253cbd0f and document decision to treat LineString#is_closed like a LinearRing. --- geo-types/CHANGES.md | 1 - geo-types/src/line_string.rs | 20 +++++++++++++------- geo-types/src/multi_line_string.rs | 5 ----- geo/src/algorithm/contains/polygon.rs | 6 ------ 4 files changed, 13 insertions(+), 19 deletions(-) diff --git a/geo-types/CHANGES.md b/geo-types/CHANGES.md index 40a2870ea..137c96984 100644 --- a/geo-types/CHANGES.md +++ b/geo-types/CHANGES.md @@ -4,7 +4,6 @@ * `Rect::new` automatically determines min/max points * Add `MultiLineString::is_closed` method -* Fixed `LineString::is_closed` to return false when empty ## 0.6.1 diff --git a/geo-types/src/line_string.rs b/geo-types/src/line_string.rs index 8f7aeef12..c0a389427 100644 --- a/geo-types/src/line_string.rs +++ b/geo-types/src/line_string.rs @@ -183,9 +183,9 @@ impl LineString { /// new coordinate is added to the end with the value of the first coordinate. pub fn close(&mut self) { if !self.is_closed() { - if self.0.len() > 0 { - self.0.push(self.0[0]); - } + // by definition, we treat empty LineString's as closed. + debug_assert!(self.0.len() > 0); + self.0.push(self.0[0]); } } @@ -217,11 +217,17 @@ impl LineString { /// let line_string: LineString = coords.into_iter().collect(); /// assert!(line_string.is_closed()); /// ``` + /// + /// Note that we diverge from some libraries (JTS et al), which have a LinearRing type, + /// separate from LineString. Those libraries treat an empty LinearRing as closed, by + /// definition, while treating an empty LineString as open. Since we don't have a separate + /// LinearRing type, and use a LineString in its place, we adopt the JTS LinearRing `is_closed` + /// behavior in all places, that is, we consider an empty LineString as closed. + /// + /// This is expected when used in the context of a Polygon.exterior and elswhere; And there + /// seems to be no reason to maintain the separate behavior for LineStrings used in + /// non-LinearRing contexts. pub fn is_closed(&self) -> bool { - if self.0.is_empty() { - return false; - } - self.0.first() == self.0.last() } } diff --git a/geo-types/src/multi_line_string.rs b/geo-types/src/multi_line_string.rs index fc44f2f34..6b50f1014 100644 --- a/geo-types/src/multi_line_string.rs +++ b/geo-types/src/multi_line_string.rs @@ -54,11 +54,6 @@ impl MultiLineString { /// /// // An empty MultiLineString is not closed /// assert!(!MultiLineString::(vec![]).is_closed()); - /// - /// // Because an empty LineString is not closed, a MultiLineString containing an empty - /// // LineString cannot be closed. - /// let empty_line_string: LineString = line_string![]; - /// assert!(!MultiLineString(vec![empty_line_string]).is_closed()); /// ``` pub fn is_closed(&self) -> bool { if self.0.is_empty() { diff --git a/geo/src/algorithm/contains/polygon.rs b/geo/src/algorithm/contains/polygon.rs index db14b403c..b0abd6ab6 100644 --- a/geo/src/algorithm/contains/polygon.rs +++ b/geo/src/algorithm/contains/polygon.rs @@ -12,12 +12,6 @@ where T: HasKernel, { fn contains(&self, coord: &Coordinate) -> bool { - // Officially, we require the exterior to be a valid ring, but this isn't curently enforced - // by the Polygon contructor. We bail out here rather than trigger later debug asserts. - if self.exterior().0.is_empty() { - return false; - } - match utils::coord_pos_relative_to_ring(*coord, &self.exterior()) { utils::CoordPos::OnBoundary | utils::CoordPos::Outside => false, _ => self.interiors().iter().all(|ls| { From 15e48948a8bbdf53c119e9d3b3abebc8234b87dd Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 21 Oct 2020 10:09:01 -0700 Subject: [PATCH 5/8] fixup! update changelog --- geo-types/CHANGES.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/geo-types/CHANGES.md b/geo-types/CHANGES.md index 137c96984..64c4909f2 100644 --- a/geo-types/CHANGES.md +++ b/geo-types/CHANGES.md @@ -1,9 +1,11 @@ # Changes -## 0.6.2 +## Unreleased * `Rect::new` automatically determines min/max points + * * Add `MultiLineString::is_closed` method + * ## 0.6.1 From 7a54f2c48d4d9fd4d71d929bd60a9eac44e52d6c Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 21 Oct 2020 13:07:30 -0700 Subject: [PATCH 6/8] per CR: make empty MultiLineString act as closed --- geo-types/src/multi_line_string.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/geo-types/src/multi_line_string.rs b/geo-types/src/multi_line_string.rs index 6b50f1014..75de59365 100644 --- a/geo-types/src/multi_line_string.rs +++ b/geo-types/src/multi_line_string.rs @@ -35,8 +35,8 @@ where T: CoordinateType; impl MultiLineString { - /// True if the MultiLineString is non-empty and each of its LineStrings is closed - that is, - /// that the first and last coordinates of each line string are the same. + /// True if the MultiLineString is empty or if all of its LineStrings are closed - see + /// [`LineString::is_closed`]. /// /// # Examples /// @@ -56,10 +56,7 @@ impl MultiLineString { /// assert!(!MultiLineString::(vec![]).is_closed()); /// ``` pub fn is_closed(&self) -> bool { - if self.0.is_empty() { - return false; - } - + // Note: Unlike JTS et al, we consider an empty MultiLineString as closed. self.0.iter().all(LineString::is_closed) } } From 6ad4cbcfb992d6beda4a962591706a8655c32a89 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 21 Oct 2020 13:07:49 -0700 Subject: [PATCH 7/8] fixup docs --- geo-types/src/multi_line_string.rs | 2 +- geo/src/algorithm/is_convex.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/geo-types/src/multi_line_string.rs b/geo-types/src/multi_line_string.rs index 75de59365..6c57a05dd 100644 --- a/geo-types/src/multi_line_string.rs +++ b/geo-types/src/multi_line_string.rs @@ -3,7 +3,7 @@ use std::iter::FromIterator; /// A collection of /// [`LineString`s](line_string/struct.LineString.html). Can -/// be created from a `Vec` of `LineString`s, or from an +/// be created from a `Vec` of `LineString`s or from an /// Iterator which yields `LineString`s. Iterating over this /// object yields the component `LineString`s. /// diff --git a/geo/src/algorithm/is_convex.rs b/geo/src/algorithm/is_convex.rs index 5159e16fa..bdc19557f 100644 --- a/geo/src/algorithm/is_convex.rs +++ b/geo/src/algorithm/is_convex.rs @@ -15,7 +15,7 @@ use crate::{Coordinate, LineString}; /// /// - This definition is closely related to the notion /// of [convexity of polygons][convex set]. In particular, a -/// [`Polygon`] is convex, if and only if its `exterior` is +/// [`Polygon`](crate::Polygon) is convex, if and only if its `exterior` is /// convex, and `interiors` is empty. /// /// - The [`ConvexHull`] algorithm always returns a strictly From c6b505ce5c8aec88f80c73810446dd25a68084f0 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 21 Oct 2020 13:20:24 -0700 Subject: [PATCH 8/8] update doc test --- geo-types/src/multi_line_string.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/geo-types/src/multi_line_string.rs b/geo-types/src/multi_line_string.rs index 6c57a05dd..bb0a3a870 100644 --- a/geo-types/src/multi_line_string.rs +++ b/geo-types/src/multi_line_string.rs @@ -52,8 +52,8 @@ impl MultiLineString { /// // MultiLineString is not closed if *any* of it's LineStrings are not closed /// assert!(!MultiLineString(vec![open_line_string, closed_line_string]).is_closed()); /// - /// // An empty MultiLineString is not closed - /// assert!(!MultiLineString::(vec![]).is_closed()); + /// // An empty MultiLineString is closed + /// assert!(MultiLineString::(vec![]).is_closed()); /// ``` pub fn is_closed(&self) -> bool { // Note: Unlike JTS et al, we consider an empty MultiLineString as closed.