Expose snapping indexing strategy [FEATURE] #8862

mhugo · 2019-01-15T11:07:33Z

This adds a combo box in the snapping widget that allows to choose the
snapping indexing strategy.

When snapping indexes get destroyed frequently (by declaring "data
dependencies" between layers for example), it becomes useful to tune
the strategy used to build snap indexes.

Description

Include a few sentences describing the overall goals for this PR (pull request). If applicable also add screenshots.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include fixes #11111 in the commit message next to the description
Commits which add new features are tagged with [FEATURE] in the commit message
Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
I have read the QGIS Coding Standards and this PR complies with them
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit

This adds a combo box in the snapping widget that allows to choose the snapping indexing strategy. When snapping indexes get detroyed frequently (by declaring "data dependencies" between layers for example), it becomes useful to tune the strategy used to build snap indexes.

m-kuhn · 2019-01-16T07:23:33Z

src/gui/qgsmapcanvas.cpp

+        QDomAttr attr = snappingNode.toElement().attributeNode( QStringLiteral( "strategy" ) );
+        if ( !attr.isNull() )
+        {
+          mSnappingUtils->setIndexingStrategy( static_cast<QgsSnappingUtils::IndexingStrategy>( attr.value().toInt() ) );


I think @3nids wrote some methods to make saving enums more semantic and avoiding casts. @3nids does that only work for QSettings or for xml docs too?

Ah I did not know, I'll have a look, thanks

Suggested change

mSnappingUtils->setIndexingStrategy( static_cast<QgsSnappingUtils::IndexingStrategy>( attr.value().toInt() ) );

mSnappingUtils->setIndexingStrategy( attr.value().value<QgsSnappingUtils::IndexingStrategy>() );

indeed, but it was in QgsSettings only. I will move this in qgis.h so it could be used anywhere.

integrated in #8879

m-kuhn · 2019-01-16T07:25:19Z

Sounds like a good proposal to me.
Is it only shown under "advanced configuration"?

CC @wonder-sk

nyalldawson · 2019-01-16T07:34:25Z

Just playing devils advocate here -- is this something we really want to expose to users? Is there no way we can tune the automatic method selection to handle this use case without user intervention?

mhugo · 2019-01-16T08:32:15Z

@nyalldawson To be honest, I am also still a bit hesitating to expose this kind of implementation detail to users. So, thanks for your concern :)

This feature is related to the problem I face with database layers that have geometry triggers: http://osgeo-org.1560.x6.nabble.com/QGIS-Developer-Vertex-tool-and-cache-invalidation-td5389381.html

I found that using the IndexExtent strategy and dataDependencies declared on layers allows to have the new vertex tool usable with layers that have side effects from database triggers. The default strategy makes it barely usable because snapping indexes get rebuilt with each edition and it is very slow.

This is not a perfect situation, since a lot of work have to be done by the user just to have a usable snapping (add "data dependencies", and now switch to "IndexExtent") and should probably be seen as a temporary solution waiting for a less "intrusive" one (if that only exists). But I agree adding things to the UI make them not really "temporary" ...

3nids · 2019-01-16T13:42:04Z

src/app/qgssnappingwidget.cpp

@@ -412,6 +433,16 @@ void QgsSnappingWidget::changeUnit( int idx )
  updateToleranceDecimals();
 }

+void QgsSnappingWidget::changeStrategy( int idx )
+{
+  QgsSnappingUtils::IndexingStrategy strategy = static_cast<QgsSnappingUtils::IndexingStrategy>( mStrategyComboBox->itemData( idx ).toInt() );


Suggested change

QgsSnappingUtils::IndexingStrategy strategy = static_cast<QgsSnappingUtils::IndexingStrategy>( mStrategyComboBox->itemData( idx ).toInt() );

QgsSnappingUtils::IndexingStrategy strategy = mStrategyComboBox->itemData( idx ).value<QgsSnappingUtils::IndexingStrategy>();

3nids · 2019-01-16T16:57:49Z

I am also -0 on adding this to the configuration, this is a bit obscure.
If added, it should not be shown only in advanced mode as this is not related. The advanced mode only means per layer configuration.
The only thing is that it should not be added to the toolbar I'd say!

nyalldawson · 2019-01-17T06:58:08Z

@mhugo

I found that using the IndexExtent strategy and dataDependencies declared on layers allows to have the new vertex tool usable with layers that have side effects from database triggers. The default strategy makes it barely usable because snapping indexes get rebuilt with each edition and it is very slow.

If this is the case, could we just make IndexExtent automatically set whenever a layer has dataDependencies set?

nirvn · 2019-01-17T07:00:16Z

For what it's worth, I'd be in favor of avoiding further cluttering the UI here in favor of coming up with an improved logic / fix too.

m-kuhn · 2019-01-17T07:41:45Z

... and/or trigger cache invalidation when a dataDependency indicates a change?

mhugo · 2019-01-17T10:55:02Z

@nyalldawson

If this is the case, could we just make IndexExtent automatically set whenever a layer has dataDependencies set?

I get your point, but ... there may be projects where a few data dependencies work also well with the default indexing strategy but does not work well with IndexExtent. It is hard to tell.

@m-kuhn

... and/or trigger cache invalidation when a dataDependency indicates a change?

This is already what happens (and is the only real usage of dataDependency AFAIK): it emits "dataChanged" whenever a feature is added/changed/deleted on a dependent layer. And dataChanged generates an index destruction (in QgsPointLocator).
The problem is that the reconstruction of the index may be quite slow with the default indexing strategy. Because it has not been initially thought for being rebuilt after each edition.

@m-kuhn @nyalldawson @nirvn
What about something a bit more hidden ? Like an option in Options / Digitizing / Snapping ?
Or even more hidden : a QgsSetting ?

m-kuhn · 2019-01-17T11:27:42Z

This is already what happens (and is the only real usage of dataDependency AFAIK): it emits "dataChanged" whenever a feature is added/changed/deleted on a dependent layer. And dataChanged generates an index destruction (in QgsPointLocator).

Ah nice, thanks!

I get your point, but ... there may be projects where a few data dependencies work also well with the default indexing strategy but does not work well with IndexExtent. It is hard to tell.

As it's hard to tell: could you check with the customer if this changed heuristic works for him and then we decide if it really needs to be an option?
If it needs to be an option, this "in Options / Digitizing / Snapping" sounds best to me.

nirvn · 2019-01-17T14:46:01Z

If this is a project-level setting (as currently implemented?) we'd be better finding it a place in the project properties.

mhugo · 2019-01-18T08:27:20Z

As it's hard to tell: could you check with the customer if this changed heuristic works for him and then we decide if it really needs to be an option?

I've tested a bit more the different strategies in the context of a project with layers that have lots of dependencies between them:

IndexNeverFull: it actually means 'No Index' and in situations where data sources are fast enough, this could work pretty well and could even work without having to declare data dependencies (that is, once other geometry caches get disabled or reworked - like in vertex tool :-/)
IndexAlwaysFull: only usable in very rare situations where layers' extents are very small
IndexHybrid: editing a layer even at small scales (in which case would you want to edit at a high scale ?) may be very slow because at each edition indexes may be rebuilt for the whole extent of each layer
IndexExtent: limit the rebuilt to the canvas visible extent, which is usually not too bad if the edition is made at a small scale, but can be worse than IndexHybrid if the current extent displays more than 50k features

There are also situations where people get bad performances and do not understand why (https://issues.qgis.org/issues/20843)

All of this tells me the default 'IndexHybrid', like any heuristics, cannot fits all situations and the heuristic choice should be changeable for "advanced" use cases.

If this is a project-level setting (as currently implemented?) we'd be better finding it a place in the project properties.

I would also prefer a project-level setting.
So, a new groupbox "Snapping" in Project Properties > General ?

wonder-sk · 2019-01-18T10:57:41Z

Please please... can we keep the indexing strategy as a little secret away for the poor users? :-)

The indexing strategy is a very low level implementation detail and it was never meant to be exposed to users... hybrid strategy should be the answer for everyone. If the current hybrid strategy is not good enough, I am sure that we can come up with some improved heuristics to make it better.

I am worried that by exposing indexing strategy we do not really solve the issues some people may be having - things need to work out of the box.

Some thoughts I had earlier on how to improve indexing: snapping could have two modes - strict and relaxed. Right now we only have strict mode, so all answers from the API must be correct. The new relaxed mode (which would be the default) would allow some requests to be answered temporarily incorrectly, but the plus side it would have better UX, because re-indexing could be done in background thread (not blocking GUI like now) and the "old" locators would not need to be destroyed immediately, just marked as needing update. Like this, any updates (including data dependencies / triggers) should not disrupt the user experience and everything should feel snappy... even without having to choose indexing strategy.

There are also situations where people get bad performances and do not understand why

QgsSnappingUtils::dump() gives some pointers on what is going on, so if needed this debugging information could be queried (e.g. in python console) - not sure if we need a GUI for that...?

haubourg · 2019-01-18T19:42:30Z

Sorry, I'm late to the party again :)

My dream - as a user - would be to have no cache at all, and no additional option to control snapping behavior. I tend to agree with @wonder-sk here. However, I'm not sure I really get how the 'relaxed mode' would look like. You make me curious Martin!

3nids · 2019-01-18T20:06:31Z

I can see in @wonder-sk 's mind
a technology preview of the relax mode, just for you:

wonder-sk · 2019-01-20T22:31:43Z

@3nids pretty much that's how I thought about it :-)

@haubourg to elaborate a bit on what I mean... Right now the main problem with UX with snapping is in situations when a snap at a map point is requested - and the implementation realizes that the cache is not present or cannot satisfy the request. In such case, the cache gets built and that may lock up the GUI for a bit. That's because the cache is built in the main thread (which also did the snap request). Once the cache is updated, the snap request is (quickly) processed and GUI is unlocked again. This wait time may be unpleasant but necessary to make sure that we return correct response to the snap request.

My thinking about this "relaxed" mode is the fact that not all the time we need to get the perfect answer for the snap requests. So the "relaxed" snap request would check the caches and give the result immediately without extra wait - even if some caches are not ready. At the same time it would start cache update in a background thread to make sure that caches are ready in full power sometime very soon. This way we "risk" that users would not see some snaps when they first arrive with their cursor on the desired position, but maybe a second later the snap would start to appear and mainly the GUI would not get locked.

This is just an idea I thought about before - maybe there are better solutions so I am keen to hear others' opinions. But the main point stays - users should not be required to tweak some obscure settings to get good results.

nyalldawson · 2019-01-20T22:45:45Z

@wonder-sk sounds like a nice plan, especially if users are shown some feedback to alert them that the index is being updated.

@m-hugo Given the concerns raised here, can we close this PR, or do something like rework to be a hidden custom property on the layer so that you can satisfy your immediate needs without exposing anything to users?

mhugo · 2019-01-21T07:43:34Z

@wonder-sk Hi. Thanks for your feedback.

About this "relaxed" mode. Indexing things using multiple thread may of course speed up things, which is a good thing and probably reduce the need to tweak the indexing strategy in most cases.

However, I am not sure about the second part where you say we will get wrong results immediately, followed by snap fixes. If I understand correctly, it will give the very strange results we'd like to avoid where the snapping index is out of date and other tools are aware of the new geometry. As it is the case right now with the vertex tool for instance:

Or am I missing something ?

While you are here, I have two questions :)

Just to understand: why are there multiple strategies, if the main application only uses one ? Are some people using the other strategies for some plugins ? If not, wouldn't it be better to deprecate them and only keep one strategy ?
The second is another idea about this cache index for our use case. What if the snapping cache get rebuilt on each canvas display, during the rendering loop ? That would make sure the cache is always up to date with what is displayed on the canvas, and we also could probably avoid requesting layers and call willRenderFeature() and so on a second time and also benefit from the multithreading already in place ?

@nyalldawson

Given the concerns raised here, can we close this PR, or do something like rework to be a hidden custom property on the layer so that you can satisfy your immediate needs without exposing anything to users?

Sure ! I've closed. Anyway we can continue discussing questions raised here on a closed PR :)

wonder-sk · 2019-01-22T12:24:54Z

If I understand correctly, it will give the very strange results we'd like to avoid where the snapping index is out of date and other tools are aware of the new geometry.

This is an independent problem I would say - what you mention is a case when we don't even know that snapping index is out of date and needs refreshing. That is something that providers would need to signal.

Just to understand: why are there multiple strategies, if the main application only uses one ? Are some people using the other strategies for some plugins ? If not, wouldn't it be better to deprecate them and only keep one strategy ?

The other strategies were intended to be used for anyone using snapping API directly without a map canvas. If I need fast queries in a layer, I can construct my own QgsSnappingUtils class and have a better control of how the index behaves. For usage in map canvas the hybrid (heuristic) approach should be fine (not saying that it is perfect now). So I would not deprecate it.

The second is another idea about this cache index for our use case. What if the snapping cache get rebuilt on each canvas display, during the rendering loop ? That would make sure the cache is always up to date with what is displayed on the canvas, and we also could probably avoid requesting layers and call willRenderFeature() and so on a second time and also benefit from the multithreading already in place ?

We used to have that before with old QgsSnapper implementation - its cache would get populated in rendering loop. I did not like such approach for various reasons...

rendering time would be increased because every single time a layer is rendered, the cache is cleared and populated again. That seems like a big waste of CPU time and memory given that you typically need snapping only for some editing map tools and it only needs to be refreshed from time to time. (under normal conditions the area covered by snapping index should be much larger than a single map view extent, so even when you move around the map, most of the time the snapping index should not need refresh)
map rendering and indexing features are quite different concerns - mixing them together makes the code much more complicated
if you want to snap also to features that are not visible with the current renderer, you need to cache separately
if you want to snap outside of the current map extent, you need to cache separately

mhugo · 2019-01-24T09:34:09Z

Sorry I did not get a notification for your reply

This is an independent problem I would say - what you mention is a case when we don't even know that snapping index is out of date and needs refreshing.

Hmmm. Let's suppose we have a reworked indexing code following your proposition where the indexing takes place in a separated thread and with a "relaxed" mode where the user can still plays on the canvas in the meantime.
Geometries have been modified by the user, and other geometries as well by, say a database trigger. Data dependencies exist between layers so that we know the snapping index is out of date.
So everything that requests directly the layer will "see" the new geometry, while other tools that rely on the indexing cache will see the "old" geometry, waiting for the new one to come when the background index rebuilt is over.
Am I missing something ?

That is something that providers would need to signal

Indeed, that could be another solution. But it seems to me quite complex and quite intrusive "just" to overcome a cache problem (especially for the postgres provider). This is why I am still trying to investigate other possibilities.

The other strategies were intended to be used for anyone using snapping API directly without a map canvas

Sure. My question was: do you know if it is actually used outside of a mapcanvas ?

We used to have that before with old QgsSnapper implementation - its cache would get populated in rendering loop. I did not like such approach for various reasons...

Thanks for your answer. Some points that I don't understand:

under normal conditions the area covered by snapping index should be much larger than a single map view extent, so even when you move around the map, most of the time the snapping index should not need refresh

Why ? If the snapping index follows the rendering loop, why should it be much larger ? Why not refreshing the index when moving the map ?

if you want to snap outside of the current map extent, you need to cache separately

You mean for applications outside of a mapcanvas I guess ?

map rendering and indexing features are quite different concerns - mixing them together makes the code much more complicated
if you want to snap also to features that are not visible with the current renderer, you need to cache separately

Yes I totally agree on these two points, it may adds a lot of complexity.

mhugo · 2019-01-24T09:34:43Z

@wonder-sk ^ (not sure if we get notifications on a closed PR)

mhugo added the Feature label Jan 15, 2019

mhugo added this to the 3.6 milestone Jan 15, 2019

m-kuhn reviewed Jan 16, 2019

View reviewed changes

3nids reviewed Jan 16, 2019

View reviewed changes

m-kuhn changed the title ~~[FEATURE] [needs-doc] Expose snapping indexing strategy~~ Expose snapping indexing strategy [FEATURE] [needs-doc] Jan 16, 2019

m-kuhn changed the title ~~Expose snapping indexing strategy [FEATURE] [needs-doc]~~ Expose snapping indexing strategy [FEATURE] Jan 16, 2019

nyalldawson modified the milestones: 3.6, 3.8 Jan 18, 2019

mhugo closed this Jan 21, 2019

MorriganR mentioned this pull request Jan 4, 2021

QgsPointLocator - unexpected behaviour #40845

Closed

troopa81 mentioned this pull request Aug 16, 2023

Build snapping index at render time qgis/QGIS-Enhancement-Proposals#275

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose snapping indexing strategy [FEATURE] #8862

Expose snapping indexing strategy [FEATURE] #8862

mhugo commented Jan 15, 2019

m-kuhn Jan 16, 2019

mhugo Jan 16, 2019

3nids Jan 16, 2019 •

edited

Loading

3nids Jan 16, 2019

3nids Jan 17, 2019

m-kuhn commented Jan 16, 2019

nyalldawson commented Jan 16, 2019

mhugo commented Jan 16, 2019

3nids Jan 16, 2019

3nids commented Jan 16, 2019

nyalldawson commented Jan 17, 2019

nirvn commented Jan 17, 2019

m-kuhn commented Jan 17, 2019

mhugo commented Jan 17, 2019

m-kuhn commented Jan 17, 2019

nirvn commented Jan 17, 2019

mhugo commented Jan 18, 2019

wonder-sk commented Jan 18, 2019

haubourg commented Jan 18, 2019

3nids commented Jan 18, 2019

wonder-sk commented Jan 20, 2019

nyalldawson commented Jan 20, 2019

mhugo commented Jan 21, 2019

wonder-sk commented Jan 22, 2019

mhugo commented Jan 24, 2019

mhugo commented Jan 24, 2019

	mSnappingUtils->setIndexingStrategy( static_cast<QgsSnappingUtils::IndexingStrategy>( attr.value().toInt() ) );
	mSnappingUtils->setIndexingStrategy( attr.value().value<QgsSnappingUtils::IndexingStrategy>() );

	QgsSnappingUtils::IndexingStrategy strategy = static_cast<QgsSnappingUtils::IndexingStrategy>( mStrategyComboBox->itemData( idx ).toInt() );
	QgsSnappingUtils::IndexingStrategy strategy = mStrategyComboBox->itemData( idx ).value<QgsSnappingUtils::IndexingStrategy>();

Expose snapping indexing strategy [FEATURE] #8862

Expose snapping indexing strategy [FEATURE] #8862

Conversation

mhugo commented Jan 15, 2019

Description

Checklist

m-kuhn Jan 16, 2019

Choose a reason for hiding this comment

mhugo Jan 16, 2019

Choose a reason for hiding this comment

3nids Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

3nids Jan 16, 2019

Choose a reason for hiding this comment

3nids Jan 17, 2019

Choose a reason for hiding this comment

m-kuhn commented Jan 16, 2019

nyalldawson commented Jan 16, 2019

mhugo commented Jan 16, 2019

3nids Jan 16, 2019

Choose a reason for hiding this comment

3nids commented Jan 16, 2019

nyalldawson commented Jan 17, 2019

nirvn commented Jan 17, 2019

m-kuhn commented Jan 17, 2019

mhugo commented Jan 17, 2019

m-kuhn commented Jan 17, 2019

nirvn commented Jan 17, 2019

mhugo commented Jan 18, 2019

wonder-sk commented Jan 18, 2019

haubourg commented Jan 18, 2019

3nids commented Jan 18, 2019

wonder-sk commented Jan 20, 2019

nyalldawson commented Jan 20, 2019

mhugo commented Jan 21, 2019

wonder-sk commented Jan 22, 2019

mhugo commented Jan 24, 2019

mhugo commented Jan 24, 2019

3nids Jan 16, 2019 •

edited

Loading