Geometry.reverse inconsistency

[FObermaier]

The implementations of Geometry.reverse can be categorized by ones using Geometry.copy and others that don't:

• Reverse using copy

  • Point

    jts/modules/core/src/main/java/org/locationtech/jts/geom/Point.java

    Lines 193 to 196 in 8d433ca

    	public Geometry reverse()
    	{
    	return copy();
    	}

  • Polygon

    jts/modules/core/src/main/java/org/locationtech/jts/geom/Polygon.java

    Lines 432 to 440 in 8d433ca

    	public Geometry reverse() {
    	Polygon poly = (Polygon) copy();
    	poly.shell = (LinearRing) shell.copy().reverse();
    	poly.holes = new LinearRing[holes.length];
    	for (int i = 0; i < holes.length; i++) {
    	poly.holes[i] = (LinearRing) holes[i].copy().reverse();
    	}
    	return poly;// return the clone
    	}

• Reverse not using copy

  • LineString

    jts/modules/core/src/main/java/org/locationtech/jts/geom/LineString.java

    Lines 183 to 189 in 8d433ca

    	public Geometry reverse()
    	{
    	CoordinateSequence seq = points.copy();
    	CoordinateSequences.reverse(seq);
    	LineString revLine = getFactory().createLineString(seq);
    	return revLine;
    	}

  • LinearRing

    jts/modules/core/src/main/java/org/locationtech/jts/geom/LinearRing.java

    Lines 135 to 141 in 8d433ca

    	public Geometry reverse()
    	{
    	CoordinateSequence seq = points.copy();
    	CoordinateSequences.reverse(seq);
    	LinearRing rev = getFactory().createLinearRing(seq);
    	return rev;
    	}

  • MultiPoint -> GeometryCollection
  • MultiLineString

    jts/modules/core/src/main/java/org/locationtech/jts/geom/MultiPolygon.java

    Lines 123 to 131 in 8d433ca

    	public Geometry reverse()
    	{
    	int n = geometries.length;
    	Polygon[] revGeoms = new Polygon[n];
    	for (int i = 0; i < geometries.length; i++) {
    	revGeoms[i] = (Polygon) geometries[i].reverse();
    	}
    	return getFactory().createMultiPolygon(revGeoms);
    	}

  • MultiPolygon

    jts/modules/core/src/main/java/org/locationtech/jts/geom/MultiPolygon.java

    Lines 123 to 131 in 8d433ca

    	public Geometry reverse()
    	{
    	int n = geometries.length;
    	Polygon[] revGeoms = new Polygon[n];
    	for (int i = 0; i < geometries.length; i++) {
    	revGeoms[i] = (Polygon) geometries[i].reverse();
    	}
    	return getFactory().createMultiPolygon(revGeoms);
    	}

  • GeometryCollection

    jts/modules/core/src/main/java/org/locationtech/jts/geom/GeometryCollection.java

    Lines 281 to 289 in 8d433ca

    	public Geometry reverse()
    	{
    	int n = geometries.length;
    	Geometry[] revGeoms = new Geometry[n];
    	for (int i = 0; i < geometries.length; i++) {
    	revGeoms[i] = geometries[i].reverse();
    	}
    	return getFactory().createGeometryCollection(revGeoms);
    	}

This has the effect that Geometry.SRID and Geometry.UserData are copied for Polygon and Point but not for the others.

This is inconsistent. I have no preference which way is better.

[dr-jts]

It's inefficient to use copy in most cases, because that copies the internal coordinate sequences which are going to be replaced anyway.

But the SRID should be copied. Not sure about the userData, since that may or may not be desired depending on use case.

Probably there should be a protected copySRID method which can be called by Geometry methods which should preserve it.

[dr-jts]

Further thinking: all the reverse methods should NOT use copy, they should use the factory create methods. This makes them consistent.

The SRID should be set by the factory, so that's taken care of.

Copying user data is in general not done for constructed geometries, since the library can't know if that was the client intention or not. So this is left up to client code. It might be nice to have a convenience method defined for this (which would definitely help if and when there is more than one field to copy).

[FObermaier]

The SRID should be set by the factory, so that's taken care of.

This is only true if Geometry.srid == Geometry.getFactory().getSRID(). Once Geometry.setSRID(sridValue) has been called on a geometry this is not guaranteed to be true.

[FObermaier]

Other issues found while working on a fix:

• MultiPoint does not override GeometryCollection.reverse() so a reversed MULTIPOINT becomes a GEOMETRYCOLLECTION
• MultiLineString.reverse() reverses the order of the LineStrings, GeometryCollection and MultiPolygon don't

[dr-jts]

The SRID should be set by the factory, so that's taken care of.

This is only true if Geometry.srid == Geometry.getFactory().getSRID(). Once Geometry.setSRID(sridValue) has been called on a geometry this is not guaranteed to be true.

Good point. I guess the SRID will then need to be copied manually.

[dr-jts]

Other issues found while working on a fix:

• MultiPoint does not override GeometryCollection.reverse() so a reversed MULTIPOINT becomes a GEOMETRYCOLLECTION
• MultiLineString.reverse() reverses the order of the LineStrings, GeometryCollection and MultiPolygon don't

Good catch. My thinking on the semantics for this method is that ONLY the order of the Coordinate sequences is reversed, NOT the order of the component geometries in their parents. So the code for MultiLineString should be changed.

[dr-jts]

@FObermaier are you working on a PR for this?

[dr-jts]

Closed by #513