Required Changes for NetTopologySuite.Curve

[FObermaier]

This PR contains changes that are required to make an extension for curve geometry work

[FObermaier]

@airbreather, I'd really appreciate your input on these changes to NTS core to make NTS.Curve work.
Thanks

[airbreather]

In general, I'm seeing some great auxiliary helpers (CoordinateList change and CoordinateSequences changes all make me happy), but I'm not sure about punching holes into our types and methods just so that an "external" project can fill them, especially (but not exclusively) given that it can only be completely done by something with InternalsVisibleTo access.

In general, I feel like NetTopologySuite is not in a position where it makes sense to try to "plug in" new Geometry subclasses externally and have them work with the rest of the system, and that it would take some dedicated refactoring / redesign in order to make it that way (the NtsGeometryServices changes are a start, but I think even those need to be revisited).

At the moment, I am strongly in favor of bringing the curve geometry support into this project.

I haven't COMPLETELY reviewed this, as the deeper I got into it, the more strongly I started to feel the above opinions. If we do wind up taking this approach, then I will want to spend more time on this.

[airbreather]

instead of shadowing, we should probably make the base method virtual and override it here

[FObermaier]

Summarizing:

• I'd like to keep NetTopologySuite.Curve a seperate project. We don't know what JTS will do in the future regarding curve geometry. Pulling them in now may lead to additional burden for us to follow JTS. That is where the interfaces come in handy.
  We may need to use them internally for better support, and I'm open to limit their members to SFS CA defined members.
• I think we shouldn't lock up our library so no external extensions regarding geometry types is possible. InternalsVisibleTo is only necessary for internal protected abstract int CompareToSameClass(object[, IComparer<CoordinateSequence>]). I can live with making it public.

[airbreather]

I think we shouldn't lock up our library so no external extensions regarding geometry types is possible. InternalsVisibleTo is only necessary for internal protected abstract int CompareToSameClass(object[, IComparer<CoordinateSequence>]). I can live with making it public.

I'm not claiming that we should. I'm claiming that, for all intents and purposes, geometry types that aren't defined inside of NetTopologySuite already are second-class citizens at best. In other words, it's already locked up.

I'm also not claiming that we need to address all of the below before releasing support for curves. On the contrary, I think that it's a really good idea to release with some support for this as soon as we can get a viable subset of features integrated with NTS, as long as we make sure that the release comes with prominent warning labels making it clear what's supported and what isn't.

I'm just listing some of the additional issues that I see, which prevent these external types from being fully integrated into the system. You've already identified WKT and WKB, so I'm not going to repeat the details of that.

Here are the specific issues I can see regarding subclasses of Geometry defined in external projects:

Casting to the NTS-specific geometry types

There are many places throughout the code base that check against a specific list of built-in types, assuming that a geometry can't be anything other than those. In order to support unknown types completely, I believe that we will need to evaluate every block that does a type check like this to figure out what we can possibly

Just the first five that I found (there are many!):

NetTopologySuite/src/NetTopologySuite/Algorithm/Centroid.cs

Lines 95 to 115 in 7a54d3b

	if (geom is Point)
	{
	AddPoint(geom.Coordinate);
	}
	else if (geom is LineString)
	{
	AddLineSegments(geom.Coordinates);
	}
	else if (geom is Polygon)
	{
	var poly = (Polygon)geom;
	Add(poly);
	}
	else if (geom is GeometryCollection)
	{
	var gc = (GeometryCollection)geom;
	for (int i = 0; i < gc.NumGeometries; i++)
	{
	Add(gc.GetGeometryN(i));
	}
	}

NetTopologySuite/src/NetTopologySuite/Algorithm/Distance/DistanceToPoint.cs

Lines 21 to 42 in 7a54d3b

	if (geom is LineString)
	{
	ComputeDistance((LineString) geom, pt, ptDist);
	}
	else if (geom is Polygon)
	{
	ComputeDistance((Polygon) geom, pt, ptDist);
	}
	else if (geom is GeometryCollection)
	{
	var gc = (GeometryCollection) geom;
	for (int i = 0; i < gc.NumGeometries; i++)
	{
	var g = gc.GetGeometryN(i);
	ComputeDistance(g, pt, ptDist);
	}
	}
	else
	{
	// assume geom is Point
	ptDist.SetMinimum(geom.Coordinate, pt);
	}

NetTopologySuite/src/NetTopologySuite/Geometries/Utilities/GeometryEditor.cs

Lines 285 to 307 in 7a54d3b

	var linearRing = geometry as LinearRing;
	if (linearRing != null)
	{
	return factory.CreateLinearRing(EditSequence(
	(linearRing).CoordinateSequence, geometry));
	}
	var lineString = geometry as LineString;
	if (lineString != null)
	{
	return factory.CreateLineString(EditSequence(
	(lineString).CoordinateSequence,
	geometry));
	}
	var point = geometry as Point;
	if (point != null)
	{
	return factory.CreatePoint(EditSequence(
	(point).CoordinateSequence, geometry));
	}
	return geometry;

NetTopologySuite/src/NetTopologySuite/Geometries/Utilities/GeometryTransformer.cs

Lines 99 to 115 in 7a54d3b

	if (inputGeom is Point)
	return TransformPoint((Point)inputGeom, null);
	if (inputGeom is MultiPoint)
	return TransformMultiPoint((MultiPoint)inputGeom, null);
	if (inputGeom is LinearRing)
	return TransformLinearRing((LinearRing)inputGeom, null);
	if (inputGeom is LineString)
	return TransformLineString((LineString)inputGeom, null);
	if (inputGeom is MultiLineString)
	return TransformMultiLineString((MultiLineString)inputGeom, null);
	if (inputGeom is Polygon)
	return TransformPolygon((Polygon)inputGeom, null);
	if (inputGeom is MultiPolygon)
	return TransformMultiPolygon((MultiPolygon)inputGeom, null);
	if (inputGeom is GeometryCollection)
	return TransformGeometryCollection((GeometryCollection)inputGeom, null);
	throw new ArgumentException("Unknown Geometry subtype: " + inputGeom.GetType().FullName);

NetTopologySuite/src/NetTopologySuite/GeometriesGraph/GeometryGraph.cs

Lines 185 to 201 in 7a54d3b

	if (g is Polygon)
	AddPolygon((Polygon)g);
	// LineString also handles LinearRings
	else if (g is LineString)
	AddLineString((LineString)g);
	else if (g is Point)
	AddPoint((Point)g);
	else if (g is MultiPoint)
	AddCollection((MultiPoint)g);
	else if (g is MultiLineString)
	AddCollection((MultiLineString)g);
	else if (g is MultiPolygon)
	AddCollection((MultiPolygon)g);
	else if (g is GeometryCollection)
	AddCollection((GeometryCollection)g);
	else
	throw new NotSupportedException(g.GetType().FullName);

Implicit Straight-Line Assumptions

Just as a random example (not the ONLY issue), a LinearLocation is defined as something like "the point that's 30% of the way through the 5th line segment of the 3rd line of the linear geometry". Our API, for better or for worse, has the LinearLocation itself take responsibility for being able to examine a linear geometry to get the LineSegment for that "5th line segment of the 3rd line" part:

NetTopologySuite/src/NetTopologySuite/LinearReferencing/LinearLocation.cs

Lines 266 to 283 in 7a54d3b

	/// <summary>
	/// Gets a <see cref="LineSegment"/> representing the segment of the given linear <see cref="Geometry"/> which contains this location.
	/// </summary>
	/// <param name="linearGeom">A linear geometry</param>
	/// <returns>the <c>LineSegment</c> containing the location</returns>
	public LineSegment GetSegment(Geometry linearGeom)
	{
	var lineComp = (LineString)linearGeom.GetGeometryN(_componentIndex);
	var p0 = lineComp.GetCoordinateN(_segmentIndex);
	// check for endpoint - return last segment of the line if so
	if (_segmentIndex >= NumSegments(lineComp))
	{
	var prev = lineComp.GetCoordinateN(lineComp.NumPoints - 2);
	return new LineSegment(prev, p0);
	}
	var p1 = lineComp.GetCoordinateN(_segmentIndex + 1);
	return new LineSegment(p0, p1);
	}

Which callers then use to figure out the "30%" part by calling this method or something like it:

NetTopologySuite/src/NetTopologySuite/LinearReferencing/LinearLocation.cs

Lines 51 to 61 in 7a54d3b

	public static Coordinate PointAlongSegmentByFraction(Coordinate p0, Coordinate p1, double fraction)
	{
	if (fraction <= 0.0) return p0;
	if (fraction >= 1.0) return p1;
	double x = (p1.X - p0.X) * fraction + p0.X;
	double y = (p1.Y - p0.Y) * fraction + p0.Y;
	// interpolate Z value. If either input Z is NaN, result z will be NaN as well.
	double z = (p1.Z - p0.Z) * fraction + p0.Z;
	return new CoordinateZ(x, y, z);
	}

To put it together:

NetTopologySuite/src/NetTopologySuite/LinearReferencing/LengthIndexedLine.cs

Lines 59 to 64 in 7a54d3b

	public Coordinate ExtractPoint(double index, double offsetDistance)
	{
	var loc = LengthLocationMap.GetLocation(_linearGeom, index);
	var locLow = loc.ToLowest(_linearGeom);
	return locLow.GetSegment(_linearGeom).PointAlongOffset(locLow.SegmentFraction, offsetDistance);
	}

In this example, in order to even allow for supporting curves, LengthIndexedLine would need to start knowing more about what kind of linear geometry we're looking at, or at the bare minimum, how to form the linear shape between two points.

How would we even begin to upgrade this in a way that supports the broad concept of curves without defining them in our project? I guess you kind-of answered this:

That is where the interfaces come in handy. We may need to use them internally for better support, [...]

But that brings us back to the situation we were in with GeoAPI / NetTopologySuite: interfaces that nobody else other than us will actually implement, each with its own set of constraints that the framework / SDK really doesn't help validate for the unfortunate soul who wants to implement it, both managed independently yet intrinsically tied together. I don't know about how everybody else felt about this situation, but I found this to be a major hassle when doing NTS development, and a minor hassle when doing application development using NTS.

The only advantage that I can see is what you've said:

We don't know what JTS will do in the future regarding curve geometry. Pulling them in now may lead to additional burden for us to follow JTS.

Agreed, we don't know what JTS will do in the future. However, as long as we name our types and any curve-specific members according to conventions that JTS would never follow (e.g., instead of CircularString, calling it NtsCircularString), I don't see any real downsides. If JTS adds curve support, then we will follow. The only way to avoid having to seriously consider supporting the users who have used the NTS-specific version is by never releasing that version at all (whether through the same OR a separate project).

Edit: I ran out of time at the end of this comment. See my next comment on this thread for the rest.

[FObermaier]

Joe, I don't know if you have looked at the implementation side on NetTopologySuite.Curve. All the issues you listed above can be solved/approximated by calling ICurve.Flatten([arcSegmentLength]) prior to invoking them as in CurveGeometry.

Yes, this is a simplification, but it is the way it is done in both GeoTools and GDAL/OGR and thus I assume Postgis, too. I don't know about SQLServer.

[airbreather]

All the issues you listed above can be solved/approximated by calling ICurve.Flatten([arcSegmentLength]) prior to invoking them

That doesn't solve the problem of NTS core needing to have built-in support for this geometry type in its own routines, which is the main point I was trying to make. In other words, the fact that NTS core defines an interface called ICurve instead of a class called CircularString doesn't change the fact that fully integrating the new types with the system still requires updating many places in NTS core to accommodate the special needs of these new types.

So why bother with the indirection and extra overhead of maintaining an additional project long-term to make it possible for us to define new geometry types in a separate GitHub project, with its own separate GitHub issue tracker, maintained in a separate VS solution, deployed as a separate NuGet package that users have to know to look for in order to get this support (potentially with those weird issues like MissingMethodException that we would occasionally have with GeoAPI), when it's not even enough for other people to seriously integrate their own alternative Geometry implementations? What's the actual, practical benefit to adding this support in a way that requires punching all these holes and adding friction to any development we do that needs to touch both sides?

I literally don't see the benefit. I ran out of time yesterday to finish off this part of my response, but the fact that we don't know what JTS is going to do about curves later seems like a non-sequitur on its face. The only way that I can relate it to the proposal of defining our own invention in a separate project is if the plan is to just stop maintaining the other project whenever we port what JTS does into NTS, which is problematic:

• When we release our invention as a separate project, EF Core is going to want to do something to take a dependency on that project, and a very significant number of users will be exposed to it that way. Most likely, they're going to take the dependency in their existing NTS "extension" packages. So, to a lot of users (according to NuGet download numbers, roughly half), initial curve support won't be some opt-in side thing that they had to do extra work to get at; rather, it's as much a part of NTS as Polygon or LineString.
• When we do eventually port JTS stuff to NTS, we will almost certainly find a bunch of spots throughout the project that JTS touched as well. It would be nice if we could guarantee that our own use of an interface would mean that we don't have to touch them again, but... the theme is that we don't know that. So it's certainly possible (likely?) that we're going to have something that calls ICurve.Flatten(someMagicNumber) in one branch, and PortedCurveThing.ToLineString(someParameters) (or something like that) in another, both to do the same thing.
• Not only would we need to deal with the algorithms and such, but the WKT / WKB would also need to be tweaked... somehow. Users of the external project aren't going to universally accept automatically switching to the new built-in version, but neither should new code prefer the old thing... there would probably need to be support for both for a while in order to allow people the time to transition. That's harder to do if the two implementations are in separate locations, and it seems like it invariably would have to involve allowing external code to conditionally "turn off" the handling for some tag or another.