Improve PolyUtil.isLocationOnPath to also tell where the location is on the polyline by noberasco · Pull Request #361 · googlemaps/android-maps-utils

noberasco · 2017-02-21T23:45:05Z

The methods provided to know whether a given location lies on the path of a polyline are good, but they could be even better if they also told where on the polyline the location is.

This in turn allows, for instance, to tap on any point on a polyline and get statistical information about the tapped point:

… a polyline

noberasco · 2017-02-22T00:07:06Z

It seems travis fails because the emulator doesn't respond in time...

barbeau · 2017-02-22T01:13:22Z

@noberasco thanks! Don't worry about Travis, the emulator images are messed up so it currently always fails. Could you please add a unit test for the methods that take the index as a parameter?

barbeau · 2017-02-22T01:17:09Z

Sorry, I meant methods that return index.

preda · 2017-02-22T03:47:02Z

                double lng2 = toRadians(point2.longitude);
                if (isOnSegmentGC(lat1, lng1, lat2, lng2, lat3, lng3, havTolerance)) {
-                    return true;
+                    return idx;


It may be useful to clearly state the meaning of a return of 0 (idx==0), notably for closed==true. It may be more intuitive to map the "closing" segment on idx==n-1 instead of idx==0 as is now.

See my comment above: tap on the closing segment ==> return N

preda · 2017-02-22T03:47:09Z

+     *
+     * with a default tolerance of 0.1 meters.
+     */
+    public static int locationIndexOnPath(LatLng point, List<LatLng> polyline,


Please document the returned "int"; mention the range and in which situations negative values are returned.

OK, will do.

preda · 2017-02-22T03:47:12Z

+     * is true, and of Rhumb segments otherwise. The polyline is not closed -- the closing
+     * segment between the first point and the last point is not included.
+     */
+    public static int locationIndexOnPath(LatLng point, List<LatLng> polyline,


Please document the returned int.

OK, will do.

preda · 2017-02-22T03:58:28Z

+        return locationIndexOnPath(point, polyline, geodesic, DEFAULT_TOLERANCE);
+    }
+
+    private static int locationIndexOnEdgeOrPath(LatLng point, List<LatLng> poly, boolean closed,


You may want to document here the interaction of "closed" with the returned value.

I think a useful return could be:
returns 0 if "point" is between poly[0] and poly[1], 1 if between poly[1] and poly[2], etc. If closed==true, returns N-1 if point is between poly[n-1] and poly[0]. -- but the current implementation does not achieve this.

In my opinion, it would make more sense to return N if the point is between poly[n-1] and poly[0]. This would allow to distinguish between:

point is between poly[n-2] and poly[n-1]

point is between poly[n-1] and poly[0]

Yes; but if size(polyline)==N, and you number the segments starting at 0, then the last segment (between poly[n-2] and poly[n-1]) is N-2; thus the segment "after last" is N-1. (i.e., I was taking N to be the number of points of the polyline, not the number of segments). Otherwise I think we agree.

noberasco · 2017-02-22T10:19:32Z

As requested I:

added documentation on the returned int values
added unit tests for the new methods

Additionally, I:

fixed a compilation issue on KmlPolygonTest.java (possibly my java compiler is more up to date than yours?)

For the time being, I did not change the logic for the corner case 'closed==true and point is on the return segment'. I'm waiting for your feedback on my own counter-proposal. However, it has to be said that the new public methods locationIndexOnPath always pass 'false' to closed, so the whole point is kind of moot.

preda · 2017-02-22T10:28:21Z

+
+        // Two points.
+        locationIndexCase(makeList(1, 2, 3, 5), new LatLng(1, 2), 0);
+        locationIndexCase(makeList(1, 2, 3, 5), new LatLng(3, 5), 1);


Probably here we should have here a return of 0, because (3,5) is considered part of the first segment [(1,2), (3,5)]. The 1 you get is a side-effect of the current implem, which considers [(1,2), (1,2)] as segment0 and [(1,2), (3,5)] as segment1.

preda · 2017-02-22T10:39:13Z

I agree to leave or change the "closed" case as you see fit, just document the behavior (of the private method) so that the case with closed=true is clear (e.g. for a future developer). Please fix the test case and implementation so that, if polyline = [A, B, C] (3 points), locationIndexOnPath(A, polyline) == 0, locationIndexOnPath(in-between-A-B, polyline) == 0, locationIndexOnPath(B, polyline) == 0, locationIndexOnPath(in-between-B-C, polyline) == 1.

…

On 22 February 2017 at 21:19, noberasco ***@***.***> wrote: As requested I: - added documentation on the returned int values - added unit tests for the new methods Additionally, I: - fixed a compilation issue on KmlPolygonTest.java (possibly my java compiler is more up to date than yours?) For the time being, I did not change the logic for the corner case 'closed==true and point is on the return segment'. I'm waiting for your feedback on my own counter-proposal. However, it has to be said that the new public methods locationIndexOnPath always pass 'false' to closed, so the whole point is kind of moot. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#361 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArHenz39uGOsbHF1KdhsEVM71nXqvOiks5rfAu1gaJpZM4MIBGd> .

noberasco · 2017-02-22T11:22:21Z

Done!

preda

Looks good to me, thanks!
(feel free to address the little test observation as you see fit).

barbeau · 2017-02-22T14:46:02Z

 import java.util.ArrayList;
+import java.util.List;

 public class KmlPolygonTest extends TestCase {


I believe these changes to KmlPolygonTest are unrelated to adding the ability to return the index for the closest point on the line. If so, can you please remove these changes from this changeset?

They fix a compilation issue I have on my side (possibly my java compiler is more up to date than yours?). They are pretty harmless, but if you prefer I'll file them as a separate pull request.

Try rebasing this on master branch - there were some changes to the KML package name and method return types in the last release. That should fix it.

barbeau · 2017-02-22T14:56:27Z

+        return (idx >= 0);
+    }
+
+    /**


Could you please change the comment format for all methods to include the @param and @return fields in the comments so the Javadocs generate correctly? See simplify() for an example. On a related note, the poly name in the comments doesn't match the name of any method parameters (I assume this should be polyline).

This reverts commit 732d673.

noberasco · 2017-02-22T22:17:16Z

OK, this should address the latest changes requested:

@preda rationalize test cases
@barbeau revert compilation issue patch
@barbeau change comment format

preda · 2017-02-23T00:02:21Z

Looks good to me, thanks!

…

On 23 February 2017 at 09:17, noberasco ***@***.***> wrote: OK, this should address the latest changes requested: - @preda <https://github.com/preda> rationalize test cases - @barbeau <https://github.com/barbeau> revert compilation issue patch - @barbeau <https://github.com/barbeau> change comment format — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#361 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArHeu2CRd1QpZFv44C_v2CS7_Ohil0xks5rfLPugaJpZM4MIBGd> .

barbeau · 2017-02-23T02:36:35Z

@noberasco Thanks, this looks good to me!

@stephenmcd Do you want to be the one to squash and merge?

@noberasco BTW, you're right on the KmlPolygonTest test compilation issue - looks like this must have snuck in at the end of #351 just prior to merge. Could you please open a separate PR for that?

stephenmcd · 2017-02-23T02:54:09Z

Thanks everyone!

Added methods that to tell whether, and where, a given location is on…

ba21666

… a polyline

preda reviewed Feb 22, 2017

View reviewed changes

noberasco added 3 commits February 22, 2017 09:19

Fix compilation issue on KmlPolygonTest.java

732d673

Document possible return values

8337078

Added test cases for PolyUtil.locationIndexOnPath

89bde7a

preda reviewed Feb 22, 2017

View reviewed changes

Change logic to include points at ending edge of segments

5238ce2

preda approved these changes Feb 22, 2017

View reviewed changes

barbeau reviewed Feb 22, 2017

View reviewed changes

noberasco added 3 commits February 22, 2017 22:45

Improved documentation format

8abd078

Updated and rationalized location index and tolerance tests

26e187d

Revert "Fix compilation issue on KmlPolygonTest.java"

c8def4a

This reverts commit 732d673.

stephenmcd merged commit f7320a8 into googlemaps:master Feb 23, 2017

noberasco mentioned this pull request Feb 23, 2017

Fix compilation issue on KmlPolygonTest.java #362

Closed

Conversation

noberasco commented Feb 21, 2017

Uh oh!

noberasco commented Feb 22, 2017

Uh oh!

barbeau commented Feb 22, 2017

Uh oh!

barbeau commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noberasco commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preda commented Feb 22, 2017 via email

Uh oh!

noberasco commented Feb 22, 2017

Uh oh!

preda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noberasco Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barbeau Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noberasco commented Feb 22, 2017

Uh oh!

preda commented Feb 23, 2017 via email

Uh oh!

barbeau commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenmcd commented Feb 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noberasco Feb 22, 2017 •

edited

Loading

barbeau Feb 22, 2017 •

edited

Loading

barbeau commented Feb 23, 2017 •

edited

Loading