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

Vertical transformation of features in iterators #58079

Merged
merged 9 commits into from
Jul 17, 2024

Conversation

nyalldawson
Copy link
Collaborator

This PR adds API to ensure that z values in features are correctly transformed by iterators when a coordinate transform involving vertical components is in effect.

Sets the coordinate transform which will be used to transform
the feature's geometries.

If this transform is valid then it will always be used to transform
features, regardless of the destinationCrs() setting or the underlying
feature source's actual CRS.

This method should be used with caution, and it is recommended
to use the high-level setDestinationCrs() method instead. Setting a specific
transform should only be done when there is a requirement to use a particular
transform, eg when a specific, non-default coordinate operation
MUST be used for the transformation.
@nyalldawson nyalldawson added the API API improvement only, no visible user interface changes label Jul 12, 2024
@github-actions github-actions bot added this to the 3.40.0 milestone Jul 12, 2024
Copy link

github-actions bot commented Jul 12, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 87fad86)

@@ -533,6 +537,196 @@ def callback(feature):
self.assertEqual(res, ['a', 'b'])
layer.rollBack()

def test_vertical_transformation_gda2020_to_AVWS(self):
"""
Test vertical transformations are correctly handled during iteration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests are fragile as they require grids to be present, and may depend on the actual PROJ version. Perhaps just testing a transformation between WGS 84 geocentric (EPSG:4978) and WGS 84 3D (EPSG:4979) would be more robust.
Or passing a dummy PROJ pipeline for example with a +proj=affine +xoff=... +yoff=.... +zoff=.... (https://proj.org/en/9.4/operations/transformations/affine.html)
It is also not obvious to me those 4 slightly different tests test different aspect of this PR (some of them might test aspects already implemented regarding 3D / compound CRS)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rouault what if I made them skipped if the grid isn't available?

I really want tests which check real world conversions which match with officially published results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer the second part of your question -- they are "katamari" tests which check that the full set of vertical crs handling is working. They'll fail without any of the commits in this pr, but also depend on earlier commits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your call... but that's still fragile. For example if a new geoid model is released in the future for EPSG:5711 / AHD height, the results could change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I'm leaning towards adding the stable test using 4978/4979, and then leaving the existing ones for security.

(I once worked in an office where someone screwed up a multi-million dollar project by messing up a vertical transform ... So I'm kinda paranoid here 😱)

@nyalldawson
Copy link
Collaborator Author

Hmm, the crashing vector layer cache test isn't caused by this PR . I've fixed the underlying issue in #58093

@nyalldawson nyalldawson reopened this Jul 13, 2024
@nyalldawson nyalldawson merged commit 49a8d2f into qgis:master Jul 17, 2024
63 of 64 checks passed
@nyalldawson nyalldawson deleted the request_transform branch July 17, 2024 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants