Skip to content
Permalink
Browse files

Fix geometry simplification for rendering altering feature geometry

I noticed this when using memory layers - zooming out and then in
would result in the features getting more and more generalised.
Possibly it would affect other areas of the codebase too.

The geometry simplifier was unhelpfully casting away the const
from the pointer to the geometry's wkb, and was happily overwriting
the wkb for its own purposes. With QgsGeometry now implicitly
sharing this wkb pointer the non-const cast meant that the geometry
was not getting correctly detached and the original geometry was
being modified.
  • Loading branch information
nyalldawson committed Aug 3, 2015
1 parent 7934a92 commit f00c52d5a6e17c11c5f4caa42bd4f1b86fbdc389
Showing with 21 additions and 17 deletions.
  1. +20 −16 src/core/qgsmaptopixelgeometrysimplifier.cpp
  2. +1 −1 src/core/qgsmaptopixelgeometrysimplifier.h
@@ -41,7 +41,7 @@ float QgsMapToPixelSimplifier::calculateLengthSquared2D( double x1, double y1, d
}

//! Returns the BBOX of the specified WKB-point stream
inline static QgsRectangle calculateBoundingBox( QGis::WkbType wkbType, unsigned char* wkb, size_t numPoints )
inline static QgsRectangle calculateBoundingBox( QGis::WkbType wkbType, const unsigned char* wkb, size_t numPoints )
{
double x, y;
QgsRectangle r;
@@ -63,7 +63,7 @@ inline static QgsRectangle calculateBoundingBox( QGis::WkbType wkbType, unsigned
//! Generalize the WKB-geometry using the BBOX of the original geometry
static bool generalizeWkbGeometryByBoundingBox(
QGis::WkbType wkbType,
unsigned char* sourceWkb, size_t sourceWkbSize,
const unsigned char* sourceWkb, size_t sourceWkbSize,
unsigned char* targetWkb, size_t& targetWkbSize,
const QgsRectangle& envelope, bool writeHeader )
{
@@ -151,7 +151,7 @@ static bool generalizeWkbGeometryByBoundingBox(
//! Simplify the WKB-geometry using the specified tolerance
bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
int simplifyFlags, QGis::WkbType wkbType,
unsigned char* sourceWkb, size_t sourceWkbSize,
const unsigned char* sourceWkb, size_t sourceWkbSize,
unsigned char* targetWkb, size_t& targetWkbSize,
const QgsRectangle& envelope, double map2pixelTol,
bool writeHeader, bool isaLinearRing )
@@ -161,7 +161,7 @@ bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
bool result = false;

// Save initial WKB settings to use when the simplification creates invalid geometries
unsigned char* sourcePrevWkb = sourceWkb;
const unsigned char* sourcePrevWkb = sourceWkb;
unsigned char* targetPrevWkb = targetWkb;
size_t targetWkbPrevSize = targetWkbSize;

@@ -194,7 +194,7 @@ bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
targetWkbSize += 5;
}

unsigned char* wkb1 = sourceWkb;
const unsigned char* wkb1 = sourceWkb;
unsigned char* wkb2 = targetWkb;
unsigned int flatType = QGis::flatType( wkbType );

@@ -230,10 +230,10 @@ bool QgsMapToPixelSimplifier::simplifyWkbGeometry(
{
double x1, y1, x2, y2;

unsigned char* startWkbX = sourceWkb;
unsigned char* startWkbY = startWkbX + sizeOfDoubleX;
unsigned char* finalWkbX = sourceWkb + ( numPoints - 1 ) * ( sizeOfDoubleX + sizeOfDoubleY );
unsigned char* finalWkbY = finalWkbX + sizeOfDoubleX;
const unsigned char* startWkbX = sourceWkb;
const unsigned char* startWkbY = startWkbX + sizeOfDoubleX;
const unsigned char* finalWkbX = sourceWkb + ( numPoints - 1 ) * ( sizeOfDoubleX + sizeOfDoubleY );
const unsigned char* finalWkbY = finalWkbX + sizeOfDoubleX;

memcpy( &x1, startWkbX, sizeof( double ) );
memcpy( &y1, startWkbY, sizeof( double ) );
@@ -418,7 +418,7 @@ QgsGeometry* QgsMapToPixelSimplifier::simplify( QgsGeometry* geometry ) const
//! Simplifies the geometry (Removing duplicated points) when is applied the specified map2pixel context
bool QgsMapToPixelSimplifier::simplifyGeometry( QgsGeometry* geometry, int simplifyFlags, double tolerance )
{
size_t targetWkbSize = 0;
size_t finalWkbSize = 0;

// Check whether the geometry can be simplified using the map2pixel context
QGis::GeometryType geometryType = geometry->type();
@@ -428,17 +428,21 @@ bool QgsMapToPixelSimplifier::simplifyGeometry( QgsGeometry* geometry, int simpl
QgsRectangle envelope = geometry->boundingBox();
QGis::WkbType wkbType = geometry->wkbType();

unsigned char* wkb = ( unsigned char* )geometry->asWkb();
const unsigned char* wkb = geometry->asWkb();
size_t wkbSize = geometry->wkbSize();

// Simplify the geometry rewriting temporally its WKB-stream for saving calloc's.
if ( simplifyWkbGeometry( simplifyFlags, wkbType, wkb, wkbSize, wkb, targetWkbSize, envelope, tolerance ) )
unsigned char* targetWkb = new unsigned char[wkbSize];
memcpy( targetWkb, wkb, wkbSize );

if ( simplifyWkbGeometry( simplifyFlags, wkbType, wkb, wkbSize, targetWkb, finalWkbSize, envelope, tolerance ) )

This comment has been minimized.

Copy link
@ahuarte47

ahuarte47 Aug 3, 2015

Contributor

Hi @nyalldawson, thanks for this fix, the simplification used the original wkb stream to avoid an extra copy of the geometry. But it worked assuming that each feature to paint is readed each time from data provider. Other alternatve might be just copy it when the memory layer returns the features avoiding this extra malloc for features from other providers.

I guess the performance penalty will not be perceptible and is safer.

Best Regards
Alvaro

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Aug 3, 2015

Author Collaborator

@ahuarte47 I think the impact of the extra malloc should be counteracted by the win from the implicit sharing. That said, there's potentially a better way to do this... I wasn't sure if the memcpy from wkb->targetWkb was required, or whether simplifyWkbGeometry will overwrite all the contents of targetWkb anyway? Or does it assume that it can skip parts of targetWkb which it doesn't need to modify?

I'm also not exactly sure if this impacted outside of the memory layer provider, but in any case we need to remove all these non-const casting for geometry so that the implicit sharing detachment works correctly.

This comment has been minimized.

Copy link
@ahuarte47

ahuarte47 Aug 3, 2015

Contributor

simplifyWkbGeometry always overwrites the source wkb (simultaneously it is used to read and write the new simplified version of points), when no simplification is needed, the wkb remains unchanged, but rarely it happens. As you say, to avoid problems outside, it is safer copy the geometry.

Thanks @nyalldawson
Alvaro

{
unsigned char* targetWkb = new unsigned char[targetWkbSize];
memcpy( targetWkb, wkb, targetWkbSize );
geometry->fromWkb( targetWkb, targetWkbSize );
unsigned char* finalWkb = new unsigned char[finalWkbSize];
memcpy( finalWkb, targetWkb, finalWkbSize );
geometry->fromWkb( finalWkb, finalWkbSize );
delete [] targetWkb;
return true;
}
delete [] targetWkb;
return false;
}

@@ -45,7 +45,7 @@ class CORE_EXPORT QgsMapToPixelSimplifier : public QgsAbstractGeometrySimplifier

private:
//! Simplify the WKB-geometry using the specified tolerance
static bool simplifyWkbGeometry( int simplifyFlags, QGis::WkbType wkbType, unsigned char* sourceWkb, size_t sourceWkbSize, unsigned char* targetWkb, size_t& targetWkbSize, const QgsRectangle& envelope, double map2pixelTol, bool writeHeader = true, bool isaLinearRing = false );
static bool simplifyWkbGeometry( int simplifyFlags, QGis::WkbType wkbType, const unsigned char* sourceWkb, size_t sourceWkbSize, unsigned char* targetWkb, size_t& targetWkbSize, const QgsRectangle& envelope, double map2pixelTol, bool writeHeader = true, bool isaLinearRing = false );

protected:
//! Current simplification flags

1 comment on commit f00c52d

@NathanW2

This comment has been minimized.

Copy link
Member

@NathanW2 NathanW2 commented on f00c52d Aug 3, 2015

ouch

Please sign in to comment.
You can’t perform that action at this time.