-
Notifications
You must be signed in to change notification settings - Fork 200
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
Implement Simplification traits for more things #135
Conversation
I was tempted to implement the simplification for But I didn't do it this time, I'm in two minds about whether it sensible. |
We should note that in the case of Polygons and MultiPolygons, the simplification operation is not guaranteed to return a valid geometry (and we're back to validity!). A well-performing fix is dependent upon a usable R* implementation (I have a look occasionally, but results have been mixed). |
(I've had a look at the JTS topology-preserving implementation – I don't do much Java, but it looks to be the naïve version described above. I'm not sure about GEOS, but I assume it's the same) |
I have expanded the comments w.r.t. validity and topology |
src/algorithm/simplify.rs
Outdated
@@ -52,7 +52,7 @@ fn rdp<T>(points: &[Point<T>], epsilon: &T) -> Vec<Point<T>> | |||
/// The [Ramer–Douglas–Peucker | |||
/// algorithm](https://en.wikipedia.org/wiki/Ramer–Douglas–Peucker_algorithm) simplifes a | |||
/// linestring. Polygons are simplified by running the RDP algorithm on all their constituant | |||
/// rings. | |||
/// rings. This may result in invalid Polygons, and has no guarantee of perserving topology. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "constituent" and "preserving".
src/algorithm/simplifyvw.rs
Outdated
/// are simplified by simplifing all their constituant geometries individually. | ||
/// Polygons are simplified by running the algorithm on all their constituant rings. This may | ||
/// result in invalid Polygons, and has no guarantee of perserving topology. Multi* objects are | ||
/// simplified by simplifing all their constituant geometries individually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "simplifying" and "constituent".
If you can simplify LineStrings, it makes sense that you can simplify a collection of linestrings (i.e. a multilinestring).
It simplifies their constituent linestrings.
Just simplify their constituent Polygons
c6769d0
to
cd8770f
Compare
I have rebased & fixed up those typo fixes now. |
Thanks for making the changes. PR looks good to me. 👍 |
this is great, thanks! opened a new issue for reminding us about this potentially creating invalid geometries (#142) bors r+ |
135: Implement Simplification traits for more things r=frewsxcv If you can simplify a `LineString`, then you can simplify a `MultiLineString`. I also added implementations for the simplification traits for`Polygon` (and `MultiPolygon`), which just perform the simplification on the constituant linestrings that make up polygon. PostGIS's `ST_Simplfiy` does this AFAIK. The link for VM algorithm shows a polygon (the USA) being simplified, so the concept of "simplifying a polygon" is well understood.
Build succeeded |
If you can simplify a
LineString
, then you can simplify aMultiLineString
.I also added implementations for the simplification traits for
Polygon
(andMultiPolygon
), which just perform the simplification on the constituant linestrings that make up polygon. PostGIS'sST_Simplfiy
does this AFAIK. The link for VM algorithm shows a polygon (the USA) being simplified, so the concept of "simplifying a polygon" is well understood.