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.

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

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

Merged
merged 6 commits into from
May 1, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
8 changes: 7 additions & 1 deletion python/core/qgspointlocator.sip.in
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ Get extent of the area point locator covers - if null then it caches the whole l
Configure extent - if not null, it will index only that area

.. versionadded:: 2.14
%End

void setRenderContext( const QgsRenderContext *context );
%Docstring
Configure render context - if not null, it will use to index only visible feature

.. versionadded:: 3.2
%End

enum Type
Expand Down Expand Up @@ -199,7 +206,6 @@ Override of edgesInRect that construct rectangle from a center point and toleran
find out if the point is in any polygons
%End


int cachedGeometryCount() const;
%Docstring
Return how many geometries are cached in the index
Expand Down
11 changes: 10 additions & 1 deletion python/core/qgssnappingutils.sip.in
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 enable );
%Docstring
Set if invisible features must be snapped or not.

:param enable: Enable or not this feature

.. versionadded:: 3.2
%End

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




class QgsMapCanvasSnappingUtils : QgsSnappingUtils
{
%Docstring
Expand Down
3 changes: 3 additions & 0 deletions resources/qgis_global_settings.ini
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ digitizing\default_snap_type=1
# 2 = Project units
digitizing\default_snapping_tolerance_unit=1

# Snap on invisble feature
digitizing\snap_invisible_feature=false

# Default XYZ tile servers to include
connections-xyz\OpenStreetMap\authcfg=
connections-xyz\OpenStreetMap\password=
Expand Down
2 changes: 2 additions & 0 deletions src/app/qgsoptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -971,6 +971,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() );

//vertex marker
mMarkersOnlyForSelectedCheckBox->setChecked( mSettings->value( QStringLiteral( "/qgis/digitizing/marker_only_for_selected" ), true ).toBool() );
Expand Down Expand Up @@ -1483,6 +1484,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
1 change: 1 addition & 0 deletions src/core/layertree/qgslayertreemodellegendnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ bool QgsSymbolLegendNode::setData( const QVariant &value, int role )
vlayer->renderer()->checkLegendSymbolItem( mItem.ruleKey(), value == Qt::Checked );

emit dataChanged();
vlayer->emitStyleChanged();

vlayer->triggerRepaint();

Expand Down
83 changes: 83 additions & 0 deletions src/core/qgspointlocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "qgswkbptr.h"
#include "qgis.h"
#include "qgslogger.h"
#include "qgsrenderer.h"

#include <SpatialIndex.h>

Expand Down Expand Up @@ -635,6 +636,7 @@ QgsPointLocator::QgsPointLocator( QgsVectorLayer *layer, const QgsCoordinateRefe
connect( mLayer, &QgsVectorLayer::featureAdded, this, &QgsPointLocator::onFeatureAdded );
connect( mLayer, &QgsVectorLayer::featureDeleted, this, &QgsPointLocator::onFeatureDeleted );
connect( mLayer, &QgsVectorLayer::geometryChanged, this, &QgsPointLocator::onGeometryChanged );
connect( mLayer, &QgsVectorLayer::attributeValueChanged, this, &QgsPointLocator::onAttributeValueChanged );
connect( mLayer, &QgsVectorLayer::dataChanged, this, &QgsPointLocator::destroyIndex );
}

Expand All @@ -661,6 +663,20 @@ void QgsPointLocator::setExtent( const QgsRectangle *extent )
destroyIndex();
}

void QgsPointLocator::setRenderContext( const QgsRenderContext *context )
{
disconnect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex );

destroyIndex();
mContext.reset( nullptr );

if ( context )
{
mContext = std::unique_ptr<QgsRenderContext>( new QgsRenderContext( *context ) );
connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex );
}

}
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me there is no way to turn off visibility-dependent behavior of point locator after it has been enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible to configure the behavior in the preferences and then rebuild the index

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is it necessary to destroy the index?

It's necessary to destroy the index, if you change the style of your layer, since the subject is especially to filter invisible feature.
Initially there was a bug when you checked a "sub style". I added possible cases.
In use, only styleChanged is really needed. What do you think if I do this:

remove:
connect( mLayer, &QgsVectorLayer::rendererChanged, this, &QgsPointLocator::destroyIndex ); connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex ); connect( mLayer, &QgsVectorLayer::layerModified, this, &QgsPointLocator::destroyIndex );

add connect( mLayer, &QgsVectorLayer::styleChanged, this, &QgsPointLocator::destroyIndex ); in QgsPointLocator::setRenderContext


bool QgsPointLocator::init( int maxFeaturesToIndex )
{
Expand All @@ -686,6 +702,7 @@ bool QgsPointLocator::rebuildIndex( int maxFeaturesToIndex )

QgsFeatureRequest request;
request.setSubsetOfAttributes( QgsAttributeList() );

if ( mExtent )
{
QgsRectangle rect = *mExtent;
Expand All @@ -704,13 +721,40 @@ bool QgsPointLocator::rebuildIndex( int maxFeaturesToIndex )
}
request.setFilterRect( rect );
}

bool filter = false;
std::unique_ptr< QgsFeatureRenderer > renderer( mLayer->renderer() ? mLayer->renderer()->clone() : nullptr );
QgsRenderContext *ctx = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you just use mContext directly instead of ctx?

Copy link
Member Author

Choose a reason for hiding this comment

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

No sorry, it doesn't work :/

if ( mContext )
{
mContext->expressionContext() << QgsExpressionContextUtils::layerScope( mLayer );
ctx = mContext.get();
if ( renderer )
{
// setup scale for scale dependent visibility (rule based)
renderer->startRender( *ctx, mLayer->fields() );
filter = renderer->capabilities() & QgsFeatureRenderer::Filter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

set request's filter expression here to renderer->filter()

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, if I do request.setFilterExpression(renderer->filter()); with a request.setSubsetOfAttributes( QgsAttributeList() ); before. I can't snap on features when the style is RuleBasedRenderer and one of filter is unchecked. For a layer with a categorized renderer, if you uncheck a category, invisible feature can be snapped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's why in this branch you should set the subset of attributes to those required by the renderer (renderer->usedAttributes())

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes. Thank you!

request.setSubsetOfAttributes( renderer->usedAttributes( *ctx ), mLayer->fields() );
}
}

QgsFeatureIterator fi = mLayer->getFeatures( request );
int indexedCount = 0;

while ( fi.nextFeature( f ) )
{
if ( !f.hasGeometry() )
continue;

if ( filter && ctx && renderer )
{
ctx->expressionContext().setFeature( f );
if ( !renderer->willRenderFeature( f, *ctx ) )
{
continue;
}
}

if ( mTransform.isValid() )
{
try
Expand Down Expand Up @@ -761,6 +805,12 @@ bool QgsPointLocator::rebuildIndex( int maxFeaturesToIndex )
QgsPointLocator_Stream stream( dataList );
mRTree = RTree::createAndBulkLoadNewRTree( RTree::BLM_STR, stream, *mStorage, fillFactor, indexCapacity,
leafCapacity, dimension, variant, indexId );

if ( ctx && renderer )
{
renderer->stopRender( *ctx );
renderer.release();
}
return true;
}

Expand Down Expand Up @@ -792,6 +842,30 @@ void QgsPointLocator::onFeatureAdded( QgsFeatureId fid )
if ( !f.hasGeometry() )
return;

std::unique_ptr< QgsFeatureRenderer > renderer( mLayer->renderer() ? mLayer->renderer()->clone() : nullptr );
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to be cloned inside "if ( mContext )" block, so we do not copy it when snapping also to invisible features.

QgsRenderContext *ctx = nullptr;
if ( mContext )
{
mContext->expressionContext() << QgsExpressionContextUtils::layerScope( mLayer );
ctx = mContext.get();
if ( renderer && ctx )
{
bool pass = false;
renderer->startRender( *ctx, mLayer->fields() );

ctx->expressionContext().setFeature( f );
if ( !renderer->willRenderFeature( f, *ctx ) )
{
pass = true;
}

renderer->stopRender( *ctx );
renderer.release();
if ( pass )
return;
}
}

if ( mTransform.isValid() )
{
try
Expand Down Expand Up @@ -832,6 +906,7 @@ void QgsPointLocator::onFeatureDeleted( QgsFeatureId fid )
mRTree->deleteData( rect2region( mGeoms[fid]->boundingBox() ), fid );
delete mGeoms.take( fid );
}

}

void QgsPointLocator::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geom )
Expand All @@ -841,6 +916,14 @@ void QgsPointLocator::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &ge
onFeatureAdded( fid );
}

void QgsPointLocator::onAttributeValueChanged( QgsFeatureId fid, int idx, const QVariant &value )
{
Q_UNUSED( idx );
Q_UNUSED( value );
onFeatureDeleted( fid );
onFeatureAdded( fid );
Copy link
Member

Choose a reason for hiding this comment

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

These two functions should be called only when snapping to visible features...

}


QgsPointLocator::Match QgsPointLocator::nearestVertex( const QgsPointXY &point, double tolerance, MatchFilter *filter )
{
Expand Down
15 changes: 13 additions & 2 deletions src/core/qgspointlocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@

class QgsPointXY;
class QgsVectorLayer;
class QgsFeatureRenderer;
class QgsRenderContext;

#include "qgis_core.h"
#include "qgsfeature.h"
#include "qgspointxy.h"
#include "qgscoordinatereferencesystem.h"
#include "qgscoordinatetransform.h"
#include <memory>

class QgsPointLocator_VisitorNearestVertex;
class QgsPointLocator_VisitorNearestEdge;
Expand Down Expand Up @@ -92,6 +95,12 @@ class CORE_EXPORT QgsPointLocator : public QObject
*/
void setExtent( const QgsRectangle *extent );

/**
* Configure render context - if not null, it will use to index only visible feature
* \since QGIS 3.2
*/
void setRenderContext( const QgsRenderContext *context );

/**
* The type of a snap result or the filter type for a snap request.
*/
Expand Down Expand Up @@ -251,8 +260,6 @@ class CORE_EXPORT QgsPointLocator : public QObject
//! find out if the point is in any polygons
MatchList pointInPolygon( const QgsPointXY &point );

//

/**
* Return how many geometries are cached in the index
* \since QGIS 2.14
Expand All @@ -267,6 +274,7 @@ class CORE_EXPORT QgsPointLocator : public QObject
void onFeatureAdded( QgsFeatureId fid );
void onFeatureDeleted( QgsFeatureId fid );
void onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geom );
void onAttributeValueChanged( QgsFeatureId fid, int idx, const QVariant &value );

private:
//! Storage manager
Expand All @@ -278,11 +286,14 @@ class CORE_EXPORT QgsPointLocator : public QObject
//! flag whether the layer is currently empty (i.e. mRTree is null but it is not necessary to rebuild it)
bool mIsEmptyLayer;


//! R-tree containing spatial index
QgsCoordinateTransform mTransform;
QgsVectorLayer *mLayer = nullptr;
QgsRectangle *mExtent = nullptr;

std::unique_ptr<QgsRenderContext> mContext;

friend class QgsPointLocator_VisitorNearestVertex;
friend class QgsPointLocator_VisitorNearestEdge;
friend class QgsPointLocator_VisitorArea;
Expand Down
23 changes: 18 additions & 5 deletions src/core/qgssnappingutils.cpp
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,7 +94,6 @@ 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 QgsPointLocator::Match _findClosestSegmentIntersection( const QgsPointXY &pt, const QgsPointLocator::MatchList &segments )
{
if ( segments.isEmpty() )
Expand Down Expand Up @@ -156,9 +157,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 )
{

// is candidate match relevant?
if ( !candidateMatch.isValid() || candidateMatch.distance() > maxDistance )
return;
Expand All @@ -174,7 +175,6 @@ static void _replaceIfBetter( QgsPointLocator::Match &bestMatch, const QgsPointL
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 )
{
if ( type & QgsPointLocator::Vertex )
Expand Down Expand Up @@ -329,7 +329,6 @@ QgsPointLocator::Match QgsSnappingUtils::snapToMap( const QgsPointXY &pointMap,
return QgsPointLocator::Match();
}


void QgsSnappingUtils::prepareIndex( const QList<LayerAndAreaOfInterest> &layers )
{
if ( mIsIndexing )
Expand All @@ -341,6 +340,7 @@ void QgsSnappingUtils::prepareIndex( const QList<LayerAndAreaOfInterest> &layers
Q_FOREACH ( const LayerAndAreaOfInterest &entry, layers )
{
QgsVectorLayer *vl = entry.first;

if ( vl->geometryType() == QgsWkbTypes::NullGeometry || mStrategy == IndexNeverFull )
continue;

Expand All @@ -359,7 +359,15 @@ void QgsSnappingUtils::prepareIndex( const QList<LayerAndAreaOfInterest> &layers
QgsVectorLayer *vl = entry.first;
QTime tt;
tt.start();

QgsPointLocator *loc = locatorForLayer( vl );

if ( !mEnableSnappingForInvisibleFeature )
{
QgsRenderContext ctx = QgsRenderContext::fromMapSettings( mMapSettings );
loc->setRenderContext( &ctx );
}

if ( mStrategy == IndexExtent )
{
QgsRectangle rect( mMapSettings.extent() );
Expand Down Expand Up @@ -428,6 +436,11 @@ QgsSnappingConfig QgsSnappingUtils::config() const
return mSnappingConfig;
}

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

void QgsSnappingUtils::setConfig( const QgsSnappingConfig &config )
{
if ( mSnappingConfig == config )
Expand Down
Loading