Thanks Nuno, comments/discussion inline:
First comment, the current API breaks backward compatibility by requiring that a measure should always be provided when building a coordinate sequence, IMHO it should have a fallback method that assumes zero measures if none if provided … otherwise changes in jaitools will also be required.
The CoordinateSequenceFactory provides both create( dimension ) and create( dimension, measure ). Your link [1] is to a specific implementation, PackedCoordinateSequence.java, it would be wise to use the factory if you can?
When investigating a failing test I step on a visibility change [1] that doesn’t allow us to change LiteCoordinateSequence coordinates\dimension in place anymore.
If I understand correctly LiteCoordinateSequence is willing to swap out its array on the fly right, and as part of that the dimensions and measures used to navigate the array.
This is not a use-case I have inside JTS, but it is one that we can support … will mark the field “protected” rather than “final protected”.
Another issues, the following test [3] is failing when invoking the normalize function [4] on polygon:
Caused by: java.lang.IllegalArgumentException: Invalid ordinate index: 2
at org.locationtech.jts.geom.CoordinateXY.getOrdinate(CoordinateXY.java:77)
at org.locationtech.jts.geom.impl.CoordinateArraySequence.getOrdinate(CoordinateArraySequence.java:260)
at org.locationtech.jts.geom.CoordinateSequences.swap(CoordinateSequences.java:47)
at org.locationtech.jts.geom.CoordinateSequences.reverse(CoordinateSequences.java:32)
at org.locationtech.jts.geom.Polygon.normalize(Polygon.java:429)
at org.locationtech.jts.geom.Polygon.normalized(Polygon.java:416)
at org.locationtech.jts.geom.Polygon.normalize(Polygon.java:374)
at it.geosolutions.jaiext.vectorbin.ROIGeometry.(ROIGeometry.java:269)
at it.geosolutions.jaiext.vectorbin.ROIGeometry.(ROIGeometry.java:195)
at it.geosolutions.jaiext.vectorbin.ROIGeometry.(ROIGeometry.java:150)
at org.geotools.coverage.grid.io.footprint.MultiLevelROIGeometry$FastClipROIGeometry.(MultiLevelROIGeometry.java:162)
at org.geotools.coverage.grid.io.footprint.MultiLevelROIGeometry.getTransformedROI(MultiLevelROIGeometry.java:128)
at org.geotools.coverage.grid.io.footprint.MultiLevelROIGeometry.getTransformedROI(MultiLevelROIGeometry.java:45)
at org.geotools.gce.imagemosaic.GranuleDescriptor.loadRaster(GranuleDescriptor.java:1348)
at org.geotools.gce.imagemosaic.GranuleLoader.call(GranuleLoader.java:108)
… 44 moreI’m not familiar with JTS code, so I’m clueless in find out what may be causing this
This is the kind of feedback I needed thank you. Looking at the swap method it seems fine:
public static void swap(CoordinateSequence seq, int i, int j)
{
if (i == j) return;
for (int dim = 0; dim < seq.getDimension(); dim++) {
double tmp = seq.getOrdinate(i, dim);
seq.setOrdinate(i, dim, seq.getOrdinate(j, dim));
seq.setOrdinate(j, dim, tmp);
}
}
Something has gotten in consistent, the dimension of the coordinate sequence must have changed from 2 (producing a cache of CoordinateXY) to 3 (producing a reference to getOrdinate( x, 2 ).
I will try and reproduce the failure locally…
Last finding, the previous coordinate object, even if only a dimension of 2 was provided, was always allowing to access or set a Z value, this is not the case anymore in the new API [5].
I agree that the behavior of the new API is the correct behavior, but unfortunately it breaks existing expectations. The issue here is that again this will prevent us to change geometries in place, which means that some not so trivial re-factor will need to be done in a few places.
Let’s figure out what is needed so you can change geometries in place and avoid a refactor unless there is a benefit.
This are the issues I found when using the new JTS API, all of the found issues (at exception the polygon normalize) are the consequence of the new JTS API breaking backward compatibility.
I am going to make an issue for the polygon normalize now (https://github.com/locationtech/jts/issues/296) and pull request (https://github.com/locationtech/jts/pull/297).
