Add Geometry.getCoordinateDimension - #1220
Conversation
Signed-off-by: Matthew de Detrich <mdedetrich@gmail.com>
52f33f0 to
460d385
Compare
grootstebozewolf
left a comment
grootstebozewolf
left a comment
There was a problem hiding this comment.
Request changes. The method is the right answer to #654 — getDimension() is topological, and WKB needs the coordinate count — but the API shape and the empty contract both freeze the wrong thing. Dan already pointed at the port: GEOS Geometry::getCoordinateDimension() is an instance method that trusts the sequence, including on empty.
Blockers
- This should be
g.getCoordinateDimension(), notGeometry.getCoordinateDimension(g). Implement it on the four types GEOS does. Do not add another 50-lineinstanceofswitch toGeometry.java. - Do not force empty sequences to 2.
WKTReaderalready stores dimension onPOINT Z EMPTY; GEOS reports 3 / 3 / 4. Trustseq.getDimension(). Document that factory-built empties may report 3 (CoordinateArrays.dimension([])defaults to 3). AddPOINT Z EMPTYexpecting 3.
Nits: dead shell != null; test file needs a license header; tests are Packed-only; XYZ vs XYM both being 3 matches GEOS and does not fully drive WKBWriter.setOutputOrdinates.
| if (g instanceof Point) { | ||
| return coordinateSequenceDimension(((Point) g).getCoordinateSequence()); | ||
| } | ||
| if (g instanceof LineString) { | ||
| return coordinateSequenceDimension(((LineString) g).getCoordinateSequence()); | ||
| } | ||
| if (g instanceof Polygon) { | ||
| Polygon poly = (Polygon) g; | ||
| int dimension = 2; | ||
| LinearRing shell = poly.getExteriorRing(); | ||
| if (shell != null) { | ||
| dimension = coordinateSequenceDimension(shell.getCoordinateSequence()); | ||
| } | ||
| for (int i = 0; i < poly.getNumInteriorRing(); i++) { | ||
| dimension = Math.max(dimension, coordinateSequenceDimension(poly.getInteriorRingN(i).getCoordinateSequence())); | ||
| } | ||
| return dimension; | ||
| } | ||
| if (g instanceof GeometryCollection) { | ||
| GeometryCollection gc = (GeometryCollection) g; | ||
| int dimension = 2; | ||
| for (int i = 0; i < gc.getNumGeometries(); i++) { | ||
| dimension = Math.max(dimension, getCoordinateDimension(gc.getGeometryN(i))); | ||
| } | ||
| return dimension; | ||
| } | ||
| return 2; | ||
| } | ||
|
|
There was a problem hiding this comment.
This should be g.getCoordinateDimension(), not Geometry.getCoordinateDimension(g).
getDimension() is already an instance method. GEOS — the port Dan pointed at on #654 — is virtual uint8_t getCoordinateDimension() const on Point / SimpleCurve / Surface / GeometryCollection. A static instanceof chain will not override for a later type and teaches the wrong call site. Put an instance method next to getDimension() and implement it on those four types.
A CoordinateSequenceFilter looks more “JTS,” but Point.apply / LineString.apply return without visiting a size-0 sequence, so a filter cannot see empty-sequence metadata. Direct getCoordinateSequence() (what you have, what GEOS has) is the right access — just not as a type switch on Geometry.
| return seq.size() > 0 ? seq.getDimension() : 2; | ||
| } | ||
|
|
There was a problem hiding this comment.
Do not force empty sequences to 2.
WKTReader.createCoordinateSequenceEmpty already builds csFactory.create(0, toDimension(flags), measures). POINT Z EMPTY therefore already has a 0-size sequence of dimension 3. This helper throws that away. GEOS pins the same cases at 3 / 3 / 4 (WKBReaderTest).
The size check is papering over a different footgun: GeometryFactory.createPoint() uses create(new Coordinate[]{}), and CoordinateArrays.dimension([]) is 3 (“unknown, assume default”). Packed’s default is also 3. Trust the sequence anyway. Document that factory-built empties may report 3. Add POINT Z EMPTY / LINESTRING Z EMPTY expecting 3.
| if (shell != null) { | ||
| dimension = coordinateSequenceDimension(shell.getCoordinateSequence()); | ||
| } | ||
| for (int i = 0; i < poly.getNumInteriorRing(); i++) { |
There was a problem hiding this comment.
shell != null is dead: Polygon replaces a null shell with createLinearRing().
| public void testEmptyPoint() { | ||
| checkDimension("POINT EMPTY", 2); | ||
| } |
There was a problem hiding this comment.
POINT EMPTY only locks the untagged case. Add POINT Z EMPTY / LINESTRING Z EMPTY expecting 3 (GEOS / the WKTReader empty sequence). Also run one case on the default CoordinateArraySequence factory — these tests are Packed-only, which hides the factory empty-default-3 footgun.
New test file also needs the license header CONTRIBUTING asks for.
There was a problem hiding this comment.
XY / XYZ / XYM / XYZM, mixed collections taking the max, and “3 means Z or M” all match GEOS. LinearRing falls through LineString, Multi* through GeometryCollection. The strict WKT reader is the correct fixture. That is enough for the original #654 2-vs-3 examples.
This getter does not distinguish XYZ from XYM (both 3). That matches GEOS. Callers who need the ordinate set still have to look at hasZ / hasM on the sequence — fine for #654; do not claim it fully drives WKBWriter.setOutputOrdinates.
Empty MultiLineString / MultiPolygon from WKT already drop the Z flag in WKTReader (createMultiLineString() with no flags). Pre-existing; this method cannot invent a dimension those objects do not store.
2 participants
Resolves: #654
Tests have also been added to make sure that it works as expected.