georust / geos

Rust bindings for GEOS
https://docs.rs/geos/
MIT License
122 stars 42 forks source link

Implement voronoi binding #22

Closed antoine-de closed 5 years ago

antoine-de commented 5 years ago

add a binding to the libgeos voronoi computation function.

To use it more easily with rust-geo I added a simple wrapper to the geos binding.

It can thus be used either with pure geos:

let points = "MULTIPOINT ((150 200), (180 270), (275 163))";
let input = GGeom::new(points)?;
let voronoi = input.voronoi(None, 0., false)?;

or with rust-geo:

use geo_types::Point;
let points = vec![
    Point::new(0., 0.),
    Point::new(0., 1.),
    Point::new(1., 1.),
    Point::new(1., 0.),
];
let _voronoi = geos::compute_voronoi(&points, 0.)?;

For the moment to make the geos -> geo conversion, since I don't have much time (It's my last day in my current position :tada: ) I used wkt as a pivot format (thus geos -> wkt -> geo) It's obviously very inefficient. We should at least use wkb, or even better custom converter.

This PR is usable, ibut it's still a bit WIP since I'd like to test it's use in a real use case (voronoi computation in cosmogony It's also WIp because it depends on https://github.com/georust/wkt/pull/38

antoine-de commented 5 years ago

Note: the travis fail due to a too old libgeos version. we (well mostly @amatissart :stuck_out_tongue_winking_eye: ) need to investigate this

mthh commented 5 years ago

@antoine-de maybe we should just use "xenial" distribution instead of "trusty" on travis ? I guess "xenial" is providing GEOS 3.5 instead of GEOS 3.4.2 (Voronoi API was introduced in 3.5)

antoine-de commented 5 years ago

that's ok for me, especially since geos 3.4 also caused undefined behavior on our end :stuck_out_tongue_winking_eye:

is it ok for you if we drop the support for geos 3.4 ?

mthh commented 5 years ago

Yeah it's ok for me to drop support for 3.4 - the 3.5 is around since 2 years now i guess. (or at worst we might have been able to use the conditional compilation depending on the version available on the user's system? Maybe for some future release if we want new geos features without dropping support for still recents version ?)

antoine-de commented 5 years ago

well conditional compilation is sure possible, but it will makes the use of this library a bit more difficult, I think if we can avoid it we could :wink:

antoine-de commented 5 years ago

To finish this PR, I moved to xenial, so we drop the support for geos 3.4.

This PR is far from being perfect (especially the geos -> wkt -> geo conversion), but I won't manage to continue working on this in the short term, and I think it's worth merging it as it still brings voronois to rust-geos (even if a bit slow for the moment)

It also update the geo-type dependency which I need for other things :stuck_out_tongue_winking_eye:

amatissart commented 5 years ago

Could you also add a note about supported GEOS versions in the readme ?

antoine-de commented 5 years ago

ping, do you think we can merge this PR ?

mthh commented 5 years ago

@antoine-de Sorry if you were waiting on me, I thought it had already been merged ! So it's OK for me , don't hesitate to merge it and publish a new release.

antoine-de commented 5 years ago

thanks, released the version 3.0.0 on crates.io!