-
Notifications
You must be signed in to change notification settings - Fork 834
Remove data/epsg, IGNF and esri.* files / support legacy +init=epsg:XXXX syntax #1182
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
Conversation
…e area of use argument, and make createFromUserInput() recognize init=epsg: / init=IGNF: in legacy mode, that is when proj_context_get_use_proj4_init_rules() is used
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.
This PR seems important enough that I (and others) should give a proper review. This is just to say that I haven't done so and please don't merge before I (and others) have had a change to look it over. I am travelling the next couple of days so there's a good chance I'll have a few hours of uninterrupted quality review time in an airport some where :-)
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.
Overall, I think you have done a good job finding a nice solution to a very difficult problem and I have only minor details to discuss.
I don't have that many specific comments to the code, only a few in cs2cs.cpp
.
Now that proj_create_crs_to_crs()
accepts proj-strings, EPSG-like codes and WKT(2) strings the documentation should be adjusted accordingly. I also would suggest renaming the arguments srid_from
/srid_to
to from
/to
to make it clear that more than just an EPSG-code can be supplied.
Concerning the input to cs2cs
, I think that using +to
looks weird when used with EPSG-codes and WKT strings. How about dropping the +to
, so that calls to cs2cs
are on the form cs2cs <FROM> <TO>
? The +to
will still have to be used when using proj-strings as input, unless they are in quotes: cs2cs "+proj=latlong +ellps=GRS80" "+proj=utm +zone=32"
. This also opens the door for a removal of the +to
syntax in the future.
In case there are several transformations available between two crs'es, is it possible with cs2cs
to select which one to use? Or would that require a combination of projinfo
and cct
?
Before this is merged the cs2cs
docs should be updated.
emess(3, "projection initialization failure\ncause: %s", | ||
pj_strerrno(pj_errno)); | ||
PJ_OBJ *dst = nullptr; | ||
if (!toStr.empty()) { |
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.
This block looks more or less identical to the one above (lines 418-448). Maybe refactor into a function?
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.
done
"missing target CRS and source CRS is not a projected CRS"); | ||
} | ||
} else if (fromStr.empty()) { | ||
assert(dst); |
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.
Similar to the above comment. These looks like to identical blocks that can be refactored into a function.
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.
done
src/cs2cs.cpp
Outdated
pipeline = proj_create_crs_to_crs(nullptr, fromStr.c_str(), toStr.c_str(), | ||
nullptr); | ||
if (!pipeline) { | ||
emess(3, "cannot initialize pipeline\ncause: %s", |
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.
Is "pipeline" the best word to use here? I think "transformation" would be more familiar to the users. I know that it is not necessarily correct, depending on the input, but I don't think "operation" is any better than "pipeline"...
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.
done
src/pj_fwd.c
Outdated
|
||
if( P->is_long_wrap_set ) { | ||
if( coo.lpz.lam != HUGE_VAL ) { | ||
double val = fmod(coo.lpz.lam , M_TWOPI); |
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.
Isn't this doing the same thing as adjlon()
does? Or am I missing something?
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.
Similar indeed. adjlon() reframe in [-PI,PI], whereas the long_wrap mechanism reframe in [long_wrap - PI, long_wrap + PI]. I just copied this snippet from pj_transform.c. I've modified this as P->long_wrap_center + adjlon(coo.lpz.lam - P->long_wrap_center);
@@ -794,7 +794,12 @@ PJ *proj_create_crs_to_crs (PJ_CONTEXT *ctx, const char *srid_from, const char | |||
return NULL; | |||
} | |||
|
|||
P = proj_create(ctx, proj_string); | |||
if( proj_string[0] == '\0' ) { | |||
/* Null transform ? */ |
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.
Maybe we should add a proper null operation? We can call it noop
.
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.
Traced as #1185 . Low priority
ok
Hum, I initially had the same idea, but I discovered while refactoring it that cs2cs could accept input files :
No, only the one that is sorted first by create_operations() will be used.
Yes
If you don't mind, I'll rather create a ticket about that. I'd like to advance on the coding side of GDAL, and then libgeotiff, to check that the PROJ side of things has no holes. |
ea95dad
to
02bdfd0
Compare
…remove the data/epsg and data/IGNF files
02bdfd0
to
47cada7
Compare
I've found that if building PROJ with lto, but GDAL without lto, exceptions thrown in PROJ are not properly captured by try {} blocks in PROJ C API, and unexpectedly go back up to GDAL. Might be a defect of the particular compiler I use (gcc 5.4 Ubuntu 16.04)
Done |
- proj_obj_create_projected_XXXXX() are renamed to proj_obj_create_conversion_snake_case() and just instanciate a Conversion object - Advanced manipulation functions are moved to a dedicated section at bottom of proj.h - New C API needed for GDAL OGRSpatialReference
Sure, it can wait, as long as you are confident you can remember the detail in a couple of months when you get around to doing it. Also, I am a bit unsure, how does the workings of |
The intro text of this PR onto which I've pointed #1186 captures well the changes
Wasn't specific to cs2cs, but more a general remark on being sure PROJ is ready for GDAL and libgeotiff needs. I feel I'm behind the schedule I anticipated and want to be sure that the PROJ 6 release will be OK for their needs. PROJ received all love until now, so time to give some to the other projects :-) I've just pushed the changes regarding function renaming and moving them at bottom ofin proj.h |
@kbevers I believe I've addressed your comments (or created tickets for defered actions) |
Actually, to me a PJ is just a bag of bits with a few embedded function pointers defining how to interpret the bits (actually a kind of limited RTTI implemented in C). At this level of (lack of) abstraction, it can represent anything. The "one API entry per operation type" helps enforcing correct initialization, even though a sufficiently clever user can ram garbage into any parameter anyway if not understanding it properly. But at least the individual API entries provide a reference frame for providing the relevant documentation, so as I said: You may very well convince me that this is the better choice. On the other hand it was also you, a few years ago, who convinced me that the first versions of proj.h/proj_4D_api.c included too much material into the "proj_..." API namespace, so I learned this API surface skepticism from a highly experienced and competent schoolmaster :-)
Which is of course A-OK in an evolutionary world. And having taken the step and accepting C++11 code into the PROJ code base really opens up for some major code reductions and improvements in the historical department of the code base.
Yes, that would probably be a superior design, but would require a re-implementation of most of the library. Entirely dropping the PROJ syntax on the other hand, would probably not be good either: Despite all its quirkiness, to me it really relates to WKT as JSON or YAML relates to XML: The less strict, but typing friendly, and human-readable version of the formal uncle. Instantiating in any syntax, and letting the object serialize itself in whatever syntax preferred is a nice feature. |
"Do what I say, not what I do" :-) That said, the API, even if large, is still an abstraction. For example, it does not expose that the database implementation is SQLite (and even in the C++ code, the interaction with SQLite has been carefully isolated in the DatabaseContext class). The C++ API itself tries to limit what is visible as public exported symbols.
Wasn't suggesting that either. There are a number of data formats using PROJ string to express the CRS of the data. It has the benefit of being compact (with the associated drawback of lacking some usefu metadata). My view is that PROJ strings are good to express coordinate transformation, especially since the pipeline introduction, but inappropriate to express a CRS object. |
…experimental.h header
ce473d0
to
2e37e1a
Compare
2e37e1a
to
270d044
Compare
6d3c0bd
to
75739f3
Compare
75739f3
to
03c7263
Compare
03c7263
to
c81ad96
Compare
5bca86a
to
f294346
Compare
f294346
to
0ab1867
Compare
@kbevers @busstoptaktik How do we go forward with this PR ? (my recent commits aren't related. fixes there and there) |
I still have a few things that I am not completely happy with. Most of them because I still don't fully understand the complexity of all the various aspects of the new code. I also think they go deeper than just this PR so I can create individual tickets for those. So disregarding those, I would say that this PR is ready for merging. You have found a nice way to solve the problem in #1178. I hope this is the last big PR in relation to GDALbarn so reviews will be simpler from now on :-) |
This is an attempt at resolving the issues raised in #1178
- using cs2cs,
- using proj_api.h API: pj_init(), pj_init_ctx(), pj_init_plus(), pj_init_plux_ctx()
- or setting PROJ_USE_PROJ4_INIT_RULES to YES / proj_context_use_proj4_init_rules(ctx, TRUE)
cs2cs [-options] <SOURCE> <TARGET>
(without +to), but in that mode no input files may be specified (only from standard input)