Ref functions #62

m-kuhn · 2019-07-12T06:30:20Z

Introduction of overlay_{method} functions into the QGIS expression engine.

TODO:

Limit parameter
Deciding on how to / implementing "test_only" boolean return type
Investigate use in virtual fields etc.
Investigate crashes
Function documentation
CRS conversion where required
Nearest neighbour functionality (let's postpone that for when everything else is working)

m-kuhn

Good job so far!

m-kuhn · 2019-07-12T06:49:39Z

src/core/expression/qgsexpressionfunction.cpp

-
-    subExpString = node->dump();
-    ++ paramIndex;
+  if ( values.length() > 1 ){


There will always be the maximum number of parameters (currently 3). If the user does not specify a parameter, it will have the default value of the parameter. So this check is not required.

m-kuhn · 2019-07-12T06:56:38Z

src/core/expression/qgsexpressionfunction.cpp

+      node = QgsExpressionUtils::getNode( values.at( 1 ), parent );
+      ENSURE_NO_EVAL_ERROR
+      subExpString = node->dump();
+      if ( subExpString == "NULL" ) {


Ok, let's leave this for now and clearly document this special meaning of NULL.
I am still somewhat undecided on it.
Let's be prepared though that in a final review this still might have to be replaced with some other possibility.

m-kuhn · 2019-07-12T07:59:26Z

src/core/expression/qgsexpressionfunction.cpp

@@ -4938,7 +4960,7 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress

  if ( !context->hasCachedValue( cacheIndex ) )
  {
-    spatialIndex = QgsSpatialIndex( cachedTarget->getFeatures() );
+    spatialIndex = QgsSpatialIndex( cachedTarget->getFeatures(), nullptr, QgsSpatialIndex::FlagStoreFeatureGeometries );


Is this for the nearest neighbor function?

Yes I looked into new join by nearest processing algoritm code and I found it: https://github.com/qgis/QGIS/blob/8416e7588c4f05d3c69f792e0c0ef4589084e49d/src/analysis/processing/qgsalgorithmjoinbynearest.cpp#L189
It is for indexing full geometry instead of their boundary

I would suggest we either do this conditionally in here or make a new nearestNeighbourOverlay function.
Anyway, let's defer any work on nearest neighbour until everything else is in place, the nearest neighbour implementation is different enough from the others to have some special treatment, and I would like to try to avoid any distraction for the base functionality.

m-kuhn · 2019-07-12T08:00:58Z

src/core/expression/qgsexpressionfunction.cpp

 {
-  const QList<QgsFeatureId> targetFeatureIds = spatialIndex.intersects( geometry.boundingBox() );
+  QgsRectangle intDomain = geometry.boundingBox();
+  intDomain.grow(0.001); //include touches geom


Maybe "bboxGrow" could be an additional parameter to this method, so it's only done when required and 0 otherwise?

I don't know. It can be easily done, but I'm not sure of its pratical utility. Anyway it seems to me a bit hard to explain in docs and helps, and could be confusing for the user.

That's only an internal implementation detail, nothing to expose publicly.
The only effect it has is a slightly improved performance (because of a more efficient usage of the spatial index for functions which don't need to grow the bbox).

Okkk. Now I see what You mean. A new optional parameter in https://github.com/enricofer/QGIS/blob/8c4a04c7153001b6a89419999acea17e4372a038/src/core/expression/qgsexpressionfunction.cpp#L4887 to be used internally by touches and equals

Yep, exactly 👍

…equals and touches

enricofer · 2019-07-12T14:37:51Z

src/core/expression/qgsexpressionfunction.cpp

+  filterString = node->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


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.

m-kuhn · 2019-07-12T14:44:43Z

src/core/expression/qgsexpressionfunction.cpp

@@ -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


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

m-kuhn · 2019-07-12T14:47:44Z

src/core/expression/qgsexpressionfunction.cpp

-  //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


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.

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

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

m-kuhn · 2019-07-12T14:53:22Z

src/core/expression/qgsexpressionfunction.cpp

@@ -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


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 )).

m-kuhn · 2019-07-12T14:55:23Z

src/core/expression/qgsexpressionfunction.cpp

+  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
+  if (limit > 0) request.setLimit (limit);


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

enricofer · 2019-07-12T19:20:12Z

src/core/expression/qgsexpressionfunction.cpp

@@ -4926,8 +4926,13 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress
    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
-  request.setLimit( limit );
+  node = QgsExpressionUtils::getNode( values.at( 3 ), parent ); //in expressions overlay functions throw the exception: Eval Error: Cannot convert '' to int


It seems that further optional parameters have to be interpreted as an expression and then converted to int. The result is a bit tricky, but goes.

I don't know if it is the proper way to do this....

m-kuhn · 2019-07-12T19:49:39Z

src/core/expression/qgsexpressionfunction.cpp

+
+  node = QgsExpressionUtils::getNode( values.at( 3 ), parent ); //in expressions overlay functions throw the exception: Eval Error: Cannot convert '' to int
+  ENSURE_NO_EVAL_ERROR
+  QString limitString = node->dump();


I think you need to go for the following:

Suggested change

QString limitString = node->dump();

QVariant limitValue = node->eval( parent, context );

ENSURE_NO_EVAL_ERROR

qlonglong limit = QgsExpressionUtils::getIntValue( limitValue, parent );

request.setLimit( limit );

The issue is, that we need to do "lazy evaluation" in this method. We can't let the expression evaluate everything because we need to take care of the "filter" string manually and evaluate it with a different context.

m-kuhn · 2019-07-12T19:51:54Z

src/core/expression/qgsexpressionfunction.cpp

+  QString filterString;
+  node = QgsExpressionUtils::getNode( values.at( 2 ), parent );
+  ENSURE_NO_EVAL_ERROR
+  filterString = node->dump();


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().

enricofer · 2019-07-13T08:26:18Z

src/core/expression/qgsexpressionfunction.cpp

+  if ( bboxGrow != 0 )
+  {
+    intDomain.grow( bboxGrow ); //optional parameter to enlarge boundary context for touches and equals methods
+  }

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


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...

Interesting idea.
You might be successful using

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

enricofer · 2019-07-15T06:20:27Z

src/core/expression/qgsexpressionfunction.cpp

+  if (targetLayer->crs().authid() != sourceLayer->crs().authid())
+  {
+      QgsCoordinateTransformContext TransformContext;
+      request.setDestinationCrs(sourceLayer->crs(), TransformContext); //if crs are not the same, cached target will be reprojected to source crs


A new project for self overlay and different crs test

overlay_test5.zip

m-kuhn · 2019-07-15T11:47:27Z

src/core/expression/qgsexpressionfunction.cpp

+    request.setFilterExpression( filterString ); //filter cached features
+  }
+
+  if (targetLayer->crs().authid() != sourceLayer->crs().authid())


Better compare CRS directly (and not only authid)

Suggested change

if (targetLayer->crs().authid() != sourceLayer->crs().authid())

if ( targetLayer->crs() != sourceLayer->crs() )

m-kuhn · 2019-07-15T11:49:34Z

src/core/expression/qgsexpressionfunction.cpp

+
+  if (targetLayer->crs().authid() != sourceLayer->crs().authid())
+  {
+      QgsCoordinateTransformContext TransformContext;


The "real" transform context is available from the expression context.

Suggested change

QgsCoordinateTransformContext TransformContext;

QgsCoordinateTransformContext transformContext = context->variable( QStringLiteral( "_project_transform_context" ) ).value<QgsCoordinateTransformContext>();

olivierdalang · 2020-07-07T14:51:35Z

continued here : #64

olivierdalang · 2020-08-20T16:39:23Z

actually continued here : qgis#38405

enricofer added 5 commits July 3, 2019 22:13

new overlay functions

4368205

overlay functions first implementation

78d5400

optional parameter handling, disjoint, nearest multiresult

5b203a6

limit overlay nearest neighbors to 1

8b26b46

geometry overlay functions test suite

8c4a04c

m-kuhn commented Jul 12, 2019

View reviewed changes

limit features parameter; grow spatial index query geometry bbox for …

8a25613

…equals and touches

enricofer reviewed Jul 12, 2019

View reviewed changes

m-kuhn commented Jul 12, 2019

View reviewed changes

enricofer added 2 commits July 12, 2019 19:25

limit parameter default changed to -1

d493303

further optional parameters decode hack

35bba7f

enricofer reviewed Jul 12, 2019

View reviewed changes

m-kuhn commented Jul 12, 2019

View reviewed changes

right limit parameter extraction method

32b7a00

enricofer reviewed Jul 13, 2019

View reviewed changes

enricofer added 2 commits July 15, 2019 00:06

exclude current feature for self overlay layer

cd58105

target layer with different crs reproject support

d370b50

enricofer reviewed Jul 15, 2019

View reviewed changes

m-kuhn commented Jul 15, 2019

View reviewed changes

different crs reprojection code revision

569f1a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ref functions #62

Ref functions #62

m-kuhn commented Jul 12, 2019 •

edited

Loading

m-kuhn left a comment

m-kuhn Jul 12, 2019

m-kuhn Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019 •

edited

Loading

m-kuhn Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019

m-kuhn Jul 12, 2019

enricofer Jul 12, 2019

enricofer Jul 12, 2019

m-kuhn Jul 12, 2019 •

edited

Loading

m-kuhn Jul 12, 2019

enricofer Jul 13, 2019

m-kuhn Jul 14, 2019

enricofer Jul 15, 2019

m-kuhn Jul 15, 2019 •

edited

Loading

m-kuhn Jul 15, 2019 •

edited

Loading

olivierdalang commented Jul 7, 2020

olivierdalang commented Aug 20, 2020

	if (targetLayer->crs().authid() != sourceLayer->crs().authid())
	if ( targetLayer->crs() != sourceLayer->crs() )

	QgsCoordinateTransformContext TransformContext;
	QgsCoordinateTransformContext transformContext = context->variable( QStringLiteral( "_project_transform_context" ) ).value<QgsCoordinateTransformContext>();

Ref functions #62

Are you sure you want to change the base?

Ref functions #62

Conversation

m-kuhn commented Jul 12, 2019 • edited Loading

m-kuhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricofer Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kuhn Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-kuhn Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

m-kuhn Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

olivierdalang commented Jul 7, 2020

olivierdalang commented Aug 20, 2020

m-kuhn commented Jul 12, 2019 •

edited

Loading

enricofer Jul 12, 2019 •

edited

Loading

m-kuhn Jul 12, 2019 •

edited

Loading

m-kuhn Jul 15, 2019 •

edited

Loading

m-kuhn Jul 15, 2019 •

edited

Loading