qgis / QGIS

QGIS is a free, open source, cross platform (lin/win/mac) geographical information system (GIS)
https://qgis.org
GNU General Public License v2.0
10.35k stars 2.98k forks source link

Decimal Precision Error in File Extent #44586

Open ar-siddiqui opened 3 years ago

ar-siddiqui commented 3 years ago

What is the bug or the crash?

I have a layer in geojson and the same layer in shapefile, with all features geometry rounding off to one decimal place. QGIS is calculating the wrong extent for them, with errors in the decimal.

image

I checked the geojson text to be sure that the error is not coming from it. I checked the shapefile in ArcPro to be sure that the extent is only incorrect in QGIS. I calculated geometry as an attribute and errors are not visible there as well.

image

I installed the Get WKT plugin and it is displaying the decimal error in features geometries as well, but only in WKT, EWKT format, and not in JSON format.

Steps to reproduce the issue

See the attached zip folder for the file. CRS is EPSG 2284 decimal_issue.zip

Versions

QGIS version 3.16.8-Hannover QGIS code revision 8c50902e Compiled against Qt 5.15.2 Running against Qt 5.15.2 Compiled against GDAL/OGR 3.3.0 Running against GDAL/OGR 3.3.0 Compiled against GEOS 3.9.1-CAPI-1.14.2 Running against GEOS 3.9.1-CAPI-1.14.2 Compiled against SQLite 3.35.2 Running against SQLite 3.35.2 PostgreSQL Client Version 13.0 SpatiaLite Version 5.0.1 QWT Version 6.1.3 QScintilla2 Version 2.11.5 Compiled against PROJ 8.0.1 Running against PROJ Rel. 8.0.1, March 5th, 2021 OS Version Windows 10 Version 1909 Active python plugins area_weighted_average; batchvectorlayersaver; changeDataSource; curve_number_generator; DataPlotly; mapswipetool_plugin; MemoryLayerSaver; mmqgis; MongoConnector; MultipleLayerSelection; plugin_reloader; profiletool; searchlayers; send2google_earth; spline; StreetView; valuetool; db_manager; processing; QGIS3-getWKT

Additional context

No response

SebastienPeillet commented 2 years ago

The bug seems to come directly from Qt.

It happened when we use qgsDoubleToString function:

inline QString qgsDoubleToString( double a, int precision = 17 )
{
  QString str = QString::number( a, 'f', precision );
  if ( precision )
  {
    if ( str.contains( QLatin1Char( '.' ) ) )
    {
      // remove ending 0s
      int idx = str.length() - 1;
      while ( str.at( idx ) == '0' && idx > 1 )
      {
        idx--;
      }
      if ( idx < str.length() - 1 )
        str.truncate( str.at( idx ) == '.' ? idx : idx + 1 );
    }
  }
  // avoid printing -0
  // see https://bugreports.qt.io/browse/QTBUG-71439
  if ( str == QLatin1String( "-0" ) )
  {
    return QLatin1String( "0" );
  }
  return str;
}

For example, the max X extent stored is really 12451007.6000001 like in the geojson, but the wkt display is wrong because at the first line of qgsDoubleToString, QString::number( 12451007.6000001, 'f', 17 ) returns '12451007.60000010021030903'. Then we try to remove useless 0 but the double conversion failed already.

agiudiceandrea commented 2 years ago

The bug seems to come directly from Qt.

I think it's not a bug in Qt. It's a typical issue when dealing with floating point numbers.

The decimal number 12451007.6000001 cannot be "exactly" represented as a 64 bit (double) IEEE754 floating point: its hexadecimal 64 bit (double) IEEE754 representation is 0x4167BF97F3333369 whose "most accurate" decimal representation is 12451007.6000001002103090286255 which rounded to 17 decimal places becomes 12451007.60000010021030903 https://www.binaryconvert.com/result_double.html?decimal=049050052053049048048055046054048048048048048049

SebastienPeillet commented 2 years ago

@agiudiceandrea Yes, indeed my comment was not fully accurate. And thank you for the binary converter, I supposed the problem was here but i didn't have the tool to check my assumption ;)

agiudiceandrea commented 2 years ago

@SebastienPeillet is qgsDoubleToString() actually used to display the extent in the layer Properties panel?

It seems to me QgsRectangle::toString() is used instead: https://github.com/qgis/QGIS/blob/6f38fd64d33e15be4d5a06e7c33b135f569bd9a4/src/core/vector/qgsvectorlayer.cpp#L5195-L5196 https://github.com/qgis/QGIS/blob/6f38fd64d33e15be4d5a06e7c33b135f569bd9a4/src/core/geometry/qgsrectangle.cpp#L127-L155

in which a different logic than qgsDoubleToString() is implemented.

Maybe it could be possible to implement a better logic to convert a floating point number to a decimal string representation with the fewest digits sufficient for maximum accuracy: https://en.wikipedia.org/wiki/Floating-point_arithmetic#Binary-to-decimal_conversion_with_minimal_number_of_digits https://stackoverflow.com/a/55326309

Anyway, the two layers provided by @ar-siddiqui don't have identical geometries and their extent is not the same and the GeoJson layer have overlapping polygons.

SebastienPeillet commented 2 years ago

It seems to me QgsRectangle::toString() is used instead

Yes, I thought it came from qgslayermetadataformatter, but is not :

https://github.com/qgis/QGIS/blob/6f38fd64d33e15be4d5a06e7c33b135f569bd9a4/src/core/metadata/qgslayermetadataformatter.cpp#L142-L145

But using QgsRectangle::toString(), we are switching from QString::number() function to QString::arg() function, so the problem is quite the same. But it's true that a change in QgsRectangle::toString() is less intrusive than in qgsDoubleToString()

Anyway, the two layers provided by @ar-siddiqui don't have identical geometries and their extent is not the same and the GeoJson layer have overlapping polygons.

Yeah I saw it too, but the geojson has only 7 decimals, so I considered there was still a problem :smile:

ar-siddiqui commented 2 years ago

A difference in shapefile/geojson might have occurred in the transformation between formats. Otherwise, they are the same layers.

Irrespective of the difference the original issue is still valid, no?