Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add wrappers for GDALBuildVRT, GDALApplyGeoTransform, GDALInvGeoTransform #239
Add wrappers for GDALBuildVRT, GDALApplyGeoTransform, GDALInvGeoTransform #239
Changes from 7 commits
453c536
869d9d0
7f69212
a95d519
23dcedc
50e56dd
4a0ae2f
dc2dc4e
41f8359
b6616a9
881bd21
304fdf2
c763e0c
6e510ba
9a7b9da
888df7f
bee242d
3f1844e
b07c316
3be783d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines need to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
GeoTransform
relevant methods might be better placed inside thedataset
or therasters
module. It is anyway only relevant to rasters IIUC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more ergonomic to throw these inside a trait that is impl by
GeoTransform
so we get the nicer:Optional suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
args
have to be aVec<String>
? Wouldn't anyIntoIterator<Item = ...>
work? This would avoid allocations if it's a fixed sized array.Going one more step, does the inner type have to be
String
or anything that implsTryInto<CString>
(likeVec<U8>
orVec<NonZeroU8>
, etc.)?We can leave it as-is if this makes the API too complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to implement this as follows:
However, I do not know how to convert
TryInto<CString>::Error
toGdalError::FfiNulError
.Can you show me how its done? Is the rest of it as you intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good pt.; we can't give a good error for arbitrary conversion failures, and the user could anyway
map
as req. before passing it. How about:This is inline with the rest of the lib, where we expect
&str
to passCString
to the FFI. The above seems to accept both&str
, andString
, so that should be good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Untested, but:
This looks nicer and (as a minor point) pre-allocates the destination
Vec
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we expose the
BuildVRTOptions
struct, and let this method take anOption<BuildVrtOptions>
instead of aVec<String>
? Seems more similar to the C api, and also a bit more natural.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for missing this. I guess
dest: Option<P> ... where P: AsRef<Path>
would be nicer, so you can call it with strings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not great, but let's use
utils::_path_to_c_string
here.(untested)