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.

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

Min/Max Value Settings for QgsPointCloudLayer #273

Open
jmkerloch opened this issue May 24, 2023 · 4 comments
Open

Min/Max Value Settings for QgsPointCloudLayer #273

jmkerloch opened this issue May 24, 2023 · 4 comments

Comments

@jmkerloch
Copy link

QGIS Enhancement: Min/Max Value Settings for QgsPointCloudLayer

Date 2023/05/24

Author Oslandia

Contact jean-marie.kerloch at oslandia dot com

maintainer @jmkerloch

Version QGIS 3.30

Summary

In QgsRasterLayer symbology for Multiband color / Singleband gray / Singleband pseudocolor renderers there is an option to define Min/Max values from layer statistics.
Here user can define if the statistics are computed from Whole raster / Current canvas / Updated canvas

The goal of this QEP is to implement this behavior but for QgsPointCloudLayer. This will be available only for Attribute by Ramp renderer : QgsPointCloudAttributeByRampRenderer.

To avoid further development in the pointcloud interface, some options will not be available:

  • Cumulative count cut option (not available in current point cloud statistics)
  • Accuracy option (for now same nb of sample as Statistics panel will be used.)

Proposed Solution

core

All created classes shall be in src/core/pointcloud directory.

QgsPointCloudMinMaxOrigin

Introduction of new class for option definition : QgsPointCloudMinMaxOrigin

We can reuse QgsRasterMinMaxOrigin class code but we may need to adapt it for PointCloud (no StatAccuracy use for current statistics compute)

There is also no cumulative cut calculation in QgsPointCloudStatistics, so CumulativeCut will be removed.

QgsPointCloudStatsCalculator

Adaptation of QgsPointCloudStatsCalculator to add option for extent filtering

bool calculateStats( QgsFeedback *feedback, const QVector<QgsPointCloudAttribute> &attributes, qint64 pointsLimit = -1 , const QgsRectangle& extent = QgsRectangle());

If the extent is not null, filtering will be applied on the processed node before statistics computation.

QgsPointCloudAttributeByRampRenderer

Update of QgsPointCloudAttributeByRampRenderer to add mMinMaxOrigin attribute, like QgsRasterRenderer:

//! Returns const reference to origin of min/max values
const QgsPointCloudMinMaxOrigin &minMaxOrigin() const { return mMinMaxOrigin; }

//! Sets origin of min/max values
void setMinMaxOrigin( const QgsPointCloudMinMaxOrigin &origin ) { mMinMaxOrigin = origin; }

QgsPointCloudLayer

Update of QgsPointCloudLayer to check if renderer min/max should be calculated

Like QgsRasterLayer, add function void refreshRendererIfNeeded( QgsPointCloudRenderer *rasterRenderer, const QgsRectangle &extent ) SIP_SKIP;

This will be called in QgsPointCloudLayerRenderer.

In void QgsPointCloudLayer::refreshRendererIfNeeded(QgsPointCloudRenderer *pointcloudRenderer, const QgsRectangle &extent) check if the render is a QgsPointCloudAttributeByRampRenderer and compute statistics on the displayed point cloud attribute

flowchart TD
 subgraph refresh["refreshRendererIfNeeded(QgsPointCloudRenderer *pointcloudRenderer, const QgsRectangle &extent)"]
     RenderType[Check renderer type] 
     RenderType-->|QgsPointCloudAttributeByRampRenderer|RampRendererLimit
     RenderType-->|other|NoChanges
     RampRendererLimit["pointcloudRenderer.minMaxOrigin().limits()"]
     RampRendererLimit-->|None|NoChanges
     RampRendererLimit-->|MinMax|RampRendererExtent
     RampRendererLimit-->|StdDev|RampRendererExtent
     RampRendererExtent["pointcloudRenderer.minMaxOrigin().extent()"]
     RampRendererExtent------>|WholePointCloudLayer|NoChanges
     RampRendererExtent------>|CurrentCanvas|NoChanges
     RampRendererExtent--->|UpdatedCanvas|Compute 
     Compute["Compute statistics for displayed attribute with current canvas extent\n Use new option of `QgsPointCloudStatsCalculator`"]-->DefineMinMax 
     DefineMinMax["Define min/max value from mMinMaxOrigin.limits()"]-->UpdateRenderer 
     UpdateRenderer["Update renderer with calculated min/max"]-->EmitSignals
     EmitSignals["emit signals : \nrepaintRequested / styleChanged / rendererChanged"]
     NoChanges["No update of renderer"]
 end
Loading

QgsPointCloudLayerRenderer

QgsPointCloudLayerRenderer must be updated to call refreshRendererIfNeeded from QgsPointCloudLayer

gui

All created classes shall be in src/gui/pointcloud directory.

QgsPointCloudMinMaxWidget

Introduction of new class for option definition : QgsPointCloudMinMaxWidget

We can reuse QgsRasterMinMaxWidget with some adaptations:

  • remove CumulativeCut options
  • remove accuracy options

void load( int bandNo, double min, double max ); will be replaced by void load(const QString& attribute, double min, double max ); that will indicate when new min/max value are computed.

For computation of min/max value, new parameter for extent filtering will be used with QgsPointCloudStatsCalculator.
Statitics compute will be done only if needed, statistics from layer can be use.

flowchart TD
 RampRendererLimit["current limit"]
 RampRendererLimit----->|None|NoSignal["No signal emitted"]
 RampRendererLimit-->|MinMax|RampRendererExtent
 RampRendererLimit-->|StdDev|RampRendererExtent
 RampRendererExtent["renderer.mMinMaxOrigin.extent()"]
 RampRendererExtent-->|WholePointCloudLayer|NoCompute
 RampRendererExtent-->|CurrentCanvas|Compute
 RampRendererExtent-->|UpdatedCanvas|Compute
 NoCompute["Use statistics from QgsPointCloudLayer mStatistics"]-->DefineMinMax
 Compute["Compute statistics for displayed attribute with current canvas extent\n Use new option of `QgsPointCloudStatsCalculator`"]-->DefineMinMax 
 DefineMinMax["Define min/max value limit"]-->EmitSignal 
 EmitSignal["Emit load signal with new min/max values"]
Loading

QgsPointCloudAttributeByRampRendererWidget

Add QgsPointCloudMinMaxWidget in QgsPointCloudAttributeByRampRendererWidgetBase

We will connect to signal void load(const QString& attribute, double min, double max ); and update user selected value for min/max definition

Affected Files

  • src/core/pointcloud/qgspointcloudstatscalculator.h / src/core/pointcloud/qgspointcloudstatscalculator.cpp

Add option for extent filtering.

  • src/core/pointcloud/qgsattributebyramprenderer.h

Add attribute mMinMaxOrigin

  • src/core/pointcloud/qgspointcloudlayer.h / src/core/pointcloud/qgspointcloudlayer.cpp

Update renderer min/max value if needed

  • src/core/pointcloud/qgspointcloudlayerrenderer.cpp

Call refreshRendererIfNeeded from QgsPointCloudLayer

  • src/gui/pointcloud/qgspointcloudattributebyramprendererwidget.h / src/gui/pointcloud/qgspointcloudattributebyramprendererwidget.cpp

Connect to new QgsPointCloudMinMaxWidget widget for min/max definition.

Performance Implications

Since we could be computing statistic at each map move with UpdatedCanvas extent option, there could be some overload in render.

Backwards Compatibility

New option for min/max should be stored in settings. Default value will use None limits so there is no issue for backwards compatibility.

Votes

(required)

@nyalldawson
Copy link
Contributor

This would be a very much appreciated feature!

However, can I request that we approach this in a different way to rasters? The raster approach worked ok-ish in the single-canvas world, but since it was implemented we've now got multi-canvas and 3d canvas at play (also layout maps). And the raster approach just doesn't work well anymore for these use cases.

The issue comes from trying to update the renderer values from the calculated values during rendering. I'd much prefer to completely handle this in the renderer code alone, and omit any GUI updates of the calculated ranges.

If your use case doesn't require an updated legend text then it would be relatively easy to remove the layer renderer -> renderer widget updates and completely remove refreshRendererIfNeeded.

If this IS a requirement, then we need a different approach to handle this. I would instead go for an approach of allowing the layer renderers to report statistics collected during rendering back to the canvas. (There's already a similar mechanism used for handle errors reported by layer renderers, this could be reused with a flexible QVariantMap "calculatedRendererResults" object or similar). Then only the MAIN canvas would handle responding to the gathered statistics and updating the legend accordingly.

@jmkerloch
Copy link
Author

@nyalldawson I think that the legend must be updated or the user won't be able to understand what's displayed, don't you think ?

If I understand what you mean, you would prefer that the logic of refreshRendererIfNeeded should be implemented the the QgsPointCloudLayerRenderer ?

For the legend update, could you indicate me where there is the mechanism that handle errors reported by layer renderers ? I don't see how I will be able to update the legend and the symbology widget.

@jmkerloch
Copy link
Author

@nyalldawson From my understanding of QGIS, there is only one legend for each layer. How can we fix this without some major changes in code.

I think it's out of the scope of this QEP to treat the issue of multiple legend.

Is the proposed solution a No Go for you or can I start a PR ?

@nyalldawson
Copy link
Contributor

I think it's out of the scope of this QEP to treat the issue of multiple legend.

That's completely fair enough. But in that case I'd ask that you omit the legend component and add the logic in the QgsPointCloudLayerRenderer class alone, and without any two-way logic to change the layer's renderer settings based on the results of rendering a particular extent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants