Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#5219 closed enhancement (fixed)

Support for 3D ESRI geojson features

Reported by: aamgroup Owned by: warmerdam
Priority: low Milestone:
Component: OGR_SF Version: 1.10.0
Severity: normal Keywords:
Cc: Kyle Shannon

Description

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

Attachments (11)

5219.patch (4.6 KB) - added by Kyle Shannon 4 years ago.
patch against branches/1.10/gdal
5219_test.patch (2.1 KB) - added by Kyle Shannon 4 years ago.
test patch
ogresrijsonreader.2.cpp (31.3 KB) - added by aamgroup 4 years ago.
new file (not sure on patches)
ogresrijsonreader.cpp.patch (8.9 KB) - added by aamgroup 4 years ago.
patch for current trunk version
ogresrijsonreader.cpp (32.0 KB) - added by aamgroup 4 years ago.
new file (not sure on patches)
ogresrijsonreader.cpp.2.patch (6.8 KB) - added by aamgroup 4 years ago.
Updated patch for trunk
ogresrijsonreader.3.cpp (28.0 KB) - added by aamgroup 4 years ago.
Updated reader for trunk
ogrgeojsondatasource.cpp (19.8 KB) - added by aamgroup 4 years ago.
ogrgeojsondatasource.cpp.patch (680 bytes) - added by aamgroup 4 years ago.
5219_v2.patch (6.9 KB) - added by Kyle Shannon 4 years ago.
patch ogresrijsonreader.cpp
5219_v2_test.patch (3.2 KB) - added by Kyle Shannon 4 years ago.
add test and dataset

Download all attachments as: .zip

Change History (41)

comment:1 Changed 4 years ago by aamgroup

Component: defaultOGR_SF
Priority: normallow
Version: unspecified1.10.0

comment:2 Changed 4 years ago by aamgroup

Type: defectenhancement

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by Kyle Shannon

Cc: Kyle Shannon added

comment:5 Changed 4 years ago by 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.

comment:6 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by 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.

Changed 4 years ago by Kyle Shannon

Attachment: 5219.patch added

patch against branches/1.10/gdal

Changed 4 years ago by Kyle Shannon

Attachment: 5219_test.patch added

test patch

Changed 4 years ago by aamgroup

Attachment: ogresrijsonreader.2.cpp added

new file (not sure on patches)

comment:8 Changed 4 years ago by aamgroup

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

comment:9 Changed 4 years ago by aamgroup

OK, so i should compile before i send stuff

working file coming

Changed 4 years ago by aamgroup

Attachment: ogresrijsonreader.cpp.patch added

patch for current trunk version

Changed 4 years ago by aamgroup

Attachment: ogresrijsonreader.cpp added

new file (not sure on patches)

comment:10 Changed 4 years ago by aamgroup

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

comment:11 Changed 4 years ago by 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.

Changed 4 years ago by aamgroup

Updated patch for trunk

Changed 4 years ago by aamgroup

Attachment: ogresrijsonreader.3.cpp added

Updated reader for trunk

comment:12 Changed 4 years ago by aamgroup

How are these ones?

Changed 4 years ago by aamgroup

Attachment: ogrgeojsondatasource.cpp added

Changed 4 years ago by aamgroup

comment:13 Changed 4 years ago by 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.

comment:14 Changed 4 years ago by 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.

comment:15 Changed 4 years ago by Kyle Shannon

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

comment:16 Changed 4 years ago by 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.

comment:17 Changed 4 years ago by 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.

Changed 4 years ago by Kyle Shannon

Attachment: 5219_v2.patch added

patch ogresrijsonreader.cpp

Changed 4 years ago by Kyle Shannon

Attachment: 5219_v2_test.patch added

add test and dataset

comment:18 Changed 4 years ago by 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.

comment:19 Changed 4 years ago by 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.

comment:20 Changed 4 years ago by aamgroup

Good spot, maybe !(coordDimension == 2
coordDimension == 3)

just reads better to me:

not 2d or 3d

comment:21 Changed 4 years ago by aamgroup

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

Looks better formatted

comment:22 Changed 4 years ago by 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)?

comment:23 Changed 4 years ago by aamgroup

The data looks valid too me

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

has some feature services you can use to test

comment:24 Changed 4 years ago by 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.
  1. If neither hasZ or hasM is specified, and there are 3 values in the tuple, assume it is z.
  1. If hasM is true, but hasZ is false, ignore the third value.
  1. 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?

comment:25 Changed 4 years ago by 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.

comment:26 Changed 4 years ago by Kyle Shannon

Resolution: fixed
Status: newclosed

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.

comment:27 Changed 4 years ago by 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?

comment:28 Changed 4 years ago by 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.

comment:29 Changed 4 years ago by 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
   }
  },

comment:30 Changed 4 years ago by Kyle Shannon

Committed in r26438. Thanks for the update.

Note: See TracTickets for help on using tickets.