Allow to transform ogr geometries to other SRS #29

mthh · 2017-01-02T22:01:56Z

Hi!

Thanks for providing an access to GDAL/OGR api in rust!
I was interested to transform ogr geometries to an other SRS so I wrapped some functions of the OGR spatial reference part of the gdal api (in order to be able to use OGRSpatialReferenceH and OGRCoordinateTransformationH types from C in rust).

It only uses a few major functions, as my main objective was simply to do something like :

let spatial_ref1 = SpatialRef::new_from_epsg(4326);
let spatial_ref2 = SpatialRef::new_from_epsg(3035);
let htransform = CoordTransform::new(&spatial_ref2, &spatial_ref1);
// an ogr Geometry :
geometry.transform(&htransform);
// or :
// geometry.transform_to(&spatial_ref1)

Could you be interested to merge these changes ? It also includes a simple example and some tests.

Also don't hesitate to tell me if there is some modifications requested in order to merge this (i'm not really experienced in rust) or if there is some other features of that API that you think should be wrapped.

Cheers!

frewsxcv · 2017-01-03T18:29:36Z

Have time to review this @jdroenner? If not, I can

jdroenner · 2017-01-03T18:59:47Z

I will look into it tomorrow.

jdroenner · 2017-01-04T11:41:38Z

@mthh thank you for your great contribution.
There is just one thing: I'm going to merge #28 today or tomorrow. This will replace panicking and returning Option with Result. Would you adapt your pull request to this before we merge it?

mthh · 2017-01-04T13:36:18Z

Sure, you can count on it! (today or tomorrow as well)

mthh · 2017-01-05T10:05:27Z

I updated the code of this PR following your latests changes regarding error handling.

jdroenner

Looks good to me. There are three small issues. I guess they are caused by copy & paste? ;)

One test fails for me with GDAL 2.x :(
---- spatial_ref::tests::transform_ogr_geometry stdout ----
thread 'spatial_ref::tests::transform_ogr_geometry' panicked at 'assertion failed: (left == right) (left: "POLYGON ((5509543.15080969966948 1716062.191619222285226,5467122.000330002047122 1980151.204280242323875,5623571.028492721728981 2010213.31025367998518,5671834.921544362790883 1746968.078280256595463,5509543.15080969966948 1716062.191619222285226))", right: "POLYGON ((5509543.1508097 1716062.19161922,5467122.00033 1980151.20428024,5623571.02849272 2010213.31025368,5671834.92154436 1746968.07828026,5509543.1508097 1716062.19161922))")', src/spatial_ref/tests.rs:59

jdroenner · 2017-01-05T22:48:01Z

src/spatial_ref/srs.rs

+        let mut c_wkt: *const c_char = ptr::null_mut();
+        let _err = unsafe { osr::OSRExportToWkt(self.0, &mut c_wkt) };
+        if _err != ogr_enums::OGRErr::OGRERR_NONE {
+            Err(ErrorKind::OgrError(_err, "OSRImportFromProj4").into())


The method name of the error should be "OSRExportToWkt".

jdroenner · 2017-01-05T22:48:38Z

src/spatial_ref/srs.rs

+        let mut c_wkt: *const c_char = ptr::null_mut();
+        let _err = unsafe { osr::OSRExportToPrettyWkt(self.0, &mut c_wkt, false as c_int) };
+        if _err != ogr_enums::OGRErr::OGRERR_NONE {
+            Err(ErrorKind::OgrError(_err, "OSRImportFromProj4").into())


"OSRExportToPrettyWkt"

jdroenner · 2017-01-05T22:49:04Z

src/spatial_ref/srs.rs

+        let mut c_proj4str: *const c_char = ptr::null_mut();
+        let _err = unsafe { osr::OSRExportToProj4(self.0, &mut c_proj4str) };
+        if _err != ogr_enums::OGRErr::OGRERR_NONE {
+            Err(ErrorKind::OgrError(_err, "OSRImportFromProj4").into())


"OSRExportToProj4"

…ats + allow to morph to esri format

mthh · 2017-01-05T23:48:23Z

Oops! Is it so obvious for the copy-paste ;) ? These 3 issues are fixed by my last commit.

If you don't mind I also wrapped the OSRSetFromUserInput function (as it is pretty convenient, accepting a large range of spatial reference formats, cf. the function doc.) and the OSRMorphToEsri (to be able to possibly write the esri wkt representation to a .prj file).

About the failing test, maybe we shouldn't rely on the geometry wkt representation (or not directly like that) to check that the geometry was transformed ? (as it is more an issue of coordinate precision (i'm on a 32bits laptop so I don't have either exactly the same values than the one I set for travis..) and the precision seems to be fixed with gdal > 2.0)

…om definition

jdroenner · 2017-01-06T10:48:56Z

src/spatial_ref/srs.rs

@@ -65,19 +65,25 @@ impl PartialEq for SpatialRef {
 }

 impl SpatialRef {
-    pub fn new() -> SpatialRef {
+    pub fn new(definition: &str) -> Result<SpatialRef> {


Is there no use-case for new() without parameters? Maybe the wrapped OSRSetFromUserInput should be a separate method like new_from_definition(definition: &str) or something like this?

You're right, there is use-cases for new() without parameters (I did this as a shortcut as currently the other exposed functions only allow to "import" a SRS, but it might be better to keep a method to create an empty Spatial Reference).

As you advised, it keeps now a new() method to create a spatial reference without parameters and a new_from_definition() using OSRSetFromUserInput.

frewsxcv · 2017-01-06T17:58:13Z

src/spatial_ref/srs.rs

+        Ok(SpatialRef(c_obj))
+    }
+
+    pub fn new_from_definition(definition: &str) -> Result<SpatialRef> {


This is not a big deal, but if you wanted to match the conventions in std, this would be renamed from new_from_definition to from_definition.

Thanks, I didn't know (but it clearly makes sense).
I guess it also apply to from_wkt, from_epsg and from_proj4 ?
I will rename theses functions.

To back up my previous comment, you can see some examples here:

https://doc.rust-lang.org/std/?search=from

(scroll down below the from method/function names)

This is indeed a significant list, thx (by looking around i also came across the RFC 430 which mention this from_some_other_type kind of constructor).

jdroenner · 2017-01-07T21:29:16Z

I merged this with master. @mthh thanks again for your contribution :)

mthh added 2 commits January 5, 2017 09:41

Apply changes regarding to returning a Result instead of panicking

98f9273

Remove unused value

3f53134

jdroenner reviewed Jan 5, 2017

View reviewed changes

Fix mistakes with OgrError + allow to import from wider range of form…

4bcf5b0

…ats + allow to morph to esri format

Re-allow to create new empty Spatial Reference + Spatial Reference fr…

ed4cb4f

…om definition

jdroenner reviewed Jan 6, 2017

View reviewed changes

frewsxcv reviewed Jan 6, 2017

View reviewed changes

Rename some functions to follow naming conventions

65b74bc

jdroenner merged commit 0ad1998 into georust:master Jan 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to transform ogr geometries to other SRS #29

Allow to transform ogr geometries to other SRS #29

mthh commented Jan 2, 2017

frewsxcv commented Jan 3, 2017

jdroenner commented Jan 3, 2017

jdroenner commented Jan 4, 2017

mthh commented Jan 4, 2017

mthh commented Jan 5, 2017

jdroenner left a comment

jdroenner Jan 5, 2017

jdroenner Jan 5, 2017

jdroenner Jan 5, 2017

mthh commented Jan 5, 2017 •

edited

Loading

jdroenner Jan 6, 2017

mthh Jan 6, 2017

frewsxcv Jan 6, 2017

mthh Jan 6, 2017

frewsxcv Jan 6, 2017

mthh Jan 6, 2017

jdroenner commented Jan 7, 2017

Allow to transform ogr geometries to other SRS #29

Allow to transform ogr geometries to other SRS #29

Conversation

mthh commented Jan 2, 2017

frewsxcv commented Jan 3, 2017

jdroenner commented Jan 3, 2017

jdroenner commented Jan 4, 2017

mthh commented Jan 4, 2017

mthh commented Jan 5, 2017

jdroenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthh commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdroenner commented Jan 7, 2017

mthh commented Jan 5, 2017 •

edited

Loading