Split point cloud index into implementation & wrapper

[dvdkon]

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]

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

[nyalldawson]

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 );

[nyalldawson]

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

[wonder-sk]

@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)

[nyalldawson]

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

[dvdkon]

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.

[dvdkon]

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.

[nyalldawson]

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]

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

[dvdkon]

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.