search

terraformer-js / terraformer

A geographic toolkit for dealing with geometry, geography, formats, and building geodatabases
MIT License
182 stars 28 forks source link

polygonContainsPoint returns false for intersections. Is this intended? #71

Open BlueWater86 opened 2 years ago

BlueWater86 commented 2 years ago

I swapped out a pile of custom helpers in favour of your library (thankyou!). A couple of my unit tests expected points intersecting with polygon boundaries to be contained within said polygon.

I went looking through your unit tests but they do not cover this edge case. Were point intersections with polygon boundaries excluded from being within the polygon on purpose?

const coordinates = [
  [1, 1],
  [0, 2],
  [0, 0]
];

coordinatesContainPoint(coordinates, [1, 1]); //false
coordinatesContainPoint(coordinates, [0.5, 1.5]); //false
coordinatesContainPoint(coordinates, [0, 2]); //false
coordinatesContainPoint(coordinates, [0.5, 1]); //true
jgravois commented 2 years ago

Were point intersections with polygon boundaries excluded from being within the polygon on purpose?

as far as terraformer is concerned, if the point and polygon share a vertex or the point falls exactly on the line formed between polygon verticies, the point is not considered to be contained by the polygon.

intersects is the spatial operator to capture the relationship you describe, but as noted in #33, point/polygon intersection isn't implemented in this library.

to be perfectly honest, i'm not sure why this is the case.

BlueWater86 commented 2 years ago

Thank you for the response. I'll fork and make the changes I need for it to match with the behaviour of leaflet plugins. It does seem strange to me that there are so many inconsistencies in implementations.

Do you mind if I raise a pull request to add a test that shows that point/polygon intersections are not supposed to be contained within the polygon? Also, would you consider another pull request to add options to change this behaviour?

jgravois commented 2 years ago

would you consider another pull request to add options to change this behaviour?

there's no need to expose a user-facing option.

a pull request to make the behavior of coordinatesContainPoint() match your expectation (and the definition of 'Contains' in the Esri Geometry API) would be welcome.

BlueWater86 commented 2 years ago

Perfect. That solves everything. I'll get the work done either tonight or on the 3rd. Have a good new year.

BlueWater86 commented 2 years ago

This was more complicated than I originally thought! Much learnt. I'll commit tomorrow.