Open kylebarron opened 6 months ago
I'm a total noob to the PROJ C API, but from reading https://proj.org/en/9.3/development/reference/functions.html#transformation-setup it seems that a transformation object
and a CRS object
are two different things? Looking at the instances where we call proj_create
, it seems that the current rust API only allows creating these transformation objects, and it may be that the proj_as_projjson
and similar wkt function expect CRS objects?
If that diagnosis is accurate, would the best course of action be to create a new CRS
struct that is separate from the Proj
struct?
Looking at the API, it looks like it returns a PJ object. I would be surprised if they were somehow different, but I'm not at an actual screen right now. I'll try to have a look tomorrow though.
We are not even making it as far as the call to _string
…
When I get time to come back to this, I'll try to look at how pyproj is using the proj api
It looks like the options aren't being constructed correctly:
if I do
let opts_pp = CString::new("").unwrap();
let opts_pp = opts_pp.as_ptr();
…
let out_ptr = proj_as_projjson(self.ctx, self.c_proj, opts_pp as _);
I get a JSON string back. So this should be an easy fix.
Whoops 😅
NULL-terminated list of strings with "KEY=VALUE" format
I'm not exactly sure where I went wrong looking at the code
Actually perhaps I spoke too soon: this doesn't work, though it's possible I've messed up the pointer logic to go from *const i8
to *const *const c_char
:
let opts_p = CString::new("MULTILINE=YES");
let ptr = opts_p.as_ptr(); // *const i8
let c_char_ptr = ptr as *const c_char;
let correct_ptr = &c_char_ptr as *const *const c_char;
That gives me:
Unknown option :??Z??
And then panics because a null pointer is returned which triggers our todo()
. Still, better than the SIGSEGV?
I had been trying to use this as a model: https://github.com/georust/proj/blob/8db09d6ab2d6480b137f06e07fa8391e4fdebb37/src/proj.rs#L322-L329
It turns out that two things were wrong here: one is a bit more obvious and we should have caught it. The other thing is not obvious and caused this to take a couple of hours to debug because every time I changed the test from "pass some options" to "pass no options" it would start failing again.
libproj
function expects a pointer to a pointer to a single string. If there are multiple options the string must be built up from them and internally delimited by ","
or ", "
. We were converting a vec of pointers-to-CString to a pointer. It has to be a pointer to a vec containing a single pointer-to-CString.I pushed my working changes to your branch – feel free to adapt!
Sorry to make you take a couple hours of debugging, but awesome to see it working!
With this learning, is there anything that should be reflected in the set_search_paths
function? Looking at the C definitions, proj_context_set_search_paths
takes const char *const *paths
and proj_as_projjson
takes const char *const *options
, which seems like the same input type? (I'm bad at reading C types)
It looks like the CI is failing on a dependency that was bumped above proj's msrv
Oh it's nobody's fault. I think the reason it hasn't been caught is that I had no expectation that someone would try to pass an empty string to set_search_paths
which will presumably also cause SIGSEGV. I'll write a test when I get a chance to get back to this, and add the same null pointer logic if necessary.
On the failing CI, I think we'll have to switch to the newest georust CI images in proj.
is_empty
didn't become available until 1.71. I'm going for a walk.
Well, I was testing a little more with the latest commit I pushed and I'm getting
Unknown option :�p%
which looks like it's coming from here: https://github.com/OSGeo/PROJ/blob/f377ffa7d62fddf18e9d1c5814c0051cfb35d994/src/iso19111/c_api.cpp#L1821
So it would be nice to understand what about these cstrings is going wrong
@urschrei I think I'm just learning that the way you had it was correct, but I had to make changes to learn that 😅
I think this way works, and is just a different style from @urschrei 's commit here, but it separates out the two unsafe calls to proj_as_projjson
which may not be desired
No, the previous version was fine except for the missing null at the end.
The options are an array of C-style strings, not a single string. But the array has no length, so it had to end with a null pointer.
You can see the implementation at https://github.com/OSGeo/PROJ/blob/master/src/iso19111/c_api.cpp#L1812.
https://github.com/georust/gdal/blob/master/src/dataset.rs#L108 follows the same pattern.
https://github.com/georust/gdal/blob/master/src/dataset.rs#L108 follows the same pattern.
Ohhhh OK. Now I get it.
@kylebarron I've added a new error (since libproj
returns NULL
from this function in case of error), and we now return it in case of error in libproj
.
I think this is ready for review / merge now.
Awesome, sounds good!
This is a first attempt at solving #183 but the doctest segfaults 😕 . Does anyone have guidance here?