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

Generic geometry traits for EWKB/PostGIS output and support for Tiny WKB geometries #9

Merged
merged 44 commits into from
Oct 26, 2016

Conversation

pka
Copy link
Collaborator

@pka pka commented Sep 17, 2016

Hi,
Here's a proposal for a rust-postgis refactoring to use rust-geo geometries instead of specific geometry types. The goal is to share geometries with rust-gdal etc. without the need to convert the geometry structs. This pull request covers points and lines so far, but before writing the macros to cover all geometry types I wanted to show the implementation for reviewing. There are some topics to discuss:

  1. XYZM support: rust-geo has support for XY geometries only. I would prefer to extent rust-geo (see point types georust/geo#5) and could live with XY only for some time.
  2. SRID implementation: rust-geo doesn't plan to include SRID information in the geometry types (see Spatial Reference System of geometries georust/geo#32). The proposed implementation stores the SRID in a EWBK struct
  3. Support for Tiny WKB and OGC WKB: TWKB is important for my current project (performance), WKB is mainly for completeness.

Regards,
Pirmin

@andelf
Copy link
Owner

andelf commented Sep 17, 2016

good job 👍

for 1. 2.
rust-wkt offers wkt support.
It has its own Coord type and uses ToWkt trait as a bridge to rust-geo.
How about create a ToEwkb trait and introduce rust-wkt?

for 3.
No idea if we should do it in rust-postgis.

@pka
Copy link
Collaborator Author

pka commented Sep 17, 2016

Thanks for the postitive feedback!

rust-wkt: I wanted to use it for writing tests, but didn't find a way for creating rust-geo geometries from WKT. After looking at the source code again, it seems possible. ToEwkb shouldn't be a problem, since conversion from rust-geo is as easy as point = EwkbPoint { srid: Some(4326), geom: geo::Point::new(10.0, -20.0) }. Maybe a direct EwkbGeometry::fromWkt would be even more useful?

TWKB: ST_AsTWKB is included since PostGIS 2.2. See also https://carto.com/blog/smaller-faster/

@andelf
Copy link
Owner

andelf commented Sep 19, 2016

EwkbGeometry::fromWkt is OK.
This crate should support as many return formats as possible.
Should we work out a rust-twkb?

@pka
Copy link
Collaborator Author

pka commented Sep 19, 2016

I'm currently trying to improve geometry type conversions on the rust-geo side: georust/geo#67

My idea is to implement traits like

pub trait PointType<T: Float> {
    fn x(&self) -> T where Self: Sized;
    fn y(&self) -> T where Self: Sized;
    //...
}


pub trait PointIter<'a, T: 'a + Float>: Iterator {
}


pub trait LineStringType<T: Float, P: PointType<T>>
{
    fn points(&self) -> PointIter<T, Item=&PointType<T>> where Self: Sized;
}


// ...

This would allow accessing different rust-geo compatible geometry formats without converting data.

@pka
Copy link
Collaborator Author

pka commented Sep 30, 2016

After successful implementation of generic geometry traits, I went back to almost the original EWKB geometry structs. Compared to rust-geo geometry types, they have the following advantages:

  • Support for XYZM
  • No superfluous Coordinate struct
  • Possibllity to implement integer type points (screen coordinates)

I'm currently working on implementing ToSql for the geometry traits, which would allow storing any geometry type implementing these traits (like rust-wkt or hopefully rust-gdal) without conversion.

@pka pka force-pushed the rust-geo branch 2 times, most recently from 904afc0 to 92136d0 Compare October 16, 2016 23:03
@pka
Copy link
Collaborator Author

pka commented Oct 18, 2016

In the meantime I'm quite happy with the implemented types. See also the updated README. One drawback I've just realized is that adding srid to all geometry structs results in more memory consumption for line types, because all line points have an redundant srid field. But I'm hesitating to add additional point types without srid (coordinates?), because of the additional complexity. What do you think, @andelf?

@andelf
Copy link
Owner

andelf commented Oct 19, 2016

A new type with srid is OK for occasions where srid check/handling is needed.
👍

@pka
Copy link
Collaborator Author

pka commented Oct 23, 2016

The pull request is now feature complete.

Changes:

  • EWKB/PostGIS output based on generic geometry traits
  • Support for Tiny WKB (TWKB) geometries
  • Moved EWKB implementation into separate module
  • Unit tests for all geometry types added
  • More examples in Readme

Fixes #5, #6

@pka pka changed the title Use rust-geo geometries instead of specific rust-postgis geometry types [WIP] Generic geometry traits for EWKB/PostGIS output and support for Tiny WKB geometries Oct 23, 2016
@andelf
Copy link
Owner

andelf commented Oct 24, 2016

👍 I'll check it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants