Navigation Menu

Skip to content

Commit

Permalink
Fix geometry simplification for rendering altering feature geometry
Browse files Browse the repository at this point in the history 
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
nyalldawson committed Aug 3, 2015
1 parent 7934a92 commit f00c52d
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 17 deletions.
36 changes: 20 additions & 16 deletions src/core/qgsmaptopixelgeometrysimplifier.cpp
View file
Expand Up @@ -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;
Expand All @@ -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 )
{
Expand Down Expand Up @@ -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 )
Expand All @@ -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;

Expand Down Expand Up @@ -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 );

Expand Down Expand Up @@ -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 ) );
Expand Down Expand Up @@ -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();
Expand All @@ -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.

Sign in to view

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.

Sign in to view

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.

Sign in to view

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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/qgsmaptopixelgeometrysimplifier.h
View file
Expand Up @@ -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
Expand Down

1 comment on commit f00c52d

@NathanW2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch

Please sign in to comment.