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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca2450d
Fix snapping on invisible geometry
lbartoletti Mar 20, 2018
a5be868
Add an option to enable or not this feature. Add a spatial filter
lbartoletti Mar 21, 2018
0d1ff73
Use willRenderFeature
lbartoletti Mar 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion python/core/qgssnappingutils.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ which keeps the configuration in sync with map canvas (e.g. current view, active
%End
public:

QgsSnappingUtils( QObject *parent /TransferThis/ = 0 );
QgsSnappingUtils( QObject *parent /TransferThis/ = 0, bool enableSnappingForInvisibleFeature = true );
%Docstring
Constructor for QgsSnappingUtils
%End
Expand Down Expand Up @@ -139,6 +139,15 @@ Get extra information about the instance
QgsSnappingConfig config() const;
%Docstring
The snapping configuration controls the behavior of this object
%End

void setEnableSnappingForInvisibleFeature( bool enableIt );
%Docstring
Set if invisible features must be snapped or not.

:param enableIt: Enable or not this feature

.. versionadded:: 3.2
%End

public slots:
Expand Down
1 change: 1 addition & 0 deletions python/gui/qgsmapcanvassnappingutils.sip.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@




class QgsMapCanvasSnappingUtils : QgsSnappingUtils
{
%Docstring
Expand Down
2 changes: 2 additions & 0 deletions src/app/qgsoptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add this new setting to resources/qgis_global_settings.ini


//vertex marker
mMarkersOnlyForSelectedCheckBox->setChecked( mSettings->value( QStringLiteral( "/qgis/digitizing/marker_only_for_selected" ), true ).toBool() );
Expand Down Expand Up @@ -1482,6 +1483,7 @@ void QgsOptions::saveOptions()

mSettings->setValue( QStringLiteral( "/qgis/digitizing/snap_color" ), mSnappingMarkerColorButton->color() );
mSettings->setValue( QStringLiteral( "/qgis/digitizing/snap_tooltip" ), mSnappingTooltipsCheckbox->isChecked() );
mSettings->setValue( QStringLiteral( "/qgis/digitizing/snap_invisible_feature" ), mEnableSnappingOnInvisibleFeatureCheckbox->isChecked() );

mSettings->setValue( QStringLiteral( "/qgis/digitizing/marker_only_for_selected" ), mMarkersOnlyForSelectedCheckBox->isChecked() );

Expand Down
79 changes: 62 additions & 17 deletions src/core/qgssnappingutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@
#include "qgsproject.h"
#include "qgsvectorlayer.h"
#include "qgslogger.h"
#include "qgsrenderer.h"

QgsSnappingUtils::QgsSnappingUtils( QObject *parent )
QgsSnappingUtils::QgsSnappingUtils( QObject *parent, bool enableSnappingForInvisibleFeature )
: QObject( parent )
, mSnappingConfig( QgsProject::instance() )
, mEnableSnappingForInvisibleFeature( enableSnappingForInvisibleFeature )
{
}

Expand Down Expand Up @@ -92,8 +94,44 @@ bool QgsSnappingUtils::isIndexPrepared( QgsVectorLayer *vl, const QgsRectangle &
return ( mStrategy == IndexHybrid || mStrategy == IndexExtent ) && loc->hasIndex() && ( !loc->extent() || loc->extent()->contains( aoi ) ); // the index - even if it exists - is not suitable
}

static bool _isMatchAVisibleLayer( const QgsPointLocator::Match &candidateMatch, QgsMapSettings mapSettings, bool snapOnInvisibleFeature )
{

bool visible = true;
if ( !snapOnInvisibleFeature )
{
// segmentIntersection haven't a layer
if ( candidateMatch.layer() )
{
bool filter = false;
QgsRenderContext context( QgsRenderContext::fromMapSettings( mapSettings ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

context.expressionContext() << QgsExpressionContextUtils::layerScope( candidateMatch.layer() );
std::unique_ptr< QgsFeatureRenderer > renderer( candidateMatch.layer()->renderer() ? candidateMatch.layer()->renderer()->clone() : nullptr );
if ( renderer )
{
// setup scale for scale dependent visibility (rule based)
renderer->startRender( context, candidateMatch.layer()->fields() );
filter = renderer->capabilities() & QgsFeatureRenderer::Filter;
}

QgsFeature feat = candidateMatch.layer()->getFeature( candidateMatch.featureId() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

context.expressionContext().setFeature( feat );

if ( filter && !renderer->willRenderFeature( feat, context ) )
visible = false;

if ( renderer )
{
renderer->stopRender( context );
}
}
}

return visible;

}

static QgsPointLocator::Match _findClosestSegmentIntersection( const QgsPointXY &pt, const QgsPointLocator::MatchList &segments )
static QgsPointLocator::Match _findClosestSegmentIntersection( const QgsPointXY &pt, const QgsPointLocator::MatchList &segments, QgsMapSettings mapSettings, bool snapOnInvisibleFeature )
{
if ( segments.isEmpty() )
return QgsPointLocator::Match();
Expand All @@ -104,7 +142,7 @@ static QgsPointLocator::Match _findClosestSegmentIntersection( const QgsPointXY
QVector<QgsGeometry> geoms;
Q_FOREACH ( const QgsPointLocator::Match &m, segments )
{
if ( m.hasEdge() )
if ( m.hasEdge() && _isMatchAVisibleLayer( m, mapSettings, snapOnInvisibleFeature ) )
{
QgsPolylineXY pl( 2 );
m.edgePoints( pl[0], pl[1] );
Expand Down Expand Up @@ -156,9 +194,9 @@ static QgsPointLocator::Match _findClosestSegmentIntersection( const QgsPointXY
return QgsPointLocator::Match( QgsPointLocator::Vertex, nullptr, 0, std::sqrt( minSqrDist ), minP );
}


static void _replaceIfBetter( QgsPointLocator::Match &bestMatch, const QgsPointLocator::Match &candidateMatch, double maxDistance )
static void _replaceIfBetter( QgsPointLocator::Match &bestMatch, const QgsPointLocator::Match &candidateMatch, double maxDistance, QgsMapSettings mapSettings, bool snapOnInvisibleFeature )
{

// is candidate match relevant?
if ( !candidateMatch.isValid() || candidateMatch.distance() > maxDistance )
return;
Expand All @@ -171,26 +209,28 @@ static void _replaceIfBetter( QgsPointLocator::Match &bestMatch, const QgsPointL
if ( bestMatch.type() == QgsPointLocator::Vertex && candidateMatch.type() == QgsPointLocator::Edge )
return;

if ( !_isMatchAVisibleLayer( candidateMatch, mapSettings, snapOnInvisibleFeature ) )
return;

bestMatch = candidateMatch; // the other match is better!
}


static void _updateBestMatch( QgsPointLocator::Match &bestMatch, const QgsPointXY &pointMap, QgsPointLocator *loc, QgsPointLocator::Types type, double tolerance, QgsPointLocator::MatchFilter *filter )
static void _updateBestMatch( QgsPointLocator::Match &bestMatch, const QgsPointXY &pointMap, QgsPointLocator *loc, QgsPointLocator::Types type, double tolerance, QgsPointLocator::MatchFilter *filter, QgsMapSettings mapSettings, bool snapOnInvisibleFeature )
{
if ( type & QgsPointLocator::Vertex )
{
_replaceIfBetter( bestMatch, loc->nearestVertex( pointMap, tolerance, filter ), tolerance );
_replaceIfBetter( bestMatch, loc->nearestVertex( pointMap, tolerance, filter ), tolerance, mapSettings, snapOnInvisibleFeature );
}
if ( bestMatch.type() != QgsPointLocator::Vertex && ( type & QgsPointLocator::Edge ) )
{
_replaceIfBetter( bestMatch, loc->nearestEdge( pointMap, tolerance, filter ), tolerance );
_replaceIfBetter( bestMatch, loc->nearestEdge( pointMap, tolerance, filter ), tolerance, mapSettings, snapOnInvisibleFeature );
}
if ( bestMatch.type() != QgsPointLocator::Vertex && bestMatch.type() != QgsPointLocator::Edge && ( type & QgsPointLocator::Area ) )
{
// if edges were detected, set tolerance to 0 to only do pointInPolygon (and avoid redo nearestEdge)
if ( type & QgsPointLocator::Edge )
tolerance = 0;
_replaceIfBetter( bestMatch, loc->nearestArea( pointMap, tolerance, filter ), tolerance );
_replaceIfBetter( bestMatch, loc->nearestArea( pointMap, tolerance, filter ), tolerance, mapSettings, snapOnInvisibleFeature );
}
}

Expand Down Expand Up @@ -247,13 +287,13 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
return QgsPointLocator::Match();

QgsPointLocator::Match bestMatch;
_updateBestMatch( bestMatch, pointMap, loc, type, tolerance, filter );
_updateBestMatch( bestMatch, pointMap, loc, type, tolerance, filter, mMapSettings, mEnableSnappingForInvisibleFeature );

if ( mSnappingConfig.intersectionSnapping() )
{
QgsPointLocator *locEdges = locatorForLayerUsingStrategy( mCurrentLayer, pointMap, tolerance );
QgsPointLocator::MatchList edges = locEdges->edgesInRect( pointMap, tolerance );
_replaceIfBetter( bestMatch, _findClosestSegmentIntersection( pointMap, edges ), tolerance );
_replaceIfBetter( bestMatch, _findClosestSegmentIntersection( pointMap, edges, mMapSettings, mEnableSnappingForInvisibleFeature ), tolerance, mMapSettings, mEnableSnappingForInvisibleFeature );
}

return bestMatch;
Expand All @@ -277,7 +317,7 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
double tolerance = QgsTolerance::toleranceInProjectUnits( layerConfig.tolerance, layerConfig.layer, mMapSettings, layerConfig.unit );
if ( QgsPointLocator *loc = locatorForLayerUsingStrategy( layerConfig.layer, pointMap, tolerance ) )
{
_updateBestMatch( bestMatch, pointMap, loc, layerConfig.type, tolerance, filter );
_updateBestMatch( bestMatch, pointMap, loc, layerConfig.type, tolerance, filter, mMapSettings, mEnableSnappingForInvisibleFeature );

if ( mSnappingConfig.intersectionSnapping() )
{
Expand All @@ -288,7 +328,7 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
}

if ( mSnappingConfig.intersectionSnapping() )
_replaceIfBetter( bestMatch, _findClosestSegmentIntersection( pointMap, edges ), maxSnapIntTolerance );
_replaceIfBetter( bestMatch, _findClosestSegmentIntersection( pointMap, edges, mMapSettings, mEnableSnappingForInvisibleFeature ), maxSnapIntTolerance, mMapSettings, mEnableSnappingForInvisibleFeature );

return bestMatch;
}
Expand All @@ -313,15 +353,15 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
QgsVectorLayer *vl = entry.first;
if ( QgsPointLocator *loc = locatorForLayerUsingStrategy( vl, pointMap, tolerance ) )
{
_updateBestMatch( bestMatch, pointMap, loc, type, tolerance, filter );
_updateBestMatch( bestMatch, pointMap, loc, type, tolerance, filter, mMapSettings, mEnableSnappingForInvisibleFeature );

if ( mSnappingConfig.intersectionSnapping() )
edges << loc->edgesInRect( pointMap, tolerance );
}
}

if ( mSnappingConfig.intersectionSnapping() )
_replaceIfBetter( bestMatch, _findClosestSegmentIntersection( pointMap, edges ), tolerance );
_replaceIfBetter( bestMatch, _findClosestSegmentIntersection( pointMap, edges, mMapSettings, mEnableSnappingForInvisibleFeature ), tolerance, mMapSettings, mEnableSnappingForInvisibleFeature );

return bestMatch;
}
Expand Down Expand Up @@ -428,6 +468,11 @@ QgsSnappingConfig QgsSnappingUtils::config() const
return mSnappingConfig;
}

void QgsSnappingUtils::setEnableSnappingForInvisibleFeature( bool enableIt )
{
mEnableSnappingForInvisibleFeature = enableIt;
}

void QgsSnappingUtils::setConfig( const QgsSnappingConfig &config )
{
if ( mSnappingConfig == config )
Expand Down Expand Up @@ -460,7 +505,7 @@ QgsPointLocator::Match QgsSnappingUtils::snapToCurrentLayer( QPoint point, QgsPo
return QgsPointLocator::Match();

QgsPointLocator::Match bestMatch;
_updateBestMatch( bestMatch, pointMap, loc, type, tolerance, filter );
_updateBestMatch( bestMatch, pointMap, loc, type, tolerance, filter, mMapSettings, mEnableSnappingForInvisibleFeature );
return bestMatch;
}

Expand Down
14 changes: 13 additions & 1 deletion src/core/qgssnappingutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Copy link
Member

Choose a reason for hiding this comment

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

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?

~QgsSnappingUtils() override;

// main actions
Expand Down Expand Up @@ -159,6 +159,15 @@ class CORE_EXPORT QgsSnappingUtils : public QObject
*/
QgsSnappingConfig config() const;

/**
* Set if invisible features must be snapped or not.
*
* \param enableIt Enable or not this feature
*
* \since QGIS 3.2
*/
void setEnableSnappingForInvisibleFeature( bool enableIt );
Copy link
Member

Choose a reason for hiding this comment

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

s/enableIt/enable/


public slots:

/**
Expand Down Expand Up @@ -242,6 +251,9 @@ class CORE_EXPORT QgsSnappingUtils : public QObject

//! internal flag that an indexing process is going on. Prevents starting two processes in parallel.
bool mIsIndexing = false;

//! Disable or not the snapping on all features. By default is always true except for non visible features on map canvas.
bool mEnableSnappingForInvisibleFeature = true;
};


Expand Down
10 changes: 9 additions & 1 deletion src/gui/qgsmapcanvassnappingutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@

#include "qgsmapcanvas.h"
#include "qgsvectorlayer.h"
#include "qgssettings.h"

#include <QApplication>
#include <QProgressDialog>

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

Choose a reason for hiding this comment

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

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

, mCanvas( canvas )

{
Expand All @@ -30,13 +31,15 @@ QgsMapCanvasSnappingUtils::QgsMapCanvasSnappingUtils( QgsMapCanvas *canvas, QObj
connect( canvas, &QgsMapCanvas::layersChanged, this, &QgsMapCanvasSnappingUtils::canvasMapSettingsChanged );
connect( canvas, &QgsMapCanvas::currentLayerChanged, this, &QgsMapCanvasSnappingUtils::canvasCurrentLayerChanged );
connect( canvas, &QgsMapCanvas::transformContextChanged, this, &QgsMapCanvasSnappingUtils::canvasTransformContextChanged );
connect( canvas, &QgsMapCanvas::mapToolSet, this, &QgsMapCanvasSnappingUtils::canvasMapToolChanged );
canvasMapSettingsChanged();
canvasCurrentLayerChanged();
}

void QgsMapCanvasSnappingUtils::canvasMapSettingsChanged()
{
setMapSettings( mCanvas->mapSettings() );
setEnableSnappingForInvisibleFeature( QgsSettings().value( QStringLiteral( "/qgis/digitizing/snap_invisible_feature" ), false ).toBool() );
}

void QgsMapCanvasSnappingUtils::canvasTransformContextChanged()
Expand All @@ -51,6 +54,11 @@ void QgsMapCanvasSnappingUtils::canvasCurrentLayerChanged()
setCurrentLayer( qobject_cast<QgsVectorLayer *>( mCanvas->currentLayer() ) );
}

void QgsMapCanvasSnappingUtils::canvasMapToolChanged()
{
setEnableSnappingForInvisibleFeature( QgsSettings().value( QStringLiteral( "/qgis/digitizing/snap_invisible_feature" ), false ).toBool() );
}

void QgsMapCanvasSnappingUtils::prepareIndexStarting( int count )
{
QApplication::setOverrideCursor( Qt::WaitCursor );
Expand Down
3 changes: 3 additions & 0 deletions src/gui/qgsmapcanvassnappingutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include "qgssnappingutils.h"
#include "qgis_gui.h"

#include "qgsmaptool.h"

class QgsMapCanvas;

class QProgressDialog;
Expand All @@ -42,6 +44,7 @@ class GUI_EXPORT QgsMapCanvasSnappingUtils : public QgsSnappingUtils
void canvasMapSettingsChanged();
void canvasTransformContextChanged();
void canvasCurrentLayerChanged();
void canvasMapToolChanged();

private:
QgsMapCanvas *mCanvas = nullptr;
Expand Down
Loading