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 3D extent to providers (1st stage) #54941

Merged
merged 9 commits into from
Nov 16, 2023

Conversation

benoitdm-oslandia
Copy link
Contributor

@benoitdm-oslandia benoitdm-oslandia commented Oct 16, 2023

This PR brings core modifications to allow providers to handle 3D extent. This is the first stage to prepare next PRs with dedicated modifications per provider.

  • we add basic member functions to QgsDataProvider to handle 3D extent
  • homogenize elevationdataproperties usage
  • update QgsMapLayer to support 3D extent

Funded by CEA/DAM @renardf

cc @ptitjano

@benoitdm-oslandia benoitdm-oslandia self-assigned this Oct 16, 2023
@github-actions github-actions bot added this to the 3.34.0 milestone Oct 16, 2023
@benoitdm-oslandia benoitdm-oslandia added Data Provider Related to specific vector, raster or mesh data providers 3D Relates to QGIS' 3D engine or rendering labels Oct 16, 2023
@benoitdm-oslandia benoitdm-oslandia force-pushed the gh/feature/provider-3d branch 2 times, most recently from f68481b to aa362be Compare October 16, 2023 11:51
@nyalldawson
Copy link
Collaborator

This is great! Can you add the missing doxygen since notations too? (I assume this is targeting 3.36?)

@nirvn
Copy link
Contributor

nirvn commented Oct 17, 2023

@benoitdm-oslandia , exciting stuff! What immediately comes to mind here is the possibility to do elevation filtering for the map canvas (the same way we do temporal filtering) with provider-side filtering capability. Lovely.

Out of curiosity, what are you guys planning to do with this?

@benoitdm-oslandia
Copy link
Contributor Author

@benoitdm-oslandia , exciting stuff! What immediately comes to mind here is the possibility to do elevation filtering for the map canvas (the same way we do temporal filtering) with provider-side filtering capability. Lovely.

Out of curiosity, what are you guys planning to do with this?

@nirvn we are looking to push an improved extent filtering in the 3D view like this preview:
bounding-box

@benoitdm-oslandia
Copy link
Contributor Author

@rouault @nyalldawson can you review this PR please?

Copy link
Member

@lbartoletti lbartoletti left a comment

Choose a reason for hiding this comment

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

LGTM, except for a minor nitpick

tests/src/core/testqgsvectordataprovider.cpp Outdated Show resolved Hide resolved
tests/src/core/testqgsvectordataprovider.cpp Outdated Show resolved Hide resolved
@rouault
Copy link
Contributor

rouault commented Nov 16, 2023

As I mentionned I'm a bit nervous for performance reasons seing QgsVectorLayer::extent() to be redirected to QgsVectorLayer::extent3D().toRectangle().
At least, perhaps just as a temporary measure, it would probably be best if QgsVectorLayer::extent() implementation was left unmodified (as well as QgsVectorLayer::sourceExtent(). If after all the dust has settled there's a way of unifying QgsVectorLayer::extent() and extent3D(), that's be good of course

@benoitdm-oslandia
Copy link
Contributor Author

As I mentionned I'm a bit nervous for performance reasons seing QgsVectorLayer::extent() to be redirected to QgsVectorLayer::extent3D().toRectangle().
At least, perhaps just as a temporary measure, it would probably be best if QgsVectorLayer::extent() implementation was left unmodified (as well as QgsVectorLayer::sourceExtent(). If after all the dust has settled there's a way of unifying QgsVectorLayer::extent() and extent3D(), that's be good of course

I can re-add the previous code for the QgsVectorLayer::extent().

@benoitdm-oslandia
Copy link
Contributor Author

@rouault @lbartoletti @nyalldawson all requests are fulfilled. Is it OK for you?

@benoitdm-oslandia
Copy link
Contributor Author

🍾

@lbartoletti lbartoletti merged commit 665576e into qgis:master Nov 16, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3D Relates to QGIS' 3D engine or rendering Data Provider Related to specific vector, raster or mesh data providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants