Implement CoordTransformOptions and CoordTransform::new_with_options constructor #372
Conversation
ttencate
force-pushed
the
feature/coord_transform_options
branch
4 times, most recently
from
8493ad9 to
08d883b
Compare
src/spatial_ref/srs.rs
Outdated
| /// | ||
| /// The pipeline may be provided as a PROJ string (single step operation or multiple step | ||
| /// string starting with +proj=pipeline), a WKT2 string describing a CoordinateOperation, or a | ||
| /// "urn:ogc:def:coordinateOperation:EPSG::XXXX" URN. |
There was a problem hiding this comment.
Is there a canonical description on these operation definitions you could link to?
There was a problem hiding this comment.
I have absolutely no idea what any of this even means, I just copied the docs from upstream GDAL.
There was a problem hiding this comment.
I found https://gdal.org/tutorials/osr_api_tut.html#advanced-coordinate-transformation which sheds some light. I don't think our API docs are the right place to expand this fully, so I added a link.
| let err = trafo.unwrap_err(); | ||
| assert!(matches!(err, GdalError::NullPointer { .. }), "{:?}", err); | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it be viable to provide a test for set_coordinate_operation?
There was a problem hiding this comment.
If somebody could tell me input and expected output, sure. I only implemented it for completeness. If it's not cool to have untested (but also rarely used!) code in there, I'm also happy to remove it again.
There was a problem hiding this comment.
I think we could call it just to check it doesn't crash.
ttencate
force-pushed
the
feature/coord_transform_options
branch
from
08d883b to
6c3577a
Compare
src/spatial_ref/srs.rs
Outdated
| ) | ||
| }; | ||
| if ret_val == 0 { | ||
| return Err(GdalError::OctError { |
There was a problem hiding this comment.
I'm not sure how our errors are supposed to work, but this function returns an error message, e.g. Invalid dfWestLongitudeDeg. You can get it using utils::_last_cpl_err, but of course, that will return a CplError instead. So I don't really know what would be the best.
There was a problem hiding this comment.
I don't know why this OCT prefix is even used here (OGR Coordinate Transformation??), so it seems excessive to add another error type for it. CplError will do fine for now. Used this for the other setters as well, though these are (currently) infallible.
src/spatial_ref/srs.rs
Outdated
| /// | ||
| /// If this option is specified with PROJ < 8, the `OGR_CT_OP_SELECTION` configuration option | ||
| /// will default to `BEST_ACCURACY`. | ||
| #[cfg(all(major_ge_3, minor_ge_3))] |
There was a problem hiding this comment.
This is wrong, it will fail in 4.0. We want major > 3 || major == 3 && minor >= 3. There are some places where we do this correctly.
But honestly, I wish we could improve on these somehow.
There was a problem hiding this comment.
Done. Yeah, definitely not the only occurrence of this mistake in the codebase, or even in this file.
Merging major and minor into a single gdal_ge_3_3 option would make these lines shorter, at the expense of adding a bunch of (autogenerated) cfg options.
A custom proc macro with custom syntax could of course also work.
src/spatial_ref/srs.rs
Outdated
| @@ -33,6 +175,24 @@ impl CoordTransform { | |||
| }) | |||
| } | |||
|
|
|||
| pub fn new_ex( | |||
There was a problem hiding this comment.
| pub fn new_ex( | |
| pub fn new_with_options( |
?
There was a problem hiding this comment.
Done, but note that it's inconsistent with Dataset::new_ex. That one was named after GDALOpenEx, however, so there's some argument to be made for the inconsistency.
ttencate
changed the title
ttencate
force-pushed
the
feature/coord_transform_options
branch
from
6c3577a to
75bf1d6
Compare
ttencate
force-pushed
the
feature/coord_transform_options
branch
from
75bf1d6 to
8c964e0
Compare
|
Rebased and all comments addressed. |
|
bors r+ |
CHANGES.mdif knowledge of this change could be valuable to users.I only needed this for the ballpark option, because I found it surprising that it's possible by default to transform between CRSes that are only valid on opposite sides of the world. I didn't change this default though.
The other three options are wrapped, but I don't understand them well enough to add tests for them.