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.

Sign up for GitHub

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

Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerRenderer and QgsPointCloudLayerProfileGenerator #60086

Merged
merged 6 commits into from
Jan 25, 2025
Merged

Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerRenderer and QgsPointCloudLayerProfileGenerator #60086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c5775a
Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerRenderer
dvdkon Jan 8, 2025
185113b
Don't use data provider before we check it's non-null
dvdkon Jan 9, 2025
3f8495a
Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerProfile…
dvdkon Jan 12, 2025
a3866f4
Make thread check in QgsPointCloudLayer::dataProvider() fatal
dvdkon Jan 12, 2025
4174255
Use correct index() method to use editing index
dvdkon Jan 22, 2025
ed28933
Initialize members
nyalldawson Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/core/pointcloud/qgspointcloudlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -1081,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;

Expand Down
18 changes: 8 additions & 10 deletions src/core/pointcloud/qgspointcloudlayerprofilegenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,8 @@ void QgsPointCloudLayerProfileResults::copyPropertiesFromGenerator( const QgsAbs

QgsPointCloudLayerProfileGenerator::QgsPointCloudLayerProfileGenerator( QgsPointCloudLayer *layer, const QgsProfileRequest &request )
: mLayer( layer )
, mIndex( layer->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() )
Expand All @@ -361,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();
}
}

Expand All @@ -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<QgsPointCloudIndex> 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 ) )
Expand Down
4 changes: 4 additions & 0 deletions src/core/pointcloud/qgspointcloudlayerprofilegenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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;
Expand Down
47 changes: 25 additions & 22 deletions src/core/pointcloud/qgspointcloudlayerrenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,42 +43,48 @@

QgsPointCloudLayerRenderer::QgsPointCloudLayerRenderer( QgsPointCloudLayer *layer, QgsRenderContext &context )
: QgsMapLayerRenderer( layer->id(), &context )
, mLayer( layer )
, mLayerName( layer->name() )
, mLayerAttributes( layer->attributes() )
, mSubIndexes( layer && layer->dataProvider() ? layer->dataProvider()->subIndexes() : QVector<QgsPointCloudSubIndex>() )
, 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;

mIndex = layer->index();

QElapsedTimer timer;
timer.start();

mRenderer.reset( mLayer->renderer()->clone() );
mRenderer.reset( layer->renderer()->clone() );
if ( !mSubIndexes.isEmpty() )
{
mSubIndexExtentRenderer.reset( new QgsPointCloudExtentRenderer() );
mSubIndexExtentRenderer->setShowLabels( mRenderer->showLabels() );
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<QgsVirtualPointCloudProvider *>( layer->dataProvider() ) )
{
mAverageSubIndexWidth = vpcProvider->averageSubIndexWidth();
mAverageSubIndexHeight = vpcProvider->averageSubIndexHeight();
mOverviewIndex = vpcProvider->overview();
}

mCloudExtent = layer->dataProvider()->polygonBounds();

mClippingRegions = QgsMapClippingUtils::collectClippingRegionsForLayer( *renderContext(), layer );

Expand Down Expand Up @@ -129,9 +135,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;
Expand Down Expand Up @@ -195,9 +199,9 @@ bool QgsPointCloudLayerRenderer::render()
bool canceled = false;
if ( mSubIndexes.isEmpty() )
{
canceled = !renderIndex( pc );
canceled = !renderIndex( mIndex );
}
else if ( const QgsVirtualPointCloudProvider *vpcProvider = dynamic_cast<QgsVirtualPointCloudProvider *>( mLayer->dataProvider() ) )
else if ( mOverviewIndex )
{
QVector< QgsPointCloudSubIndex > visibleIndexes;
for ( const QgsPointCloudSubIndex &si : mSubIndexes )
Expand All @@ -207,23 +211,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 )
Expand Down
7 changes: 6 additions & 1 deletion src/core/pointcloud/qgspointcloudlayerrenderer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <QString>
#include <QPainter>
#include <QElapsedTimer>
#include <optional>

class QgsRenderContext;
class QgsPointCloudLayer;
Expand Down Expand Up @@ -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;
Expand All @@ -94,7 +95,11 @@ class CORE_EXPORT QgsPointCloudLayerRenderer: public QgsMapLayerRenderer
QgsPointCloudAttributeCollection mAttributes;
QgsGeometry mCloudExtent;
QList< QgsMapClippingRegion > mClippingRegions;

const QVector< QgsPointCloudSubIndex > mSubIndexes;
std::optional<QgsPointCloudIndex> mOverviewIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, is there a benefit of using std::optional instead of just a QgsPointCloudIndex() / QgsPointCloudIndex( nullptr ), which evaluates as false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. There isn't, I just personally default to using std::optional when a member can be explicitly left out, and don't much like putting nullptr in smart pointers.

It is redundant (and probably inconsistent with the rest of the codebase), so if you think it's better, I'll change it to the bare index object.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a nice habit I guess!
It would be ever slightly more readable without it, as optional is barely used in the codebase, but I don't have a strong opinion :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd personally love to see optional used more often -- it's quite an elegant solution to some tricky API considerations... just so long as we don't need to worry about exposure to Python 🤣

double mAverageSubIndexWidth = 0;
double mAverageSubIndexHeight = 0;

int mRenderTimeHint = 0;
bool mBlockRenderUpdates = false;
Expand Down
Loading