Skip to content

Commit

Permalink
Fix incorrect handling of unsigned short values in point cloud providers
Browse files Browse the repository at this point in the history 
These were getting incorrectly treated as signed short values, and
accordingly were subject to overflows
  • Loading branch information
@nyalldawson
nyalldawson committed Dec 1, 2020
1 parent 47a6e36 commit d842a96
Show file tree
Hide file tree
Showing 10 changed files with 52 additions and 9 deletions.
1 change: 1 addition & 0 deletions python/core/auto_generated/pointcloud/qgspointcloudattribute.sip.in
View file
Expand Up @@ -27,6 +27,7 @@ pair of name and size in bytes
{
Char,
Short,
UShort,
Int32,
Float,
Double,
Expand Down
4 changes: 4 additions & 0 deletions src/core/pointcloud/qgspointcloudattribute.cpp
View file
Expand Up @@ -35,6 +35,8 @@ QString QgsPointCloudAttribute::displayType() const
return QObject::tr( "Character" );
case DataType::Short:
return QObject::tr( "Short" );
case DataType::UShort:
return QObject::tr( "Unsigned Short" );
case DataType::Float:
return QObject::tr( "Float" );
case DataType::Int32:
Expand All @@ -52,6 +54,7 @@ bool QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType type )
case DataType::Char:
return false;
case DataType::Short:
case DataType::UShort:
case DataType::Float:
case DataType::Int32:
case DataType::Double:
Expand All @@ -68,6 +71,7 @@ void QgsPointCloudAttribute::updateSize()
mSize = 1;
break;
case DataType::Short:
case DataType::UShort:
mSize = 2;
break;
case DataType::Float:
Expand Down
1 change: 1 addition & 0 deletions src/core/pointcloud/qgspointcloudattribute.h
View file
Expand Up @@ -39,6 +39,7 @@ class CORE_EXPORT QgsPointCloudAttribute
{
Char, //!< Char 1 byte
Short, //!< Short int 2 bytes
UShort, //!< Unsigned short int 2 bytes
Int32, //!< Int32 4 bytes
Float, //!< Float 4 bytes
Double, //!< Double 8 bytes
Expand Down
2 changes: 2 additions & 0 deletions src/core/pointcloud/qgspointcloudattributemodel.cpp
View file
Expand Up @@ -227,6 +227,7 @@ QIcon QgsPointCloudAttributeModel::iconForAttributeType( QgsPointCloudAttribute:
switch ( type )
{
case QgsPointCloudAttribute::Short:
case QgsPointCloudAttribute::UShort:
case QgsPointCloudAttribute::Int32:
{
return QgsApplication::getThemeIcon( "/mIconFieldInteger.svg" );
Expand Down Expand Up @@ -281,6 +282,7 @@ bool QgsPointCloudAttributeProxyModel::filterAcceptsRow( int source_row, const Q

if ( ( mFilters.testFlag( Char ) && type == QgsPointCloudAttribute::Char ) ||
( mFilters.testFlag( Short ) && type == QgsPointCloudAttribute::Short ) ||
( mFilters.testFlag( Short ) && type == QgsPointCloudAttribute::UShort ) ||
( mFilters.testFlag( Int32 ) && type == QgsPointCloudAttribute::Int32 ) ||
( mFilters.testFlag( Float ) && type == QgsPointCloudAttribute::Float ) ||
( mFilters.testFlag( Double ) && type == QgsPointCloudAttribute::Double ) )
Expand Down
4 changes: 4 additions & 0 deletions src/core/pointcloud/qgspointcloudrenderer.cpp
View file
Expand Up @@ -214,6 +214,10 @@ void QgsDummyPointCloudRenderer::renderBlock( const QgsPointCloudBlock *block, Q
atr = *( short * )( ptr + i * recordSize + attributeOffset );
break;

case QgsPointCloudAttribute::UShort:
atr = *( unsigned short * )( ptr + i * recordSize + attributeOffset );
break;

case QgsPointCloudAttribute::Float:
atr = *( float * )( ptr + i * recordSize + attributeOffset );
break;
Expand Down
12 changes: 12 additions & 0 deletions src/core/pointcloud/qgspointcloudrgbrenderer.cpp
View file
Expand Up @@ -141,6 +141,10 @@ void QgsPointCloudRgbRenderer::renderBlock( const QgsPointCloudBlock *block, Qgs
red = *( short * )( ptr + i * recordSize + redOffset );
break;

case QgsPointCloudAttribute::UShort:
red = *( unsigned short * )( ptr + i * recordSize + redOffset );
break;

case QgsPointCloudAttribute::Float:
red = *( float * )( ptr + i * recordSize + redOffset );
break;
Expand All @@ -164,6 +168,10 @@ void QgsPointCloudRgbRenderer::renderBlock( const QgsPointCloudBlock *block, Qgs
green = *( short * )( ptr + i * recordSize + greenOffset );
break;

case QgsPointCloudAttribute::UShort:
green = *( unsigned short * )( ptr + i * recordSize + greenOffset );
break;

case QgsPointCloudAttribute::Float:
green = *( float * )( ptr + i * recordSize + greenOffset );
break;
Expand All @@ -187,6 +195,10 @@ void QgsPointCloudRgbRenderer::renderBlock( const QgsPointCloudBlock *block, Qgs
blue = *( short * )( ptr + i * recordSize + blueOffset );
break;

case QgsPointCloudAttribute::UShort:
blue = *( unsigned short * )( ptr + i * recordSize + blueOffset );
break;

case QgsPointCloudAttribute::Float:
blue = *( float * )( ptr + i * recordSize + blueOffset );
break;
Expand Down
17 changes: 15 additions & 2 deletions src/core/providers/ept/qgseptdecoder.cpp
View file
Expand Up @@ -51,6 +51,14 @@ bool _storeToStream( char *s, size_t position, QgsPointCloudAttribute::DataType
memcpy( s + position, ( char * )( &val ), sizeof( short ) );
break;
}

case QgsPointCloudAttribute::UShort:
{
unsigned short val = static_cast< unsigned short>( value );
memcpy( s + position, ( char * )( &val ), sizeof( unsigned short ) );
break;
}

case QgsPointCloudAttribute::Float:
{
float val = float( value );
Expand Down Expand Up @@ -95,6 +103,11 @@ bool _serialize( char *data, size_t outputPosition, QgsPointCloudAttribute::Data
short val = *( short * )( input + inputPosition );
return _storeToStream<short>( data, outputPosition, outputType, val );
}
case QgsPointCloudAttribute::UShort:
{
unsigned short val = *reinterpret_cast< const unsigned short * >( input + inputPosition );
return _storeToStream<unsigned short>( data, outputPosition, outputType, val );
}
case QgsPointCloudAttribute::Float:
{
float val = *( float * )( input + inputPosition );
Expand Down Expand Up @@ -162,12 +175,12 @@ QgsPointCloudBlock *_decompressBinary( const QByteArray &dataUncompressed, const
}

// now loop through points
size_t outputOffset = 0;
for ( int i = 0; i < count; ++i )
{
size_t outputOffset = 0;
for ( const AttributeData &attribute : attributeData )
{
_serialize( destinationBuffer, i * requestedPointRecordSize + outputOffset,
_serialize( destinationBuffer, outputOffset,
attribute.requestedType, s,
attribute.inputType, attribute.inputSize, i * pointRecordSize + attribute.inputOffset );

Expand Down
8 changes: 6 additions & 2 deletions src/core/providers/ept/qgseptpointcloudindex.cpp
View file
Expand Up @@ -96,18 +96,22 @@ bool QgsEptPointCloudIndex::load( const QString &fileName )

int size = schemaObj.value( QLatin1String( "size" ) ).toInt();

if ( type == QStringLiteral( "float" ) && ( size == 4 ) )
if ( type == QLatin1String( "float" ) && ( size == 4 ) )
{
attributes.push_back( QgsPointCloudAttribute( name, QgsPointCloudAttribute::Float ) );
}
else if ( type == QStringLiteral( "float" ) && ( size == 8 ) )
else if ( type == QLatin1String( "float" ) && ( size == 8 ) )
{
attributes.push_back( QgsPointCloudAttribute( name, QgsPointCloudAttribute::Double ) );
}
else if ( size == 1 )
{
attributes.push_back( QgsPointCloudAttribute( name, QgsPointCloudAttribute::Char ) );
}
else if ( type == QLatin1String( "unsigned" ) && size == 2 )
{
attributes.push_back( QgsPointCloudAttribute( name, QgsPointCloudAttribute::UShort ) );
}
else if ( size == 2 )
{
attributes.push_back( QgsPointCloudAttribute( name, QgsPointCloudAttribute::Short ) );
Expand Down
2 changes: 2 additions & 0 deletions tests/src/core/testqgspointcloudattribute.cpp
View file
Expand Up @@ -86,6 +86,7 @@ void TestQgsPointCloudAttribute::testAttributeDisplayType()
{
QCOMPARE( QgsPointCloudAttribute( QStringLiteral( "x" ), QgsPointCloudAttribute::DataType::Char ).displayType(), QStringLiteral( "Character" ) );
QCOMPARE( QgsPointCloudAttribute( QStringLiteral( "x" ), QgsPointCloudAttribute::DataType::Short ).displayType(), QStringLiteral( "Short" ) );
QCOMPARE( QgsPointCloudAttribute( QStringLiteral( "x" ), QgsPointCloudAttribute::DataType::UShort ).displayType(), QStringLiteral( "Unsigned Short" ) );
QCOMPARE( QgsPointCloudAttribute( QStringLiteral( "x" ), QgsPointCloudAttribute::DataType::Int32 ).displayType(), QStringLiteral( "Integer" ) );
QCOMPARE( QgsPointCloudAttribute( QStringLiteral( "x" ), QgsPointCloudAttribute::DataType::Float ).displayType(), QStringLiteral( "Float" ) );
QCOMPARE( QgsPointCloudAttribute( QStringLiteral( "x" ), QgsPointCloudAttribute::DataType::Double ).displayType(), QStringLiteral( "Double" ) );
Expand All @@ -95,6 +96,7 @@ void TestQgsPointCloudAttribute::testIsNumeric()
{
QVERIFY( !QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType::Char ) );
QVERIFY( QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType::Short ) );
QVERIFY( QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType::UShort ) );
QVERIFY( QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType::Int32 ) );
QVERIFY( QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType::Float ) );
QVERIFY( QgsPointCloudAttribute::isNumeric( QgsPointCloudAttribute::DataType::Double ) );
Expand Down
10 changes: 5 additions & 5 deletions tests/src/providers/testqgseptprovider.cpp
View file
Expand Up @@ -206,7 +206,7 @@ void TestQgsEptProvider::attributes()
QCOMPARE( attributes.at( 2 ).name(), QStringLiteral( "Z" ) );
QCOMPARE( attributes.at( 2 ).type(), QgsPointCloudAttribute::Int32 );
QCOMPARE( attributes.at( 3 ).name(), QStringLiteral( "Intensity" ) );
QCOMPARE( attributes.at( 3 ).type(), QgsPointCloudAttribute::Short );
QCOMPARE( attributes.at( 3 ).type(), QgsPointCloudAttribute::UShort );
QCOMPARE( attributes.at( 4 ).name(), QStringLiteral( "ReturnNumber" ) );
QCOMPARE( attributes.at( 4 ).type(), QgsPointCloudAttribute::Char );
QCOMPARE( attributes.at( 5 ).name(), QStringLiteral( "NumberOfReturns" ) );
Expand All @@ -222,15 +222,15 @@ void TestQgsEptProvider::attributes()
QCOMPARE( attributes.at( 10 ).name(), QStringLiteral( "UserData" ) );
QCOMPARE( attributes.at( 10 ).type(), QgsPointCloudAttribute::Char );
QCOMPARE( attributes.at( 11 ).name(), QStringLiteral( "PointSourceId" ) );
QCOMPARE( attributes.at( 11 ).type(), QgsPointCloudAttribute::Short );
QCOMPARE( attributes.at( 11 ).type(), QgsPointCloudAttribute::UShort );
QCOMPARE( attributes.at( 12 ).name(), QStringLiteral( "GpsTime" ) );
QCOMPARE( attributes.at( 12 ).type(), QgsPointCloudAttribute::Double );
QCOMPARE( attributes.at( 13 ).name(), QStringLiteral( "Red" ) );
QCOMPARE( attributes.at( 13 ).type(), QgsPointCloudAttribute::Short );
QCOMPARE( attributes.at( 13 ).type(), QgsPointCloudAttribute::UShort );
QCOMPARE( attributes.at( 14 ).name(), QStringLiteral( "Green" ) );
QCOMPARE( attributes.at( 14 ).type(), QgsPointCloudAttribute::Short );
QCOMPARE( attributes.at( 14 ).type(), QgsPointCloudAttribute::UShort );
QCOMPARE( attributes.at( 15 ).name(), QStringLiteral( "Blue" ) );
QCOMPARE( attributes.at( 15 ).type(), QgsPointCloudAttribute::Short );
QCOMPARE( attributes.at( 15 ).type(), QgsPointCloudAttribute::UShort );
}


Expand Down

0 comments on commit d842a96

Please sign in to comment.