[vertex tool] Fix topo editing when moving vertices/edges (fixes #20158)

- when some "extra" vertices are selected when moving a vertex, their coincident vertices will be also moved (#20158) - when moving an edge, coincident vertices to its endpoints will be also moved - new tests to cover the above scenarios - made the code hopefully easier to read (cherry picked from commit 93e737e)
qgis · Nov 6, 2018 · 2f5a87f · 2f5a87f
1 parent d737e56
commit 2f5a87f
Show file tree

Hide file tree

Showing 3 changed files with 168 additions and 61 deletions.
diff --git a/src/app/vertextool/qgsvertextool.cpp b/src/app/vertextool/qgsvertextool.cpp
@@ -474,12 +474,9 @@ void QgsVertexTool::cadCanvasReleaseEvent( QgsMapMouseEvent *e )
 
     // for each editable layer, select vertices
     const auto layers = canvas()->layers();
-    for ( QgsMapLayer *layer : layers )
+    const auto editableLayers = editableVectorLayers();
+    for ( QgsVectorLayer *vlayer : editableLayers )
     {
-      QgsVectorLayer *vlayer = qobject_cast<QgsVectorLayer *>( layer );
-      if ( !vlayer || !vlayer->isEditable() || !vlayer->isSpatial() )
-        continue;
-
       if ( mMode == ActiveLayer && vlayer != currentVectorLayer() )
         continue;
 
@@ -1165,6 +1162,65 @@ void QgsVertexTool::startDragging( QgsMapMouseEvent *e )
   }
 }
 
+QList<QgsVectorLayer *> QgsVertexTool::editableVectorLayers()
+{
+  QList<QgsVectorLayer *> editableLayers;
+  const auto layers = canvas()->layers();
+  for ( QgsMapLayer *layer : layers )
+  {
+    QgsVectorLayer *vlayer = qobject_cast<QgsVectorLayer *>( layer );
+    if ( vlayer && vlayer->isEditable() && vlayer->isSpatial() )
+      editableLayers << vlayer;
+  }
+  return editableLayers;
+}
+
+QSet<Vertex> QgsVertexTool::findCoincidentVertices( const QSet<Vertex> &vertices )
+{
+  QSet<Vertex> topoVertices;
+  const auto editableLayers = editableVectorLayers();
+
+  for ( const Vertex &v : qgis::as_const( vertices ) )
+  {
+    QgsPointXY origPointV = cachedGeometryForVertex( v ).vertexAt( v.vertexId );
+    QgsPointXY mapPointV = toMapCoordinates( v.layer, origPointV );
+    for ( QgsVectorLayer *vlayer : editableLayers )
+    {
+      const auto snappedVertices = layerVerticesSnappedToPoint( vlayer, mapPointV );
+      for ( const QgsPointLocator::Match &otherMatch : snappedVertices )
+      {
+        Vertex otherVertex( otherMatch.layer(), otherMatch.featureId(), otherMatch.vertexIndex() );
+        if ( !vertices.contains( otherVertex ) )
+          topoVertices << otherVertex;
+      }
+    }
+  }
+  return topoVertices;
+}
+
+void QgsVertexTool::buildExtraVertices( const QSet<Vertex> &vertices, const QgsPointXY &anchorPoint, QgsVectorLayer *anchorLayer )
+{
+  for ( const Vertex &v : qgis::as_const( vertices ) )
+  {
+    if ( mDraggingVertex && v == *mDraggingVertex )
+      continue;
+
+    QgsPointXY pointV = cachedGeometryForVertex( v ).vertexAt( v.vertexId );
+
+    // Calculate position of the anchor point in the coordinates of our vertex v
+    // Try to avoid reprojection so we do not loose precision when map CRS does not match layer CRS
+    QgsPointXY anchorPointLayer;
+    if ( v.layer->crs() == anchorLayer->crs() )
+      anchorPointLayer = anchorPoint;
+    else  // reprojection is necessary: ANCHOR -> MAP -> V
+      anchorPointLayer = toLayerCoordinates( v.layer, toMapCoordinates( anchorLayer, anchorPoint ) );
+    QgsVector offset = pointV - anchorPointLayer;
+
+    mDraggingExtraVertices << v;
+    mDraggingExtraVerticesOffset << offset;
+  }
+}
+
 void QgsVertexTool::startDraggingMoveVertex( const QgsPointLocator::Match &m )
 {
   Q_ASSERT( m.hasVertex() );
@@ -1179,59 +1235,22 @@ void QgsVertexTool::startDraggingMoveVertex( const QgsPointLocator::Match &m )
 
   setHighlightedVerticesVisible( false );  // hide any extra highlight of vertices until we are done with moving
 
-  QgsPointXY origDraggingVertexPoint = geom.vertexAt( mDraggingVertex->vertexId );
-
-  // if there are other highlighted vertices, they should be dragged as well with their offset
+  QSet<Vertex> movingVertices;
+  movingVertices << *mDraggingVertex;
   for ( const Vertex &v : qgis::as_const( mSelectedVertices ) )
-  {
-    if ( v != *mDraggingVertex )
-    {
-      QgsPointXY origPointV = cachedGeometryForVertex( v ).vertexAt( v.vertexId );
-      QgsPointXY origPointLayer = origDraggingVertexPoint;
-      if ( v.layer->crs() != mDraggingVertex->layer->crs() )  // reproject if necessary
-        origPointLayer = toLayerCoordinates( v.layer, toMapCoordinates( m.layer(), origDraggingVertexPoint ) );
-      QgsVector offset = origPointV - origPointLayer;
-
-      mDraggingExtraVertices << v;
-      mDraggingExtraVerticesOffset << offset;
-    }
-  }
-
-  cadDockWidget()->setPoints( QList<QgsPointXY>() << m.point() << m.point() );
+    movingVertices << v;
 
   if ( QgsProject::instance()->topologicalEditing() )
   {
-    // support for topo editing - find extra features
-    // that have coincident point with the vertex being dragged
-    const auto layers = canvas()->layers();
-    for ( QgsMapLayer *layer : layers )
-    {
-      QgsVectorLayer *vlayer = qobject_cast<QgsVectorLayer *>( layer );
-      if ( !vlayer || !vlayer->isEditable() )
-        continue;
+    movingVertices.unite( findCoincidentVertices( movingVertices ) );
+  }
 
-      const auto snappedVertices = layerVerticesSnappedToPoint( vlayer, m.point() );
-      for ( const QgsPointLocator::Match &otherMatch : snappedVertices )
-      {
-        if ( otherMatch.layer() == m.layer() &&
-             otherMatch.featureId() == m.featureId() &&
-             otherMatch.vertexIndex() == m.vertexIndex() )
-          continue;
+  buildExtraVertices( movingVertices, geom.vertexAt( m.vertexIndex() ), m.layer() );
 
-        // start dragging of snapped point of current layer
-        mDraggingExtraVertices << Vertex( otherMatch.layer(), otherMatch.featureId(), otherMatch.vertexIndex() );
-        mDraggingExtraVerticesOffset << QgsVector(); // topo vertices have the same position
-      }
-    }
-  }
+  cadDockWidget()->setPoints( QList<QgsPointXY>() << m.point() << m.point() );
 
   // now build drag rubber bands for extra vertices
 
-  QSet<Vertex> movingVertices;
-  movingVertices << *mDraggingVertex;
-  for ( const Vertex &v : qgis::as_const( mDraggingExtraVertices ) )
-    movingVertices << v;
-
   QgsPointXY dragVertexMapPoint = m.point();
 
   buildDragBandsForVertices( movingVertices, dragVertexMapPoint );
@@ -1423,13 +1442,9 @@ void QgsVertexTool::startDraggingAddVertex( const QgsPointLocator::Match &m )
   {
     // find other segments coincident with the one user just picked and store them in a list
     // so we can add a new vertex also in those to keep topology correct
-    const auto layers = canvas()->layers();
-    for ( QgsMapLayer *layer : layers )
+    const auto editableLayers = editableVectorLayers();
+    for ( QgsVectorLayer *vlayer : editableLayers )
     {
-      QgsVectorLayer *vlayer = qobject_cast<QgsVectorLayer *>( layer );
-      if ( !vlayer || !vlayer->isEditable() )
-        continue;
-
       if ( vlayer->geometryType() != QgsWkbTypes::LineGeometry && vlayer->geometryType() != QgsWkbTypes::PolygonGeometry )
         continue;
 
@@ -1506,14 +1521,14 @@ void QgsVertexTool::startDraggingEdge( const QgsPointLocator::Match &m, const Qg
 
   buildDragBandsForVertices( movingVertices, mapPoint );
 
-  QgsPointXY layerPoint = toLayerCoordinates( m.layer(), mapPoint );
-
-  for ( const Vertex &v : qgis::as_const( movingVertices ) )
+  if ( QgsProject::instance()->topologicalEditing() )
   {
-    mDraggingExtraVertices << v;
-    mDraggingExtraVerticesOffset << ( geom.vertexAt( v.vertexId ) - QgsPoint( layerPoint ) );
+    movingVertices.unite( findCoincidentVertices( movingVertices ) );
   }
 
+  QgsPointXY layerPoint = toLayerCoordinates( m.layer(), mapPoint );
+  buildExtraVertices( movingVertices, layerPoint, m.layer() );
+
   cadDockWidget()->setPoints( QList<QgsPointXY>() << m.point() << m.point() );
 }
 

diff --git a/src/app/vertextool/qgsvertextool.h b/src/app/vertextool/qgsvertextool.h
@@ -184,6 +184,20 @@ class APP_EXPORT QgsVertexTool : public QgsMapToolAdvancedDigitizing
 
     void applyEditsToLayers( VertexEdits &edits );
 
+    /**
+     * For the given set of vertices (possibly from multiple layers) find any another vertices that are coincident with
+     * them and not yet included in the set. This is used for topological editing to find all vertices that should be moved.
+     */
+    QSet<Vertex> findCoincidentVertices( const QSet<Vertex> &vertices );
+
+    /**
+     * For the given set of vertices, prepare mDraggingExtraVertices and mDraggingExtraVerticesOffset arrays.
+     * The parameters anchorPoint and anchorLayer are used to calculate offsets.
+     */
+    void buildExtraVertices( const QSet<Vertex> &vertices, const QgsPointXY &anchorPoint, QgsVectorLayer *anchorLayer );
+
+    //! Returns a list of canvas layers filtered to just editable spatial vector layers
+    QList<QgsVectorLayer *> editableVectorLayers();
 
     enum HighlightMode
     {
@@ -382,7 +396,7 @@ class APP_EXPORT QgsVertexTool : public QgsMapToolAdvancedDigitizing
      */
     std::unique_ptr<QgsPointLocator::Match> mNewVertexFromDoubleClick;
 
-    //! Geometry cache for fast access to geometries
+    //! Geometry cache for fast access to geometries (coordinates are in their layer's CRS)
     QHash<const QgsVectorLayer *, QHash<QgsFeatureId, QgsGeometry> > mCache;
 
     // support for vertex editor

diff --git a/tests/src/app/testqgsvertextool.cpp b/tests/src/app/testqgsvertextool.cpp
@@ -62,9 +62,11 @@ class TestQgsVertexTool : public QObject
     void testAddVertexAtEndpoint();
     void testDeleteVertex();
     void testMoveMultipleVertices();
+    void testMoveMultipleVertices2();
     void testMoveVertexTopo();
     void testDeleteVertexTopo();
     void testAddVertexTopo();
+    void testMoveEdgeTopo();
     void testActiveLayerPriority();
     void testSelectedFeaturesPriority();
 
@@ -475,6 +477,25 @@ void TestQgsVertexTool::testMoveMultipleVertices()
   QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(2 1, 1 1, 1 3)" ) );
 }
 
+void TestQgsVertexTool::testMoveMultipleVertices2()
+{
+  // this time select two vertices with shift
+  mouseClick( 1, 1, Qt::LeftButton, Qt::ShiftModifier );
+  mouseClick( 2, 1, Qt::LeftButton, Qt::ShiftModifier );
+
+  // move them by +1, +1
+  mouseClick( 1, 1, Qt::LeftButton );
+  mouseClick( 2, 2, Qt::LeftButton );
+
+  QCOMPARE( mLayerLine->undoStack()->index(), 2 );
+  QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(3 2, 2 2, 1 3)" ) );
+
+  mLayerLine->undoStack()->undo();
+  QCOMPARE( mLayerLine->undoStack()->index(), 1 );
+
+  QCOMPARE( mLayerLine->getFeature( mFidLineF1 ).geometry(), QgsGeometry::fromWkt( "LINESTRING(2 1, 1 1, 1 3)" ) );
+}
+
 void TestQgsVertexTool::testMoveVertexTopo()
 {
   // test moving of vertices of two features at once
@@ -564,6 +585,63 @@ void TestQgsVertexTool::testAddVertexTopo()
   QgsProject::instance()->setTopologicalEditing( false );
 }
 
+void TestQgsVertexTool::testMoveEdgeTopo()
+{
+  // test move of an edge shared with another feature
+
+  // add a temporary polygon
+  QgsFeature fTmp;
+  fTmp.setGeometry( QgsGeometry::fromWkt( "POLYGON((4 4, 7 4, 7 6, 4 6, 4 4))" ) );
+  bool resAdd = mLayerPolygon->addFeature( fTmp );
+  QVERIFY( resAdd );
+  QgsFeatureId fTmpId = fTmp.id();
+
+  QCOMPARE( mLayerPolygon->undoStack()->index(), 2 );
+
+  QgsProject::instance()->setTopologicalEditing( true );
+
+  // move shared segment
+  mouseClick( 6, 4, Qt::LeftButton );
+  mouseClick( 6, 5, Qt::LeftButton );
+
+  QCOMPARE( mLayerPolygon->undoStack()->index(), 3 );
+
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 1, 7 1, 7 5, 4 5, 4 1))" ) );
+  QCOMPARE( mLayerPolygon->getFeature( fTmpId ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 5, 7 5, 7 6, 4 6, 4 5))" ) );
+
+  mLayerPolygon->undoStack()->undo();
+
+  // another test to move a shared segment - but this time we just pick two individual points of a feature
+  // and do vertex move
+
+  QgsProject::instance()->setTopologicalEditing( false );
+
+  // this time select two vertices with shift
+  mouseClick( 4, 4, Qt::LeftButton, Qt::ShiftModifier );
+  mouseClick( 7, 4, Qt::LeftButton, Qt::ShiftModifier );
+
+  QgsProject::instance()->setTopologicalEditing( true );
+
+  // now move the shared segment
+  mouseClick( 4, 4, Qt::LeftButton );
+  mouseClick( 4, 3, Qt::LeftButton );
+
+  QCOMPARE( mLayerPolygon->undoStack()->index(), 3 );
+
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 1, 7 1, 7 3, 4 3, 4 1))" ) );
+  QCOMPARE( mLayerPolygon->getFeature( fTmpId ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 3, 7 3, 7 6, 4 6, 4 3))" ) );
+
+  mLayerPolygon->undoStack()->undo();
+
+  //
+
+  mLayerPolygon->undoStack()->undo();
+
+  QCOMPARE( mLayerPolygon->getFeature( mFidPolygonF1 ).geometry(), QgsGeometry::fromWkt( "POLYGON((4 1, 7 1, 7 4, 4 4, 4 1))" ) );
+
+  QgsProject::instance()->setTopologicalEditing( false );
+}
+
 void TestQgsVertexTool::testActiveLayerPriority()
 {
   // check that features from current layer get priority when picking points