GeoJSON-Net / GeoJSON.Net

.Net library for GeoJSON types & corresponding Json.Net (de)serializers
MIT License
446 stars 167 forks source link

`LineString.IsClosed()` does not use `DoubleComparer` for Position #137

Open jnyrup opened 4 years ago

jnyrup commented 4 years ago

Two instances of Position are considered equal when their Latitude, Longitude and Altitude are approximately equal using a DoubleComparer.

LineString.IsClosed considers an instance to be closed if Latitude, Longitude and Altitude of its first and last IPosition coordinate are exactly equals.

This seems to be potential inconsistent when constructing a LineString with Position as IPosition.

Is that intended behavior?

xfischer commented 4 years ago

Well it's a dilema here. I don't know what was the intended behavior when the project was initiated. It may be for that reason : when LineStrings are not closed, this will break some systems (SQL Server is strict on that for instance).

But comparing until the 10th decimal digit (as DoubleComparer does) is fair enough to state it's the same coordinate. We are talking about micrometers here.

More over equality with double is something variable upon where the code is executed and strict equality should be replaced by difference is below double.Epsilon. So the DoubleComparer is a good choice here, as it checks for equality for acceptable ranges.

So I think we could remove this strict equality check and prefer the DoubleComparer.

We then may add candies for strict closeness such as EnsureIsClosed() which can replace last position by exactly the start position only if DoubleComparer states that they are equal.

If anyone watching this thinks it's a bad idea please comment down here.

And by the way, thanks for your implication @jnyrup !