Repository navigation
PackedCoordinateSequenceFactory does not work correctly when dealing with CoordinateXY. #434
Description
PackedCoordinateSequence$Double calls getOrdinate(2) or getOrdinate(3) on the coordinates and that throws an exception in CoordinateXY.
Polygon polygonA = new GeometryFactory(
new PackedCoordinateSequenceFactory()).createPolygon(
new Coordinate[] {
new CoordinateXY(0, 0),
new CoordinateXY(10, 0),
new CoordinateXY(10, 10),
new CoordinateXY(0, 10),
new CoordinateXY(0, 0)
}
);
Polygon polygonB = new GeometryFactory().createPolygon(
new Coordinate[] {
new CoordinateXY(5, 5),
new CoordinateXY(15, 5),
new CoordinateXY(15, 15),
new CoordinateXY(5, 15),
new CoordinateXY(5, 5)
}
);
polygonB.intersection(polygonA);
polygonA.intersection(polygonB);
In the above example, the first intersection does not throw an exception while the second does.
PackedCoordinateSequence$Float does this differently by calling getZ() on the coordinates. So possibly the solution is to call getZ() from getOrdinate() in CoordinateXY.
Activity
dr-jts commentedon Jun 7, 2019
The root cause of this is that internally the intersection algorithm winds up working with an array of
Coordinates of mixed dimension. Also, there isn't enough metadata around to determine that the output should be XY, not XYZ.Not sure how to fix this. I suspect it would take quite a bit to figure out how to get everything to work correctly with the right dimensions and coordinate types where needed.
Part of the problem is the decision to throw error when accessing dimensions that do not exist in
CoordinateXY(and otherCoordinatesubclasses). I know @jodygarnett has the opinion that it's better to fail fast, but if we don't understand how to prevent this problem, then it may be better to Do No Harm and simply return NaN.dr-jts commentedon Jun 7, 2019
I don't think this will work, because the
getOrdinate(2)call may be accessing either Z or M depending on the source.dr-jts commentedon Jun 7, 2019
Further to the idea of making invalid ordinate access succeed, this is already done in
CoordinateXY.getZ(). So makinggetOrdinate(2)return NaN would be consistent.Add future test for mixed factory with CoordinateXY
airbreather commentedon Aug 28, 2019
NetTopologySuite/NetTopologySuite#336 has another repro that I found independently on the .NET side of the world, though perhaps the root cause of that one is that JTS itself is creating the mixed-dimension coordinates, and not any fault of
PackedCoordinateSequence?Pasting the repro here:
The stack trace is:
I guess I should mention that I'm using
org.locationtech.jts:jts-core:1.16.1from Maven, using whatever magic IntelliJ IDEA used to make it possible for me to run this code (I have no clue how the Java world works, I just wanted to speak the same language as you guys).airbreather commentedon Aug 28, 2019
For the record, I'd like to voice my strong support for consistency around this area.
Trying to fetch the value of an ordinate that we know is not present should either return
NaNor throw an exception, consistently, regardless of whether we're using a name or a number, regardless of whether we're accessing it throughCoordinateorCoordinateSequence, with the possible exception of these kinds of things where it was decided to sacrifice safety / correctness / consistency to gain performance.ordinate >= dimensionisn't raising any warning flags in my head (though my intuition is admittedly more geared towards .NET implementations).Release v2.6.0