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

Update vertex tool to base its snapping parameters on global ones #36231

Merged
merged 3 commits into from
Jun 4, 2020
Merged

Update vertex tool to base its snapping parameters on global ones #36231

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
79fa9dc
Update vertex tool to not override some of the global snapping parame…
May 6, 2020
a367bce
Fix vertextool test : Specific tolerance, units and typeflag are mand…
May 6, 2020
d21b743
Initialize vertex tool snapping config with old config.
May 12, 2020
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
43 changes: 37 additions & 6 deletions src/app/vertextool/qgsvertextool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,11 +748,14 @@ QgsPointLocator::Match QgsVertexTool::snapToEditableLayer( QgsMapMouseEvent *e )
QgsPointXY mapPoint = toMapCoordinates( e->pos() );
double tol = QgsTolerance::vertexSearchRadius( canvas()->mapSettings() );

QgsSnappingConfig config;
QgsSnappingConfig config = oldConfig;
config.setEnabled( true );
config.setMode( QgsSnappingConfig::AdvancedConfiguration );
config.setIntersectionSnapping( false ); // only snap to layers
Q_ASSERT( config.individualLayerSettings().isEmpty() );
config.individualLayerSettings().clear();

typedef QHash<QgsVectorLayer *, QgsSnappingConfig::IndividualLayerSettings> SettingsHashMap;
SettingsHashMap oldLayerSettings = oldConfig.individualLayerSettings();

// if there is a current layer, it should have priority over other layers
// because sometimes there may be match from multiple layers at one location
Expand All @@ -768,8 +771,23 @@ QgsPointLocator::Match QgsVertexTool::snapToEditableLayer( QgsMapMouseEvent *e )
if ( !vlayer )
continue;

config.setIndividualLayerSettings( vlayer, QgsSnappingConfig::IndividualLayerSettings(
vlayer == currentVlayer, static_cast<QgsSnappingConfig::SnappingTypeFlag>( QgsSnappingConfig::VertexFlag | QgsSnappingConfig::SegmentFlag ), tol, QgsTolerance::ProjectUnits, 0.0, 0.0 ) );
QgsSnappingConfig::IndividualLayerSettings layerSettings;
SettingsHashMap::const_iterator existingSettings = oldLayerSettings.find( vlayer );
if ( existingSettings != oldLayerSettings.constEnd() )
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be simpler to initialize config with oldconfig and just changing the IndividualLayerSettings enabled boolean according to whether its the currentLayer or not (or editable layer for the following code) ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes probably if we are sure there is always an existing IndividualLayerSetting for each layer which I am not. Also tolerance, typeflag and units are overridden here for each setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! I still wondering if it would not be better to initialize config with oldConfig one. If someone adds a new SnapingConfig parameter, it would not be takein into count here.

But maybe it's better this way

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think your are right for the initial config init with old config. I will do that.

{
layerSettings = existingSettings.value();
layerSettings.setEnabled( vlayer == currentVlayer );
layerSettings.setTolerance( tol );
layerSettings.setTypeFlag( static_cast<QgsSnappingConfig::SnappingTypeFlag>( QgsSnappingConfig::VertexFlag | QgsSnappingConfig::SegmentFlag ) );
layerSettings.setUnits( QgsTolerance::ProjectUnits );
}
else
{
layerSettings = QgsSnappingConfig::IndividualLayerSettings(
vlayer == currentVlayer, static_cast<QgsSnappingConfig::SnappingTypeFlag>( QgsSnappingConfig::VertexFlag | QgsSnappingConfig::SegmentFlag ), tol, QgsTolerance::ProjectUnits, 0.0, 0.0 );
}

config.setIndividualLayerSettings( vlayer, layerSettings );
}

snapUtils->setConfig( config );
Expand All @@ -795,8 +813,21 @@ QgsPointLocator::Match QgsVertexTool::snapToEditableLayer( QgsMapMouseEvent *e )
if ( !vlayer )
continue;

config.setIndividualLayerSettings( vlayer, QgsSnappingConfig::IndividualLayerSettings(
vlayer->isEditable(), static_cast<QgsSnappingConfig::SnappingTypeFlag>( QgsSnappingConfig::VertexFlag | QgsSnappingConfig::SegmentFlag ), tol, QgsTolerance::ProjectUnits, 0.0, 0.0 ) );
QgsSnappingConfig::IndividualLayerSettings layerSettings;
SettingsHashMap::const_iterator existingSettings = oldLayerSettings.find( vlayer );
if ( existingSettings != oldLayerSettings.constEnd() )
{
layerSettings = existingSettings.value();
layerSettings.setEnabled( vlayer->isEditable() );
layerSettings.setTolerance( tol );
layerSettings.setTypeFlag( static_cast<QgsSnappingConfig::SnappingTypeFlag>( QgsSnappingConfig::VertexFlag | QgsSnappingConfig::SegmentFlag ) );
layerSettings.setUnits( QgsTolerance::ProjectUnits );
}
else
{
layerSettings = QgsSnappingConfig::IndividualLayerSettings( vlayer->isEditable(), static_cast<QgsSnappingConfig::SnappingTypeFlag>( QgsSnappingConfig::VertexFlag | QgsSnappingConfig::SegmentFlag ), tol, QgsTolerance::ProjectUnits, 0.0, 0.0 );
}
config.setIndividualLayerSettings( vlayer, layerSettings );
}

snapUtils->setConfig( config );
Expand Down
2 changes: 1 addition & 1 deletion src/core/qgssnappingutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ bool QgsSnappingUtils::isIndexPrepared( QgsPointLocator *loc, const QgsRectangle
if ( mStrategy == IndexAlwaysFull && loc->hasIndex() )
return true;

if ( mStrategy == IndexExtent && loc->hasIndex() && loc->extent()->intersects( areaOfInterest ) )
if ( mStrategy == IndexExtent && loc->hasIndex() && ( !loc->extent() || loc->extent()->intersects( areaOfInterest ) ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be possible to get in a situation where the strategy is IndexExtent and the locator has no extent defined, isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

Well I had to add this check to avoid snapping to crash, so yes It seems to be possible with the state of the code. I did not really understood why though. Might be worth it to spend further time to investigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discuss, it could happen when some QgsPointLocator methods (nearestvertex for instance) are called first outside of QgsSnappingUtils (in QgsVertexTool basically).

Then, when prepareIndex is called and so isIndexPrepared, extent is null and strategy is IndexExtent. So, your modification make sense.

return true;

QgsRectangle aoi( areaOfInterest );
Expand Down