Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What the fix does
Fixes a bug in
CoordinateOperation::normalizeForVisualization()(thefunction behind pyproj's
always_xy=Trueandproj_normalize_for_visualization())where a Geographic3D <-> VerticalCRS pipeline keeps a leftover
+proj=axisswap +order=2,1step. In practice this means the horizontalcoordinates come out lat/lon instead of lon/lat even though
always_xy=Truewas requested, with no warning or error.
Example:
EPSG:4937(ETRS89, geographic 3D) ->EPSG:5776(NN54 height).Why it happens
A
VerticalCRShas no horizontal axes, soCRS::mustAxisOrderBeSwitchedForVisualization()always returnsfalseforit, which is correct on its own - there's no axis order to reverse on a 1D
CRS.
The problem is that the actual operation between a geographic and a vertical
CRS still carries a horizontal pass-through internally (built from the
geographic side on both ends of the pipeline), and that pass-through is
symmetric by construction.
normalizeForVisualization()decides whether toswap each end independently, so it ends up swapping only the geographic end
and leaving the vertical end alone - which breaks that symmetry and leaves
one of the two swaps stranded in the final pipeline.
The fix
Added
CRS::hasNoHorizontalAxes(), and innormalizeForVisualization(),when exactly one end of the operation has no horizontal axes, that end now
just inherits the other end's swap decision instead of being evaluated on
its own.
I deliberately kept this at the operation level rather than touching
mustAxisOrderBeSwitchedForVisualization()itself, since that function isalso used to transpose area-of-use extents, and making a VerticalCRS report
truethere would mess up bounding boxes for unrelated code paths.Testing
TEST(operation, normalizeForVisualization)(test/unit/test_operation.cpp) coveringEPSG:4937 -> EPSG:5776and its reverse, asserting no residualaxisswapremains in the normalized pipelineScope
This only covers Geographic3D <-> VerticalCRS.
EngineeringCRSpairs withaxis directions like north/west (e.g. EPSG:5800) have a related problem, but
fixing that needs an axis direction flip that
Conversion::createAxisOrderReversalcan't express today, so it's aseparate, bigger change and out of scope here.