From 4c5775af4d036fceede33afafad9c6fd7c543589 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Ko=C5=88a=C5=99=C3=ADk?= Date: Wed, 8 Jan 2025 20:16:19 +0100 Subject: [PATCH 1/6] Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerRenderer --- .../pointcloud/qgspointcloudlayerrenderer.cpp | 46 ++++++++++--------- .../pointcloud/qgspointcloudlayerrenderer.h | 7 ++- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp index 809cc7fb548c..7c2195ead186 100644 --- a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp +++ b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp @@ -43,22 +43,20 @@ QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *layer, QgsRenderContext &context ) : QgsMapLayerRenderer( layer->id(), &context ) - , mLayer( layer ) + , mIndex( layer->dataProvider()->index() ) , mLayerName( layer->name() ) , mLayerAttributes( layer->attributes() ) , mSubIndexes( layer && layer->dataProvider() ? layer->dataProvider()->subIndexes() : QVector() ) , mFeedback( new QgsFeedback ) , mEnableProfile( context.flags() & Qgis::RenderContextFlag::RecordProfile ) { - // TODO: we must not keep pointer to mLayer (it's dangerous) - we must copy anything we need for rendering - // or use some locking to prevent read/write from multiple threads - if ( !mLayer || !mLayer->dataProvider() || !mLayer->renderer() ) + if ( !layer->dataProvider() || !layer->renderer() ) return; QElapsedTimer timer; timer.start(); - mRenderer.reset( mLayer->renderer()->clone() ); + mRenderer.reset( layer->renderer()->clone() ); if ( !mSubIndexes.isEmpty() ) { mSubIndexExtentRenderer.reset( new QgsPointCloudExtentRenderer() ); @@ -66,19 +64,26 @@ QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *laye mSubIndexExtentRenderer->setLabelTextFormat( mRenderer->labelTextFormat() ); } - if ( mLayer->index() ) + if ( mIndex ) { - mScale = mLayer->index().scale(); - mOffset = mLayer->index().offset(); + mScale = mIndex.scale(); + mOffset = mIndex.offset(); } - if ( const QgsPointCloudLayerElevationProperties *elevationProps = qobject_cast< const QgsPointCloudLayerElevationProperties * >( mLayer->elevationProperties() ) ) + if ( const QgsPointCloudLayerElevationProperties *elevationProps = qobject_cast< const QgsPointCloudLayerElevationProperties * >( layer->elevationProperties() ) ) { mZOffset = elevationProps->zOffset(); mZScale = elevationProps->zScale(); } - mCloudExtent = mLayer->dataProvider()->polygonBounds(); + if ( const QgsVirtualPointCloudProvider *vpcProvider = dynamic_cast( layer->dataProvider() ) ) + { + mAverageSubIndexWidth = vpcProvider->averageSubIndexWidth(); + mAverageSubIndexHeight = vpcProvider->averageSubIndexHeight(); + mOverviewIndex = vpcProvider->overview(); + } + + mCloudExtent = layer->dataProvider()->polygonBounds(); mClippingRegions = QgsMapClippingUtils::collectClippingRegionsForLayer( *renderContext(), layer ); @@ -129,9 +134,7 @@ bool QgsPointCloudLayerRenderer::render() return true; } - // TODO cache!? - QgsPointCloudIndex pc = mLayer->index(); - if ( mSubIndexes.isEmpty() && ( !pc || !pc.isValid() ) ) + if ( mSubIndexes.isEmpty() && ( !mIndex || !mIndex.isValid() ) ) { mReadyToCompose = true; return false; @@ -195,9 +198,9 @@ bool QgsPointCloudLayerRenderer::render() bool canceled = false; if ( mSubIndexes.isEmpty() ) { - canceled = !renderIndex( pc ); + canceled = !renderIndex( mIndex ); } - else if ( const QgsVirtualPointCloudProvider *vpcProvider = dynamic_cast( mLayer->dataProvider() ) ) + else if ( mOverviewIndex ) { QVector< QgsPointCloudSubIndex > visibleIndexes; for ( const QgsPointCloudSubIndex &si : mSubIndexes ) @@ -207,23 +210,22 @@ bool QgsPointCloudLayerRenderer::render() visibleIndexes.append( si ); } } - const bool zoomedOut = renderExtent.width() > vpcProvider->averageSubIndexWidth() || - renderExtent.height() > vpcProvider->averageSubIndexHeight(); - QgsPointCloudIndex overviewIndex = vpcProvider->overview(); + const bool zoomedOut = renderExtent.width() > mAverageSubIndexWidth || + renderExtent.height() > mAverageSubIndexHeight; // if the overview of virtual point cloud exists, and we are zoomed out, we render just overview - if ( vpcProvider->overview() && zoomedOut && + if ( mOverviewIndex && zoomedOut && mRenderer->zoomOutBehavior() == Qgis::PointCloudZoomOutRenderBehavior::RenderOverview ) { - renderIndex( overviewIndex ); + renderIndex( *mOverviewIndex ); } else { // if the overview of virtual point cloud exists, and we are zoomed out, but we want both overview and extents, // we render overview - if ( vpcProvider->overview() && zoomedOut && + if ( mOverviewIndex && zoomedOut && mRenderer->zoomOutBehavior() == Qgis::PointCloudZoomOutRenderBehavior::RenderOverviewAndExtents ) { - renderIndex( overviewIndex ); + renderIndex( *mOverviewIndex ); } mSubIndexExtentRenderer->startRender( context ); for ( const QgsPointCloudSubIndex &si : visibleIndexes ) diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.h b/src/core/pointcloud/qgspointcloudlayerrenderer.h index 8f94afc753ff..731a34c4615d 100644 --- a/src/core/pointcloud/qgspointcloudlayerrenderer.h +++ b/src/core/pointcloud/qgspointcloudlayerrenderer.h @@ -37,6 +37,7 @@ #include #include #include +#include class QgsRenderContext; class QgsPointCloudLayer; @@ -79,7 +80,7 @@ class CORE_EXPORT QgsPointCloudLayerRenderer: public QgsMapLayerRenderer void renderTriangulatedSurface( QgsPointCloudRenderContext &context ); bool renderIndex( QgsPointCloudIndex &pc ); - QgsPointCloudLayer *mLayer = nullptr; + QgsPointCloudIndex mIndex; QString mLayerName; std::unique_ptr< QgsPointCloudRenderer > mRenderer; @@ -94,7 +95,11 @@ class CORE_EXPORT QgsPointCloudLayerRenderer: public QgsMapLayerRenderer QgsPointCloudAttributeCollection mAttributes; QgsGeometry mCloudExtent; QList< QgsMapClippingRegion > mClippingRegions; + const QVector< QgsPointCloudSubIndex > mSubIndexes; + std::optional mOverviewIndex; + double mAverageSubIndexWidth; + double mAverageSubIndexHeight; int mRenderTimeHint = 0; bool mBlockRenderUpdates = false; From 185113b147987605d3063f1c4d1c450f6bae673a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Ko=C5=88a=C5=99=C3=ADk?= Date: Thu, 9 Jan 2025 11:28:10 +0100 Subject: [PATCH 2/6] Don't use data provider before we check it's non-null --- src/core/pointcloud/qgspointcloudlayerrenderer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp index 7c2195ead186..c1e76470ee08 100644 --- a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp +++ b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp @@ -43,7 +43,6 @@ QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *layer, QgsRenderContext &context ) : QgsMapLayerRenderer( layer->id(), &context ) - , mIndex( layer->dataProvider()->index() ) , mLayerName( layer->name() ) , mLayerAttributes( layer->attributes() ) , mSubIndexes( layer && layer->dataProvider() ? layer->dataProvider()->subIndexes() : QVector() ) @@ -53,6 +52,8 @@ QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *laye if ( !layer->dataProvider() || !layer->renderer() ) return; + mIndex = layer->dataProvider()->index(); + QElapsedTimer timer; timer.start(); From 3f8495a0a350c4ecabcf39719a994b0bbab4f5df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Ko=C5=88a=C5=99=C3=ADk?= Date: Sun, 12 Jan 2025 17:55:14 +0100 Subject: [PATCH 3/6] Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerProfileGenerator --- .../qgspointcloudlayerprofilegenerator.cpp | 12 +++++------- .../pointcloud/qgspointcloudlayerprofilegenerator.h | 4 ++++ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp b/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp index 7c9bee1362dc..2b6388a00308 100644 --- a/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp +++ b/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp @@ -341,6 +341,8 @@ void QgsPointCloudLayerProfileResults::copyPropertiesFromGenerator( const QgsAbs QgsPointCloudLayerProfileGenerator::QgsPointCloudLayerProfileGenerator( QgsPointCloudLayer *layer, const QgsProfileRequest &request ) : mLayer( layer ) + , mIndex( layer->dataProvider()->index() ) + , mSubIndexes( layer->dataProvider()->subIndexes() ) , mLayerAttributes( layer->attributes() ) , mRenderer( qgis::down_cast< QgsPointCloudLayerElevationProperties* >( layer->elevationProperties() )->respectLayerColors() && mLayer->renderer() ? mLayer->renderer()->clone() : nullptr ) , mMaximumScreenError( qgis::down_cast< QgsPointCloudLayerElevationProperties* >( layer->elevationProperties() )->maximumScreenError() ) @@ -386,17 +388,13 @@ bool QgsPointCloudLayerProfileGenerator::generateProfile( const QgsProfileGenera if ( !mLayer || !mProfileCurve || mFeedback->isCanceled() ) return false; - // this is not AT ALL thread safe, but it's what QgsPointCloudLayerRenderer does ! - // TODO: fix when QgsPointCloudLayerRenderer is made thread safe to use same approach - QVector indexes; - QgsPointCloudIndex mainIndex = mLayer->index(); - if ( mainIndex && mainIndex.isValid() ) - indexes.append( mainIndex ); + if ( mIndex && mIndex.isValid() ) + indexes.append( mIndex ); // Gather all relevant sub-indexes const QgsRectangle profileCurveBbox = mProfileCurve->boundingBox(); - for ( const QgsPointCloudSubIndex &subidx : mLayer->dataProvider()->subIndexes() ) + for ( const QgsPointCloudSubIndex &subidx : mSubIndexes ) { QgsPointCloudIndex index = subidx.index(); if ( index && index.isValid() && subidx.polygonBounds().intersects( profileCurveBbox ) ) diff --git a/src/core/pointcloud/qgspointcloudlayerprofilegenerator.h b/src/core/pointcloud/qgspointcloudlayerprofilegenerator.h index 53d3e40206c9..92702793ea28 100644 --- a/src/core/pointcloud/qgspointcloudlayerprofilegenerator.h +++ b/src/core/pointcloud/qgspointcloudlayerprofilegenerator.h @@ -25,6 +25,8 @@ #include "qgscoordinatetransform.h" #include "qgspointcloudattribute.h" #include "qgslinesymbol.h" +#include "qgspointcloudindex.h" +#include "qgspointcloudsubindex.h" #include "qgsvector3d.h" #include "qgsgeos.h" @@ -150,6 +152,8 @@ class CORE_EXPORT QgsPointCloudLayerProfileGenerator : public QgsAbstractProfile void visitBlock( const QgsPointCloudBlock *block, const QgsDoubleRange &zRange ); QPointer< QgsPointCloudLayer > mLayer; + QgsPointCloudIndex mIndex; + const QVector< QgsPointCloudSubIndex > mSubIndexes; QgsPointCloudAttributeCollection mLayerAttributes; std::unique_ptr< QgsPointCloudRenderer > mRenderer; double mMaximumScreenError = 0.3; From a3866f45732d29b3466e610717dc0505d7a1c312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Ko=C5=88a=C5=99=C3=ADk?= Date: Sun, 12 Jan 2025 19:30:27 +0100 Subject: [PATCH 4/6] Make thread check in QgsPointCloudLayer::dataProvider() fatal After the last 3 commits there should be no place left that calls this from a worker thread. --- src/core/pointcloud/qgspointcloudlayer.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/pointcloud/qgspointcloudlayer.cpp b/src/core/pointcloud/qgspointcloudlayer.cpp index 74da1df3688a..665006688e6d 100644 --- a/src/core/pointcloud/qgspointcloudlayer.cpp +++ b/src/core/pointcloud/qgspointcloudlayer.cpp @@ -133,8 +133,7 @@ QgsAbstractProfileGenerator *QgsPointCloudLayer::createProfileGenerator( const Q QgsPointCloudDataProvider *QgsPointCloudLayer::dataProvider() { - // BAD! 2D rendering of point clouds is NOT thread safe - QGIS_PROTECT_QOBJECT_THREAD_ACCESS_NON_FATAL + QGIS_PROTECT_QOBJECT_THREAD_ACCESS return mDataProvider.get(); } From 41742552f9f80b571c7a62e1dc7a08f1f710b0ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Ko=C5=88a=C5=99=C3=ADk?= Date: Wed, 22 Jan 2025 22:54:34 +0100 Subject: [PATCH 5/6] Use correct index() method to use editing index --- src/core/pointcloud/qgspointcloudlayer.cpp | 2 +- .../pointcloud/qgspointcloudlayerprofilegenerator.cpp | 8 ++++---- src/core/pointcloud/qgspointcloudlayerrenderer.cpp | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/pointcloud/qgspointcloudlayer.cpp b/src/core/pointcloud/qgspointcloudlayer.cpp index 665006688e6d..292d52d46e10 100644 --- a/src/core/pointcloud/qgspointcloudlayer.cpp +++ b/src/core/pointcloud/qgspointcloudlayer.cpp @@ -1080,7 +1080,7 @@ bool QgsPointCloudLayer::changeAttributeValue( const QgsPointCloudNodeId &n, con QgsPointCloudIndex QgsPointCloudLayer::index() const { - QGIS_PROTECT_QOBJECT_THREAD_ACCESS_NON_FATAL + QGIS_PROTECT_QOBJECT_THREAD_ACCESS if ( mEditIndex ) return mEditIndex; diff --git a/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp b/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp index 2b6388a00308..9db7d06101b4 100644 --- a/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp +++ b/src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp @@ -341,7 +341,7 @@ void QgsPointCloudLayerProfileResults::copyPropertiesFromGenerator( const QgsAbs QgsPointCloudLayerProfileGenerator::QgsPointCloudLayerProfileGenerator( QgsPointCloudLayer *layer, const QgsProfileRequest &request ) : mLayer( layer ) - , mIndex( layer->dataProvider()->index() ) + , mIndex( layer->index() ) , mSubIndexes( layer->dataProvider()->subIndexes() ) , mLayerAttributes( layer->attributes() ) , mRenderer( qgis::down_cast< QgsPointCloudLayerElevationProperties* >( layer->elevationProperties() )->respectLayerColors() && mLayer->renderer() ? mLayer->renderer()->clone() : nullptr ) @@ -363,10 +363,10 @@ QgsPointCloudLayerProfileGenerator::QgsPointCloudLayerProfileGenerator( QgsPoint , mZScale( layer->elevationProperties()->zScale() ) , mStepDistance( request.stepDistance() ) { - if ( mLayer->index() ) + if ( mIndex ) { - mScale = mLayer->index().scale(); - mOffset = mLayer->index().offset(); + mScale = mIndex.scale(); + mOffset = mIndex.offset(); } } diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp index c1e76470ee08..d3afec0657f7 100644 --- a/src/core/pointcloud/qgspointcloudlayerrenderer.cpp +++ b/src/core/pointcloud/qgspointcloudlayerrenderer.cpp @@ -52,7 +52,7 @@ QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *laye if ( !layer->dataProvider() || !layer->renderer() ) return; - mIndex = layer->dataProvider()->index(); + mIndex = layer->index(); QElapsedTimer timer; timer.start(); From ed28933ed64d8b7d7e4ceb3e624d5171962894a1 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Sat, 25 Jan 2025 10:48:57 +1000 Subject: [PATCH 6/6] Initialize members --- src/core/pointcloud/qgspointcloudlayerrenderer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/pointcloud/qgspointcloudlayerrenderer.h b/src/core/pointcloud/qgspointcloudlayerrenderer.h index 731a34c4615d..f480f749e593 100644 --- a/src/core/pointcloud/qgspointcloudlayerrenderer.h +++ b/src/core/pointcloud/qgspointcloudlayerrenderer.h @@ -98,8 +98,8 @@ class CORE_EXPORT QgsPointCloudLayerRenderer: public QgsMapLayerRenderer const QVector< QgsPointCloudSubIndex > mSubIndexes; std::optional mOverviewIndex; - double mAverageSubIndexWidth; - double mAverageSubIndexHeight; + double mAverageSubIndexWidth = 0; + double mAverageSubIndexHeight = 0; int mRenderTimeHint = 0; bool mBlockRenderUpdates = false;