Support for 3D ESRI geojson features

[aamgroup]

The current ersi geojson reader only support 2D features.

I have added code to handle 3D features in esri geojson

I have attached the updated file

ogr\ogrsf_frmts\geojson\ogresrijsonreader.cpp

[Kyle Shannon]

aamgroup, I downloaded your cpp file and it failed to compile in the trunk. I believe there have been recent changes in the internal json-c library that you may not have in 1.10.0. It would be a good idea to make changes against the trunk.

It's quite a bit easier to review a patch rather than a whole new file. If you can upload a unified diff, that makes it easier to test. I took a quick look (not knowing too much about the geojson driver), and I would imagine with the amount of changes submitted, autotests to verify changes and backwards compatibility would be good as well.

[aamgroup]

I beleive the changes I made are not that major, the major changes are, as you stated, the json lib changes.

I will have a look at doing the later, it's doing what I need at the moment.

[Kyle Shannon]

aamgroup, You are probably right. I compiled it against 1.10 branch and it did compile, and all of the geojson tests passed.

[Kyle Shannon]

Attaching 2 patches. One is a unified diff against the 1.10 branch for the changes. The other adds a very simple test and test dataset. Root for the code is gdal/ root for the test patch is autotest/. I didn't fix the code to meet the developer guidelines, but I would be willing to clean everything up for a patch against the trunk if it checks out, just need a second opinion.

[Kyle Shannon]

patch against branches/1.10/gdal

[Kyle Shannon]

test patch

[aamgroup]

new file (not sure on patches)

[aamgroup]

I have attached another few files that should work with the latest release

[aamgroup]

OK, so i should compile before i send stuff

working file coming

[aamgroup]

patch for current trunk version

[aamgroup]

new file (not sure on patches)

[aamgroup]

see last 2 files added, I tried to use the replace feature but it didn't do what i expected

[Even Rouault]

Stylistically speaking, ogresrijsonreader.cpp.patch looks better. But I guess it does not compile since there's a typo in the definition of OGRESRIJSONReaderParseXYZArray, where the Z letter is omitted ( so there are 2 OGRESRIJSONReaderParseXYArray() functions). The support for 3D point has also been lost compared to 5219.patch.

I also feel that there's a bit too much of code duplication both in OGRESRIJSONReaderParseXYZArray() and its call sites. Perhaps OGRESRIJSONReaderParseXYArray() could be just extended with extra arguments : double& dfZ, int& bHasZ and its callers adapted to deal with that.

[aamgroup]

Updated patch for trunk

[aamgroup]

Updated reader for trunk

[aamgroup]

How are these ones?

[aamgroup]

I have added files for ogrgeojsondatasource.cpp aswell, I have changed the strstr that checks for esri nodes from "esriFieldTypeOID" to "esriFieldType" as you can query the feature service with returnGeometry=false or without the OID field.

[aamgroup]

Are these files OK?

I tried to follow the style of the existing code as much as possible, mt preference would be to pass in the x,y,z,is3d by reference (as rouault suggested) rather than use a pointer.

[Kyle Shannon]

Sorry, I'm on vacation. Ill xheck the patches tomorrow and post a reply.

[Kyle Shannon]

Okay, I downloaded the patch and built tested against the trunk. Several tests failed due to differing extents for the layer. The same tests pass in 1.10, so I will need a little time to investigate what changes were made in the trunk recently. The patch looks good for the most part, and I don't see a huge problem with the change to the data source, although I don't have any reference currently. I should be able to get more to you tomorrow when I am back home.

[Kyle Shannon]

I had to make a few changes. Attached patches are for trunk/gdal and trunk/autotest. Can you test briefly and report that it is working for you.

[Kyle Shannon]

patch ogresrijsonreader.cpp

[Kyle Shannon]

add test and dataset

[aamgroup]

Looks good, was the only change the error message reading the y coordinate?

I have not idea how to get the autotest stuff to run so i'll assume it's good.

[Kyle Shannon]

There was a logical OR (

) where it should have been an AND (&&). I'll double check the tests and check it in to the trunk.

[aamgroup]

Good spot, maybe !(coordDimension == 2

coordDimension == 3)

just reads better to me:

not 2d or 3d

[aamgroup]

if(!(coordDimension == 2 || coordDimension == 3))

Looks better formatted

[Kyle Shannon]

I just realized that I kind of made up the test dataset. Can you provide me with a valid test dataset (preferably one with points and linestrings and polygons), or verify at least that my test data is valid (see the test patch)?

[aamgroup]

The data looks valid too me

​http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/

has some feature services you can use to test

[Kyle Shannon]

Forgive the lengthy comment, I was writing tests when I came across this:

​http://resources.arcgis.com/en/help/rest/apiref/

According to the spec, for geometries other than point, the hasZ and hasM attributes can be used. If they are omitted, it is assumed they are false (from the spec):

Omitting an hasZ or hasM field is equivalent to setting it to false.

For example, this multipoint geometry is valid:

{

"hasZ" : true,

"points" : [ [ 1, 2, 3 ] , [ 2, 3, 4 ] ], 

}

the third value in the point tuple is considered z. If hasM replace hasZ, the third value would be m. but:

{

"points" : [ [ 1, 2, 3 ] , [ 2, 3, 4 ] ], 

}

is ambiguous. It is not known if the third value is z or m. Currently, OGR doesn't handle xyzm geometries, so there are a few ways to handle this.

1. Strictly follow the spec, if hasZ is not specified, or false, ignore the values after the 2nd value in the tuple.

2. If neither hasZ or hasM is specified, and there are 3 values in the tuple, assume it is z.

3. If hasM is true, but hasZ is false, ignore the third value.

4. Always assume third value is z (not good, in my opinion).

I have currently implemented a check that follows 2 and 3. It is lenient if there are 3 values, but hasZ is false/absent as long as hasM is false/absent.

When xyzm geometry support is added to ogr (on the wishlist for 2.0) it would be trivial to add m support as well.

This works with the patch you submitted, but doesn't mistake m for z. Does this work for you?

[aamgroup]

That really depends on which version we are targeting, it appears the hasZ/hasM doesn't exist prior to 10.1 so support for this would be ideal.

I beleive your approach should cover these situations.

[Kyle Shannon]

Checked into trunk with r26407 with tests. Refactored to work with 10.1 esri geojson (10.0 didn't seem to support z at all), and it will also be trivial to add M support if needed.

[aamgroup]

Looks good, the only thing that limits my usage is the datasource where it searches for the esriFieldTypeOID or esriGeometry, should I create a new ticket for this?

[Kyle Shannon]

No need to do that. Can you give me a quick description for the change, or why it is holding you back? I don't mind committing it, but I'd like some sort of documentation.

[aamgroup]

When using an esri geoservices endpoint I use GDAL to query attribute values, get min/max, to keep the response request as small as possible I set flags like returnGeometry=false and outFields=NotAnOIDField, with these 2 options the returned data does not contain esriGeometry or esriFieldTypeOID which in turn fails to read with the OGRESRIJSONReader, if we drop "OID" from from the comparison data like the following will be handled using the ESRI reader

{
 "displayFieldName": "NAME",
 "fieldAliases": {
  "AREA": "AREA"
 },
 "fields": [
  {
   "name": "AREA",
   "type": "esriFieldTypeDouble",
   "alias": "AREA"
  }
 ],
 "features": [
  {
   "attributes": {
    "AREA": 4.6579805500699997e-005
   }
  },
  {
   "attributes": {
    "AREA": 0.00029019173000399998
   }
  },

[Kyle Shannon]

Committed in r26438. Thanks for the update.