Skip to content
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 proj_normalize_for_visualization() #1387

Merged
merged 1 commit into from Mar 29, 2019

Conversation

Projects
None yet
3 participants
@rouault
Copy link
Member

commented Mar 28, 2019

Fixes #1301

This function takes the output PJ from proj_create_crs_to_crs(),
and add (or undo) the needed axis swap operations so that the
object returned by proj_normalize_for_visualization() has the usual
GIS axis order.
In this implementation, this does something only if the coordinate
system of the source or target CRS, geographic or projected, has
NORTH, EAST ordering.
CompoundCRS wrapping those objects are also handled.

Question: should this be backported to 6.0.x ?

@rouault rouault force-pushed the rouault:proj_normalize_for_visualization branch from 990e1c5 to d4fc128 Mar 28, 2019

@kbevers

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

In this implementation, this does something only if the source or
target CRS have latitude,longitude ordering.

But it is not limited to only coordinate systems with geodetic coordinates, right? The code seems to be checking projected coordinates as well but from the documentation it seems as if it only applies when latitudes and longitudes (i.e. angular coordinates) are involved.

Question: should this be backported to 6.0.x ?

No. This adds a new function so should not be included in a bug-fix release.

@rouault

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

But it is not limited to only coordinate systems with geodetic coordinates, right? The code seems to be checking projected coordinates as well but from the documentation it seems as if it only applies when latitudes and longitudes (i.e. angular coordinates) are involved.

ah sorry, the commit message is wrong. I'm going to fix it. It does the inversion for geographic or projected CRS with axis order direction NORTH, EAST.

@rouault rouault force-pushed the rouault:proj_normalize_for_visualization branch from d4fc128 to d658ed1 Mar 28, 2019

@kbevers kbevers added this to the 6.1.0 milestone Mar 28, 2019

@nyalldawson

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

I'd like to humbly request that this IS backported to 6.0 -- without it you're requiring all clients to implement their own version of https://github.com/pramsey/postgis/blob/7ecf6839c57a838e2c8540001a3cd35b78a730db/liblwgeom/lwgeom_transform.c#L299 in order to correctly project x/y coordinates. I think it's MUCH safer to have this sitting in the underlying proj library then have multiple varying versions of this test floating out their in the client applications.

Add proj_normalize_for_visualization()
Fixes #1301

This function takes the output PJ from proj_create_crs_to_crs(),
and add (or undo) the needed axis swap operations so that the
object returned by proj_normalize_for_visualization() has the usual
GIS axis order.
In this implementation, this does something only if the coordinate
system of the source or target CRS, geographic or projected, has
NORTH, EAST ordering.
CompoundCRS wrapping those objects are also handled.

@rouault rouault force-pushed the rouault:proj_normalize_for_visualization branch from d658ed1 to 6a7e24d Mar 28, 2019

@kbevers

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I think it's MUCH safer to have this sitting in the underlying proj library then have multiple varying versions of this test floating out their in the client applications.

I absolutely agree. I am just not very keen on deviating from semantic versioning rules. The simple solution is to release 6.1.0 right away instead of releasing a few patch releases first. Thoughts?

@rouault

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

The simple solution is to release 6.1.0 right away instead of releasing a few patch releases first. Thoughts?

So the 6.0.1 release of May 1st would become a 6.1.0 ? I'm fine with that.

@kbevers

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

So the 6.0.1 release of May 1st would become a 6.1.0 ? I'm fine with that.

Yes. It would also fit well with the changes to the database. Are there any other functions that would be beneficial to add before May 1st?

@rouault rouault merged commit 8d7a01b into OSGeo:master Mar 29, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.03%) to 85.324%
Details

@kbevers kbevers modified the milestones: 6.2.0, 6.1.0 Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.