fixed off-by-one length assignment to ensure (RFC 7946) compliance an…#13
Open
Abdumbo99 wants to merge 1 commit intomapbox:masterfrom
Open
fixed off-by-one length assignment to ensure (RFC 7946) compliance an…#13Abdumbo99 wants to merge 1 commit intomapbox:masterfrom
Abdumbo99 wants to merge 1 commit intomapbox:masterfrom
Conversation
…d fix inconsistency in calculations
Author
Contributor
|
I stopped working at Mapbox and having commit access in 2017. |
Author
|
Oh that's weird, seems like you are the only person in thr contributors list, aside from one one-time contributor, do you know who should I mention? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of all, I would like to thank the contributors behind Mapbox and all of its subprojects for their amazing work. I want to point out a small mistake that I found while re-writing some of the geo calculations for a library in GO, using Mapbox, amongst other projects, as a reference.
As per The GeoJSON Specification (RFC 7946). A Polygon consists of Linear Rings and a linear ring "... is a closed LineString with four or more positions", the reason for the four positions is that "The first and last positions are equivalent, and they MUST contain identical values; their representation SHOULD also be identical", i.e. to ensure the linear ring is closed the first and last position must be identical, deeming the first, or the last for that matter, position redundant and opens the door for some miscalculations similar to the one that I am attempting to fix in this PR.
The "-1" added in this PR solved two problems one is much simpler than the other but it is the main focus of this PR. The issues are:
The
coordsLength > 2in line 60 does not comply with the four positions requirement mentioned above, so a polygon with the invalid coordinates below would pass it, even though it is not a proper polygon (has less than four coordinates).[[[46.78401786031469,24.672371366746333 ], [46.78401786031469, 24.69016675081427 ], [46.78401786031469, 24.672371366746333]]]The second issue is in the calculations. The code cites the source
* Reference: Robert. G. Chamberlain and William H. Duquette, "Some Algorithms for Polygons on a Sphere", JPL Publication 07-03, Jet propulsion Laboratory, Pasadena, CA, June 2007 http://trs-new.jpl.nasa.gov/dspace/handle/2014/40409then uses it properly, almost, at least. From this source we can see the calculation of the area:"Assume we are dealing with a polygon with N vertices", after some work the equation becomes:
In this equation we have the same conditions as Geojson's polygons; N and 0 are identical. Thus the equation stops at N-1, i.e. the point just before the duplicate 0th point.
In the code, however, this is not the case, as it stops at the Nth point instead. This extra iteration on the Nth point results in an inconsistent behavior. For example, if we use this multipolygon:
{"type":"MultiPolygon","coordinates":[[[[28.321755510202507,16.35627490376781],[20.424575867090823,1.7575215418945476],[48.254218513706036,20.42650462625916],[36.310934132380964,14.226760576846956],[28.321755510202507,16.35627490376781]]]]}We will get the good and rather accurate approximated area of: 1141910591938.0227, but if we rotate the coordinates upwards, while maintaining the order and right-hand rule we will get the following calculations: 1141910591938.023, 1141910591938.0222, 1141910591938.0227, and 1141910591938.0227. The coordinates of each rotation are respectively:
[ [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ], [ 36.310934132380964, 14.226760576846956 ], [ 28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ] ][[ 48.254218513706036, 20.42650462625916 ], [ 36.310934132380964, 14.226760576846956 ], [ 28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ]][[ 36.310934132380964, 14.226760576846956 ], [28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ], [ 36.310934132380964, 14.226760576846956 ]][[ 28.321755510202507, 16.35627490376781 ], [ 20.424575867090823, 1.7575215418945476 ], [ 48.254218513706036, 20.42650462625916 ], [36.310934132380964, 14.226760576846956 ], [ 28.321755510202507, 16.35627490376781 ]]Now while the difference in the calculations is trivial, the inconsistency is, definitely, problematic and is caused by the the extra iteration on the Nth point. If we stop the iterations at N-1 we will get the consistent calculation of: 1141910591938.0225.
I hope my points are clear, I am ready to clarify any unclear points if needed.