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.

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

wants to merge 12 commits into from

Conversation

m-kuhn
Copy link
Owner

@m-kuhn m-kuhn commented Jul 12, 2019

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)

Copy link
Owner Author

@m-kuhn m-kuhn left a comment

Choose a reason for hiding this comment

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

Good job so far!


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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

@@ -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 );
Copy link
Owner Author

Choose a reason for hiding this comment

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

Is this for the nearest neighbor function?

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

okkk

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, exactly 👍

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
Copy link

@enricofer enricofer Jul 12, 2019

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.

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

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

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

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.

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

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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


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();
Copy link
Owner Author

@m-kuhn m-kuhn Jul 12, 2019

Choose a reason for hiding this comment

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

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.

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 ( bboxGrow != 0 )
{
intDomain.grow( bboxGrow ); //optional parameter to enlarge boundary context for touches and equals methods
}

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

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

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

Choose a reason for hiding this comment

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

A new project for self overlay and different crs test

overlay_test5.zip

request.setFilterExpression( filterString ); //filter cached features
}

if (targetLayer->crs().authid() != sourceLayer->crs().authid())
Copy link
Owner Author

@m-kuhn m-kuhn Jul 15, 2019

Choose a reason for hiding this comment

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

Better compare CRS directly (and not only authid)

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


if (targetLayer->crs().authid() != sourceLayer->crs().authid())
{
QgsCoordinateTransformContext TransformContext;
Copy link
Owner Author

@m-kuhn m-kuhn Jul 15, 2019

Choose a reason for hiding this comment

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

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
Copy link

continued here : #64

@olivierdalang
Copy link

actually continued here : qgis#38405

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants