go-spatial / geom

Geometry interfaces to help drive interoperability within the Go geospatial community
MIT License
169 stars 37 forks source link

Add support for extended WKB encoding and PostGIS EWKB. #16

Open gdey opened 6 years ago

gdey commented 6 years ago

Here is postgis documentation on their extention to wkb to support ZM and srid values. https://github.com/postgis/postgis/blob/2.1.0/doc/ZMSgeoms.txt

Wiki page on WKB with other consts.

https://en.wikipedia.org/wiki/Well-known_text#Well-known_binary

Hoovs commented 6 years ago

@gdey @ARolek I am looking at this ticket but I am not 100% how to accomplish this / what this ticket success looks like.

If change the test case to be:

desc: Simple Point
expected: 1,2s 
bytes:{{
//01 02 03 04 05 06 07 08
  01                      // Byte order Marker little
  01 00 00 00             // Type 1 Point
  00 00 00 00 00 00 F0 3F // x 1
  00 00 00 00 00 00 00 40 // y 2
  00 00 00 00 00 00 F0 3F // z 1
}}

It does parse the Z if I made a [3]float type: 2018-09-11-213151_1366x768_scrot

So it seems like it is already clipping the Z cord because when I use the same test case as above with the unaltered code I get: 2018-09-11-213742_1366x768_scrot

It does leave the bytes on the reader because the data didn't fully use up the values but it "clips" them to the point that is need. I didn't try this for anything beyond point; can you point me to any examples of this

Also one of the big things of EWKB is that you can set SRID, is that needed for this?

Any help would be great. Thank you

gbroccolo commented 4 years ago

To include SRID you have to add 1 byte that encode the integer related to the SRID:

bytes:{{
//01 02 03 04 05 06 07 08
  01                      // Byte order Marker little
  01 00 00 00             // Type 1 Point
  00 00 10 E6             // <-- example for WGS84 (4326)
  00 00 00 00 00 00 F0 3F // x 1
  00 00 00 00 00 00 00 40 // y 2
  00 00 00 00 00 00 F0 3F // z 1
}}
ARolek commented 4 years ago

@gbroccolo would you be interested in implementing this? I think it would be nice to have, especially when working with PostGIS.

gbroccolo commented 4 years ago

@ARolek I'm pretty new with Golang, more confident with PostGIS. I can try...

ARolek commented 4 years ago

@gbroccolo your call, we're happy to help with code review if this is of interest to you. You could look at the wkb package for inspiration: https://github.com/go-spatial/geom/tree/master/encoding/wkb.

On another note, we're also looking to have native PostGIS geometry decoders if you're interested in tackling those.

gbroccolo commented 4 years ago

@ARolek I have one question here: do you think is it better to create a dedicated package for the extended wkb? I.e. something like geom/encoding/ewkb/? Or are you thinking to just extend the already existing methods?

ARolek commented 4 years ago

@gbroccolo I would need to look at the wkb code again (@gdey authored most of that) but I think ideally we would have another package, ewkb which could leverage the wkb package. That way we don't have to make updates in two places if a bug is found in encoding/decoding geometries and such.

@gdey @ear7h any thoughts?

gdey commented 4 years ago

I need to look at the package. Give me a couple of days to look at it and get back to you.

gdey commented 4 years ago

@gbroccolo I think it makes to make a dedicated package. It looks like there isn't a way to tell wether the bin is an extended or not, from the type. You have to attempt the decode. So, the user will have to know. You may want to restructure some of the internal packages for better sharing, but overall I think a dedicated package makes sense.

gbroccolo commented 4 years ago

Hi @gdey, thanks for the feedback! Yeah, even in other frameworks generally wkb and ewkb are defined in dedicated modules/packages. I'll start creating a ewkb package, following more or less what is done in wkb. Maybe some refactor for shared utilities could be needed, indeed.

gbroccolo commented 4 years ago

@gdey @ARolek I added a comment related to this issue in go-spatial/tegola#449, just because I saw the two issues were related (maybe it was the wrong place).