Fix bugs in Tracks Cleaner.

1) Implement more robust connection checking so we don't decide very
short segments with both endpoints within a single track are connected
at both ends.

2) Make use of the IS_DELETED flag to perform iteration during dryRun
so that all deletions to be made are flagged.

Fixes https://gitlab.com/kicad/code/kicad/issues/8883

pcbnew/connectivity/connectivity_data.cpp

@@ -588,7 +588,7 @@ void CONNECTIVITY_DATA::GetUnconnectedEdges( std::vector<CN_EDGE>& aEdges) const
 
 bool CONNECTIVITY_DATA::TestTrackEndpointDangling( PCB_TRACK* aTrack, wxPoint* aPos )
 {
-    auto items = GetConnectivityAlgo()->ItemEntry( aTrack ).GetItems();
+    std::list<CN_ITEM*> items = GetConnectivityAlgo()->ItemEntry( aTrack ).GetItems();
 
     // Not in the connectivity system.  This is a bug!
     if( items.empty() )
@@ -602,23 +602,63 @@ bool CONNECTIVITY_DATA::TestTrackEndpointDangling( PCB_TRACK* aTrack, wxPoint* a
     if( !citem->Valid() )
         return false;
 
-    for( const std::shared_ptr<CN_ANCHOR>& anchor : citem->Anchors() )
+    if( aTrack->Type() == PCB_TRACE_T || aTrack->Type() == PCB_ARC_T )
     {
-        if( anchor->IsDangling() )
+        // Test if a segment is connected on each end.
+        //
+        // NB: be wary of short segments which can be connected to the *same* other item on
+        // each end.  If that's their only connection then they're still dangling.
+
+        PCB_LAYER_ID layer = aTrack->GetLayer();
+        int          accuracy = KiROUND( aTrack->GetWidth() / 2 );
+        int          start_count = 0;
+        int          end_count = 0;
+
+        for( CN_ITEM* connected : citem->ConnectedItems() )
         {
-            if( aPos )
+            BOARD_CONNECTED_ITEM* item = connected->Parent();
-                *aPos = static_cast<wxPoint>( anchor->Pos() );
 
-            return true;
+            if( item->GetFlags() & IS_DELETED )
+                continue;
+
+            std::shared_ptr<SHAPE> shape = item->GetEffectiveShape( layer );
+
+            int  startDist;
+            int  endDist;
+            bool hitStart = shape->Collide( aTrack->GetStart(), accuracy, &startDist );
+            bool hitEnd = shape->Collide( aTrack->GetEnd(), accuracy, &endDist );
+
+            if( hitStart && hitEnd )
+            {
+                if( startDist <= endDist )
+                    start_count++;
+                else
+                    end_count++;
+            }
+            else if( hitStart )
+            {
+                start_count++;
+            }
+            else if( hitEnd )
+            {
+                end_count++;
+            }
+
+            if( start_count > 0 && end_count > 0 )
+                return false;
         }
+
+        if( aPos )
+            *aPos = (start_count == 0 ) ? aTrack->GetStart() : aTrack->GetEnd();
+
+        return true;
     }
-
+    else if( aTrack->Type() == PCB_VIA_T )
-    // Test if a via is only connected on one layer
-    if( aTrack->Type() == PCB_VIA_T )
     {
-        const CN_ITEM::CONNECTED_ITEMS& connected = citem->ConnectedItems();
+        // Test if a via is only connected on one layer
+
+        const std::vector<CN_ITEM*>& connected = citem->ConnectedItems();
 
-        // This is a bit redundant but better safe than sorry here
         if( connected.empty() )
         {
             if( aPos )
@@ -628,11 +668,16 @@ bool CONNECTIVITY_DATA::TestTrackEndpointDangling( PCB_TRACK* aTrack, wxPoint* a
         }
 
         // Here, we check if the via is connected only to items on a single layer
-        int first_layer = connected.front()->Layer();
+        int first_layer = UNDEFINED_LAYER;
 
-        for( auto& item : connected )
+        for( CN_ITEM* item : connected )
         {
-           if( item->Layer() != first_layer )
+            if( item->Parent()->GetFlags() & IS_DELETED )
+                continue;
+
+            if( first_layer == UNDEFINED_LAYER )
+                first_layer = item->Layer();
+            else if( item->Layer() != first_layer )
                return false;
         }
 
@@ -641,6 +686,10 @@ bool CONNECTIVITY_DATA::TestTrackEndpointDangling( PCB_TRACK* aTrack, wxPoint* a
 
         return true;
     }
+    else
+    {
+        wxFAIL_MSG( "CONNECTIVITY_DATA::TestTrackEndpointDangling: unknown track type" );
+    }
 
     return false;
 }

pcbnew/tracks_cleaner.cpp

@@ -171,7 +171,7 @@ bool TRACKS_CLEANER::deleteDanglingTracks( bool aTrack, bool aVia )
     do // Iterate when at least one track is deleted
     {
         item_erased = false;
-        // Ensure the connectivity is up to date, especially after removind a dangling segment
+        // Ensure the connectivity is up to date, especially after removing a dangling segment
         m_brd->BuildConnectivity();
 
         // Keep a duplicate deque to all deleting in the primary
@@ -179,7 +179,7 @@ bool TRACKS_CLEANER::deleteDanglingTracks( bool aTrack, bool aVia )
 
         for( PCB_TRACK* track : temp_tracks )
         {
-            if( track->IsLocked() )
+            if( track->IsLocked() || ( track->GetFlags() & IS_DELETED ) > 0 )
                 continue;
 
             if( !aVia && track->Type() == PCB_VIA_T )
@@ -200,19 +200,18 @@ bool TRACKS_CLEANER::deleteDanglingTracks( bool aTrack, bool aVia )
 
                 item->SetItems( track );
                 m_itemsList->push_back( item );
+                track->SetFlags( IS_DELETED );
+
+                // keep iterating, because a track connected to the deleted track
+                // now perhaps is not connected and should be deleted
+                item_erased = true;
 
                 if( !m_dryRun )
                 {
                     m_brd->Remove( track );
                     m_commit.Removed( track );
-
-                    /* keep iterating, because a track connected to the deleted track
-                     * now perhaps is not connected and should be deleted */
-                    item_erased = true;
                     modified = true;
                 }
-                // Fix me: In dry run we should disable the track to erase and retry with this disabled track
-                // However the connectivity algo does not handle disabled items.
             }
         }
     } while( item_erased ); // A segment was erased: test for some new dangling segments
@@ -249,11 +248,12 @@ void TRACKS_CLEANER::deleteTracksInPads()
 
                 if( poly.IsEmpty() )
                 {
-                    std::shared_ptr<CLEANUP_ITEM> item = std::make_shared<CLEANUP_ITEM>( CLEANUP_TRACK_IN_PAD );
+                    auto item = std::make_shared<CLEANUP_ITEM>( CLEANUP_TRACK_IN_PAD );
                     item->SetItems( track );
                     m_itemsList->push_back( item );
 
                     toRemove.insert( track );
+                    track->SetFlags( IS_DELETED );
                 }
             }
         }