Skip to content

Comments

fixed off-by-one length assignment to ensure (RFC 7946) compliance an…#13

Open
Abdumbo99 wants to merge 1 commit intomapbox:masterfrom
Abdumbo99:off-by-one-fix
Open

fixed off-by-one length assignment to ensure (RFC 7946) compliance an…#13
Abdumbo99 wants to merge 1 commit intomapbox:masterfrom
Abdumbo99:off-by-one-fix

Conversation

@Abdumbo99
Copy link

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:

  1. The coordsLength > 2 in 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]]]

  2. 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/40409 then 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:

Screenshot from 2024-07-26 00-17-56
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.

@Abdumbo99
Copy link
Author

@tmcw

@tmcw
Copy link
Contributor

tmcw commented Jul 29, 2024

I stopped working at Mapbox and having commit access in 2017.

@Abdumbo99
Copy link
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?

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.

2 participants