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.

Sign up for GitHub

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

Jump to bottom

Delegate bounding box transformation to proj on >= 8.2 #60331

Merged
merged 15 commits into from
Feb 4, 2025
Merged

Delegate bounding box transformation to proj on >= 8.2 #60331

merged 15 commits into from
Feb 4, 2025

Conversation

nyalldawson
Copy link
Collaborator

@nyalldawson nyalldawson commented Jan 29, 2025 •
edited
Loading

Fixes #59821

@nyalldawson nyalldawson added the NOT FOR MERGE Don't merge! label Jan 29, 2025
@github-actions github-actions bot added this to the 3.42.0 milestone Jan 29, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 29, 2025 •
edited
Loading

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 537daf1)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 537daf1)

@qgis qgis deleted a comment from github-actions bot Jan 30, 2025
@qgis qgis deleted a comment from github-actions bot Jan 30, 2025
@nyalldawson
Copy link
Collaborator Author

@rouault is 8edbe0d a known requirement for proj_trans_bounds?

@rouault
Copy link
Contributor

rouault commented Jan 30, 2025

is 8edbe0d a known requirement for proj_trans_bounds?

on released versions of PROJ, yes, since I fixed 3D support only in PROJ master per OSGeo/PROJ#4333 a couple months ago

@nyalldawson
Copy link
Collaborator Author

Thanks @rouault !

I noticed that fix is similar to my workaround in 8edbe0d. Do you think we should:

  1. Defer this whole change till newer proj is available.
  2. Use the (buggy) old qgis logic when a compound crs is in play
  3. Or conditionally apply the same logic as the proj fix?

@rouault
Copy link
Contributor

rouault commented Jan 30, 2025

Do you think we should:

I'd just add a PROJ_VERSION < 9.6 protection around your commit

@nyalldawson
Don't transform 2d bounding boxes when geocentric crs involved
2a196ce 
This is meaningless and will give a misleading/useless result,
so raise a descriptive QgsCsException instead.
@nyalldawson
Test mask
94df518
@nyalldawson nyalldawson changed the title Attempt to delegate bounding box transformation to proj on >= 8.2 Delegate bounding box transformation to proj on >= 8.2 Feb 4, 2025
@nyalldawson nyalldawson removed the NOT FOR MERGE Don't merge! label Feb 4, 2025
@qgis qgis deleted a comment from github-actions bot Feb 4, 2025
@qgis qgis deleted a comment from github-actions bot Feb 4, 2025
@nyalldawson
Copy link
Collaborator Author

Ok, this is ready for review.

I'm also interested in thoughts on backporting -- #59821 is very serious, but these changes ARE quite risky...

@qgis qgis deleted a comment from github-actions bot Feb 4, 2025
@qgis qgis deleted a comment from github-actions bot Feb 4, 2025
@rouault
Copy link
Contributor

rouault commented Feb 4, 2025

I'm also interested in thoughts on backporting -- #59821 is very serious, but these changes ARE quite risky...

I would go for the backport

@nyalldawson nyalldawson merged commit 4b66a86 into qgis:master Feb 4, 2025
33 checks passed
@nyalldawson nyalldawson deleted the proj_bounds21 branch February 4, 2025 20:15
@qgis-bot qgis-bot mentioned this pull request Feb 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouault rouault rouault approved these changes

Assignees
No one assigned
Labels
backport release-3_40
Projects
None yet
Milestone
3.42.0
Development

Successfully merging this pull request may close these issues.

Vector Error when changing CRS
2 participants
@nyalldawson @rouault