Fix topological slowness with spatial filtering

[Joonalai]

Description

This PR fixes #60124 by:

• Filtering amount of layers to add the topological points into

Backport needed for 3.40.

[Joonalai]

I have to apologize for this PR is not being completely ready yet (tests are not fixed), but it is crucial to get the discussion started as soon as possible since we really need this to the 3.40.3 release.

Could you @troopa81 please take a look at this already at this stage if possible?

[Joonalai]

The original reason why whole geometries were added instead of just snapped points is based on this comment by @uclaros. I propose that for those specific use cases separate feature requests could be made if really needed. For our application wit over 150 editable layers, topological digitizing is extremely slow at this current stage.

[github-actions]

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 5cc1d16)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 5cc1d16)

[troopa81]

Adding only snapped points instead of whole geometries in add feature tool

That would mean a regression compare to 3.38.

In the issue you are editing 200 layers at the same time. I'm wondering if it's reasonable? Should user not select only the layer consistent between them (those for which you want to test and add topo points) and edit only them instead ?

[troopa81]

Not looping every layer in vertex tool if there are no snapped points

But what if we drag some segment or set of points and there is no snap, we still have to try to add topo points for other vertex

[uclaros]

Not looping every layer in vertex tool if there are no snapped points
Adding only snapped points instead of whole geometries in add feature tool

I don't think this is the proper way to resolve your issue, as relying on snapped points is inadequate for topological integrity, as discussed on the linked comment.
I would expect that adding features with topological editing on 150 (:scream:) layers is not lightning fast!
For the vertex tool it seems that the culprit is that vertex tool is adding its topological points too often (with each digitized segment instead of once at the end)

[Joonalai]

Unfortunately, all the layers have to be editable at the same time to ensure the topological integrity of the database. I know that reverting already implemented functionality is not optimal, but it has caused a huge performance downgrade compared to the 3.34 tree.

What do you think, could another solution be to add method Match.layers to return all the layers sharing the snapped point and to loop those layers instead of all the layers in the project? And if so, could that change still be backported to 3.40?

[troopa81]

What do you think, could another solution be to add method Match.layers to return all the layers sharing the snapped point and to loop those layers instead of all the layers in the project? And if so, could that change still be backported to 3.40?

Like @uclaros already pointed out, I think that relying on only snapped point is not enough. I'm wondering if:

• we could reduce the number of points to be added as topopoints (instead of adding the whle geometry) by better understanding what are the new/modified points like we do in qgsvertextool.
• Speed up QgsVectorLayerUtils::addTopologicalPoints by using snapping cache QgsPointLocator instead of requesting the feature

But I'm afraid that these 2 modifications are complicated enough to not make it to 3.40.3 (or even being backported)

[troopa81]

For the vertex tool it seems that the culprit is that vertex tool is adding its topological points too often (with each digitized segment instead of once at the end)

Not sure to understand this comment. We call addTopologicalPoints on every new/modified points regarding of the different action that have been done before. We add no more no less than what we need, no?

[Joonalai]

After investigating more what really takes the time here, it appears that starting and ending edit commands takes the most of the time. It can be bypassed by checking intersection beforehand. I'll do some more investigating with this approach, but it looks promising and it does not revert any existing functionality.

[Joonalai]

I changed the whole fix to just check bbox intersection with QgsFeatureRequest. I tested this change thoroughly with layers with different CRS and this change actually brings the performance back to 3.34 level. I continue testing with loads of data and latency to ensure this is enough to fix the bug.

What do you guys think about the new approach? Could this be backported?

[Joonalai]

@troopa81 Should I add a little threshold to the bounding box like in addTopologicalPoints?

QGIS/src/core/vector/qgsvectorlayereditutils.cpp

Line 821 in 9f055dd

searchRect.grow( threshold );

---

We also noticed that it is suboptimal to call addTopologicalPoints( ) and thus make new feature request for every vertex in geom override:

QGIS/src/core/vector/qgsvectorlayereditutils.cpp

Line 786 in 9f055dd

if ( addTopologicalPoints( *it ) == 0 )

Should it be better to just have own logic determining the matching features for geometry, since it could be done with just one feature request? However, I think that this could be done in its own feature request, since it's not that critical in performance-vice.

[troopa81]

I changed the whole fix to just check bbox intersection with QgsFeatureRequest. I tested this change thoroughly with layers with different CRS and this change actually brings the performance back to 3.34 level. I continue testing with loads of data and latency to ensure this is enough to fix the bug.

That would mean the request would be executed twice when there are points to be added. I understand you want to avoid beginEditCommand but why this call took so long ?

[Joonalai]

It is not just one FeatureRequest that is being made. It is number of vertices times number of layers in the project. Here is a gif from my issue data showing that adding feature consisting of four vertex creates 800 featureRequests, ie. 4 * 200 requests. The code here is without the suggested change, just with added logging.

If we can get the number of requests down to 1 for each layer that has no data near the feature, I would say it is a remarkable optimization.

[Joonalai]

I understand you want to avoid beginEditCommand but why this call took so long ?

With GPKG data (issue data) edit command took the longest time (5-6ms). As I tested with PostGIS data with latencies (Toxiproxy is good tool for container latency simulation btw), requests took the longest time but by reducing those down to typically 1 per layer, the suggested change is still a big improvement.

[komima]

Core of the issue is not necessarily the absolute amount of layers etc. but the total UX when considering latency etc. and the issue would appear at much lower layer counts in some cases.

Its much easier for the user to keep all layers editable when doing topological stuff, since that would be worse to miss the topological points on a shared segment of 3 features, if there was for example 1 of those 3 layers on that shared segment in a not editable state? And when you have like even 20-50 layers editable, the issue starts to appear, but its a perfectly valid use case for now, since there is no easy way to on demand toggle editability of a group of layers you want to make topological edits to and also ensuring all the layers for example on a shared segment are actually editable when making the change.

[komima]

For vertex tool this change would increase the amount of feature requests made only by the amount of layers actually intersected with the vertex: for layers not intersected only the initial request is made, and for layers intersected .addTopologicalPoints(QgsPoint) will make another. There could be a further improvement later to remove that extra call as well since we could possibly use the initial feature request result also when adding topological points?

For 100 layers with 5 intersected by the vertex what is now 100 feature requests becomes 105. Of course for 3 editable and 3 intersected its 6 now, but maybe that is not an issue for now and can be optimized separately to become 3 again?

For add feature tool this would reduce the amount of feature requests made by (the amount of layers not intersected * (new feature vertices - 1)), and increase by the amount of layers intersected: for layers not intersected only the initial request is made but looping each vertex is skipped, and for layers intersected the logic stays as is with the added initial request. There could be further improvement to the (currently convinience) methods accepting QgsGeometry or QgsPointSequence to do a single feature request with all the vertices or also make use of the initial feature request as well to reduce it as well to the total layer count?

For 100 layers with 5 intersected by a new feature with 100 vertices what is now 10000 feature requests would become 595, and similarly 20/5/1000 would change from 20000 to 5015. Like above, worst case with point feature intersecting all layers it would be doubled for now.

Maybe there is one downside: bringing the layer precision & topological point search distance logic to this code as well, since it seems asimilarly buffered search on the initial request might be needed? Maybe that could be a separate method in edit utils, for example getTopologySearchRadius(QgsVectorLayer) or something like that, and use it here and in the edit utils?

[troopa81]

If we decide to go that way (filter out layer for which geometry doesn't intersect at all adding an extra request before calling addTopologicalPoint(Point) ) it should be done in addTopologicalPoints(QgsPointSequence/QgsGeometry) as a matter of consistency for all map tool.

I think that it would have been way better to use snapping cache QgsPointLocator that could have give you this information really fast without accessing data.

I'm not a big fan on adding extra request but if it can spares a lot more, that's maybe worthy

What do you think @uclaros ?

[Joonalai]

I did some benchmark testing with two different datasets that are available here as download. I hope this clarifies the issue we are having and how this improvement (filter before edit command) improves it. There is most likely a better solution that will turn both tools much more performant, but this change is lightweight enough to be backported and still provides good performance benefit.

Datasets:

• GPKG = 200 empty line GPKG layers
• PG = 50 line 3067 and 50 polygon 3903 PostGIS layers with 100 features inside each layer

Feature being digitized is a line with 4 vertex. It does not intersect with other features.

Dataset	GPKG	PG (1ms latency)	PG (20ms latency)
Add feature (no topological editing)	11ms	15ms	14ms
Add feature (master, fe5f8e6)	1300ms	3950ms	27540ms
Add feature (filter before edit command)	52ms	740ms	6646ms
Add feature (filter after edit command)	1313ms	1309ms	7517ms
Add feature (only snapped - the original suggestion)	8ms	13ms	15ms
Add feature (only start and end edit)	1027ms	286ms	294ms
Continue line (no topological editing)	11ms	6ms	7ms
Continue line (master, fe5f8e6)	1210ms	630ms	3821ms
Continue line (filter before edit command)	112ms	376ms	3260ms
Continue line (only snapped - the original suggestion)	12ms	7ms	7ms

Should I add a little threshold to the bounding box like in addTopologicalPoints?

QGIS/src/core/vector/qgsvectorlayereditutils.cpp

Line 821 in 9f055dd

searchRect.grow( threshold );

I will also add this threshold growing asap as an private method for both tools with ///@cond PRIVATE.

[uclaros]

For the vertex tool it seems that the culprit is that vertex tool is adding its topological points too often (with each digitized segment instead of once at the end)

Not sure to understand this comment. We call addTopologicalPoints on every new/modified points regarding of the different action that have been done before. We add no more no less than what we need, no?

The total amount is correct, but it is called more often than actually needed. I mean on each click we immediately add the points while we could be collecting the clicks and do the call once at the end (on rightclick, esc etc).

I think that it would have been way better to use snapping cache QgsPointLocator that could have give you this information really fast without accessing data.

Wouldn't this require snapping to be enabled for the candidate layer? We want this to work even when snapping is altogether disabled and users are adding specific coordinates using other means (cad dock widget, layer precision etc)

I'm not a big fan on adding extra request but if it can spares a lot more, that's maybe worthy

Me neither, requests can be very costly for remote connections.
For me the ideal would be to reduce the requests to one per layer and only begin the edit command when actually required. This could be achieved with a new int QgsVectorLayerEditUtils::addTopologicalPoints( const QgsPointSequence &ps ) implementation that would do something along the lines of:

• build points' bbox
• do request
• iterate results
• iterate points
• if edit command has not begun and geom.addTopologicalPoint() succeeds, call beginEditCommand()
• update feature if necessary
• end edit command if necessary

Do you think that would make sense?

[troopa81]

I tend to agree on the proposed approach. It adds an extra request but spares many more in most of the scenarios.

Like I said before, I would have prefer that we try to use snapping cache, but I'm still willing to merge as is (minus the review comments).

@uclaros Don't you see any reason not to ?

[troopa81]

Wouldn't this require snapping to be enabled for the candidate layer? We want this to work even when snapping is
altogether disabled and users are adding specific coordinates using other means (cad dock widget, layer precision etc)

Yes, but we could use it when we have it and fallback when we don't. Or we could just build it as we need it.

Do you think that would make sense?

Yes, I had something alike in mind when I was asking to move the actual logic inside addTopologicalPoints.
We could also transform the geom when they have different CRS. This way, all other addTopologicalPoint would get the same behavior.

@Joonalai Do you think it's doable?

[Joonalai]

@Joonalai Do you think it's doable?

It is doable at some point and it makes a perfect sense.

However, as that would require a lot of changes in the code and would thus become impossible to backport, I must ask could this change be merged and backported as is and could the improved workflow then be added as an additional PR later on?

Me neither, requests can be very costly for remote connections.

I understand that, but this change lowers the amount of requests in most cases as @komima explained.

[komima]

I mean on each click we immediately add the points

Changing the behaviour seems a bit more complex, and would probably be a feature, not a bugfix? Even then without other changes we would hit the same issue as with add-new-feature tool, where the current addTopologicalPoints implementation requests features for each vertex separately.

This could be achieved with a new int QgsVectorLayerEditUtils::addTopologicalPoints( const QgsPointSequence &ps ) implementation

Since the addTopologicalPoints(QgsGeometry) and addTopologicalPoints(QgsPointSequence) are now convience only methods for looping the vertices, there could improvements (later, not in this scope) to those as well, even without touching the edit command location yet? Since this proposed change reduces the amount of feature requests in most cases, making those methods to request features only once for all the vertices would also reduce the count even more. That would work together with the possible above change to vertex tool as well, if that would in the future call those methods as well at the end of the digitizing operation for all the edited vertices.

It seems moving the edit command logic is a much much larger task and it would affect all the locations where addTopologicalPoints is called? The amount of work required means it is then probably outside of our interests.

[troopa81]

I agree that modifying addTopologicalPoints the way you propose @uclaros would require to modify all the callee to, at least, remove the begin/EndEditCommand. I'm also concerned of plugins that could also use addTopologicalPoints and doesn't expect it to call begin/EndEditCommand. It's maybe not that bad, but it would change their command stack.

It seems to me that the proposed approach is less prone to generate new issues and would improve things for 3.40 while we try to improve it even more later on.

[Joonalai]

@troopa81 is there anything I could do to help this get merged?

[Joonalai]

Could a backport label be added for automated backport PR after merge?

[troopa81]

Could a backport label be added for automated backport PR after merge?

Done

[uclaros]

It seems to me that the proposed approach is less prone to generate new issues and would improve things for 3.40 while we try to improve it even more later on.

OK, let's merge this as workaround then!