[FEATURE] Implement raster auto-stretching when updating canvas #3871

rouault · 2016-12-15T02:39:31Z

This commit implements the improvements described at:
https://lists.osgeo.org/pipermail/qgis-developer/2016-September/044393.html

The QgsRasterMinMaxWidget now offers a seetting to specify that the statistics
should be computed each time the canvas extent changes.

Other changes:

the content of the QgsRasterMinMaxWidget is now persistant.
there is no longer any Load button. The global Apply / OK button of the raster
properties dialog has this effect.
the default "limits" for single band raster is now MinMax and not CumulativeCut
the default "limits" can be configured for single band, multi band single byte and
multi band multi byte
"Strech using current extent" honours the "limits" instead of forcing min/max.

nyalldawson · 2016-12-15T02:47:07Z

there is no longer any Load button. The global Apply / OK button of the raster
properties dialog has this effect.

Does this also work in the live styling dock?

nirvn · 2016-12-15T03:02:50Z

Nice work. How does stretch using current extent work if you only want to take current extent to get min max once ?

nyalldawson

Looks great! Sounds like a very handy feature to have too. Have you got any more unit tests incoming for this?

nyalldawson · 2016-12-15T02:50:36Z

src/core/raster/qgscontrastenhancement.h

@@ -65,6 +66,12 @@ class CORE_EXPORT QgsContrastEnhancement
    //! \brief Helper function that returns the minimum possible value for a GDAL data type
    static double minimumValuePossible( Qgis::DataType );

+    //! \brief Return a strign to serialize ContrastEnhancementAlgorithm


Taken into account

nyalldawson · 2016-12-15T02:54:11Z

src/core/raster/qgsrasterlayer.cpp

  QSettings mySettings;

  QString myKey;
-  QString myDefault;
+  QString myDefaultAlg;


I think the general agreement is that we avoid "my"/"the" prefixes in new code to more closely follow the Qt conventions.

Taken into account

nyalldawson · 2016-12-15T02:54:39Z

src/core/raster/qgsrasterlayer.h

+    /** \brief Return default contrast enhancemnt settings for that type of raster.
+     *  @note not available in python bindings
+     */
+    bool defaultContrastEnhancementSettings( QgsContrastEnhancement::ContrastEnhancementAlgorithm& myAlgorithm,


should this be const?

Taken into account

nyalldawson · 2016-12-15T02:58:08Z

src/core/raster/qgsrasterminmaxorigin.h

+public:
+
+    //! \brief Default cumulative cut lower limit
+    static const double CUMULATIVE_CUT_LOWER;


What do you think about flipping these static consts to static constexpr double?

Taken into account

nyalldawson · 2016-12-15T03:01:11Z

src/gui/raster/qgsmultibandcolorrendererwidget.cpp

@@ -43,8 +44,8 @@ QgsMultiBandColorRendererWidget::QgsMultiBandColorRendererWidget( QgsRasterLayer
    layout->setContentsMargins( 0, 0, 0, 0 );
    mMinMaxContainerWidget->setLayout( layout );
    layout->addWidget( mMinMaxWidget );
-    connect( mMinMaxWidget, SIGNAL( load( int, double, double, int ) ),
-             this, SLOT( loadMinMax( int, double, double, int ) ) );
+    connect( mMinMaxWidget, SIGNAL( load( int, double, double ) ),


Can you use the new style connect here

Taken into account

rouault · 2016-12-15T12:52:40Z

Does this also work in the live styling dock?

Good point. It didn't. Fixed

rouault · 2016-12-15T12:54:11Z

Nice work. How does stretch using current extent work if you only want to take current extent to get min max once ?

The "current canvas" extent is only taken into account when you press the Apply button of the dialog or dock widget. This is different from the "updated canvas" choice where each canvas extent change causes recomputation to be done.

rouault · 2016-12-15T12:54:49Z

Have you got any more unit tests incoming for this?

I've added a few tests for the non-UI parts

nyalldawson

Looks good

nyalldawson · 2016-12-15T18:36:52Z

tests/src/core/testqgsrasterlayer.cpp

+  mpLandsatRasterLayer->refreshContrastEnhancementIfNeeded( mpLandsatRasterLayer->renderer(), newExtent );
+  QCOMPARE( mpLandsatRasterLayer->renderer()->minMaxOrigin().limits(), QgsRasterMinMaxOrigin::MinMax );
+  double minVal = dynamic_cast<QgsMultiBandColorRenderer*>( mpLandsatRasterLayer->renderer() )->redContrastEnhancement()->minimumValue();
+  QVERIFY( fabs( initMinVal - minVal ) < 1e-5 );


You could use QGSCOMPARENEAR here to get nicer debugging if the test fails

nirvn · 2016-12-16T01:30:59Z

@rouault , I can't compile your branch, I get the following make error:

/home/webmaster/dev/cpp/QGIS/src/core/raster/qgsrasterminmaxorigin.cpp: In member function ‘bool QgsRasterMinMaxOrigin::operator==(const QgsRasterMinMaxOrigin&) const’:
/home/webmaster/dev/cpp/QGIS/src/core/raster/qgsrasterminmaxorigin.cpp:43:64: error: ‘fabs’ was not declared in this scope
          fabs( mCumulativeCutLower - other.mCumulativeCutLower ) < 1e-5 &&

nyalldawson · 2016-12-16T01:36:33Z

@nirvn try changing it to qAbs

nirvn · 2016-12-16T01:37:25Z

@nyalldawson , yeah, done that, waiting for a successful build to report back "all ok" :)

nirvn · 2016-12-16T01:49:44Z

@rouault , ok, confirming fabs -> qAbs fixes the build.

Your PR appears to work quite well, good job. You'll need to improve the UI a bit through, as it fails to display properly in the style dock:

Glue accuracy to min /max (i.e. don't use a different grid column there)
For extent, either move the radio boxes to three separate rows (that might take too much vertical space), or use a combo box.

I love the updated canvas mode, fantastic job.

nirvn · 2016-12-16T01:54:00Z

@rouault , more comments:

The singleband pseudocolor renderer has issues on my machine. The rendering takes ages (using a single LANDSAT-8 band), with intense CPU load.
The singleband gray UI also needs some fixing to behave well in style dock mode:

nirvn · 2016-12-16T01:56:59Z

@rouault , also, I noticed the absence of an "updated canvas" mode for the singleband pseudocolor. It'd be great to add that mode too both for consistancy across renderers, as well as practical uses when analyzing LiDAR data, etc.

It'd "simply" require to move the classification code out of UI into the renderer itself. It wasn't possible until recently, when I updated the renderer to store the source color ramp used to create the raster ramp.

This commit implements the improvements described at: https://lists.osgeo.org/pipermail/qgis-developer/2016-September/044393.html The QgsRasterMinMaxWidget now offers a seetting to specify that the statistics should be computed each time the canvas extent changes. Other changes: - the content of the QgsRasterMinMaxWidget is now persistant. - there is no longer any Load button. The global Apply / OK button of the raster properties dialog has this effect. - the default "limits" for single band raster is now MinMax and not CumulativeCut - the default "limits" can be configured for single band, multi band single byte and multi band multi byte - "Strech using current extent" honours the "limits" instead of forcing min/max.

rouault · 2016-12-16T11:40:46Z

I've fixed :

the qAbs issue
the UI glitches in style dock mode
the 100% CPU (infinite cycle of notifications) effect with the pseudo color renderer when the style dock is enabled
added the "Updated canvas" possibility for pseudo color renderer to update the min/max values in the line edit

I don't feel familiar enough with the pseudo color render to do further changes. One thing I didn't do in particular is to auto-classify when the min/max are changed indirectly, because that could break customized classification done by the user (if that would be implemented some care should be taken not to emit the widgetChanged() signal if no changes are done, to avoid infinite notifications)

nirvn · 2016-12-17T05:15:00Z

@rouault , great job.

Re pseudocolor, customization classification == manual edits to the list? I think that should be overwritten if the min max is set to updated extent.

…nfinite signal notifications. Fixes #3871 (comment), ie switching between renderers in the raster layer properties dialog or in the style dock result in non optimal parameters, so better switch to default parameters that would be the ones used when adding a layer of the new renderer type to the canvas. This issue already existed in 2.18 Also fixes a potential infinite notification cycle when having both the style dock and layer properties dialog opened, and applying changes in the layer properties dialog. (related to #3871).

rouault · 2016-12-23T10:57:40Z

following this set of commits, QGIS fails to renderer a multi band raster using single band gray renderer.

I could reproduce similar behaviour with 2.18 as well. Anyway should be fixed with 645f2c8

nyalldawson approved these changes Dec 15, 2016

View reviewed changes

rouault force-pushed the auto_stretch branch from fab2fb8 to 3fb0256 Compare December 15, 2016 12:52

nyalldawson approved these changes Dec 15, 2016

View reviewed changes

rouault force-pushed the auto_stretch branch from d1d6fea to c26e9dc Compare December 15, 2016 20:53

rouault added 2 commits December 16, 2016 12:36

Add QGSCOMPARENOTNEAR(value,not_expected,epsilon) macro

cad7a8f

rouault force-pushed the auto_stretch branch from c26e9dc to 532eb58 Compare December 16, 2016 11:36

rouault merged commit 792873a into qgis:master Dec 16, 2016

qgib mentioned this pull request May 25, 2019

Error on apply qml style for a raster layer in 3.4.1 and Master 0ea86bf0a5 #28229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Implement raster auto-stretching when updating canvas #3871

[FEATURE] Implement raster auto-stretching when updating canvas #3871

rouault commented Dec 15, 2016

nyalldawson commented Dec 15, 2016

nirvn commented Dec 15, 2016

nyalldawson left a comment

nyalldawson Dec 15, 2016

rouault Dec 15, 2016

nyalldawson Dec 15, 2016

rouault Dec 15, 2016

nyalldawson Dec 15, 2016

rouault Dec 15, 2016

nyalldawson Dec 15, 2016

rouault Dec 15, 2016

nyalldawson Dec 15, 2016

rouault Dec 15, 2016

rouault commented Dec 15, 2016

rouault commented Dec 15, 2016

rouault commented Dec 15, 2016

nyalldawson left a comment

nyalldawson Dec 15, 2016

nirvn commented Dec 16, 2016

nyalldawson commented Dec 16, 2016

nirvn commented Dec 16, 2016

nirvn commented Dec 16, 2016

nirvn commented Dec 16, 2016

nirvn commented Dec 16, 2016

rouault commented Dec 16, 2016

nirvn commented Dec 17, 2016

rouault commented Dec 23, 2016

[FEATURE] Implement raster auto-stretching when updating canvas #3871

[FEATURE] Implement raster auto-stretching when updating canvas #3871

Conversation

rouault commented Dec 15, 2016

nyalldawson commented Dec 15, 2016

nirvn commented Dec 15, 2016

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault commented Dec 15, 2016

rouault commented Dec 15, 2016

rouault commented Dec 15, 2016

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirvn commented Dec 16, 2016

nyalldawson commented Dec 16, 2016

nirvn commented Dec 16, 2016

nirvn commented Dec 16, 2016

nirvn commented Dec 16, 2016

nirvn commented Dec 16, 2016

rouault commented Dec 16, 2016

nirvn commented Dec 17, 2016

rouault commented Dec 23, 2016