-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
WIP PROJ6 support #118
WIP PROJ6 support #118
Conversation
- Convert lowercase epsg to uppercase EPSG - Axis order of a CRS is no longer always easting, northing, it can also be northing, easting, e.g. EPSG:4326. If source and target CRS are available, axis order is adjusted using proj_normalize_for_visualization() to easting, northing if needed. - Previously, GRASS did conversions of degress to/from radians and meters from/to map units. Now PROJ6+ might do these conversions itself, or not. GRASS is now doing the conversions only if needed. - In PROJ6+, there can be several different operations to transform coordinates from one CRS to another CRS. If more than one operation is available, information is provided about these different operations and the user has to provide one of these as pipeline option to r.proj or v.proj. That means that automated reprojection on the fly with r.import or v.import no longer works if more than one operation is available. Further on, the user must take care about axis order and remove any axisswap step from the pipeline if needed.
I am happy to review this, but please bear in mind that I am not particularly familiar with GRASS so some of my comments may not be valid. I'll start with a few comments to the description of the PR.
Is that really needed?
Seems unnecessary. Generally PROJ will provide the best transformation available between two CRS. I would use that by default and the give the user the option to select another one if more than one candidate is found. When using
If you need the conventionel GIS axis ordering then just run your PJ through |
G_debug(1, "trying %s", info_trans->def); | ||
|
||
/* check number of operations */ | ||
source_crs = proj_create(NULL, indef); |
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.
as mentionned by Kristian, proj_create_crs_to_crs() + proj_trans() should behave reasonably well if several candidate operations are available.
If keeping that check, source_crs, target_crs, operation_ctx, op_list and op should be freed
insrid = G_store(info_in->srid); | ||
return -1; | ||
} | ||
projstr = proj_as_proj_string(NULL, info_trans->pj, PJ_PROJ_5, NULL); |
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.
coding consideration: the value of projstr only seems to be used in that part of the code, so better move the top level declaration to the beginning of this block for clarity. applies to other similar instances
lib/proj/do_proj.c
Outdated
/* PROJ6+: EPSG must uppercase EPSG */ | ||
if (info_in->srid) { | ||
if (strncmp(info_in->srid, "epsg", 4) == 0) | ||
insrid = G_store_upper(info_in->srid); |
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'm not familiar with GRASS memory management, but assuming G_store() and G_store_upper() allocates dynamic memory, insrid doesn't seem to be freed. Same for outsrid
/* what about axis order? | ||
* is it always enu? | ||
* probably yes, as long as there is no +proj=axisswap step */ | ||
G_asprintf(&(info_trans->def), "+proj=pipeline +step +inv %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.
in the case where info_in->def would contain a +towgs84/+nadgrids clause, I believe this pipeline would apply it, whereas you probably only want the reverse projection, and no datum shift.
The easiest would probably to mess up with the PROJ string. Otherwise with the PROJ API, you could instanciate a PJ object from the string, check if it is a BoundCRS with proj_get_source_crs(), and in that case, take the source CRS with proj_get_source_crs(), and do the inverse transform on it
At least I found recently there can be performance issues if not using EPSG uppercase, since when not finding the 'EPSG' match, PROJ will query the database for all available authority names, and this turns out to be quite slow, before doing case-insensitive checks. |
Thanks a lot for taking time to review this PR! I followed your suggestions as much as possible. Some issues remain, however
Are you really sure? See Providing a PJ_AREA helps in my local tests, a single operation is selected from the available operations.
proj_normalize_for_visualization() apparently works only for a PJ created with proj_create_crs_to_crs(). If I select an operation obtained with proj_create_operations() and use the selected pipeline with proj_create(), proj_normalize_for_visualization() does not work because source and/or target crs are unknown to proj_normalize_for_visualization(). But in GRASS GIS we need to enforce an axis order of easting, northing. I think it is dangerous to expect users to manually modify the proj pipeline. |
Yes, that's what the cs2cs utility uses for example (https://github.com/OSGeo/PROJ/blob/master/src/apps/cs2cs.cpp#L598). "dummy" in the sense that has it is a container for multiple internal transformations a number of query functions on PJ* object will return NULL/error, but the proj_trans() family of functions will work perfectly fine on it.
Indeed, but you should be able to apply proj_create_operations() on the PJ* returned by proj_list_get() when iterating on the results of proj_create_operations(). (Once you've extracted a PROJ pipeline string from a PJ* transformation and re-build a transformation with proj_create(), indeed there's no way to figure out which CRS was source and target) |
Thanks for the tip, applying proj_normalize_for_visualization() to each operation returned by proj_list_get() works for me. |
PROJ6 support - Convert lowercase epsg to uppercase EPSG - Axis order of a CRS is no longer always easting, northing, it can also be northing, easting, e.g. EPSG:4326. If source and target CRS are available, axis order is adjusted using proj_normalize_for_visualization() to easting, northing if needed. - Previously, GRASS did conversions of degress to/from radians and meters from/to map units. Now PROJ6+ might do these conversions itself, or not. GRASS is now doing the conversions only if needed. - In PROJ6+, there can be several different operations to transform coordinates from one CRS to another CRS. If more than one operation is available, information is provided about these different operations. The current region is used by PROJ to select an appropriate operation.
Backported to relbranch78 in 66b4ea2 |
PROJ6 support - Convert lowercase epsg to uppercase EPSG - Axis order of a CRS is no longer always easting, northing, it can also be northing, easting, e.g. EPSG:4326. If source and target CRS are available, axis order is adjusted using proj_normalize_for_visualization() to easting, northing if needed. - Previously, GRASS did conversions of degress to/from radians and meters from/to map units. Now PROJ6+ might do these conversions itself, or not. GRASS is now doing the conversions only if needed. - In PROJ6+, there can be several different operations to transform coordinates from one CRS to another CRS. If more than one operation is available, information is provided about these different operations. The current region is used by PROJ to select an appropriate operation.
PROJ6 support - Convert lowercase epsg to uppercase EPSG - Axis order of a CRS is no longer always easting, northing, it can also be northing, easting, e.g. EPSG:4326. If source and target CRS are available, axis order is adjusted using proj_normalize_for_visualization() to easting, northing if needed. - Previously, GRASS did conversions of degress to/from radians and meters from/to map units. Now PROJ6+ might do these conversions itself, or not. GRASS is now doing the conversions only if needed. - In PROJ6+, there can be several different operations to transform coordinates from one CRS to another CRS. If more than one operation is available, information is provided about these different operations. The current region is used by PROJ to select an appropriate operation.
Convert lowercase epsg to uppercase EPSG
Axis order of a CRS is no longer always easting, northing, it can also be northing, easting, e.g. EPSG:4326. If source and target CRS are available, axis order is adjusted using proj_normalize_for_visualization() to easting, northing if needed.
Previously, GRASS did conversions of degress to/from radians and meters from/to map units. Now PROJ6+ might do these conversions itself, or not. GRASS is now doing the conversions only if needed.
In PROJ6+, there can be several different operations to transform coordinates from one CRS to another CRS. If more than one operation is available, information is provided about these different operations and the user has to provide one of these as pipeline option to r.proj or v.proj. That means that automated reprojection on the fly with r.import or v.import no longer works if more than one operation is available. Further on, the user must take care about axis order and remove any axisswap step from the pipeline if needed.
Example trying to convert from EPSG:2264 to EPSG:4326:
The problems here are 1) there is more than one operation available, 2) there is an axisswap that needs to be removed. Both problems need to be resolved by the user.