Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WKTReader/-Writer enhancement #291

Merged
merged 7 commits into from Aug 20, 2018

Conversation

@FObermaier
Copy link
Contributor

commented Jun 13, 2018

The current implementation of WKTReader uses Coordinate class to read the geometries' coordinates. This is not useful when it comes to reading GEOMETRY M or GEOMETRY ZM WKTs.
This PR changes the behavior by creating a CoordinateSequence for every coordinate read and merges these sequences for the construction for the actual geometry type.

Further enhancements:

  • WKTReader can be configured to read GEOMETRY M with CoordinateArraySequence storing measure values at Coordinate.Z index (setMeasureIsZ(boolean)).
  • WKTWriter can be configured to write GEOMETRY M off geometries using CoordinateArraySequence as store by writing ordinate values at Coordinate.Z as measure values (setZToMeasure(boolean)).
  • The ordinates used for output can be configured for the WKTWriter.
  • Automatic use of PackedCoordinateSequenceFactory.Double as coordinate sequence factory when reading GEOMETRY ZM WKT.
  • Code cleanup

The behavior of the current WKTReader and WKTWriter class is preserved and can be configured.

Note: This can be further optimized by using PR271

@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

Thanks for separating this out from #271.

Can I take a moment to focus the use of coordinate sequence with z and m ordinates?

a) JTS provides an example ExtendedCoordinateSequence.java that supports XYZM, but this is only an example and not intended for use.

b) Even though ExtendedCoordinateSequence was just an example we found libraries were using it (JAI-EXT had a dependency, and GeoTools gt-coverage ended up requiring it to work with JAI-EXT).

c) GeoTools has an interface CoordinateAccess.java that extends CoordianteSequence to provide both getDimension() and getNumAttributes(). This interface was provided so that Oracle SDO could handle working with dimension+attribute data.

Your pull request also highlights the need for dimension+attribute coordinate sequences.

Can I ask what you think of CoordinateAccess linked to above?

@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

If I make use of Java 8 default methods we could update CoordinateSequence and Coordinate in place:

  default int  getNumberAttributes(){
       return 0;
  }
  double getAttribute(int coordinate, int attribute){
     if( attribute > 0 && attribute < getNumberAttributes()){
         return Double.NaN;
     }
     throw new IllegalArgumentException("Coordinate attribute "+attribute+" illegal, "+ getNumberAttributes() + " attributes stored");
  }
@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

Also noting WKTReader2 here.

@FObermaier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

Can I ask what you think of CoordinateAccess linked to above?

The approach is certainly valid but wouldn't it change the behavior of the existing getOrdinate and setOrdinate functions on CoordinateSequence? At least for sequences that can -currently- handle more than x-, y- and z- ordinates.

@FObermaier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

Regarding WKTReader2, the focus for your comment is on the support for Curves?

@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

CoordinateAccess: It provides getAttribute and setAttribute (in order not to change behaviour).

WKTReader2 supports curves (using a subclass of linestring that define a coordinate sequence using control points).

@FObermaier

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Taking the current implementation of ExtendedCoordinateSequence or PackedCoordinateSequencewith a dimension of 4 (with measures), I understand your approach would return Double.NaN for calls to getOrdinate(index, Coordinate.M) because Coordinate.M is not "spatial" and would instead return the measure value by calling getAttribute(index, 1)?

@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

@jnh5y has encouraged me to review a lot of what is out there (PostGIS, GML, SDO, etc...) and it looks like having M as a double is fine and matches what other technologies are doing.

GML3 has a complicated way of solving storing attributes on coordinates for those that need it, and it is the only example I found that was not XYZM.

@jodygarnett jodygarnett referenced this pull request Jul 1, 2018
11 of 12 tasks complete
@airbreather airbreather referenced this pull request Jul 13, 2018
0 of 4 tasks complete
@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

So what do we need to restart these WKTReader / WKTWriter improvements?

FObermaier added 6 commits Mar 6, 2018
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>
Improvements to WKTReader/-Writer
* WKTReader
  * moved ordinate flags from `WKTReader` to `CoordinateSequence`
* WKTWriter
  * now controlling `WKTWriter` output with ordinate flags
  * added OutputOrdinates property getter and setter
  * added unit test for `WKTWriter` properties

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>
* Fix mergeSequences for z- and m-ordinates
* Renamed old WKTReaderTest -> WKTReadWriteTest
* Added WKTReaderTest that compares the coordinate sequences of the
  geometries for equality
* Added lots of utility methods to GeometryTestCase, made it abstract

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>
Fix WKTReadWriteTest by setting output ordinates to XY
Signed-off-by: Felix Obermaier <felix.obermaier@netcologne.de>
Fixing GeometryFactory createMultiPoint(CoordinateSequence) to honor …
…possible measure values

Signed-off-by: Felix Obermaier <felix.obermaier@netcologne.de>
Adjusting PR to honer latest changes to CoordinateSequence in respect…
… to XYZM support. - removed ordinate flag definitions from CoordinateSequence. - added jts.IO.WKTOrdinates enum class - shifted logic from ordinate flags to WKTOrdinates enum. - fixed tests

Signed-off-by: Felix Obermaier <felix.obermaier@netcologne.de>

@FObermaier FObermaier force-pushed the FObermaier:enhancement/WKT_IO branch from 06360ec to 3679bae Aug 16, 2018

@FObermaier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

@jodygarnett, this should do.

@jodygarnett
Copy link
Contributor

left a comment

Some questions / feedback provided

@@ -57,6 +57,7 @@
* <p>This constant assumes XYZM coordinate sequence definition, please check this assumption
* using {@link #getDimension()} and {@link #getMeasures()} before use.
*/
/** Standard z-ordinate index */
int Z = 2;

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 17, 2018

Contributor

I am a bit uncomfortable having this as a constant, because for the XYM case it is not true.

This comment has been minimized.

Copy link
@FObermaier

FObermaier Aug 18, 2018

Author Contributor

It might have slipped in when resolving conficts.

/**
* An enumeration of possible Well-Known-Text ordinates
*/
public enum WKTOrdinates {

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 17, 2018

Contributor

Thanks I like this approach much better

This comment has been minimized.

Copy link
@FObermaier

FObermaier Aug 18, 2018

Author Contributor

Should perhaps be renamed to just WKOrdinates since they might apply to WKB as well.

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 19, 2018

Contributor

Not sure if there is anything "well-known" about this idea? Leaving that off as "Ordinates" sounds like a utility class, ... how about OrdinateSet, or CoordinateType?

private static final EnumSet<WKTOrdinates> XYM = EnumSet.of(X, Y, M);
private static final EnumSet<WKTOrdinates> XYZM = EnumSet.of(X, Y, Z, M);

public static EnumSet<WKTOrdinates> getXY() { return XY.clone(); }

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 17, 2018

Contributor

Do you just want this to be unmodifiable?

This comment has been minimized.

Copy link
@FObermaier

FObermaier Aug 18, 2018

Author Contributor

Yes, is there a better way?

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 19, 2018

Contributor

Collections.unmodifiableSet( XY ) is what I am used to. Not perfect as it masks that it is an EnumSet, but good enough to prevent modification.

Guava provides an Sets.immutableEnumSet(enumSet) but I do not want the dependency.

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 19, 2018

Contributor

Recommending calling these createXY, createXYZ, etc... so it is clear a copy is being produced?

@@ -0,0 +1,91 @@
package org.locationtech.jts.io;

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 17, 2018

Contributor

header needed

}

private Coordinate getPreciseCoordinate()
private Coordinate getPreciseCoordinate(StreamTokenizer tokenizer)
throws IOException, ParseException
{
Coordinate coord = new Coordinate();

This comment has been minimized.

Copy link
@jodygarnett

jodygarnett Aug 17, 2018

Contributor

Should this method consider making different kinds of Coordinates? The CoordinateSequence implementations have a createCoordainte() method that may help, also the Coordinates utility class.

Refactored WKTOrdinates to Ordinates
Added appropriate header and javadocs.
@jodygarnett

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

I have addressed the feedback above

@jodygarnett jodygarnett merged commit 3221a31 into locationtech:master Aug 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation All committers passed Eclipse ECA and Signed-off-by validation.
Details
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Feb 28, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

Signed-off-by: Felix Obemaier <felix.obermaier@netcologne.de>

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
airbreather added a commit to NetTopologySuite/NetTopologySuite that referenced this pull request Mar 2, 2019
Changed WKTReader/-Writer to work on CoordinateSequences
Changes to WKTReader
* added ALLOW_OLD_JTS_COORDINATE_SYNTAX configuration, including
  member variable that can be overridden
* added member variable to ALLOW_OLD_JTS_MULTIPOINT_SYNTAX
* added `measureToZ` flag value, that indicates for 'GEOMETRY M'
  entities the measure value is stored as z-ordinate value.
* cleaned up class

Changes to WKTWriter
* replaced StringBuffer with StringBuilder
* added zToMeasure flag to indicate that the writer should write
  'GEOMETRY M' entities in case output dimension is 3.
* re-enabled, added function parameters to make WKTWriter more
  re-entrant.
* added setter for PrecisionModel to use

Other changes:
* marginally adjusted unit tests to make them work.

JTS' commit
locationtech/jts@b464d54

NTS notes:
- lots of the unit test updates were to tests that NTS doesn't have, so I skipped those.
- I think there are still some errors in the tests, but because the original commit looks to have been rebased, nobody ever saw them at this point in time, so I'm going to wait until the end of the locationtech/jts#291 series before actually worrying about them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.