Update vertex tool to base its snapping parameters on global ones #36231
Conversation
…ters (especially snapping enabled on a scale range). Keep some of the specific behavior which is there for a reason (ie creation of of specific layer settings, now based on the existing one if present). Should fix qgis#36229
| 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() ) |
There was a problem hiding this comment.
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) ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes I think your are right for the initial config init with old config. I will do that.
| @@ -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 ) ) ) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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
|
stale
bot
added
the
stale
stale
bot
removed
the
stale
Update vertex tool to not override some of the global snapping parameters (especially snapping enabled on a scale range).
Keep some of the specific behavior of enabling snapping in priority on an active layer (ie creation of a specific layer settings, now based an existing one if present).
Should fix #36229