Parallelize snap caching

[troopa81]

Description

This PR fixes the issue related to this PR #31374.
EDIT: It now also contains PR #31374 commits because it has been reverted for 3.10.0 in #32297

The snapToCurrentLayer method doesn't prepare the index and expect to be executed synchronously. So the temporary point locator must be synchronous.

It also fixes the crashes on exit.

Checklist

• Commit messages are descriptive and explain the rationale for changes
• Commits which fix bugs include Fixes #11111 at the bottom of the commit message
• I have read the QGIS Coding Standards and this PR complies with them
• New unit tests have been added for core changes
• I have run the scripts/prepare-commit.sh script before each commit
• I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

[nyalldawson]

Thanks!

[troopa81]

I think there is other problems with my previous PR #31374.

snapToCurrenLayer is always synchronous while snapToMap depends on the asynchronous parameter.

Indeed, the idea of the PR was to make snapToMap in QGIS application asynchronous, so while the user move the cursor, the snap cache is built, and the snapping information are displayed as soon as they are built.

But, it can be possible that people wants to use the snapToMap method synchronously in the application (like in PR #31613 for instance), and that won't work.

I'm considering 3 possibilities:

• Let's the way it is, snapToMap is always asynchronous is QGIS app, tool that want to call this method has to call it on mouseMove (and display snapping information) so it will be ready when clicking
• Move asynchronous boolean on snapToMap method. If synchronous, prepareIndex waits for point locator to be finished. It looks a good solution but it's not easy to wait for QRunnable thread to be finished
• make snapToMap always synchronous, replace QgsTaskManager with a QtConcurrent blocking call. It will still be quicker than before the PR, but user will have to wait for all indexes to be ready.

[wonder-sk]

Patching just snapToCurrentLayer seems a bit ad-hoc and as you say, there may be problems with other methods as well (including the ones in QgsPointLocator).

I would suggest to keep the original API calls (snapToMap, snapToCurrentLayer etc) all synchronous, and introduce new methods with a new naming (e.g. snapToMapRelaxed?) that would be allowed to return invalid results if the index is being built... Then we would just modify some places to use the "relaxed" version so that this choice / optimization is always deliberate. A new variant of those functions compared to addition of a new true/false flag is IMHO a better indication that some special behavior is requested...

[troopa81]

@wonder-sk Thank you for your advices. I agree with your comments and will quickly propose a rewrite of my code to provide a snapToMap and snapToMapRelaxed methods.

I propose to not share the asynchronous and synchronous locators, so snapToMap will deal with its own synchronous locators map and snapToMapRelaxed with its asynchronous locators map.

[troopa81]

I added a snapToMapRelaxed method and also a boolean to get asynchronous point locators (needed in qgsvertextools). I use this method for maptools which provides some snapping information feedback and let the other use the classic snapToMap method.

[nyalldawson]

@wonder-sk?

[haubourg]

@wonder-sk any opinion here?

[stale]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[troopa81]

Please don't stale

[nyalldawson]

@troopa81 3.10 is in hard freeze now and we've still got many open bugs relating to snapping due to this work not being complete/merged in time. Can you please open a pr reverting the earlier work for 3.10.0 asap so that we can get master back in release-ready state?

[troopa81]

@nyalldawson I would rather fix bugs you are mentionning than postponed this work.

I take a quick look on qgis issues, and fail to find other issues related to this work. There is, if I recall correctly, 5 map tools that are currently broken and that PR should fix these. Is there other issues I'm not aware of?

Most important thing I think is to agree on the API, because we won't change it after hard freeze. @wonder-sk did you have the time to check API changes?

Anyway, if there is too much issues or complexity related to this work, I will revert my commit but again, I would prefer not to.

[mhugo]

@troopa81 About your proposed API changes, could you confirm what should be approved. I see it sums up to these two changes:

• QgsSnappingUtils constructor : the optional "asynchronous = false" parameter is removed
• QgsPointLocator *QgsSnappingUtils::locatorForLayer() : one new optional parameter "asynchronous = false" is added.

Is there something else ?

[troopa81]

@mhugo there is also the new method QgsSnappingUtils::snapToMapRelaxed()

But this PR is the following of this one #31374

Actually the API changes bring by this work (the 2 PR) is:

• new method QgsSnappingUtils::snapToMapRelaxed()
• QgsPointLocator *QgsSnappingUtils::locatorForLayer() : one new optional parameter "asynchronous = false" is added.
• QgsPointLocator constructor: one new optional parameter "asynchronous = false" is added.
• new method QgsPointLocator::isIndexing

[wonder-sk]

Oops sorry everyone - my notifications to this PR got lost in my mail among 500+ other unread qgis notifications :-( Going to have a look...

[wonder-sk]

Hmm I am not sure I understand why we need to have separately synchronous and asynchronous locators... Could you please explain the reasoning?

I am worried that this just adds extra memory+CPU overhead and higher code complexity without an obvious reason. I would expect to have just a single locator for each layer - if the locator is being indexed in the background and there's a call to non-relaxed version of snapToMap (or other function), it would simply block until the indexing in the background has finished.

For QgsPointLocator, rather than having the whole class either sync or async, it may be good to have init() and initAsync() and also have relaxed versions of the search functions like nearestVertex or nearestEdge.

[troopa81]

@wonder-sk Thanks for your review

I am worried that this just adds extra memory+CPU overhead

Yes, indeed

Could you please explain the reasoning?

Main reasons was :

• I cannot see a way by the time to wait for QgsPointLocatorInitTask to finish, but it looks like this is exactly the purpose of QgsTask::waitForFinished, so my bad
• I didn't want to pass an asynchronous parameter all the time like you can see in this commit 3de2eff

it may be good to have init() and initAsync() and also have relaxed versions of the search functions like nearestVertex or nearestEdge.

Don't you think that adding a relaxed version for all 8 methods (nearestVertex, nearestArea, edgesInrect...) will add complexity, meaning it would add 3 methods per each(nearestVertex, nearestVertexRelaxed and _nearestVertex private one) ? Isn't an asynchronous boolean (default to false) a better option?

Same remark apply to snapToMap

[wonder-sk]

Don't you think that adding a relaxed version for all 8 methods (nearestVertex, nearestArea, edgesInrect...) will add complexity, meaning it would add 3 methods per each(nearestVertex, nearestVertexRelaxed and _nearestVertex private one) ? Isn't an asynchronous boolean (default to false) a better option?

Yeah, it is a bit tricky decision to do. My intention was mainly to avoid the boolean trap in API, to have the code easier to read: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap
But I can live also with the extra boolean in the calls. Just not sure if 'asynchronous' would be a good name for the parameter - the queries are not asynchronous, it's rather an indication if we can live with incorrect result or not :-)

[troopa81]

My intention was mainly to avoid the boolean trap in API, to have the code easier to read: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap

interesting read, I understand your point. But I think I prefer the boolean way over having too much methods.

If we stick to the boolean way, I propose to remove the snapToMapRelaxed and add a boolean to snapToMap in order to have the QgsSnappingUtils and QgsPointLocator consistent. Are you agree?

Just not sure if 'asynchronous' would be a good name for the parameter

I renamed it "relaxed".

[wonder-sk]

If we stick to the boolean way, I propose to remove the snapToMapRelaxed and add a boolean to snapToMap in order to have the QgsSnappingUtils and QgsPointLocator consistent. Are you agree?

Yeah sounds good to me!

[troopa81]

Yeah sounds good to me!

Done b06cee2

I had to fix QgsTaskManager::waitForFinished 1bde15f so it waits the task to be started before waiting to be finished

[troopa81]

I create a PR #32453 for waitforfinished fix. This later should be merged before this one.

[wonder-sk]

The code is looking really good, just few notes from me...

[wonder-sk]

Thanks for the updates! Can we get rid of the rebuildIndexFinished() signal as well? it should be unused now - then I think we should be good to go :)

[troopa81]

Thanks for the updates! Can we get rid of the rebuildIndexFinished() signal as well?

Yes, done! Thanks for the time spending reviewing this PR (and the one before)

[wonder-sk]

Cool - let's get it merged!

[backporting]

The backport to release-3_10 failed:

Commits ["f96e1922cae31c43cded24bb87793f26c50129d5","0bf527f54da19ad9941ee4be90e30aa6ea427f4c","7fd98e8782dccc365c716fdd79e65457e8781e3b","f03da03d33c7924198493ffbfa283d6e533d044b","d246d9f5930a45a2fbd30cd3727a8cc4632b36c3","d0e26fb3bca2d984625e74cc38b1ea872ddd6107","95a100bcc4db64bafe3273a86e0d58e4f28a5e2d","2cb391aeda06b387c986d7b63136a2f83e92320a","340651b2f73c226f3eced8dc705324cbe4246f0d"] could not be cherry-picked on top of release-3_10

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport release-3_10
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick f96e1922cae31c43cded24bb87793f26c50129d5 0bf527f54da19ad9941ee4be90e30aa6ea427f4c 7fd98e8782dccc365c716fdd79e65457e8781e3b f03da03d33c7924198493ffbfa283d6e533d044b d246d9f5930a45a2fbd30cd3727a8cc4632b36c3 d0e26fb3bca2d984625e74cc38b1ea872ddd6107 95a100bcc4db64bafe3273a86e0d58e4f28a5e2d 2cb391aeda06b387c986d7b63136a2f83e92320a 340651b2f73c226f3eced8dc705324cbe4246f0d
# Create a new branch with these backported commits.
git checkout -b backport-31648-to-release-3_10
# Push it to GitHub.
git push --set-upstream origin backport-31648-to-release-3_10
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is release-3_10 and the compare/head branch is backport-31648-to-release-3_10.