Split point cloud index into implementation & wrapper #59939

dvdkon · 2024-12-17T14:09:04Z

Description

This PR renames QgsPointCloudIndex to QgsAbstractPointCloudIndex and introduces a new class, QgsPointCloudIndex, which serves as a wrapper for a shared_ptr of the abstract index. This mirrors the arrangement used for QgsTiledSceneIndex and codifies good memory management practices (use of smart pointers for a long-lived shared object) while not exposing the std::shared_pointer directly in the API.

Personally, I see the use of a wrapper class as a hack around SIP's lack of support for smart pointers and would much rather see a simple using QgsPointCloudIndex = std::shared_ptr<...>;. That would, however, require much more complex changes, and the imperfect route chosen in this PR allows us to use good memory management practices today, without any long-term debt (in the future it would be simple to switch to using smart pointers directly).

github-actions · 2024-12-17T14:37:01Z

🪟 Windows builds

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

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 486e3b0)

src/3d/qgspointcloudlayerchunkloader_p.h

nyalldawson · 2024-12-17T22:20:55Z

src/3d/qgspointcloudlayerchunkloader_p.h

@@ -127,7 +127,7 @@ class QgsPointCloudLayerChunkedEntity : public QgsChunkedEntity
 {
    Q_OBJECT
  public:
-    explicit QgsPointCloudLayerChunkedEntity( Qgs3DMapSettings *map, QgsPointCloudIndex *pc, const QgsCoordinateTransform &coordinateTransform, QgsPointCloud3DSymbol *symbol, float maxScreenError, bool showBoundingBoxes, double zValueScale, double zValueOffset, int pointBudget );
+    explicit QgsPointCloudLayerChunkedEntity( Qgs3DMapSettings *map, QgsPointCloudIndex pc, const QgsCoordinateTransform &coordinateTransform, QgsPointCloud3DSymbol *symbol, float maxScreenError, bool showBoundingBoxes, double zValueScale, double zValueOffset, int pointBudget );


Suggested change

explicit QgsPointCloudLayerChunkedEntity( Qgs3DMapSettings *map, QgsPointCloudIndex pc, const QgsCoordinateTransform &coordinateTransform, QgsPointCloud3DSymbol *symbol, float maxScreenError, bool showBoundingBoxes, double zValueScale, double zValueOffset, int pointBudget );

explicit QgsPointCloudLayerChunkedEntity( Qgs3DMapSettings *map, const QgsPointCloudIndex &pc, const QgsCoordinateTransform &coordinateTransform, QgsPointCloud3DSymbol *symbol, float maxScreenError, bool showBoundingBoxes, double zValueScale, double zValueOffset, int pointBudget );

src/core/pointcloud/qgspointcloudindex.h

src/3d/symbols/qgspointcloud3dsymbol_p.h

nyalldawson · 2024-12-17T22:23:29Z

src/core/pointcloud/qgscopcpointcloudindex.h

-    bool gpsTimeFlag() const;
+    QVariantMap extraMetadata() const override;


This is more fragile vs the explicit getter -- I'd rather not see us use QVariantMap for purposes like this

@nyalldawson the reasoning is that the gpsTimeFlag is something very specific to one particular file format, with quite minor impact, and it does not deserve to be a part of the public API. And it seemed that over time, we may want to pass more format-specific metadata read from the file header - hence introduction of this fairly generic extraMetadata() function. Given that it is not used for any core functionality, it seems okay to use QVariantMap in this case (I also don't like to see QVariantMap in API)

src/core/pointcloud/qgspointclouddataprovider.cpp

src/core/pointcloud/qgspointcloudindex.h

src/core/pointcloud/qgspointclouddataprovider.h

nyalldawson · 2024-12-17T22:26:00Z

src/core/pointcloud/qgspointclouddataprovider.h

@@ -279,10 +279,10 @@ class CORE_EXPORT QgsPointCloudDataProvider: public QgsDataProvider
    QString mSubsetString;

    //! Identify in a specific index (used for sub-indexes)
-    QVector<QVariantMap> identify( QgsPointCloudIndex *index, double maxError, const QgsGeometry &extentGeometry, const QgsDoubleRange &extentZRange, int pointsLimit ) SIP_SKIP ;
+    QVector<QVariantMap> identify( QgsPointCloudIndex index, double maxError, const QgsGeometry &extentGeometry, const QgsDoubleRange &extentZRange, int pointsLimit ) SIP_SKIP ;


Suggested change

QVector<QVariantMap> identify( QgsPointCloudIndex index, double maxError, const QgsGeometry &extentGeometry, const QgsDoubleRange &extentZRange, int pointsLimit ) SIP_SKIP ;

QVector<QVariantMap> identify( const QgsPointCloudIndex &index, double maxError, const QgsGeometry &extentGeometry, const QgsDoubleRange &extentZRange, int pointsLimit ) SIP_SKIP ;

elsewhere too

Thanks for the comments, I'll look through them tomorrow. But I don't think QgsPointCloudIndex should be passed by reference. The point of the class is that it's basically an std::shared_ptr. Passing it by reference might remove a refcount increment at the cost of more indirection, but I'd rather just pass it by value for simplicity.

If we had the full range of C++ to play with, I'd personally either use std::shared_ptr<QgsAbstractPointCloudIndex> or QgsAbstractPointCloudIndex &, but as-is I'd just use the wrapper everywhere.

Hmm -- I would still push towards references, in all cases EXCEPT those where we know that we're explicitly storing the result in a member variable (and could possibly benefit from a move).

(In general we try to follow Qt's approach for things like this, and they always pass QString by reference, even though it's implicitly shared and copies are cheap.)

nyalldawson · 2024-12-17T22:30:15Z

Looking good, thank you! Can you also add some python based tests just to ensure that the bindings are working correctly?

dvdkon · 2024-12-20T12:07:56Z

Looking good, thank you! Can you also add some python based tests just to ensure that the bindings are working correctly?

Thanks! I've exposed the index() method on the data provider as well and added a simple Python test accessing the index.

I've had some trouble getting the SIP bindings to build after this one change, hopefully it passes CI.

The typedef is only present in the .sip file, but without this annotation SIP assumes it also exists in QGIS headers. This didn't completely break the build, since we concatenate multiple SIP headers into larger files, so the typedefs from one part "fixed" another. Run sip-build without --concatenate to see the issue clearly. This fix is currently sadly not fully effective due to a SIP bug: Python-SIP/sip#66

dvdkon · 2025-01-03T11:54:19Z

This PR ran into a long-standing QGIS build issue, where if the SIP files weren't concatenated "right", missing typedefs would break the build. This was because the typedefs used in .sip files for inheriting from template classes (not possible directly) weren't annotated correctly with /NoTypeName/, so SIP assumed that the typedef was also present in QGIS headers. The concatenation process papered over this by letting a typedef from one file (SIP creates a file per class by default) affect another.

I fixed this issue in d586c28, but also had to put in a quick bodge in 486e3b0 to work around Python-SIP/sip#66. I've added the commits to this PR, since they're necessary to consistently build it.

wonder-sk

Looking great, thanks!

And thanks a lot for looking into the sip build issues! 👏

dvdkon marked this pull request as ready for review December 17, 2024 14:09

github-actions bot added this to the 3.42.0 milestone Dec 17, 2024

wonder-sk mentioned this pull request Dec 17, 2024

Clean up point cloud index and improve its thread safety qgis/QGIS-Enhancement-Proposals#290

Closed

dvdkon force-pushed the point-cloud-index-refactor-2 branch from 9a58078 to 5903cc9 Compare December 17, 2024 19:54

nyalldawson reviewed Dec 17, 2024

View reviewed changes

dvdkon force-pushed the point-cloud-index-refactor-2 branch from 0b30cf2 to 167bb26 Compare December 20, 2024 12:06

dvdkon force-pushed the point-cloud-index-refactor-2 branch from 167bb26 to fbea1ee Compare December 20, 2024 12:15

dvdkon added 4 commits December 28, 2024 14:02

Split point cloud index into implementation & wrapper

7f9f3f6

Move QgsPointCloudAccessType to Qgis

abe5640

Apply changes per review

4531093

Expose QgsPointCloudIndex to Python

c714c55

dvdkon force-pushed the point-cloud-index-refactor-2 branch from fbea1ee to c714c55 Compare December 28, 2024 15:40

dvdkon added 2 commits January 3, 2025 10:10

Work around SIP bug

486e3b0

dvdkon force-pushed the point-cloud-index-refactor-2 branch from 6caeec6 to 486e3b0 Compare January 3, 2025 09:10

wonder-sk approved these changes Jan 6, 2025

View reviewed changes

wonder-sk merged commit 5c973b1 into qgis:master Jan 6, 2025
30 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split point cloud index into implementation & wrapper #59939

Split point cloud index into implementation & wrapper #59939

dvdkon commented Dec 17, 2024

github-actions bot commented Dec 17, 2024 •

edited

Loading

nyalldawson Dec 17, 2024

nyalldawson Dec 17, 2024

wonder-sk Dec 18, 2024

nyalldawson Dec 17, 2024

dvdkon Dec 17, 2024

dvdkon Dec 17, 2024

nyalldawson Dec 17, 2024

nyalldawson commented Dec 17, 2024

dvdkon commented Dec 20, 2024

dvdkon commented Jan 3, 2025

wonder-sk left a comment

	explicit QgsPointCloudLayerChunkedEntity( Qgs3DMapSettings map, QgsPointCloudIndex pc, const QgsCoordinateTransform &coordinateTransform, QgsPointCloud3DSymbol symbol, float maxScreenError, bool showBoundingBoxes, double zValueScale, double zValueOffset, int pointBudget );
	explicit QgsPointCloudLayerChunkedEntity( Qgs3DMapSettings map, const QgsPointCloudIndex &pc, const QgsCoordinateTransform &coordinateTransform, QgsPointCloud3DSymbol symbol, float maxScreenError, bool showBoundingBoxes, double zValueScale, double zValueOffset, int pointBudget );

		bool gpsTimeFlag() const;
		QVariantMap extraMetadata() const override;

	QVector<QVariantMap> identify( QgsPointCloudIndex index, double maxError, const QgsGeometry &extentGeometry, const QgsDoubleRange &extentZRange, int pointsLimit ) SIP_SKIP ;
	QVector<QVariantMap> identify( const QgsPointCloudIndex &index, double maxError, const QgsGeometry &extentGeometry, const QgsDoubleRange &extentZRange, int pointsLimit ) SIP_SKIP ;

Split point cloud index into implementation & wrapper #59939

Split point cloud index into implementation & wrapper #59939

Conversation

dvdkon commented Dec 17, 2024

Description

github-actions bot commented Dec 17, 2024 • edited Loading

🪟 Windows builds

🪟 Windows Qt6 builds

nyalldawson Dec 17, 2024

Choose a reason for hiding this comment

nyalldawson Dec 17, 2024

Choose a reason for hiding this comment

wonder-sk Dec 18, 2024

Choose a reason for hiding this comment

nyalldawson Dec 17, 2024

Choose a reason for hiding this comment

dvdkon Dec 17, 2024

Choose a reason for hiding this comment

dvdkon Dec 17, 2024

Choose a reason for hiding this comment

nyalldawson Dec 17, 2024

Choose a reason for hiding this comment

nyalldawson commented Dec 17, 2024

dvdkon commented Dec 20, 2024

dvdkon commented Jan 3, 2025

wonder-sk left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024 •

edited

Loading