Skip to content

Add RadialDistanceByAngleSimplifier (#69) to jts-lab#83

Open
FObermaier wants to merge 4 commits intolocationtech:masterfrom
FObermaier:RadialDistanceByAngleSimplifier
Open

Add RadialDistanceByAngleSimplifier (#69) to jts-lab#83
FObermaier wants to merge 4 commits intolocationtech:masterfrom
FObermaier:RadialDistanceByAngleSimplifier

Conversation

@FObermaier
Copy link
Copy Markdown
Contributor

@FObermaier FObermaier commented Feb 28, 2017

Added RadialDistanceByAngleSimplifierTest

@dr-jts dr-jts requested a review from jnh5y February 28, 2017 17:01
@dr-jts
Copy link
Copy Markdown
Contributor

dr-jts commented Feb 28, 2017

Not sure about the provenance and sign-off on this code. @jnh5y any thoughts? The code apparently authored by harelm, but commited by FObermaier - is that acceptable?

@FObermaier
Copy link
Copy Markdown
Contributor Author

FObermaier commented Feb 28, 2017

The initial concept and coding template is by @HarelM based on NTS, the port to JTS is my work.

jnh5y
jnh5y requested changes Feb 28, 2017
View reviewed changes
Comment thread .../lab/src/test/java/org/locationtech/jtslab/simplify/RadialDistanceByAngleSimplifierTest.java
@@ -0,0 +1,123 @@
package org.locationtech.jtslab.simplify;

import org.locationtech.jts.geom.Coordinate;
Copy link
Copy Markdown
Contributor

@jnh5y jnh5y Feb 28, 2017

Choose a reason for hiding this comment

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

This file needs a license header.

Comment thread modules/lab/src/main/java/org/locationtech/jtslab/simplify/RadialDistanceByAngleSimplifier.java Outdated
* This class follows the JTS simplifier pattern
*
* @author
* Harel M (https://github.com/harelm)
Copy link
Copy Markdown
Contributor

@jnh5y jnh5y Feb 28, 2017

Choose a reason for hiding this comment

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

@FObermaier can you say more about the provenance here? If you can link to the project, that might help. Generally, if we get a quick 'ok' from the original author for creating the derivative work, that'd be best.

Copy link
Copy Markdown

@HarelM HarelM Feb 28, 2017

Choose a reason for hiding this comment

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

Original code can be seen here:
https://github.com/IsraelHikingMap/Site/blob/master/IsraelHiking.API/Services/RadialDistanceByAngleSimplifier.cs
You have my blessing.

mprins
mprins reviewed Feb 28, 2017
View reviewed changes
Comment thread modules/lab/src/main/java/org/locationtech/jtslab/simplify/RadialDistanceByAngleSimplifier.java Outdated
@@ -0,0 +1,249 @@
/*
* Copyright (c) 2016 Vivid Solutions.
Copy link
Copy Markdown
Contributor

@mprins mprins Feb 28, 2017

Choose a reason for hiding this comment

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

This copyright is incorrect

@HarelM
Copy link
Copy Markdown

HarelM commented Feb 28, 2017

Quick "OK" 😊
Go ahead, feel free to use it. Happy I could contribute to this awesome project!

@HarelM
Copy link
Copy Markdown

HarelM commented Feb 28, 2017

Also worth mentioning somewhere that this simplifier works better after Douglas-Packard simplification from my experience, not sure where you'd put such comment.

@dr-jts
Copy link
Copy Markdown
Contributor

dr-jts commented Feb 28, 2017

@HarelM That would be appropriate for the class Javadoc. Is it possible to quantify/qualify how it is better? E..g better performance, or better quality of output?

@HarelM
Copy link
Copy Markdown

HarelM commented Feb 28, 2017 •
edited
Loading

Well basically both, but mainly better quality output.
Since this is a O(n) simplifier the less points it has the faster it will perform and D-P reduces points.
But this simplifier keeps sharp angles in case the segment after the turn is long enough. the reason for this is to remove GPS points that are measured in the same location. If D-P is ran before it, sharp turns will be kept.
When in action the following will be simplified (see the original blue line and the simplified red line):
simplified

Here is a case where it wasn't simplified since the segment is long enough thanks to D-P.
image

@FObermaier
Added/fixed license header (locationtech#69)
0d85700 
Added missing information about provenance of the code
@jnh5y
Copy link
Copy Markdown
Contributor

jnh5y commented Nov 14, 2017

@FObermaier, in order to accept this PR, you'd need to fill out the Eclipse Contributor Agreement for felix.obermaier@netcologne.de. The paperwork is here: https://www.eclipse.org/legal/ECA.php.

@FObermaier
Copy link
Copy Markdown
Contributor Author

FObermaier commented Nov 15, 2017

@jnh5y I just did that.

@jnh5y
Copy link
Copy Markdown
Contributor

jnh5y commented Nov 15, 2017

@FObermaier thanks! @dr-jts back to you for the regularly scheduled PR review:).

dr-jts
dr-jts reviewed Nov 15, 2017
View reviewed changes
Comment thread modules/lab/src/main/java/org/locationtech/jtslab/simplify/RadialDistanceByAngleSimplifier.java Outdated
*
* @return A simplified {@link Geometry}
*/
public static Geometry Simplify(Geometry geometry, double distanceTolerance, double angleTolerace)
Copy link
Copy Markdown
Contributor

@dr-jts dr-jts Nov 15, 2017

Choose a reason for hiding this comment

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

This method name should start with lowercase.

@dr-jts
Copy link
Copy Markdown
Contributor

dr-jts commented Nov 20, 2017

Is there any support for changing the name of this algorithm to something slightly more descriptive? Say ShortSharpAngleSimplifier ?

@HarelM
Copy link
Copy Markdown

HarelM commented Nov 20, 2017

I have no objection to the new name, it sounds more descriptive and less confusing indeed.
My original name was based on the fact that I used the concept of radial simplifier and that it also simplfies by angle, thus this not so good name. Note that the angle is a parameter so it can simplify non sharp angles too, but that's just a technicality...

@FObermaier
Copy link
Copy Markdown
Contributor Author

FObermaier commented Nov 21, 2017

I don't have any objections either. Do you want me to refactor it?

@dr-jts
Copy link
Copy Markdown
Contributor

dr-jts commented Nov 21, 2017

@FObermaier Would be great if you can do the rename.

@FObermaier
Rename RadialDistanceByAngleSimplifier to ShortSharpAngleSimplifier
acef4d4 
Signed-off-by: FObermaier <felix.obermaier@netcologne.de>
@dr-jts dr-jts changed the title Added RadialDistanceByAngleSimplifier (#69) to jts-lab module Add RadialDistanceByAngleSimplifier (#69) to jts-lab module May 9, 2018
@dr-jts dr-jts changed the title Add RadialDistanceByAngleSimplifier (#69) to jts-lab module Add RadialDistanceByAngleSimplifier (#69) to jts-lab May 9, 2018
@FObermaier
Copy link
Copy Markdown
Contributor Author

FObermaier commented Jun 14, 2018

@dr-jts dr-jts assigned FObermaier on 9 May

@dr-jts , I'm not sure, is there anything in particular you want me to do with this?

@HarelM
Copy link
Copy Markdown

HarelM commented Jun 14, 2018

In case anyone is interested, I wrote a short wiki page about the algorithm to find unmapped OSM routes. it uses this simplifier.
https://github.com/IsraelHikingMap/Site/wiki/Candidates-algorithm:-find-unmapped-routes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dr-jts dr-jts dr-jts left review comments

@jnh5y jnh5y jnh5y requested changes

+2 more reviewers

@mprins mprins mprins left review comments

@HarelM HarelM HarelM left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@FObermaier FObermaier

Labels

jts-lab type-improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@FObermaier @dr-jts @HarelM @jnh5y @mprins