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 option to allow export of Geographic/Projected 3D CRS in WKT1_GDAL #2450

Merged

Conversation

rouault
Copy link
Member

@rouault rouault commented Nov 24, 2020

as CompoundCRS with a VerticalCRS being an ellipsoidal height, which is
not conformant. But needed for LAS 1.4 that only supports WKT1

as CompoundCRS with a VerticalCRS being an ellipsoidal height, which is
not conformant. But needed for LAS 1.4 that only supports WKT1
@rouault rouault force-pushed the setAllowEllipsoidalHeightAsVerticalCRS branch from aaa583a to 4caf32a Compare November 24, 2020 18:52
@jjimenezshaw
Copy link
Contributor

jjimenezshaw commented Nov 25, 2020

Thanks @rouault . How does it behave with projected CRSs not in meters? In the US it is common to use ft or ftUS. In this case, it makes sense to use the same unit for the three axes. Even with geographic coordinates, it could be needed with feet. (this is something I miss from promoteTo3D)

@rouault
Copy link
Member Author

rouault commented Nov 25, 2020

How does it behave with projected CRSs not in meters?

it will indicate whatever unit is used for the vertical axis

@jjimenezshaw
Copy link
Contributor

jjimenezshaw commented Nov 25, 2020

it will indicate whatever unit is used for the vertical axis

This PR is about the export to WKT1, not how this vertical axis was generated. Sorry for the misunderstanding.

@msmitherdc
Copy link
Contributor

@rouault This fix worked well, can we target it for 7.2.1 vs 8.0?

@kbevers
Copy link
Member

kbevers commented Nov 29, 2020

This fix worked well, can we target it for 7.2.1 vs 8.0?

Nope. New features does not go into bug fix releases.

@msmitherdc
Copy link
Contributor

I would counter that this isn't really a new feature, its a restoration of previous (if not quite conformant) behavior. We have a fair amount of that was written this way

@rouault
Copy link
Member Author

rouault commented Nov 29, 2020

For what is worth, I'd be OK to backport that in 7.2.1. The new code paths don't change existing behavior (you need to specify the new option), so there's no backward incompatibility

@kbevers
Copy link
Member

kbevers commented Nov 29, 2020

A new options is added to projinfo and two new functions to the C++ API. It may be fixing a problem but it is also introducing new functionality. Is this really so important that it can't way two extra months?

@msmitherdc
Copy link
Contributor

This is a function that PDAL uses and its preventing conversion of LAS 1.4 pointclouds due to the way that WKT is stored in LAS1.4. So it does have an impact.

@rouault
Copy link
Member Author

rouault commented Nov 29, 2020

A new options is added to projinfo and two new functions to the C++ API.

I could possibly make the new C++ methods internal only and remove the new projinfo option in the 7.2 backport. PDAL through GDAL will only needs the new option value in the "char**" options of proj_as_wkt()

@kbevers
Copy link
Member

kbevers commented Nov 29, 2020

Sounds like a good solution to me

@rouault rouault merged commit edd7f16 into OSGeo:master Nov 29, 2020
@msmitherdc
Copy link
Contributor

Good solution, thanks @rouault

rouault added a commit to rouault/PROJ that referenced this pull request Nov 29, 2020
as CompoundCRS with a VerticalCRS being an ellipsoidal height, which is
not conformant. But needed for LAS 1.4 that only supports WKT1

This is a partial backport of OSGeo#2450,
with only the new ALLOW_ELLIPSOIDAL_HEIGHT_AS_VERTICAL_CRS=YES option
of proj_as_wkt()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants