Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ref functions #62

Open
wants to merge 12 commits into
base: refFunctions
Choose a base branch
from
Open

Ref functions #62

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4368205
new overlay functions
enricofer Jul 3, 2019
78d5400
overlay functions first implementation
enricofer Jul 7, 2019
5b203a6
optional parameter handling, disjoint, nearest multiresult
enricofer Jul 8, 2019
8b26b46
limit overlay nearest neighbors to 1
enricofer Jul 10, 2019
8c4a04c
geometry overlay functions test suite
enricofer Jul 10, 2019
8a25613
limit features parameter; grow spatial index query geometry bbox for …
enricofer Jul 12, 2019
d493303
limit parameter default changed to -1
enricofer Jul 12, 2019
35bba7f
further optional parameters decode hack
enricofer Jul 12, 2019
32b7a00
right limit parameter extraction method
enricofer Jul 12, 2019
cd58105
exclude current feature for self overlay layer
enricofer Jul 14, 2019
d370b50
target layer with different crs reproject support
enricofer Jul 15, 2019
569f1a3
different crs reprojection code revision
enricofer Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 38 additions & 44 deletions src/core/expression/qgsexpressionfunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4884,9 +4884,9 @@ static QVariant fcnFileSize( const QVariantList &values, const QgsExpressionCont
return QFileInfo( file ).size();
}

typedef std::function < QVariant( QgsExpression &subExp, QgsExpressionContext &subContext, const QgsSpatialIndex &spatialIndex, std::shared_ptr<QgsVectorLayer> cachedTarget, const QgsGeometry &geometry, bool testOnly, bool invert, int neighbors, double max_distance ) > overlayFunc;
typedef std::function < QVariant( QgsExpression &subExp, QgsExpressionContext &subContext, const QgsSpatialIndex &spatialIndex, std::shared_ptr<QgsVectorLayer> cachedTarget, const QgsGeometry &geometry, bool testOnly, bool invert, int neighbors, double max_distance, double bboxGrow ) > overlayFunc;

static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, bool testOnly, const overlayFunc &overlayFunction, bool invert = false )
static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, bool testOnly, const overlayFunc &overlayFunction, bool invert = false, double bboxGrow = 0 )
{
// First parameter is the overlay layer
QgsExpressionNode *node = QgsExpressionUtils::getNode( values.at( 0 ), parent );
Expand All @@ -4898,13 +4898,11 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress

QString subExpString;
QgsExpression subExpression;
if ( values.length() > 1 ){
node = QgsExpressionUtils::getNode( values.at( 1 ), parent );
ENSURE_NO_EVAL_ERROR
subExpString = node->dump();
if ( subExpString == "NULL" ) {
testOnly = true;
}
node = QgsExpressionUtils::getNode( values.at( 1 ), parent );
ENSURE_NO_EVAL_ERROR
subExpString = node->dump();
if ( subExpString == "NULL" ) {
testOnly = true;
}

QgsSpatialIndex spatialIndex;
Expand All @@ -4918,15 +4916,15 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress
}

QgsFeatureRequest request; // TODO only required attributes
if ( values.length() > 2 ) { //filter cached features
QString filterString;
node = QgsExpressionUtils::getNode( values.at( 2 ), parent );
ENSURE_NO_EVAL_ERROR
filterString = node->dump();
if ( filterString != "NULL" ) {
request.setFilterExpression (filterString);
}
}
QString filterString;
node = QgsExpressionUtils::getNode( values.at( 2 ), parent );
ENSURE_NO_EVAL_ERROR
filterString = node->dump();
Copy link
Owner Author

Choose a reason for hiding this comment

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

For the filter the comment below (see limit) does not apply. We don't want it to be evaluated with the parent context but set it literally for the request, hence the dump().

if ( filterString != "NULL" ) request.setFilterExpression (filterString); //filter cached features

int limit = QgsExpressionUtils::getIntValue( values.at( 3 ), parent ); //in expressions overlay functions throw the exception: Eval Error: Cannot convert '' to int
Copy link

@enricofer enricofer Jul 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I added the limit attribute but when I try to read the third parameter as int I get this exception: Eval Error: Cannot convert '' to int
The same error I got reading nearest (neighbors and max_distance) optional parameters.

if (limit > 0) request.setLimit (limit);
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'd remove this check and set the default value for the limit to -1 (See https://qgis.org/api/classQgsFeatureRequest.html#aa724a450498eeba7a783ead6b62a2e67).


int neighbors = 1;
/*
if ( values.length() > 3 ) { //neighbors param handling
Expand All @@ -4947,7 +4945,7 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress
const QString cacheLayer { QStringLiteral( "ovrlaylyr:%1" ).arg( cacheBase ) };
const QString cacheIndex { QStringLiteral( "ovrlayidx:%1" ).arg( cacheBase ) };

if ( !context->hasCachedValue( cacheLayer ) )
if ( !context->hasCachedValue( cacheLayer ) ) // should check for same crs. if not the same we could think to reproject target layer before charging cache
Copy link
Owner Author

Choose a reason for hiding this comment

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

Good thinking! Added this to the todo list (but it should be done in the call to materialize, once cached we should just be able to assume that CRSes match

{
cachedTarget.reset( layer->materialize( request ) );
if ( layerCanBeCached )
Expand Down Expand Up @@ -4980,22 +4978,18 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress
FEAT_FROM_CONTEXT( context, feat )
const QgsGeometry geometry = feat.geometry();

return overlayFunction( subExpression, subContext, spatialIndex, cachedTarget, geometry, testOnly, invert, neighbors, max_distance);
return overlayFunction( subExpression, subContext, spatialIndex, cachedTarget, geometry, testOnly, invert, neighbors, max_distance, bboxGrow );
}

// Intersect functions:

typedef bool ( QgsGeometry::*t_relationFunction )( const QgsGeometry &geometry ) const;

template <t_relationFunction T>
static QVariant indexedFilteredOverlay( QgsExpression &subExp, QgsExpressionContext &subContext, const QgsSpatialIndex &spatialIndex, std::shared_ptr<QgsVectorLayer> cachedTarget, const QgsGeometry &geometry, bool testOnly, bool invert, int neighbors, double max_distance )
static QVariant indexedFilteredOverlay( QgsExpression &subExp, QgsExpressionContext &subContext, const QgsSpatialIndex &spatialIndex, std::shared_ptr<QgsVectorLayer> cachedTarget, const QgsGeometry &geometry, bool testOnly, bool invert, int neighbors, double max_distance, double bboxGrow = 0 )
{
QgsRectangle intDomain = geometry.boundingBox();
intDomain.grow(0.001); //include touches geom

//if ( T == &QgsGeometry::touches){
// intDomain.grow(0.1);
//}
if ( bboxGrow != 0 ) intDomain.grow(bboxGrow); //optional parameter to enlarge boundary context for touches and equals methods
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not sure how much you want me to comment on code style, but in the final version we will need to have this match the QGIS code style, where oneline if's are discouraged.
There are also a couple of other things like whitespacing and { on new lines, which will be fixed automatically by setting up the pre-commit hook if you are running on linux.

Copy link

Choose a reason for hiding this comment

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

okkkk

Copy link
Owner Author

Choose a reason for hiding this comment

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

Tell me if you don't care too much for this, I can do this for you quickly enough :)

Copy link

Choose a reason for hiding this comment

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

Don't mind. I came from python so I understand that the form is substance.


const QList<QgsFeatureId> targetFeatureIds = spatialIndex.intersects( intDomain );
Copy link

Choose a reason for hiding this comment

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

  1. What about allow layer self checking? Giving the same layer as target layer would be useful to extract cross data between features of the same layer. but to do this I should exclude current feature id. it would be simple if feature could be passed as parameter, but I see that you pass only geometry.... Any way current feature exclusion should be performed only when source layer and target layer is the same. But I can't understand where retrieve current source layer...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting idea.
You might be successful using

const QVariant layerVar = context->variable( QStringLiteral( "layer" ) );
QgsVectorLayer *sourceLayer = QgsExpressionUtils::getVectorLayer( layerVar, parent );


Expand Down Expand Up @@ -5043,7 +5037,7 @@ static QVariant indexedFilteredOverlay( QgsExpression &subExp, QgsExpressionCont
}
}

static QVariantList indexedFilteredNearest( QgsExpression &subExp, QgsExpressionContext &subContext, const QgsSpatialIndex &spatialIndex, std::shared_ptr<QgsVectorLayer> cachedTarget, const QgsGeometry &geometry, bool testOnly, bool invert, int neighbors, double max_distance)
static QVariantList indexedFilteredNearest( QgsExpression &subExp, QgsExpressionContext &subContext, const QgsSpatialIndex &spatialIndex, std::shared_ptr<QgsVectorLayer> cachedTarget, const QgsGeometry &geometry, bool testOnly, bool invert, int neighbors, double max_distance, double bboxGrow = 0)
{

const QList<QgsFeatureId> targetFeatureIds = spatialIndex.nearestNeighbor( geometry, neighbors, max_distance );
Expand Down Expand Up @@ -5089,7 +5083,7 @@ static QVariant fcnTestGeomOverlayCrosses( const QVariantList &values, const Qgs

static QVariant fcnGeomOverlayEquals( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, const QgsExpressionNodeFunction * )
{
return executeGeomOverlay( values, context, parent, false, indexedFilteredOverlay<&QgsGeometry::equals> );
return executeGeomOverlay( values, context, parent, false, indexedFilteredOverlay<&QgsGeometry::equals>, false, 0.01 ); //grow amount should adapt to current units
Copy link
Owner Author

Choose a reason for hiding this comment

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

re the comment, do the units matter here? I think we just want to grow it "slightly", where "slightly" could be layer->geometryOptions()->geometryPrecision() (or fallback to something like 10^-8 if qgsDoubleNear( precision, 0.0 )).

}

static QVariant fcnTestGeomOverlayEquals( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, const QgsExpressionNodeFunction * )
Expand All @@ -5099,7 +5093,7 @@ static QVariant fcnTestGeomOverlayEquals( const QVariantList &values, const QgsE

static QVariant fcnGeomOverlayTouches( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, const QgsExpressionNodeFunction * )
{
return executeGeomOverlay( values, context, parent, false, indexedFilteredOverlay<&QgsGeometry::touches> );
return executeGeomOverlay( values, context, parent, false, indexedFilteredOverlay<&QgsGeometry::touches>, false, 0.01 ); //grow amount should adapt to current units
}

static QVariant fcnTestGeomOverlayTouches( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, const QgsExpressionNodeFunction * )
Expand Down Expand Up @@ -5460,8 +5454,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayIntersectsFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_intersects" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: limit param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
fcnGeomOverlayIntersects, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
fcnGeomOverlayIntersectsFunc->setIsStatic( false );
Expand All @@ -5479,8 +5473,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayContainsFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_contains" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: filter param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
// TODO: limit param
fcnGeomOverlayContains, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
Expand All @@ -5499,9 +5493,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayCrossesFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_crosses" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: filter param
// TODO: limit param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
fcnGeomOverlayCrosses, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
fcnGeomOverlayCrossesFunc->setIsStatic( false );
Expand All @@ -5519,8 +5512,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayEqualsFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_equals" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: limit param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
fcnGeomOverlayEquals, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
fcnGeomOverlayEqualsFunc->setIsStatic( false );
Expand All @@ -5538,8 +5531,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayTouchesFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_touches" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: limit param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
fcnGeomOverlayTouches, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
fcnGeomOverlayTouchesFunc->setIsStatic( false );
Expand All @@ -5557,8 +5550,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayDisjointFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_disjoint" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: limit param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
fcnGeomOverlayDisjoint, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
fcnGeomOverlayDisjointFunc->setIsStatic( false );
Expand All @@ -5576,8 +5569,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
QgsStaticExpressionFunction *fcnGeomOverlayWithinFunc = new QgsStaticExpressionFunction( QStringLiteral( "geometry_overlay_within" ), QgsExpressionFunction::ParameterList()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true ),
// TODO: limit param
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 ),
fcnGeomOverlayWithin, QStringLiteral( "GeometryGroup" ), QString(), false, QSet<QString>() << QgsFeatureRequest::ALL_ATTRIBUTES, true );
// The current feature is accessed for the geometry, so this should not be cached
fcnGeomOverlayWithinFunc->setIsStatic( false );
Expand All @@ -5596,6 +5589,7 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
<< QgsExpressionFunction::Parameter( QStringLiteral( "layer" ) )
<< QgsExpressionFunction::Parameter( QStringLiteral( "expression" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "filter" ), true, QVariant(), true )
<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true, 0 )
<< QgsExpressionFunction::Parameter( QStringLiteral( "neighbors" ), true, 1 )
<< QgsExpressionFunction::Parameter( QStringLiteral( "max_distance" ), true, 0),
//<< QgsExpressionFunction::Parameter( QStringLiteral( "limit" ), true ),
Expand Down