-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[Virtual Point Clouds] Add combobox to adjust the switching between rendering overview only and sub index rendering #64351
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
base: master
Are you sure you want to change the base?
Conversation
Tests failed for Qt 6 (ALL_BUT_PROVIDERS - ubuntu)One or more tests failed using the build from commit a9eb485 virtual_pointcloud_3d_overview (testPointCloud3DOverview)virtual_pointcloud_3d_overviewTest failed at testPointCloud3DOverview at tests/src/3d/testqgspointcloud3drendering.cpp:619
The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
Tests failed for Qt 6 (ALL_BUT_PROVIDERS - fedora)One or more tests failed using the build from commit a9eb485 virtual_pointcloud_3d_overview (testPointCloud3DOverview)virtual_pointcloud_3d_overviewTest failed at testPointCloud3DOverview at tests/src/3d/testqgspointcloud3drendering.cpp:619
The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. 🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. |
slider holds values with which the sub index extent/3d bounding box is mutliplied by and depending on the map canvas containing the extent or camera being within the scaled bounding box, we render the full sub index or just the overview
8657483 to
e68fb10
Compare
rename zoomOutMultiplier to overviewSwitchingScale
| <widget class="QComboBox" name="mZoomOutOptions"/> | ||
| </item> | ||
| <item row="1" column="1" colspan="2"> | ||
| <widget class="QComboBox" name="mOverviewSwitchingScale"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a tooltip for the combo box?
Something along the lines:
Sets the zoom level at which the individual point cloud files will get rendered instead of the overview or their extents.\n
Normal means roughly when the point cloud extent no longer fits the map extent.\n
Select Earlier to switch at a smaller scale (zoomed out) or Later to switch at a larger scale (zoomed in).
| Qgis::PointCloudZoomOutRenderBehavior zoomOutBehavior() const { return mZoomOutBehavior; } | ||
|
|
||
| /** | ||
| * Sets the overview switching scale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand the two docstrings to explain the effect it has / how it works?
| @@ -712,6 +712,18 @@ class CORE_EXPORT QgsPointCloudRenderer | |||
| */ | |||
| Qgis::PointCloudZoomOutRenderBehavior zoomOutBehavior() const { return mZoomOutBehavior; } | |||
|
|
|||
| /** | |||
| * Sets the overview switching scale | |||
| * \since QGIS 4.0 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for these
| @@ -64,7 +64,16 @@ class GUI_EXPORT QgsPointCloudRendererPropertiesWidget : public QgsMapLayerConfi | |||
| void emitWidgetChanged(); | |||
|
|
|||
| private: | |||
| const QMap<double, QString> mOverviewSwitchingScaleMap { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment that we have a similar setting for 3D in QgsPointCloud3DSymbolWidget, so we update them in parallel? (also a comment in QgsPointCloud3DSymbolWidget pointing here too)
uclaros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now!
|
@ViperMiniQ A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged. Please update the description (not the comments) with helpful description and screenshot to help the work from documentors. Thank you! |
|
This pull request has been tagged for the changelog.
You can edit the description. Format available for credits
Thank you! |
This PR adds a slider that controls the point of switching between rendering overview only and rendering a sub index of the VPC.
The slider is more aggressive going to the right, less aggressive going to the left.
On 2D map canvas, if the average extent width of the sub indexes multiplied by the value from the slider is higher than the viewable map canvas, a sub index is loaded, otherwise overview only gets rendered.
Similarly, in the 3D Map, multiplier is applied on the sub index bounding box and if the camera is located within the box, sub index is fully loaded and rendered, if the camera is outside the box, overview only is rendered.
One change is that when all the sub indexes are shown in the 3D Map, we disable overview rendering to save resources.