Add NoData support to QgsVirtualRasterProvider

[kikitte]

This PR adds nodata support to the QgsVirtualRasterProvider, Fixes #46813.

During this work, another problem relevant to QgsVirtualRasterProvider is identification, that is the identification result of
virtual raster layer always shows its real value instead of 'no data' string if value equals the nodata value. Currently QgsVirtualRasterProvider inherits from QgsRasterDataProvider, and QgsRasterDataProvider::identify just check the block reading result without checking the value.

QGIS/src/core/raster/qgsrasterdataprovider.cpp

Lines 318 to 329 in 1f22e06

	std::unique_ptr< QgsRasterBlock > bandBlock( block( i, pixelExtent, 1, 1 ) );
	if ( bandBlock )
	{
	const double value = bandBlock->value( 0 );
	results.insert( i, value );
	}
	else
	{
	results.insert( i, QVariant() );
	}

if the value is nodata value, an QVariant() should be inserted, the logic from QgsGdalProvider may be more reasonable.

QGIS/src/core/providers/gdal/qgsgdalprovider.cpp

Lines 1365 to 1390 in 1f22e06

	std::unique_ptr< QgsRasterBlock > bandBlock( block( i, pixelExtent, w, h ) );
	if ( !bandBlock )
	{
	return QgsRasterIdentifyResult( ERR( tr( "Cannot read data" ) ) );
	}
	double value = bandBlock->value( r, c );
	if ( ( sourceHasNoDataValue( i ) && useSourceNoDataValue( i ) &&
	( std::isnan( value ) || qgsDoubleNear( value, sourceNoDataValue( i ) ) ) ) ||
	( QgsRasterRange::contains( value, userNoDataValues( i ) ) ) )
	{
	results.insert( i, QVariant() ); // null QVariant represents no data
	}
	else
	{
	if ( sourceDataType( i ) == Qgis::DataType::Float32 )
	{
	// Insert a float QVariant so that QgsMapToolIdentify::identifyRasterLayer()
	// can print a string without an excessive precision
	results.insert( i, static_cast<float>( value ) );
	}
	else
	results.insert( i, value );
	}

So, I think there are two ways to fix this problem, the first way is override the identify method in QgsVirtualRasterProvider with nodata value checking, the second way is modify the logic of the QgsRasterDataProvider::identify just like QgsGdalProvider does. I prefer the second way because it is in the right direction, but the first way is error-prone. Hopes to hear your thoughts!

[nyalldawson]

@kikitte thank you! Can you add a unit test covering this too?

[kikitte]

@nyalldawson Yes, and the identify method is add to the virtualrasterprovider too.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[kikitte]

Hi @nyalldawson, do you have any idea about this PR? This PR add nodata related code to QgsVirtualRasterProvider and fix the identification logic, that is the value returned by QgsRasterDataProvider::identify should be compared with the QgsVirtualRasterProvider's nodata vlaue. I am willing to hear your suggestions.
This PR should also fix another issue #47812

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

[elpaso]

Except for less autos this looks good to me, excellent job!

[elpaso]

Can you please fix the spelling errors reported from the CI tests ?

[elpaso]

@kikitte you need to replace QgsRaster::IdentifyFormat with Qgis::RasterIdentifyFormat from <qgis.h>

[kikitte]

Oh, I am forgot to sync the branch to the newest, now it should be fixed.

[elpaso]

Sorry, I missed this: did you check if is there a possibility that bandValues is empty or it does not have an item with index 1 ?

If the answer is yes you'll need to check it before calling bandValues[1] or it will crash.

The same goes for mSrcNoDataValue[0] (just check if mSrcNoDataValue can be empty).

[kikitte]

The QgsVirtualRasterProvider supports IdentifyValue only, and I think any other code that want to call QgsVirtualRasterProvider::identify should check the provider's capability before calling and that code should pass Qgis::RasterIdentifyFormat::Value as the format.

QGIS/src/providers/virtualraster/qgsvirtualrasterprovider.cpp

Lines 327 to 331 in 7d81fdd

	int QgsVirtualRasterProvider::capabilities() const
	{
	const int capability = QgsRasterDataProvider::Identify
	| QgsRasterDataProvider::IdentifyValue
	| QgsRasterDataProvider::Size

When we look into the implementation of QgsRasterDataProvider::identify, the only case that bandValues will be empty is the format not equals to Qgis::RasterIdentifyFormat::Value(line 276). However, that should not be happend acoording to the above explaination. Note that the band number of QgsVirtualRasterProvider is always 1, so the execution of QgsRasterDataProvider::identify ensures that bandValues has only one item. .

QGIS/src/core/raster/qgsrasterdataprovider.cpp

Lines 269 to 333 in 7d81fdd

	QgsRasterIdentifyResult QgsRasterDataProvider::identify( const QgsPointXY &point, Qgis::RasterIdentifyFormat format, const QgsRectangle &boundingBox, int width, int height, int /*dpi*/ )
	{
	QGIS_PROTECT_QOBJECT_THREAD_ACCESS
	QgsDebugMsgLevel( QStringLiteral( "Entered" ), 4 );
	QMap<int, QVariant> results;
	if ( format != Qgis::RasterIdentifyFormat::Value || !( capabilities() & IdentifyValue ) )
	{
	QgsDebugMsg( QStringLiteral( "Format not supported" ) );
	return QgsRasterIdentifyResult( ERR( tr( "Format not supported" ) ) );
	}
	if ( !extent().contains( point ) )
	{
	// Outside the raster
	for ( int bandNo = 1; bandNo <= bandCount(); bandNo++ )
	{
	results.insert( bandNo, QVariant() );
	}
	return QgsRasterIdentifyResult( Qgis::RasterIdentifyFormat::Value, results );
	}
	QgsRectangle finalExtent = boundingBox;
	if ( finalExtent.isEmpty() )
	finalExtent = extent();
	if ( width == 0 )
	{
	width = capabilities() & Size ? xSize() : 1000;
	}
	if ( height == 0 )
	{
	height = capabilities() & Size ? ySize() : 1000;
	}
	// Calculate the row / column where the point falls
	const double xres = ( finalExtent.width() ) / width;
	const double yres = ( finalExtent.height() ) / height;
	const int col = static_cast< int >( std::floor( ( point.x() - finalExtent.xMinimum() ) / xres ) );
	const int row = static_cast< int >( std::floor( ( finalExtent.yMaximum() - point.y() ) / yres ) );
	const double xMin = finalExtent.xMinimum() + col * xres;
	const double xMax = xMin + xres;
	const double yMax = finalExtent.yMaximum() - row * yres;
	const double yMin = yMax - yres;
	const QgsRectangle pixelExtent( xMin, yMin, xMax, yMax );
	for ( int bandNumber = 1; bandNumber <= bandCount(); bandNumber++ )
	{
	std::unique_ptr< QgsRasterBlock > bandBlock( block( bandNumber, pixelExtent, 1, 1 ) );
	if ( bandBlock )
	{
	const double value = bandBlock->value( 0 );
	results.insert( bandNumber, value );
	}
	else
	{
	results.insert( bandNumber, QVariant() );
	}
	}
	return QgsRasterIdentifyResult( Qgis::RasterIdentifyFormat::Value, results );
	}

Since QgsVirtualRasterProvider has only one band, mSrcNoDataValue is initialized with one value in the constructor, it can't be empty.

May be we should check if the bandValues is empty, and no need to check mSrcNoDataValue.

[kikitte]

This patch has committed.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.