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 Douglas-Peucker poly simplification algorithm as PolyUtil.simplify() #201

Merged
merged 1 commit into from
Sep 17, 2015

Conversation

barbeau
Copy link
Collaborator

@barbeau barbeau commented Sep 16, 2015

As discussed with @broady in #144 (comment).

@broady A few unrelated white space changes snuck into this commit via Android Studio - seems this happens on save, and I haven't found a way to disable it. Let me know if this is an issue - I'm using the official Android code template.

@broady
Copy link
Contributor

broady commented Sep 16, 2015

Whitespace changes LGTM. Rest LGTM, too.

pinging @googlebot for CLA ... (I don't think this actually does anything...)

@jfschmakeit @markmcd any thoughts?

@broady
Copy link
Contributor

broady commented Sep 16, 2015

CLA checked manually. 👍

* @param tolerance in meters. Increasing the tolerance will result in fewer points in the
* simplified line.
*/
public static void simplify(List<LatLng> line, List<LatLng> simplifiedLine, double tolerance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you using out-parameters (and void return type), instead of returning a LatLng array or List?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kept the same method signature and return type from the original MyTracks implementation. However, I'd actually prefer returning a LatLng List, so if you agree I'd be happy to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, that's preferable. Also, change the parameter name to polyline to be consistent with the other functions in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually - does this work for closed loops (i.e. polygon simplification)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good question - I think so, although I'll run some tests to see for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it doesn't modify the lat/lons themselves, so we just need to be sure that the first and last points are always included. I presume that they are.

I can't tell just by reading the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, assertEndPoints() test checks that the first and last points are unmodified.

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

@broady @markmcd Ok, just force-pushed a new commit into this PR to replace the old one. This changes to the method signature and return type to:
public static List<LatLng> simplify(List<LatLng> polyline, double tolerance) {

Don't know if I'll get to testing if this works for polygons tonight yet.

@markmcd
Copy link
Contributor

markmcd commented Sep 17, 2015

LGTM. Thanks for this - it's a valuable contribution.

Let us know when you're done so we can merge.

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

Looks like this will work for polygons too, but there are a few caveats:

  1. The polygon we run the algorithm on can't be closed (i.e., first and last LatLng in the List can't be exactly the same) - this breaks Douglas-Peuker, as it requires a positive distance measurement to start the algorithm.
  2. If we remove the last coordinate duplicate coordinate to open the polyline, it doesn't fully simplify the polygon (there is one extra point) - you can see this in the screenshots below with a simplified triangle. The polyline starts with the lower-left corner - blue is the original triangle, and yellow is the simplified version. In the left screenshot, the bottom edge of the triangle still has a point in the middle (the last point prior to closing the polygon) that should have been removed based on the tolerance. The right screenshot shows the correctly simplified yellow triangle, with the middle point in the bottom edge removed.

image

I produced the yellow polygon on the right by adding a very small offset to the final "duplicate" point that closes the polygon. So, it looks like:

ArrayList<LatLng> triangle = new ArrayList<>();
triangle.add(new LatLng(28.06025,-82.41030));  // First point
...
triangle.add(new LatLng(28.06025000001, -82.41030000001));  // Closing "duplicate" point

This seems to work without causing problems - here's the same result for an oval:

image

So, as far as handling polygons with the simplify() method, I think we have two options:

  1. Require developers to pass in a closed polygon to the simplify() method - Inside the method, we'd check to see if the first and last point were the same, and if so, add the offset to the last point. We'd add the requirement to the Javadocs for passing in a closed polygon.
  2. Add a new simplifyPolygon() method - We'd allow developers to pass in closed or open polygons into this method. If its closed, we'd add the offset to the last point, and if its open, we'd close it with the offset point - then we'd call simplify().

For the output of either option above, we would swap out the final point w/ the offset for the original final point before returning the list.

Thoughts? I think the first option is my preference, as it keeps the library simpler - worst case scenario, a developer passes in an unclosed polygon, and they get a polygon back that's not totally simplified. This won't even be noticeable in most cases, as it's only a single extra point.

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

Also, if we handle both polylines and polygons in the same simplify() method, I propose we change the parameter name to shape:
public static List<LatLng> simplify(List<LatLng> shape, double tolerance)

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

@broady @markmcd I went ahead and implemented the Option 1 above ("Require developers to pass in a closed polygon to the simplify() method"), and force-pushed a new commit for this into this PR to replace the old ones. So, simplification for polylines and polygons are now both fully supported. I also included a number of new unit tests to cover polygon simplification as well.

So, it should be ready for your final review, as long as you're good with the above design decisions.

* simplified shape.
* @return a simplified shape produced by the Douglas-Peucker algorithm
*/
public static List<LatLng> simplify(List<LatLng> shape, double tolerance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/shape/poly/

@broady
Copy link
Contributor

broady commented Sep 17, 2015

Thank you for the diligent work on this.

LGTM. Just one small nit: I think shape should be renamed to poly.

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

@broady Ok, np, I'll push a new commit shortly.

* Douglas-Peucker line simplification implementation ported from the Google MyTracks project (https://code.google.com/p/mytracks/source/browse/MyTracks/src/com/google/android/apps/mytracks/util/LocationUtils.java#81), (c) Google 2008, licensed under Apache v2.0.
* Add support for simplifying polygons to the PolyUtil.simplify() implementation
* Add demo activity for PolyUtil.simplify() - shows the original line in black, as well as simplified versions of the line with multiple tolerances in different colors.  Also shows origin polygons in blue, with simplified polygons in yellow
* Add unit tests for PolyUtil.simplify() (covering polylines and polygons), PolyUtil.distanceToLine(), and PolyUtil.isClosedPolygon() in PolyUtilTest
@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

Ok, done. I changed a few other shape references to poly as well.

broady added a commit that referenced this pull request Sep 17, 2015
Add Douglas-Peucker line simplification algorithm as PolyUtil.simplify()
@broady broady merged commit 7b1481e into googlemaps:master Sep 17, 2015
@barbeau barbeau changed the title Add Douglas-Peucker line simplification algorithm as PolyUtil.simplify() Add Douglas-Peucker poly simplification algorithm as PolyUtil.simplify() Sep 17, 2015
@barbeau
Copy link
Collaborator Author

barbeau commented Sep 17, 2015

Awesome, thanks @broady and @markmcd for the review/merge!

Would it be possible to get a new library release out with this included? New demo app release would be great too.

@broady
Copy link
Contributor

broady commented Sep 17, 2015

yup - I'm pretty busy with a bunch of other stuff, so I could only do a release next week. Maybe @markmcd will get to it first. We'll coordinate.

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 18, 2015

👍

@barbeau
Copy link
Collaborator Author

barbeau commented Sep 24, 2015

Just to make sure this doesn't fall through the cracks - before this is released, PR #205 should be reviewed/merged. That PR fixes an issue with polygon simplification.

@barbeau barbeau deleted the douglas-peucker2 branch September 24, 2015 18:16
@barbeau
Copy link
Collaborator Author

barbeau commented Oct 30, 2015

Friendly ping - @broady and @markmcd do you think you could get a new release out soon?

On an unrelated note - I'm prepping a release of a major app overhaul (including port to Android Maps v2), but seeing a bizarre issue with markers jumping around on the map that's bad enough to postpone release - here's a video:
https://youtu.be/gYBunJYZTMo

So far no traction with an gmaps-api issue I filed in August. Any ideas for next steps?

@barbeau
Copy link
Collaborator Author

barbeau commented Nov 18, 2015

Another friendly ping - @broady and @markmcd could we get a new release that includes PolyUtil.simplify()?

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 10, 2016

Yet another friendly ping - @broady and @markmcd could we get a new release that includes PolyUtil.simplify()?

@markmcd
Copy link
Contributor

markmcd commented Feb 15, 2016

Sure thing. I was hoping to have contributed a little more myself to the next release, but I've pushed 0.4.1 anyways.

@barbeau
Copy link
Collaborator Author

barbeau commented Feb 15, 2016

Awesome, thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants