RasterLayer: Properly handle GUI updates when multiple canvas are displayed #59444

ptitjano · 2024-11-14T11:18:48Z

Description

This fixes an issue mentioned by @nyalldawson in qgis/QGIS-Enhancement-Proposals#273

This occurs when the Min/Max settings is set in the renderer and multiple canvas are displayed. Indeed, in that case QgsRasterLayer updates the GUI (the renderer widget and the legend) multiple times when the rendered values are updated because each canvas causes a renderer update.

This issue is fixed by removing the GUI update from the raster layer code. With this change, when the rendered values are updated, these statistics (min-max) are propagated to the canvas by the use of a QgsRenderedItemDetails: QgsRenderedCalculatedResults. Then, only the main canvas is notified of this change and updates the GUI accordingly.

github-actions · 2024-11-14T13:08:08Z

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

src/core/qgsrenderedcalculatedresults.h

src/core/raster/qgsrasterlayer.h

src/core/raster/qgsrasterlayerrenderer.cpp

src/core/qgsrenderedcalculatedresults.h

src/gui/qgsmapcanvas.cpp

nyalldawson · 2024-11-18T00:47:33Z

Good approach, I like where this is going! 👍

ptitjano · 2024-11-21T17:12:03Z

@nyalldawson This should be ready now.

This will be used in the next commits to store the min-max values of a raster layer and report them to the main canvas.

This is similar to what is achieved in `QgsRasterLayer::refreshRendererIfNeeded()` to check if the renderer needs to be refresh according to an extent. It does not perform any refresh. It is not used at the moment. This will replace the logic to refresh a renderer in the following commits.

This is similar to what is achieved in `QgsRasterLayer::refreshRenderer()` to refresh the renderer according to an extent. Contrary to the first one, this method does not perform any GUI update or emit any signal. It is not used at the moment. This will replace the logic to refresh a renderer in the following commits.

This will be used in the next commit to refresh a renderer.

This replaces the existing logic in `QgsRasterLayerRenderer` which calls `QgsRasterLayer::refreshRendererIfNeeded()` to refresh the renderer associated `QgsRasterLayerRenderer` and the raster associated with `QgsRasterLayer`. It also makes GUI updates. With this approach, the following new logic is done: 1. `QgsRasterRenderer::needsRefresh()` is called to check if `rasterRenderer` needs to be updated. 2. If a refresh is needed, the new min/max values are computed by calling `QgsRasterLayer::computeMinMax()` 3. The min/max values are used to refresh `rasterRenderer` 4. The min/max values are stored in a `QgsRenderedLayerStatistics` and propagated to `QgisApp` 5. In QgisApp, `QgsRenderedLayerStatistics` is used to refresh the renderer of `QgsRasterLayer` and force a refresh of the style and the legend if the change comes from the main canvas.

This is not used anymore.

nyalldawson · 2024-11-25T04:20:37Z

src/core/qgsrenderedlayerstatistics.h

+    /**
+     * Constructor for QgsRenderedLayerStatistics from a list of \a minimum and \a maximum.
+     */
+    QgsRenderedLayerStatistics( const QString &layerId, QList<double> minimum, QList<double> maximum );


Suggested change

QgsRenderedLayerStatistics( const QString &layerId, QList<double> minimum, QList<double> maximum );

QgsRenderedLayerStatistics( const QString &layerId, const QList<double>& minimum, const QList<double>& maximum );

What's this mean though? Can you elaborate in the docs when this would be used and what the list values represent?

nyalldawson · 2024-11-25T04:23:08Z

src/core/qgsrenderedlayerstatistics.h

+    {
+      maximums.append( QString::number( max ) );
+    }
+    QString str = QStringLiteral( "<QgsRenderedLayerStatistics: %1 ([%2] x [%3])>" ).arg( sipCpp->layerId(), minimums.join( ',' ), maximums.join( ',' ) );


"[ ] x [ ]" is an odd choice here, that looks a bit like a matrix of values. Maybe just explicitly go "min: %2, max %3" instead?

nyalldawson · 2024-11-25T04:29:29Z

src/app/qgisapp.cpp

+
+  if ( !computedStatistics.empty() )
+  {
+    for ( QgsMapLayer *layer : mMapCanvas->layers() )


This should probably be modified to loop through the statistics, and then retrieve the layer from the project by the ID from the statistics accordingly.

nyalldawson · 2024-11-25T04:31:11Z

src/app/qgisapp.cpp

+          {
+
+            // refresh the renderer of the layer
+            rasterLayer->renderer()->refresh( statistics->boundingBox(), statistics->minimum(), statistics->maximum() );


Shouldn't this be inside the if ( mapCanvas == mMapCanvas ) check? otherwise we'll end up changing the renderer for non-main canvas too!

Actually on that note, I don't think we should be calling handleRenderedLayerStatistics at ALL for non-main canvases.

nyalldawson · 2024-11-25T04:37:47Z

src/core/raster/qgssinglebandgrayrenderer.h

+     *
+     * \since QGIS 3.42
+     */
+    virtual bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;


Suggested change

virtual bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;

bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;

nyalldawson · 2024-11-25T04:37:56Z

src/core/raster/qgssinglebandpseudocolorrenderer.h

+     *
+     * \since QGIS 3.42
+     */
+    virtual bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;


Suggested change

virtual bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;

bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;

nyalldawson · 2024-11-25T04:39:30Z

Much cleaner, this is great! 💯

JanCaha · 2024-11-25T06:02:45Z

Looks good 👍 This approach will be useful for MeshLayer as well.

ptitjano self-assigned this Nov 14, 2024

ptitjano added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Nov 14, 2024

github-actions bot added this to the 3.42.0 milestone Nov 14, 2024

ptitjano added the Rasters Related to general raster layer handling (not specific data formats) label Nov 14, 2024

ptitjano force-pushed the canvas-min-max branch 3 times, most recently from 26595b2 to 7b17df6 Compare November 14, 2024 12:11

ptitjano force-pushed the canvas-min-max branch 2 times, most recently from 61f86be to 33998d8 Compare November 15, 2024 16:34

nyalldawson reviewed Nov 18, 2024

View reviewed changes

ptitjano force-pushed the canvas-min-max branch 2 times, most recently from db19524 to abf6011 Compare November 21, 2024 17:10

ptitjano force-pushed the canvas-min-max branch 3 times, most recently from 29e68f8 to 3060e67 Compare November 21, 2024 22:07

ptitjano added 6 commits November 22, 2024 09:32

introduce qgsrenderedlayerstatistics

bf07a5e

This will be used in the next commits to store the min-max values of a raster layer and report them to the main canvas.

qgsrasterlayer: Make computeMinMax public

14a5c9f

This will be used in the next commit to refresh a renderer.

qgsrasterlayer: Remove refresh logic

a6a76a1

This is not used anymore.

ptitjano force-pushed the canvas-min-max branch from 3060e67 to a6a76a1 Compare November 22, 2024 08:34

nyalldawson mentioned this pull request Nov 25, 2024

Mesh Render Extent Settings #59305

Open

nyalldawson reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RasterLayer: Properly handle GUI updates when multiple canvas are displayed #59444

RasterLayer: Properly handle GUI updates when multiple canvas are displayed #59444

ptitjano commented Nov 14, 2024

github-actions bot commented Nov 14, 2024 •

edited

Loading

nyalldawson commented Nov 18, 2024

ptitjano commented Nov 21, 2024

nyalldawson Nov 25, 2024

nyalldawson Nov 25, 2024

nyalldawson Nov 25, 2024

nyalldawson Nov 25, 2024

nyalldawson Nov 25, 2024

nyalldawson Nov 25, 2024

nyalldawson Nov 25, 2024

nyalldawson commented Nov 25, 2024

JanCaha commented Nov 25, 2024

	QgsRenderedLayerStatistics( const QString &layerId, QList<double> minimum, QList<double> maximum );
	QgsRenderedLayerStatistics( const QString &layerId, const QList<double>& minimum, const QList<double>& maximum );

	virtual bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;
	bool refresh( const QgsRectangle &extent, const QList<double> &min, const QList<double> &max, bool forceRefresh = false ) override SIP_SKIP;

RasterLayer: Properly handle GUI updates when multiple canvas are displayed #59444

Are you sure you want to change the base?

RasterLayer: Properly handle GUI updates when multiple canvas are displayed #59444

Conversation

ptitjano commented Nov 14, 2024

Description

github-actions bot commented Nov 14, 2024 • edited Loading

🪟 Windows builds

🪟 Windows Qt6 builds

nyalldawson commented Nov 18, 2024

ptitjano commented Nov 21, 2024

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson commented Nov 25, 2024

JanCaha commented Nov 25, 2024

github-actions bot commented Nov 14, 2024 •

edited

Loading