[WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features #6657

lbartoletti · 2018-03-21T09:07:43Z

Description

Bug founded by QWAT Team. Fixes #16838.

Solution: Filter on layer rendered.

New option to enable it or not:

Screenshot for a normal case:

When this bug/feature is enabled:

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

nyalldawson

Unfortunately I don't think this approach will work - it's triggering a complete iteration through the layer (not just features in the view) for every candidate match -- it's going to be way too expensive to do.

luipir · 2018-03-21T09:20:19Z

I can't talk from a user point of view, but I feel that snapping should't be related with the fact that the layer is rendered or not (you may thing to snapToGrid, where grid is not a layer and nor is rendered).
If, from the usability point of view, this is the expected behaviour, I would expect that it would be configurable.
IMHO it's not a bug, but a feature.

wonder-sk · 2018-03-21T09:26:50Z

Agreed with both Nyall (performance) and Luigi (configuration): in my opinion the default behavior should be that all features are considered by QgsSnappingUtils (if not, we are breaking the API!), and there should be a flag whether to consider just visible features which would be turned on in QgsMapCanvasSnappingUtils.

andreasneumann · 2018-03-21T09:30:12Z

From a user point of view it is very confusing that invisible features (that aren't displayed for whatever reason) are acting as snap features. I don't agree with Luigi that it is a "feature".
It is probably a rare use-case that invisible features should act as snap features.

saberraz · 2018-03-21T09:35:44Z

Is this only for snapping or does it cover Trace tool too?

wonder-sk · 2018-03-21T09:38:36Z

To avoid confusion, here is how it should work IMHO:

QgsSnappingUtils: have a new flag whether to use all features or just the visible ones. default = all.
QgsMapCanvasSnappingUtils: the flag is set to just visible ones.

From user's point of view, in map canvas he/she only gets snapping to visible features. I don't think this needs to be configurable anywhere in the GUI... snapping to invisible features does not make much sense.

From developer's point of view, the spatial queries in QgsSnappingUtils work on all features, with extra option to use just visible features if needed. This class may be used in various analytical algorithms, so by default the behavior should not be affected by renderer configuration.

andreasneumann · 2018-03-21T09:52:02Z

@wonder-sk thanks - agreed.

lbartoletti · 2018-03-21T10:11:40Z

Thanks for your comments, I was still on it :) .
I wanted to add an option to enable or disable this portion ( bug or feature ;) )

Thanks @wonder-sk

nyalldawson · 2018-03-21T10:14:36Z

@lbartoletti

Something to keep in mind while reworking this is that renderer->filter() isn't comprehensive -- it's just a way to partially limit the features requested for the renderer.

Instead you need to check willRenderFeature instead.

lbartoletti · 2018-03-21T21:52:16Z

@nyalldawson
I'm sorry, I was not able to use willRenderFeature, I had already started using a spatial filter. Did I miss something? Here's how I thought I'd use it.

      QgsFeature feat = candidateMatch.layer()->getFeature( candidateMatch.featureId() );
      QgsRenderContext ctx = QgsRenderContext::fromMapSettings( mapSettings );
      candidateMatch.layer()->renderer()->startRender( ctx, candidateMatch.layer()->fields() );
      visible = candidateMatch.layer()->renderer()->willRenderFeature( feat, ctx );
      candidateMatch.layer()->renderer()->stopRender( ctx );

Return always true.

nyalldawson · 2018-03-21T22:04:17Z

@lbartoletti

That should work - maybe the map settings isn't correctly constructed? Check how the identify/select tools operate - they do a similar thing.

nyalldawson · 2018-03-21T22:06:03Z

Even with the spatial filter this is still going to be very slow. You need to avoid all feature requests when checking a match, because even a feature request for a single matching feature is expensive.

nyalldawson · 2018-03-21T22:10:45Z

A couple of other tips:

Don't start/stop render on every match, try to do it once for each layer in advance and once after completing the snap. Start/stop render can also be expensive, and if you start/stop it multiple times you lose all benefit of prepared expressions, etc.
use a clone of the layer renderer, not the layers renderer itself

lbartoletti · 2018-03-22T16:08:48Z

Thanks @nyalldawson willRenderFeature works, I have adapted identify tools.

lbartoletti · 2018-03-23T11:46:10Z

@nyalldawson Maybe, I have founded a bug with willRenderFeature.

Some cases works, some returns always true.

I have made a reproductible example here with a video.

Any idea?

3nids

some small details...Otherwise, it's a long awaited BUGFIX....not a feature ;) ...

3nids · 2018-03-23T12:24:14Z

src/core/qgssnappingutils.h

@@ -53,7 +53,7 @@ class CORE_EXPORT QgsSnappingUtils : public QObject
  public:

    //! Constructor for QgsSnappingUtils
-    QgsSnappingUtils( QObject *parent SIP_TRANSFERTHIS = nullptr );
+    QgsSnappingUtils( QObject *parent SIP_TRANSFERTHIS = nullptr, bool enableSnappingForInvisibleFeature = true );


I would prefer to keep the set method only to set the option (i.e. remove it) or if we keep it in the constructor, can we use an enum instead of a bool then?

3nids · 2018-03-24T10:03:27Z

src/core/qgssnappingutils.h

+     *
+     * \since QGIS 3.2
+     */
+    void setEnableSnappingForInvisibleFeature( bool enableIt );


s/enableIt/enable/

3nids · 2018-03-24T10:05:04Z

src/gui/qgsmapcanvassnappingutils.cpp


 #include <QApplication>
 #include <QProgressDialog>

 QgsMapCanvasSnappingUtils::QgsMapCanvasSnappingUtils( QgsMapCanvas *canvas, QObject *parent )
-  : QgsSnappingUtils( parent )
+  : QgsSnappingUtils( parent, QgsSettings().value( QStringLiteral( "/qgis/digitizing/snap_invisible_feature" ), false ).toBool() )


using an enum would be nicer here since you could use
QgsSettings().enumValue( QStringLiteral( "/qgis/digitizing/snap_invisible_feature" ), DoNotSnapOnInvisible );

nyalldawson

I hate to keep throwing up issues here - but this code NEEDS to be fast and anything which potentially impacts the speed of snapping is a no-go for me. Fetching features one-at-a-time from a provider is very inefficient, so that needs to be reworked.

When that's addressed it also needs unit tests before it's merge-able.

nyalldawson · 2018-03-26T01:10:37Z

src/app/qgsoptions.cpp

@@ -970,6 +970,7 @@ QgsOptions::QgsOptions( QWidget *parent, Qt::WindowFlags fl, const QList<QgsOpti

  mSnappingMarkerColorButton->setColor( mSettings->value( QStringLiteral( "/qgis/digitizing/snap_color" ), QColor( Qt::magenta ) ).value<QColor>() );
  mSnappingTooltipsCheckbox->setChecked( mSettings->value( QStringLiteral( "/qgis/digitizing/snap_tooltip" ), false ).toBool() );
+  mEnableSnappingOnInvisibleFeatureCheckbox->setChecked( mSettings->value( QStringLiteral( "/qgis/digitizing/snap_invisible_feature" ), false ).toBool() );


Please add this new setting to resources/qgis_global_settings.ini

nyalldawson · 2018-03-26T01:12:24Z

src/core/qgssnappingutils.cpp

+    if ( candidateMatch.layer() )
+    {
+      bool filter = false;
+      QgsRenderContext context( QgsRenderContext::fromMapSettings( mapSettings ) );


Can you please move this up from _isMatchAVisibleLayer so that the render context and renderer is only constructed once per layer per snap? startRender is expensive so this is also a performance killer.

nyalldawson · 2018-03-26T01:14:17Z

src/core/qgssnappingutils.cpp

+        filter = renderer->capabilities() & QgsFeatureRenderer::Filter;
+      }
+
+      QgsFeature feat = candidateMatch.layer()->getFeature( candidateMatch.featureId() );


This is going to be a performance killer too - can you find a way to avoid fetching individual features and instead do a single feature fetch for the whole canvas extent at once?

3nids · 2018-03-26T05:57:15Z

I am just thinking of another approach: what about saving the info along when building the index. Then, if a layer gets its symbology edited, you update the index (just the visible boolean flag).

@wonder-sk does it make sense?

lbartoletti · 2018-03-26T06:37:59Z

@nyalldawson
I haven't forgotten about your performance request. Before working on it, I wanted to make sure that it could work "like this".

@3nids I was also thinking about something like that.

pblottiere · 2018-03-26T17:15:04Z

Maybe, I have founded a bug with willRenderFeature.
Some cases works, some returns always true.
I have made a reproductible example here with a video.

@lbartoletti I think this issue is fixed in this PR: #6679. Can you give it a try and let me know if you have some time? Thanks!

wonder-sk · 2018-03-27T12:51:32Z

Agreed with @nyalldawson and @3nids - the checks whether features will be rendered should be IMHO done when building the index, it would be otherwise too slow.

Fix snapping on invisible geometry

ca2450d

nyalldawson reviewed Mar 21, 2018

View reviewed changes

Add an option to enable or not this feature. Add a spatial filter

a5be868

lbartoletti changed the title ~~[WIP][BUGFIX] Disable snapping on invisible features~~ [WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features Mar 22, 2018

Use willRenderFeature

0d1ff73

3nids reviewed Mar 24, 2018

View reviewed changes

nyalldawson requested changes Mar 26, 2018

View reviewed changes

pblottiere mentioned this pull request Mar 26, 2018

[bugfix] Fixes identify action on deactivated rules for QgsRuleBasedRenderer #6679

Merged

8 tasks

lbartoletti mentioned this pull request Apr 5, 2018

[BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features. Second version #6750

Merged

8 tasks

lbartoletti closed this Apr 23, 2018

lbartoletti deleted the visibleSnap branch April 23, 2018 06:49

qgib mentioned this pull request May 25, 2019

QGIS 3: vertex editor highlights features in non-visible layers #26516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features #6657

[WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features #6657

lbartoletti commented Mar 21, 2018 •

edited

Loading

nyalldawson left a comment

luipir commented Mar 21, 2018

wonder-sk commented Mar 21, 2018

andreasneumann commented Mar 21, 2018

saberraz commented Mar 21, 2018

wonder-sk commented Mar 21, 2018

andreasneumann commented Mar 21, 2018

lbartoletti commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

lbartoletti commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

lbartoletti commented Mar 22, 2018

lbartoletti commented Mar 23, 2018

3nids left a comment

3nids Mar 23, 2018

3nids Mar 24, 2018

3nids Mar 24, 2018

nyalldawson left a comment

nyalldawson Mar 26, 2018

nyalldawson Mar 26, 2018

nyalldawson Mar 26, 2018

3nids commented Mar 26, 2018

lbartoletti commented Mar 26, 2018

pblottiere commented Mar 26, 2018

wonder-sk commented Mar 27, 2018

[WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features #6657

[WIP][BUGFIX][FEATURE][NEEDS-DOCS] Disable snapping on invisible features #6657

Conversation

lbartoletti commented Mar 21, 2018 • edited Loading

Description

Checklist

nyalldawson left a comment

Choose a reason for hiding this comment

luipir commented Mar 21, 2018

wonder-sk commented Mar 21, 2018

andreasneumann commented Mar 21, 2018

saberraz commented Mar 21, 2018

wonder-sk commented Mar 21, 2018

andreasneumann commented Mar 21, 2018

lbartoletti commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

lbartoletti commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

nyalldawson commented Mar 21, 2018

lbartoletti commented Mar 22, 2018

lbartoletti commented Mar 23, 2018

3nids left a comment

Choose a reason for hiding this comment

3nids Mar 23, 2018

Choose a reason for hiding this comment

3nids Mar 24, 2018

Choose a reason for hiding this comment

3nids Mar 24, 2018

Choose a reason for hiding this comment

nyalldawson left a comment

Choose a reason for hiding this comment

nyalldawson Mar 26, 2018

Choose a reason for hiding this comment

nyalldawson Mar 26, 2018

Choose a reason for hiding this comment

nyalldawson Mar 26, 2018

Choose a reason for hiding this comment

3nids commented Mar 26, 2018

lbartoletti commented Mar 26, 2018

pblottiere commented Mar 26, 2018

wonder-sk commented Mar 27, 2018

lbartoletti commented Mar 21, 2018 •

edited

Loading