Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LinesIter algorithm to iterate over lines in a geometry #757

Merged
merged 16 commits into from
Mar 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion geo-types/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

## Unreleased

* BREAKING: Make `Rect::to_lines` return lines in winding order for `Rect::to_polygon`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was probably oversight. Good catch!

I personally don't think this is the kind of change we'd bump semantic versioning for. Does anyone disagree?

I'm ok to leave "BREAKING" or whatever in the release notes, I'm just loathe to do anything that causes all our downstream crate users to be on an old version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any context on downstream consumers yet, so I don't have a clear opinion here.

I think this falls in the "Possibly-breaking change" per cargo book and in unstable packages, it's OK to bump just the patch version for these changes. The order of returned lines isn't part of an explicit contract, so someone depending on that order to not conform to the winding order is unlikely.

Let me know if I can help make this decision (perhaps start a thread on the channel?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of a certain amount of efficiency, I don't expect to get an explicit "OK" on all things. Typically when I'm about to do something potentially controversial, my approach is to make a proposal in the code review and solicit input. If no one complains (we're all async, so I wait a couple days) I go forward.

If it were something that seemed potentially very controversial, I might wait for longer or for a more explicit OK from a larger group of people.

Since no one has yet replied to the contrary, I estimate that it's reasonable to move forward without bumping major version at this point.

* <https://github.com/georust/geo/pull/757>
* Note: All crates have been migrated to Rust 2021 edition. The MSRV when installing the latest dependencies has increased to 1.56.
* <https://github.com/georust/geo/pull/741>
* Macros `coord!`, `point!`, `line_string!`, and `polygon!` now support trailing commas such as `coord! { x: 181.2, y: 51.79, }`
* <https://github.com/georust/geo/pull/752>
* <https://github.com/georust/geo/pull/752>

## 0.7.3

Expand Down
8 changes: 4 additions & 4 deletions geo-types/src/rect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,8 +248,8 @@ impl<T: CoordNum> Rect<T> {
),
Line::new(
Coordinate {
x: self.min.x,
y: self.min.y,
x: self.max.x,
y: self.max.y,
},
Coordinate {
x: self.max.x,
Expand All @@ -262,8 +262,8 @@ impl<T: CoordNum> Rect<T> {
y: self.min.y,
},
Coordinate {
x: self.max.x,
y: self.max.y,
x: self.min.x,
y: self.min.y,
},
),
]
Expand Down
4 changes: 3 additions & 1 deletion geo/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

## Unreleased

*
* Add `LinesIter` algorithm to iterate over the lines in geometries.
* Very similar to `CoordsIter`, but only implemented where it makes sense (e.g., for `Polygon`, `Rect`, but not `Point`).
* <https://github.com/georust/geo/pull/757>

## 0.19.0

Expand Down
303 changes: 303 additions & 0 deletions geo/src/algorithm/lines_iter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,303 @@
use crate::{
CoordNum, Coordinate, Line, LineString, MultiLineString, MultiPolygon, Polygon, Rect, Triangle,
};
use core::slice;
use std::fmt::Debug;
use std::iter;

/// Iterate over lines of a geometry.
pub trait LinesIter<'a> {
type Scalar: CoordNum;
type Iter: Iterator<Item = Line<Self::Scalar>>;

/// Iterate over all exterior and (if any) interior lines of a geometry.
///
/// # Examples
///
/// ```
/// use geo::line_string;
/// use geo::lines_iter::LinesIter;
/// use geo::{Coordinate, Line};
///
/// let ls = line_string![
/// (x: 1., y: 2.),
/// (x: 23., y: 82.),
/// (x: -1., y: 0.),
/// ];
///
/// let mut iter = ls.lines_iter();
/// assert_eq!(
/// Some(Line::new(
/// Coordinate { x: 1., y: 2. },
/// Coordinate { x: 23., y: 82. }
/// )),
/// iter.next()
/// );
/// assert_eq!(
/// Some(Line::new(
/// Coordinate { x: 23., y: 82. },
/// Coordinate { x: -1., y: 0. }
/// )),
/// iter.next()
/// );
/// assert_eq!(None, iter.next());
/// ```
fn lines_iter(&'a self) -> Self::Iter;
}

impl<'a, T: CoordNum + 'a> LinesIter<'a> for Line<T> {
type Scalar = T;
type Iter = iter::Copied<iter::Once<&'a Line<Self::Scalar>>>;

fn lines_iter(&'a self) -> Self::Iter {
iter::once(self).copied()
}
}

impl<'a, T: CoordNum + 'a> LinesIter<'a> for LineString<T> {
type Scalar = T;
type Iter = LineStringIter<'a, Self::Scalar>;

fn lines_iter(&'a self) -> Self::Iter {
LineStringIter::new(self)
}
}

/// Iterator over lines in a [LineString].
#[derive(Debug)]
pub struct LineStringIter<'a, T: CoordNum>(slice::Windows<'a, Coordinate<T>>);

impl<'a, T: CoordNum> LineStringIter<'a, T> {
fn new(line_string: &'a LineString<T>) -> Self {
Self(line_string.0.windows(2))
}
}

impl<'a, T: CoordNum> Iterator for LineStringIter<'a, T> {
type Item = Line<T>;

fn next(&mut self) -> Option<Self::Item> {
// Can't use LineString::lines() because it returns an `impl Trait`
// and there is no way to name that type in `LinesIter::Iter` until [RFC 2071] is stabilized.
//
// [RFC 2071]: https://rust-lang.github.io/rfcs/2071-impl-trait-existential-types.html
Copy link
Contributor Author

@callpraths callpraths Mar 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am new(ish) to Rust, and I spent some time trying to wrap my head around this.

My current understanding is that as long as RFC-2071 is unstable, crate APIs should likely not use the impl Trait feature (in the return position, for existential types). Two reasons that I think I'd avoid this feature:

  • Clients have no way to store the returned value in a struct / trait associated type (anywhere they must name the type).
  • Conditional implementations (#[derive(Debug)] and friends) don't work well). In our case, impl Iterator<Item=Line<T> is not Debug even if both T and Line are).

Basing my understanding so far mostly off of https://www.reddit.com/r/rust/comments/8ik620/notes_on_impl_trait/ and https://users.rust-lang.org/t/embedding-trait-inside-of-struct/37176/14

In this case, I was stuck because of the first issue -- I needed to name the type for LinesIter::Iter, but had no way to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I say I'm new to Rust? Please let me know if I'm understanding this wrong / I'm missing something obvious in how impl Trait works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Update #681 in some form if impl Trait should be reconsidered in the public API of geo-types.

self.0.next().map(|w| {
// SAFETY: slice::windows(2) is guaranteed to yield a slice with exactly 2 elements
unsafe { Line::new(*w.get_unchecked(0), *w.get_unchecked(1)) }
})
}
}

type MultiLineStringIter<'a, T> =
iter::Flatten<MapLinesIter<'a, slice::Iter<'a, LineString<T>>, LineString<T>>>;
Copy link
Contributor Author

@callpraths callpraths Mar 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually like to introduce newtypes for all these concrete LinesIter::Iter types because they currently leak implementation details (about how iterators are chained etc).

I followed the pattern in CoordsIter for this PR, but if there is agreement, I'm happy to introduce newtypes for both CoordsIter and LinesIter.


impl<'a, T: CoordNum + 'a> LinesIter<'a> for MultiLineString<T> {
type Scalar = T;
type Iter = MultiLineStringIter<'a, Self::Scalar>;

fn lines_iter(&'a self) -> Self::Iter {
MapLinesIter(self.0.iter()).flatten()
}
}

type PolygonIter<'a, T> = iter::Chain<
LineStringIter<'a, T>,
iter::Flatten<MapLinesIter<'a, slice::Iter<'a, LineString<T>>, LineString<T>>>,
>;

impl<'a, T: CoordNum + 'a> LinesIter<'a> for Polygon<T> {
type Scalar = T;
type Iter = PolygonIter<'a, Self::Scalar>;

fn lines_iter(&'a self) -> Self::Iter {
self.exterior()
.lines_iter()
.chain(MapLinesIter(self.interiors().iter()).flatten())
}
}

type MultiPolygonIter<'a, T> =
iter::Flatten<MapLinesIter<'a, slice::Iter<'a, Polygon<T>>, Polygon<T>>>;

impl<'a, T: CoordNum + 'a> LinesIter<'a> for MultiPolygon<T> {
type Scalar = T;
type Iter = MultiPolygonIter<'a, Self::Scalar>;

fn lines_iter(&'a self) -> Self::Iter {
MapLinesIter(self.0.iter()).flatten()
}
}

impl<'a, T: CoordNum + 'a> LinesIter<'a> for Rect<T> {
type Scalar = T;
type Iter = <[Line<Self::Scalar>; 4] as IntoIterator>::IntoIter;

fn lines_iter(&'a self) -> Self::Iter {
self.to_lines().into_iter()
}
}

impl<'a, T: CoordNum + 'a> LinesIter<'a> for Triangle<T> {
type Scalar = T;
type Iter = <[Line<Self::Scalar>; 3] as IntoIterator>::IntoIter;

fn lines_iter(&'a self) -> Self::Iter {
self.to_lines().into_iter()
}
}

/// Utility to transform Iterator<LinesIter> into Iterator<Iterator<Line>>.
#[derive(Debug)]
pub struct MapLinesIter<'a, Iter1: Iterator<Item = &'a Iter2>, Iter2: 'a + LinesIter<'a>>(Iter1);

impl<'a, Iter1: Iterator<Item = &'a Iter2>, Iter2: LinesIter<'a>> Iterator
for MapLinesIter<'a, Iter1, Iter2>
{
type Item = Iter2::Iter;

fn next(&mut self) -> Option<Self::Item> {
self.0.next().map(|g| g.lines_iter())
}
}

#[cfg(test)]
mod test {

use super::LinesIter;
use crate::{
line_string, polygon, Coordinate, Line, LineString, MultiLineString, MultiPolygon, Rect,
Triangle,
};

#[test]
fn test_line() {
let line = Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 5., y: 10. });
let want = vec![Line::new(
Coordinate { x: 0., y: 0. },
Coordinate { x: 5., y: 10. },
)];
assert_eq!(want, line.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_empty_line_string() {
let ls: LineString<f64> = line_string![];
assert_eq!(Vec::<Line<f64>>::new(), ls.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_open_line_string() {
let ls = line_string![(x: 0., y: 0.), (x: 1., y: 1.), (x:2., y: 2.)];
let want = vec![
Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1., y: 1. }),
Line::new(Coordinate { x: 1., y: 1. }, Coordinate { x: 2., y: 2. }),
];
assert_eq!(want, ls.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_closed_line_string() {
let mut ls = line_string![(x: 0., y: 0.), (x: 1., y: 1.), (x:2., y: 2.)];
ls.close();
let want = vec![
Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1., y: 1. }),
Line::new(Coordinate { x: 1., y: 1. }, Coordinate { x: 2., y: 2. }),
Line::new(Coordinate { x: 2., y: 2. }, Coordinate { x: 0., y: 0. }),
];
assert_eq!(want, ls.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_multi_line_string() {
let mls = MultiLineString(vec![
line_string![],
line_string![(x: 0., y: 0.), (x: 1., y: 1.)],
line_string![(x: 0., y: 0.), (x: 1., y: 1.), (x:2., y: 2.)],
]);
let want = vec![
Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1., y: 1. }),
Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1., y: 1. }),
Line::new(Coordinate { x: 1., y: 1. }, Coordinate { x: 2., y: 2. }),
];
assert_eq!(want, mls.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_polygon() {
let p = polygon!(
exterior: [(x: 0., y: 0.), (x: 0., y: 10.), (x: 10., y: 10.), (x: 10., y: 0.)],
interiors: [
[(x: 1., y: 1.), (x: 1., y: 2.), (x: 2., y: 2.), (x: 2., y: 1.)],
[(x: 3., y: 3.), (x: 5., y: 3.), (x: 5., y: 5.), (x: 3., y: 5.)],
],
);
let want = vec![
// exterior ring
Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 0., y: 10. }),
Line::new(Coordinate { x: 0., y: 10. }, Coordinate { x: 10., y: 10. }),
Line::new(Coordinate { x: 10., y: 10. }, Coordinate { x: 10., y: 0. }),
Line::new(Coordinate { x: 10., y: 0. }, Coordinate { x: 0., y: 0. }),
// first interior ring
Line::new(Coordinate { x: 1., y: 1. }, Coordinate { x: 1., y: 2. }),
Line::new(Coordinate { x: 1., y: 2. }, Coordinate { x: 2., y: 2. }),
Line::new(Coordinate { x: 2., y: 2. }, Coordinate { x: 2., y: 1. }),
Line::new(Coordinate { x: 2., y: 1. }, Coordinate { x: 1., y: 1. }),
// second interior ring
Line::new(Coordinate { x: 3., y: 3. }, Coordinate { x: 5., y: 3. }),
Line::new(Coordinate { x: 5., y: 3. }, Coordinate { x: 5., y: 5. }),
Line::new(Coordinate { x: 5., y: 5. }, Coordinate { x: 3., y: 5. }),
Line::new(Coordinate { x: 3., y: 5. }, Coordinate { x: 3., y: 3. }),
];
assert_eq!(want, p.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_multi_polygon() {
let mp = MultiPolygon(vec![
polygon!(
exterior: [(x: 0., y: 0.), (x: 0., y: 10.), (x: 10., y: 10.), (x: 10., y: 0.)],
interiors: [[(x: 1., y: 1.), (x: 1., y: 2.), (x: 2., y: 2.), (x: 2., y: 1.)]],
),
polygon!(
exterior: [(x: 3., y: 3.), (x: 5., y: 3.), (x: 5., y: 5.), (x: 3., y: 5.)],
interiors: [],
),
]);
let want = vec![
// first polygon - exterior ring
Line::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 0., y: 10. }),
Line::new(Coordinate { x: 0., y: 10. }, Coordinate { x: 10., y: 10. }),
Line::new(Coordinate { x: 10., y: 10. }, Coordinate { x: 10., y: 0. }),
Line::new(Coordinate { x: 10., y: 0. }, Coordinate { x: 0., y: 0. }),
// first polygon - interior ring
Line::new(Coordinate { x: 1., y: 1. }, Coordinate { x: 1., y: 2. }),
Line::new(Coordinate { x: 1., y: 2. }, Coordinate { x: 2., y: 2. }),
Line::new(Coordinate { x: 2., y: 2. }, Coordinate { x: 2., y: 1. }),
Line::new(Coordinate { x: 2., y: 1. }, Coordinate { x: 1., y: 1. }),
// second polygon - exterior ring
Line::new(Coordinate { x: 3., y: 3. }, Coordinate { x: 5., y: 3. }),
Line::new(Coordinate { x: 5., y: 3. }, Coordinate { x: 5., y: 5. }),
Line::new(Coordinate { x: 5., y: 5. }, Coordinate { x: 3., y: 5. }),
Line::new(Coordinate { x: 3., y: 5. }, Coordinate { x: 3., y: 3. }),
];
assert_eq!(want, mp.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_rect() {
let rect = Rect::new(Coordinate { x: 0., y: 0. }, Coordinate { x: 1., y: 2. });
let want = rect.to_polygon().lines_iter().collect::<Vec<_>>();
assert_eq!(want, rect.lines_iter().collect::<Vec<_>>());
}

#[test]
fn test_triangle() {
let triangle = Triangle(
Coordinate { x: 0., y: 0. },
Coordinate { x: 1., y: 2. },
Coordinate { x: 2., y: 3. },
);
let want = triangle.to_polygon().lines_iter().collect::<Vec<_>>();
assert_eq!(want, triangle.lines_iter().collect::<Vec<_>>());
}
}
2 changes: 2 additions & 0 deletions geo/src/algorithm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ pub mod line_interpolate_point;
pub mod line_intersection;
/// Locate a point along a `Line` or `LineString`.
pub mod line_locate_point;
/// Iterate over the lines in a geometry.
pub mod lines_iter;
/// Apply a function to all `Coordinates` of a `Geometry`.
pub mod map_coords;
/// Orient a `Polygon`'s exterior and interior rings.
Expand Down
1 change: 1 addition & 0 deletions geo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
//! coordinates in a geometry in-place
//! - **[`TryMapCoords`](algorithm::map_coords::TryMapCoords)**: Map a fallible function over all
//! the coordinates in a geometry, returning a new geometry wrapped in a `Result`
//! - **[`LinesIter`](algorithm::lines_iter::LinesIter)**: Iterate over lines of a geometry
//!
//! ## Boundary
//!
Expand Down